CSHARP-2150 Add check that the serializer's ValueType matches the type when registering the serializer#1929
Conversation
kyra-rk
added
the
improvement
|
After our standup discussion, I checked whether a serializer registered for a base type works correctly for a derived type — it does (see new test added), so I changed the type equality check to |
papafe
left a comment
papafe
left a comment
There was a problem hiding this comment.
Overall it looks good, some minor comments.
| { | ||
| public override void Serialize(BsonSerializationContext context, BsonSerializationArgs args, IEnumerable<Person> value) | ||
| { | ||
| context.Writer.WriteStartArray(); |
There was a problem hiding this comment.
We're not actually using this implementation, just throw a NotImplementedException.
There was a problem hiding this comment.
Nice catch! Will fix
| var tryException = Record.Exception(() => subject.TryRegisterSerializer(typeof(long), intSerializer)); | ||
|
|
||
| tryException.Should().BeOfType<BsonSerializationException>(); | ||
| tryException.Message.Should().Contain("A serializer for Int32 cannot be registered for type Int64."); |
There was a problem hiding this comment.
If you use the full string comparison it should use Be instead of Contain
papafe
left a comment
papafe
left a comment
There was a problem hiding this comment.
LGTM! Just one minor thing
| subject.GetSerializer(typeof(List<Person>)).Should().BeSameAs(serializer); | ||
| } | ||
|
|
||
| private class Person |
There was a problem hiding this comment.
Sorry for noticing before, usually we keep utility class and other utility methods after all the test methods, so at the end of the whole test class.
There was a problem hiding this comment.
No worries, will change it!
3 participants
RegisterSerializerandTryRegisterSerializernow throw anArgumentExceptionif the serializer'sValueTypeis not compatible with the registered type (e.g. registering anInt32Serializerfortypeof(long)). Serializers registered for a base type remain valid for derived types.