CSHARP-5505: Add $geoNear stage aggregation builders #1621
Conversation
adelinowona
requested review from
JamesKovacs,
rstam and
BorisDog
and removed request for
a team and
JamesKovacs
src/MongoDB.Driver/GeoNearOptions.cs
Outdated
| /// <summary> | ||
| /// Gets or sets the distance field. Required if querying a time-series collection. | ||
| /// </summary> | ||
| public string DistanceField { get; set; } |
There was a problem hiding this comment.
We should make a note this is optional here given that we don't use the nullable reference annotations in our API.
There was a problem hiding this comment.
Is DistanceField actually optional? The server documentation doesn't say that.
There was a problem hiding this comment.
Using a string here is also not completely safe.
This name MUST match the corresponding element name (usually the same as the property name unless configured differently) in the TNewResult POCO.
There was a problem hiding this comment.
Is
DistanceFieldactually optional? The server documentation doesn't say that.
In server 8.1, it'll be optional for non-timeseries collections but for time-series collections it's still required. I mentioned the time-series restriction in the property summary to guide users appropriately. If users accidentally omit the property when working with a time-series collection, they'll receive a detailed error message from the server explaining the issue so I don't think there should be too much concern for us here.
There was a problem hiding this comment.
Using a
stringhere is also not completely safe.This name MUST match the corresponding element name (usually the same as the property name unless configured differently) in the
TNewResultPOCO.
Thanks for flagging this! I missed the fact that the element name isn't necessarily the same as the property name. We can address this by changing the type of the DistanceField option to FieldDefinition<TDocument> so that during the serialization process, the appropriate element name will be used if it's different from the property name. However, this means the type of the document passed into GeoNearOptions in the GeoNear methods should be the TOutput instead of TInput. @rstam lmk what you think about this?
src/MongoDB.Driver/GeoNearOptions.cs
Outdated
| /// <summary> | ||
| /// Gets or sets the distance field. Required if querying a time-series collection. | ||
| /// </summary> | ||
| public string DistanceField { get; set; } |
There was a problem hiding this comment.
Is DistanceField actually optional? The server documentation doesn't say that.
src/MongoDB.Driver/GeoNearOptions.cs
Outdated
| /// <summary> | ||
| /// Gets or sets the distance field. Required if querying a time-series collection. | ||
| /// </summary> | ||
| public string DistanceField { get; set; } |
There was a problem hiding this comment.
Using a string here is also not completely safe.
This name MUST match the corresponding element name (usually the same as the property name unless configured differently) in the TNewResult POCO.
| /// <summary> | ||
| /// Gets the geoNear command feature. | ||
| /// </summary> | ||
| public static Feature GeoNearCommand => __geoNearCommand; |
There was a problem hiding this comment.
This is breaking change (is this why validate-apicompat fails?).
|
|
||
| var collection = Fixture.GeoCollection; | ||
|
|
||
| collection.Indexes.CreateOne(new CreateIndexModel<Place>(Builders<Place>.IndexKeys.Geo2DSphere(p => p.GeoJsonPointLocation))); |
There was a problem hiding this comment.
Does index creation happen in ClassFixture already?
| /// <param name="near">The point for which to find the closest documents.</param> | ||
| /// <param name="options">The options.</param> | ||
| /// <returns>The stage.</returns> | ||
| internal static PipelineStageDefinition<TInput, TOutput> GeoNear<TInput, TPoint, TOutput>( |
There was a problem hiding this comment.
private and no need for comment?
| { "distanceMultiplier", options?.DistanceMultiplier, options?.DistanceMultiplier != null }, | ||
| { "key", options?.Key, options?.Key != null }, | ||
| { "query", options?.Query?.Render(args), options?.Query != null }, | ||
| { "includeLocs", options?.IncludeLocs?.Render(outputRenderArgs).FieldName, options?.IncludeLocs != null }, |
No description provided.