CSHARP-4955: Add Render overload with RenderArgs #1278
Conversation
BorisDog
commented
Feb 29, 2024
BorisDog
commented
- Added support for records (IsExternalInit.cs)
- Added RenderArgs overloads
- Removed RenderForFind and FilterDefinitionRenderContext workarounds
- Added obsoletion attributes for older Render methods
BorisDog
requested review from
sanych-sun and
rstam
and removed request for
a team
| namespace System.Runtime.CompilerServices | ||
| { | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| internal class IsExternalInit { } |
There was a problem hiding this comment.
This is needed for records support.
| { "limit", limit }, | ||
| { "numCandidates", options?.NumberOfCandidates ?? limit * 10 }, | ||
| { "index", options?.IndexName ?? "default" }, | ||
| { "filter", () => RenderFilter(s, sr, linqProvider), options?.Filter != null }, | ||
| { "filter", () => options.Filter.Render(renderArgs with { RenderDollarForm = true }), options?.Filter != null }, |
There was a problem hiding this comment.
This replaces the workaround with FilterDefinitionRenderContext, which is removed now.
| var renderedField = _field.Render(sourceSerializer, serializerRegistry, linqProvider); | ||
| var sliceArgs = argsRenderer(renderedField.FieldName); | ||
| var renderedField = _field.Render(renderArgs); | ||
| var sliceArgs = renderArgs.IsAggregateMode ? RenderArgs(renderedField.FieldName) : RenderArgsForFind(renderedField.FieldName); |
There was a problem hiding this comment.
IsAggregateMode replaces RenderForFind pattern.
src/MongoDB.Driver/RenderArgs.cs
Outdated
| @@ -0,0 +1,103 @@ | |||
| using MongoDB.Bson.Serialization; | |||
src/MongoDB.Driver/RenderArgs.cs
Outdated
| bool renderDollarForm = default, | ||
| bool isAggregateMode = true) | ||
| { | ||
| DocumentSerializer = documentSerializer; |
src/MongoDB.Driver/RenderArgs.cs
Outdated
| /// <summary> | ||
| /// Gets the document serializer. | ||
| /// </summary> | ||
| public IBsonSerializer<TDocument> DocumentSerializer { get => _documentSerializer; init => _documentSerializer = value; } |
There was a problem hiding this comment.
Good point, that was initial implementation. But in case of ArrayFilterDefinition null is passed for serializer.
But I think we can make an exception for ArrayFilterDefinition and not use RenderArgs there.
Added null check.
src/MongoDB.Driver/RenderArgs.cs
Outdated
| /// or resolves <c>IBsonSerializer{T}</c> from <see cref="SerializerRegistry"/>. | ||
| /// </summary> | ||
| public IBsonSerializer<T> GetSerializer<T>() => | ||
| DocumentSerializer as IBsonSerializer<T> ?? SerializerRegistry.GetSerializer<T>(); |
There was a problem hiding this comment.
This seems dangerous. There is no guarantee that the serializer in the registry is the right one for THIS builder.
There was a problem hiding this comment.
This is a convenience method, that replaces same functionality in lots of places.
src/MongoDB.Driver/RenderArgs.cs
Outdated
| /// <returns> | ||
| /// A new RenderArgs{TNewDocument} instance. | ||
| /// </returns> | ||
| public RenderArgs<TNewDocument> WithSerializer<TNewDocument>(IBsonSerializer<TNewDocument> serializer) => |
There was a problem hiding this comment.
At first I didn't realize why you weren't using the language support for with.
Maybe emphasize in the doc comments that we aren't just changing the serializer but most likely changing the TDocument type also.
Maybe even rename this method to something like WithNewDocumentType?
There was a problem hiding this comment.
Thanks, renamed to WithNewDocumentType.
