New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSHARP-4955: Add Render overload with RenderArgs #1278
base: master
Are you sure you want to change the base?
Conversation
BorisDog
commented
Feb 29, 2024
- Added support for records (IsExternalInit.cs)
- Added RenderArgs overloads
- Removed RenderForFind and FilterDefinitionRenderContext workarounds
- Added obsoletion attributes for older Render methods
namespace System.Runtime.CompilerServices | ||
{ | ||
[EditorBrowsable(EditorBrowsableState.Never)] | ||
internal class IsExternalInit { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsAggregateMode
replaces RenderForFind
pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with a quick review and some initial questions.
src/MongoDB.Driver/RenderArgs.cs
Outdated
@@ -0,0 +1,103 @@ | |||
using MongoDB.Bson.Serialization; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add copyright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Done.
src/MongoDB.Driver/RenderArgs.cs
Outdated
bool renderDollarForm = default, | ||
bool isAggregateMode = true) | ||
{ | ||
DocumentSerializer = documentSerializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure.IsNotNull?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure.IsNotNull in init?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, renamed to WithNewDocumentType
.