CSHARP-5226: Remove non-RenderArgs Render overloads #1417
Conversation
BorisDog
requested review from
JamesKovacs and
rstam
and removed request for
a team and
JamesKovacs
| false); // renderForElemMatch | ||
|
|
||
| var renderMethod = filterDefinitionType.GetMethod(nameof(FilterDefinition<BsonDocument>.Render), new[] { renderArgsType }); | ||
| var renderedFilter = (BsonDocument)renderMethod.Invoke(filterDefinition, new object[] { renderArgs }); |
There was a problem hiding this comment.
@rstam: Just discovered this dynamic rendering due to failing tests.
There was a problem hiding this comment.
I propose adding a new private static helper method:
// private static methods
private static BsonDocument RenderFilter<TDocument>(FilterDefinition<TDocument> filterDefinition, IBsonSerializer<TDocument> documentSerializer, IBsonSerializerRegistry serializerRegistry)
{
return filterDefinition.Render(new(documentSerializer, serializerRegistry));
}
and replacing lines 44-55 with:
var renderFilterMethod = typeof(InjectMethodToFilterTranslator).GetMethod(nameof(RenderFilter), BindingFlags.Static | BindingFlags.NonPublic).MakeGenericMethod(documentType);
var renderedFilter = (BsonDocument)renderFilterMethod.Invoke(null, new object[] { filterDefinition, documentSerializer, serializerRegistry });
This localizes the use of reflection to this file only and isolates us from (or at least turns into compile time errors) changes to the RenderArgs constructor or the signature of the Render method.
There was a problem hiding this comment.
Great idea! Done, thanks.
| false); // renderForElemMatch | ||
|
|
||
| var renderMethod = filterDefinitionType.GetMethod(nameof(FilterDefinition<BsonDocument>.Render), new[] { renderArgsType }); | ||
| var renderedFilter = (BsonDocument)renderMethod.Invoke(filterDefinition, new object[] { renderArgs }); |
There was a problem hiding this comment.
I propose adding a new private static helper method:
// private static methods
private static BsonDocument RenderFilter<TDocument>(FilterDefinition<TDocument> filterDefinition, IBsonSerializer<TDocument> documentSerializer, IBsonSerializerRegistry serializerRegistry)
{
return filterDefinition.Render(new(documentSerializer, serializerRegistry));
}
and replacing lines 44-55 with:
var renderFilterMethod = typeof(InjectMethodToFilterTranslator).GetMethod(nameof(RenderFilter), BindingFlags.Static | BindingFlags.NonPublic).MakeGenericMethod(documentType);
var renderedFilter = (BsonDocument)renderFilterMethod.Invoke(null, new object[] { filterDefinition, documentSerializer, serializerRegistry });
This localizes the use of reflection to this file only and isolates us from (or at least turns into compile time errors) changes to the RenderArgs constructor or the signature of the Render method.
rstam
left a comment
rstam
left a comment
There was a problem hiding this comment.
Some naming fell through the cracks.
| { | ||
| internal static class InjectMethodToFilterTranslator | ||
| { | ||
| private readonly static MethodInfo __renderMethodInfo; |
There was a problem hiding this comment.
I like this optimization of caching the MethodInfo in a static field.
But the field should be named __renderFilterMethodInfo.
| var renderMethod = filterDefinitionType.GetMethod("Render", renderMethodArgumentTypes); | ||
| var renderedFilter = (BsonDocument)renderMethod.Invoke(filterDefinition, new object[] { documentSerializer, serializerRegistry }); | ||
|
|
||
| var renderMethod = __renderMethodInfo.MakeGenericMethod(documentType); |
There was a problem hiding this comment.
variable should be called renderFilterMethod
3 participants
No description provided.