CSHARP-4475: Add an AllowedTypes filter to ObjectSerializer. #1008

rstam · 2023-01-06T22:03:59Z

No description provided.

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

src/MongoDB.Bson/Serialization/Serializers/DictionarySerializerBase.cs

src/MongoDB.Bson/Serialization/Serializers/DiscriminatedInterfaceSerializer.cs

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

rstam · 2023-01-06T22:09:35Z

tests/MongoDB.Bson.Tests/Jira/CSharp1559Tests.cs

+        static CSharp1559Tests()
+        {
+            TestObjectSerializerRegisterer.EnsureTestObjectSerializerIsRegistered();
+        }


This static constructor here and similarly in many classes below ensures that the correct ObjectSerializer is registered before running these tests.

I like the _

Turns out that if you don't actually need to use the fixture in your tests you can just omit this constructor.

We're only using the collection fixture for its side effect of registering the object serializer.

I'm going to remove all these constructors.

rstam · 2023-01-06T22:12:33Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DictionaryGenericSerializerTests.cs

+                cm.MapProperty(t => t.ROD).SetSerializer(readOnlyDictionarySerializer);
+                cm.MapProperty(t => t.SD).SetSerializer(sortedDictionarySerializer);
+                cm.MapProperty(t => t.SL).SetSerializer(sortedListSerializer);
+            });


The problem here is that while line 43 ensures that the correct ObjectSerializer is registered, line 43 doesn't help with any Dictionary (and similar) serializers that might already be registered and that have cached the wrong ObjectSerializer (as the key and value serializers) before line 43 was executed.

The workaround is to manually configure a class map for T that has all the correct serializers regardless of what may or may not already be in the registry.

Could do this whole setup in RegisterObjectSerializerFixture too?
Replace dictionarySerializer and the rest for all?

Originally this was done here because this was the only test affected by this.

But it does make sense to do it RegisterObjectSerializerFixture.

There is a problem though... we can't move the class map registration to the fixture.

I'm going to leave this alone for now. Think about it a bit more and let me know if you want to pursue it further.

Maybe we could just register serializers for Dictionary<>, IDictionary<> like we do with ObjectSerializer and skip the mapping?

We could register those serializers also in the fixture, but... they only apply to this one test. Note also, that they are not registered... only used in this one test.

Let's leave this alone for now.

When we add new conventions and attributes we can remove the code that creates these serializers and maps the class in code and let the conventions/attributes take care of mapping correctly.

rstam · 2023-01-06T22:12:51Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DictionarySerializerTests.cs

+                cm.MapProperty(t => t.SL).SetSerializer(sortedListDictionarySerializer);
+            });
+        }
+


Similar issue as above.

tests/MongoDB.Bson.Tests/Serialization/Serializers/ObjectSerializerTests.cs

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

BorisDog · 2023-01-10T19:06:47Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

+
+                static bool IsAllowedGenericType(Type type) =>
+                    __allowedGenericTypes.Contains(type.GetGenericTypeDefinition()) &&
+                    type.GetGenericArguments().All(a => AllowedTypes(a));


type.GetGenericArguments().All(AllowedTypes)

Good optimization. Done.

BorisDog · 2023-01-11T16:26:48Z

src/MongoDB.Bson/Serialization/BsonSerializerRegistry.cs

+        {
+            if (type == null)
+            {
+                throw new ArgumentNullException("type");


nameof(...)

Done.

For the record this was just copy pasted from existing code.

BorisDog · 2023-01-11T16:39:28Z

src/MongoDB.Bson/Serialization/BsonSerializerRegistry.cs

+                var existingSerializer = _cache[type];
+                if (!existingSerializer.Equals(serializer))
+                {
+                    var message = string.Format("There is already a different serializer registered for type {0}.", BsonUtils.GetFriendlyTypeName(type));


Maybe we can use string interpolation here and elsewhere?

Sure, I like string interpolation a lot.

This was just copy/pasted from existing code and adapted.

I will change it to string interpolation here in this new code, but I'm reluctant to change existing code.

src/MongoDB.Bson/Serialization/BsonSerializerRegistry.cs

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

tests/MongoDB.Bson.TestHelpers/RegisterObjectSerializerFixture.cs

BorisDog · 2023-01-11T16:44:29Z

tests/MongoDB.Bson.Tests/Jira/CSharp1559Tests.cs

+        static CSharp1559Tests()
+        {
+            TestObjectSerializerRegisterer.EnsureTestObjectSerializerIsRegistered();
+        }


I like the _

BorisDog · 2023-01-11T16:47:22Z

tests/MongoDB.Bson.Tests/RegisterObjectSerializerCollection.cs

+
+namespace MongoDB.Bson.Tests
+{
+    [CollectionDefinition("RegisterObjectSerializer")]


I would recommend defining a const for "RegisterObjectSerializer".
Maybe RegisterObjectSerializerFixture.CollectionName

BorisDog · 2023-01-11T16:56:29Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DictionaryGenericSerializerTests.cs

+                cm.MapProperty(t => t.ROD).SetSerializer(readOnlyDictionarySerializer);
+                cm.MapProperty(t => t.SD).SetSerializer(sortedDictionarySerializer);
+                cm.MapProperty(t => t.SL).SetSerializer(sortedListSerializer);
+            });


Could do this whole setup in RegisterObjectSerializerFixture too?
Replace dictionarySerializer and the rest for all?

JamesKovacs

LGTM

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

BorisDog · 2023-01-23T23:50:47Z

tests/MongoDB.Bson.Tests/Serialization/Serializers/DictionaryGenericSerializerTests.cs

+                cm.MapProperty(t => t.ROD).SetSerializer(readOnlyDictionarySerializer);
+                cm.MapProperty(t => t.SD).SetSerializer(sortedDictionarySerializer);
+                cm.MapProperty(t => t.SL).SetSerializer(sortedListSerializer);
+            });


rstam · 2023-01-25T17:43:26Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

@@ -165,6 +217,20 @@ public override object Deserialize(BsonDeserializationContext context, BsonDeser
            }
        }

+        /// <inheritdoc/>
+        public override bool Equals(object obj) =>
+            obj is ObjectSerializer other &&


For non-sealed classes obj is ObjectSerializer is not a good enough test because obj might be a subclass of ObjectSerializer in which case we want to return false.

Because ObjectSerializer is sealed we know that if obj is an ObjectSerializer it can't be a subclass of ObjectSerializer.

You could add simple type as alternative to sealed (might be safer in terms of BC).

obj is ObjectSerializer other && this.GetType() == other.GetType()

But I would not worry about this. The choice on whether override Equals method or not should be of the derived type.

To avoid the sealed breaking change, would the following not be sufficient?

obj is ObjectSerializer other && GetType() == obj.GetType() &&

I think implementing Equals in a class hierarchy is non-trivial. If a derived class fails to override Equals then Equals is broken. And if a derived class does override Equals (as it must, I don't think it's an option), then does it leverage the base class by calling base.Equals? But for that to work the base Equals must be implemented to allow subclasses to call it and not return false unintentionally.

I'm fine with adding the extra check... and will do so, but we should note that implementing Equals for sealed class allows some assumptions that simplify the implementation.

rstam · 2023-01-25T17:45:16Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

+        /// <inheritdoc/>
+        public override int GetHashCode() =>
+            new Hasher()
+            .Hash(_discriminatorConvention)


Deliberately left _alowedTypes out of the computation of the hash code because we don't know anything about the behavior of GetHashCode for delegates (and it's not worth finding out).

Technically public override int GetHashCode() => 0 would be correct also if we don't anticipate serializers being put into a hash table (and even if they are 0 would just be a small performance hit).

I agree with your remark that GetHashCode() => 0 would work, as this class is not designed to be used for lookup in hash tables.
Would not => base.GetHashCode() be sufficient then?
The default implementation calculates the hash based on reference, so I would recommend to offload this to .net framework.

Would not => base.GetHashCode() be sufficient then?

I think relying on the base implementation of GetHashCode would be a bug.

If Equals returns true for x and y then x.GetHashCode() MUST EQUAL y.GetHashCode(). Calling base.GetHashCode() violates that.

A hash code of 0 is never "wrong", but it also isn't very efficient in cases where a random distribution of hashcodes would be better.

If Equals returns true for x and y then x.GetHashCode() MUST EQUAL y.GetHashCode(). Calling base.GetHashCode() violates that.

I think this only applies to cases where Hashcode is actually used, which is lookups. But I agree that it's a "bon ton" not to violate this in any case.

I think 0 is a good option, to state that "we don't care", otherwise using Hasher leads to wrong impression that this class is designed and optimized to be used for lookups.

rstam · 2023-01-25T17:46:36Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs


 namespace MongoDB.Bson.Serialization.Serializers
 {
    /// <summary>
    /// Represents a serializer for objects.
    /// </summary>
-    public class ObjectSerializer : ClassSerializerBase<object>
+    public sealed class ObjectSerializer : ClassSerializerBase<object>


Changed to sealed in part to simplify the implementation of Equals.

Any concern that this could be considered a breaking change?

It will break anyone with a IBsonSerializer<T> deriving from ObjectSerializer. A quick google for mongodb objectserializer returns our API docs and a few SO questions, but no one recommending deriving a new serializer from ObjectSerializer. It's probably not common, but we can't know for certain.

We can avoid this by checking other.GetType()

Yes. But if this class is sealed we don't need to do that.

Removing sealed to avoid possible though unlikely breaking change.

rstam · 2023-01-25T17:47:24Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

@@ -165,6 +217,20 @@ public override object Deserialize(BsonDeserializationContext context, BsonDeser
            }
        }

+        /// <inheritdoc/>


There is another ticket to implement Equals for all serializers.

In this PR I'm only implementing Equals for ObjectSerializer because it is required for the tests of TryRegisterSerializer to pass.

rstam · 2023-01-25T17:48:42Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

+            obj is ObjectSerializer other &&
+            _allowedTypes.Equals(other._allowedTypes) &&
+            _discriminatorConvention.Equals(other._discriminatorConvention) &&
+            _guidRepresentation == other._guidRepresentation;


I firmly believe Equals should be implemented by calling Equals on fields in order to preserve the semantics of Equals (without being forced to analyze whether == is semantically equivalent to Equals or not on a case by case basis).

But... in the case of enum fields calling Equals would involve boxing. It would still be correct, but there would be a small performance hit.

JamesKovacs

While I suspect that no one has subclassed ObjectSerializer there is no way to know for sure. See my suggested change to Equals avoids the need for this breaking change.

JamesKovacs · 2023-01-25T23:09:48Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs


 namespace MongoDB.Bson.Serialization.Serializers
 {
    /// <summary>
    /// Represents a serializer for objects.
    /// </summary>
-    public class ObjectSerializer : ClassSerializerBase<object>
+    public sealed class ObjectSerializer : ClassSerializerBase<object>


It will break anyone with a IBsonSerializer<T> deriving from ObjectSerializer. A quick google for mongodb objectserializer returns our API docs and a few SO questions, but no one recommending deriving a new serializer from ObjectSerializer. It's probably not common, but we can't know for certain.

JamesKovacs · 2023-01-25T23:16:32Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

@@ -165,6 +217,20 @@ public override object Deserialize(BsonDeserializationContext context, BsonDeser
            }
        }

+        /// <inheritdoc/>


JamesKovacs · 2023-01-25T23:27:16Z

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs

@@ -165,6 +217,20 @@ public override object Deserialize(BsonDeserializationContext context, BsonDeser
            }
        }

+        /// <inheritdoc/>
+        public override bool Equals(object obj) =>
+            obj is ObjectSerializer other &&


To avoid the sealed breaking change, would the following not be sufficient?

obj is ObjectSerializer other && GetType() == obj.GetType() &&

JamesKovacs

LGTM

BorisDog

LGTM.
Comment re. GetHashcode for your consideration.

rstam commented Jan 6, 2023

View reviewed changes

rstam requested a review from JamesKovacs January 6, 2023 22:17

BorisDog reviewed Jan 11, 2023

View reviewed changes

rstam requested a review from BorisDog January 11, 2023 01:32

BorisDog reviewed Jan 11, 2023

View reviewed changes

rstam requested a review from BorisDog January 12, 2023 15:26

JamesKovacs approved these changes Jan 14, 2023

View reviewed changes

src/MongoDB.Bson/Serialization/Serializers/BsonClassMapSerializer.cs Show resolved Hide resolved

src/MongoDB.Bson/Serialization/Serializers/ObjectSerializer.cs Show resolved Hide resolved

BorisDog requested changes Jan 23, 2023

View reviewed changes

rstam commented Jan 25, 2023

View reviewed changes

rstam requested review from BorisDog and JamesKovacs January 25, 2023 17:50

JamesKovacs requested changes Jan 25, 2023

View reviewed changes

rstam requested a review from JamesKovacs January 26, 2023 00:10

JamesKovacs approved these changes Jan 26, 2023

View reviewed changes

BorisDog approved these changes Jan 26, 2023

View reviewed changes

CSHARP-4475: Add an AllowedTypes filter to ObjectSerializer.

790f123

rstam force-pushed the csharp4475 branch from 5a2593b to 790f123 Compare January 26, 2023 04:54

rstam merged commit 790f123 into mongodb:master Jan 26, 2023

CSHARP-4475: Add an AllowedTypes filter to ObjectSerializer. #1008

CSHARP-4475: Add an AllowedTypes filter to ObjectSerializer. #1008

Uh oh!

Conversation

rstam commented Jan 6, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!