CSHARP-4495: Add conventions and attributes to configure ObjectSerial… #1545

papafe · 2024-11-15T12:25:43Z

…izer AllowedTypes

papafe · 2024-11-15T12:30:57Z

This is an incredibly quick and dirty proof of concept for a new convention that allows users to define the allowed types for serialization/deserialization in multiple ways:

passing a delegate
passing a list of types
passing a list of accepted assemblies
passing nothing, and this would be the same as passing the calling assembly

I have also thought if it would make sense to give users the possibility of setting the allowed types with attributes, but I suppose that most of the times that list of types would be valid for the whole project, not only for certain POCOs.

As said, this just a very fast POC, so there's a lot missing, but it's to get some initial feedback.

rstam

Everything looks very reasonable!

rstam · 2024-11-15T18:35:02Z

src/MongoDB.Bson/Serialization/Conventions/AllowedTypesConvention.cs

+        /// <param name="allowedSerializationTypes"></param>
+        public AllowedTypesConvention(IEnumerable<Type> allowedDeserializationTypes, IEnumerable<Type> allowedSerializationTypes)
+        {
+            var allowedDeserializationTypesArray = allowedDeserializationTypes as Type[] ?? allowedDeserializationTypes.ToArray();


Interesting optimization...

However... we should probably make a defensive copy anyway because the caller might alter the array they passed in later.

Yes, we should just make a copy

src/MongoDB.Bson/Serialization/Conventions/AllowedTypesConvention.cs

rstam · 2024-11-15T18:46:11Z

src/MongoDB.Bson/Serialization/Conventions/AllowedTypesConvention.cs

+        ///
+        /// </summary>
+#pragma warning disable CA1044
+        public bool AllowDefaultFrameworkTypes


I like the idea of giving them a way to opt-in the default framework types.

src/MongoDB.Bson/Serialization/Conventions/AllowedTypesConvention.cs

papafe · 2024-11-18T10:36:43Z

@rstam This is ready for review now. Apart from all your notes, I've also allowed the convention to work with collections and nested collection (as we did with the EnumConvention).

BorisDog

Looks good overall

.../MongoDB.Bson.Tests/Serialization/Conventions/ObjectSerializerAllowedTypesConventionTests.cs

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

BorisDog · 2024-11-19T18:40:42Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        /// Initializes a new instance of the <see cref="ObjectSerializerAllowedTypesConvention"/> class
+        /// that allows all types contained in the calling assembly.
+        /// </summary>
+        public ObjectSerializerAllowedTypesConvention() : this(Assembly.GetCallingAssembly())


I think we should define that the default behavior does not configure any allowed types (except for DefaultFrameworkAllowedTypes). So the configuration needs to be explicit.

Makes sense

I have removed the automatic add of the types in the calling assembly and I have made AllowDefaultFrameworkTypes default to true, similar to the behaviour of the ObjectSerializer. For this reason now the delegates that are passed to the ObjectSerializer are built lazily.

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

papafe · 2024-11-20T12:42:15Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+                        ? ObjectSerializer.DefaultAllowedTypes
+                        : t => _allowedDeserializationTypes(t) || ObjectSerializer.DefaultAllowedTypes(t)
+                    : _allowedDeserializationTypes ?? ObjectSerializer.NoAllowedTypes;
+            });


Hope that this is readable enough

I think it's fine. minor optimization possible:

_lazyAllowedDeserializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedDeserializationTypes)); _lazyAllowedSerializationTypes= new Lazy<Func<Type, bool>>(() => Construct(_allowedSerializationTypes)); Func<Type, bool> Construct(Func<Type, bool> allowedTypes) => AllowDefaultFrameworkTypes ? (allowedTypes != null ? t => allowedTypes(t) || ObjectSerializer.DefaultAllowedTypes) : ObjectSerializer.DefaultAllowedTypes : allowedTypes ?? ObjectSerializer.NoAllowedTypes;

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

BorisDog · 2024-11-20T20:50:50Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+                        ? ObjectSerializer.DefaultAllowedTypes
+                        : t => _allowedDeserializationTypes(t) || ObjectSerializer.DefaultAllowedTypes(t)
+                    : _allowedDeserializationTypes ?? ObjectSerializer.NoAllowedTypes;
+            });


I think it's fine. minor optimization possible:

_lazyAllowedDeserializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedDeserializationTypes)); _lazyAllowedSerializationTypes= new Lazy<Func<Type, bool>>(() => Construct(_allowedSerializationTypes)); Func<Type, bool> Construct(Func<Type, bool> allowedTypes) => AllowDefaultFrameworkTypes ? (allowedTypes != null ? t => allowedTypes(t) || ObjectSerializer.DefaultAllowedTypes) : ObjectSerializer.DefaultAllowedTypes : allowedTypes ?? ObjectSerializer.NoAllowedTypes;

BorisDog · 2024-11-20T20:58:13Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+    /// <summary>
+    /// A convention that allows to set the types that can be safely serialized and deserialized with the <see cref="ObjectSerializer"/>.
+    /// </summary>
+    public sealed class ObjectSerializerAllowedTypesConvention


Do we want to have convenience presets, like:

ObjectSerializerAllowedTypesConvention.AllowAllTypes ObjectSerializerAllowedTypesConvention.AllowAllExecutingAssemblyTypes ObjectSerializerAllowedTypesConvention.DefaultAllowedTypes ...

?

I think that would make sense

This class needs to implement IMemberMapConvention.

Follow the pattern from other similar classes:

public sealed class ObjectSerializerAllowedTypesConvention : ConventionBase, IMemberMapConvention

To verify why implementing IMemberMapConvention is required try writing a test that registers this convention and see if it gets used during automapping.

This test will have to be disabled and only run manually in isolation, because registering this convention globally would affect other tests.

.../MongoDB.Bson.Tests/Serialization/Conventions/ObjectSerializerAllowedTypesConventionTests.cs

papafe · 2024-11-21T07:43:25Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        /// <summary>
+        /// Default <see cref="ObjectSerializerAllowedTypesConvention"/> where all calling assembly types and default framework types are allowed for both serialization and deserialization.
+        /// </summary>
+        public static ObjectSerializerAllowedTypesConvention AllowAllCallingAssemblyAndDefaultFrameworkTypes => new(Assembly.GetCallingAssembly());


Not sure if it makes sense to have both AllowAllCallingAssemblyAndDefaultFrameworkTypes and AllowAllCallingAssemblyTypes or keep only the second one (but then we need to decide if the default framework types are included or not)

I think we need only need AllowAllCallingAssemblyAndDefaultFrameworkTypes

BorisDog · 2024-11-21T23:47:27Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        /// <summary>
+        /// Default <see cref="ObjectSerializerAllowedTypesConvention"/> where all calling assembly types and default framework types are allowed for both serialization and deserialization.
+        /// </summary>
+        public static ObjectSerializerAllowedTypesConvention AllowAllCallingAssemblyAndDefaultFrameworkTypes => new(Assembly.GetCallingAssembly());


I think we need only need AllowAllCallingAssemblyAndDefaultFrameworkTypes

BorisDog · 2024-11-21T23:47:48Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        {
+            _lazyAllowedDeserializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedDeserializationTypes));
+            _lazyAllowedSerializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedSerializationTypes));
+            return;


no need for return

I agree, it's mostly to separate "visually" the method body from the local functions. I'm not super sold either way to be honest, so I'll remove it

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

rstam · 2024-11-25T22:20:30Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        public ObjectSerializerAllowedTypesConvention()
+        {
+            _lazyAllowedDeserializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedDeserializationTypes));
+            _lazyAllowedSerializationTypes = new Lazy<Func<Type, bool>>(() => Construct(_allowedSerializationTypes));


Are we sure that the values of _allowedDeserialization and _allowedSerializationTypes have already been stored when this code fetches them?

The Func is built when it's used the first time, so the variables will be set by then

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

…izer AllowedTypes

rstam · 2024-11-27T23:20:01Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+    /// <summary>
+    /// A convention that allows to set the types that can be safely serialized and deserialized with the <see cref="ObjectSerializer"/>.
+    /// </summary>
+    public sealed class ObjectSerializerAllowedTypesConvention


This class needs to implement IMemberMapConvention.

Follow the pattern from other similar classes:

public sealed class ObjectSerializerAllowedTypesConvention : ConventionBase, IMemberMapConvention

rstam · 2024-11-27T23:21:23Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+    /// <summary>
+    /// A convention that allows to set the types that can be safely serialized and deserialized with the <see cref="ObjectSerializer"/>.
+    /// </summary>
+    public sealed class ObjectSerializerAllowedTypesConvention


To verify why implementing IMemberMapConvention is required try writing a test that registers this convention and see if it gets used during automapping.

This test will have to be disabled and only run manually in isolation, because registering this convention globally would affect other tests.

rstam · 2024-11-27T23:22:19Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        /// A predefined <see cref="ObjectSerializerAllowedTypesConvention"/> where no types are allowed for both serialization and deserialization.
+        /// </summary>
+        public static ObjectSerializerAllowedTypesConvention AllowNoTypes { get; } = new(ObjectSerializer.NoAllowedTypes)
+            { AllowDefaultFrameworkTypes = false };


This formatting threw me off. I didn't realize at first that the line was continued on the next line.

Maybe this way:

public static ObjectSerializerAllowedTypesConvention AllowNoTypes { get; } = new(ObjectSerializer.NoAllowedTypes) { AllowDefaultFrameworkTypes = false };

rstam · 2024-11-27T23:25:02Z

src/MongoDB.Bson/Serialization/Conventions/ObjectSerializerAllowedTypesConvention.cs

+        private readonly Lazy<Func<Type, bool>> _effectiveAllowedDeserializationTypes;
+        private readonly Lazy<Func<Type, bool>> _effectiveAllowedSerializationTypes;
+
+        private readonly bool _allowDefaultFrameworkTypes = true;


Consider placing line 61 immediately after line 56.

papafe · 2024-11-28T11:10:11Z

tests/MongoDB.Bson.Tests/Serialization/Conventions/MemberDefaultValueConventionTests.cs

        [Fact]
        public void TestMappingUsesMemberDefaultValueConvention()
        {
+            var conventionName = Guid.NewGuid().ToString();


I found these tests that were registering conventions but never unregistering them

This was not a problem because of t => t == typeof(A)

papafe · 2024-11-28T11:11:46Z

.../MongoDB.Bson.Tests/Serialization/Conventions/ObjectSerializerAllowedTypesConventionTests.cs

+        }
+
+        [Fact]
+        public void Convention_should_be_applied_during_automapping()


@rstam I've added this test to verify that the convention gets registered during automap. I know you've said we should only run it manually, but can't we register the convention and unregister it at the end of the test like here?

As i commented above I'm surprised we even have a method to unregister a convention. You shouldn't be able to alter conventions in the middle of a program run.

We can either unregister it as you discovered (even though I didn't think you should be able to do that), or you can simply program the convention to only apply to the class in this test and just leave it registered forever.

rstam · 2024-12-01T00:09:56Z

tests/MongoDB.Bson.Tests/Serialization/Conventions/MemberDefaultValueConventionTests.cs

            Assert.IsType<int>(defaultValue);
            Assert.Equal(1, defaultValue);
+
+            ConventionRegistry.Remove(conventionName);


Wow I am surprised the Remove method even exists.

A general principle of our whole serialization configuration machinery is that you shouldn't be able to change how things are serialized in the middle of the program run.

Though I guess this Remove method is somewhat less harmful because it doesn't change how anything that is already configured is serialized, only how future classes that have not yet been configured will end up being configured after the convention has been removed.

Note also that this convention does not HAVE to be unregistered. It is already programmed to ONLY apply to this one class with t => t == typeof(A).

I understand we could have a convention that applies to a certain type and leave it registered, and while this likely does not affect anything else, shouldn’t we aim to keep the tests as independent as possible?

Yes, tests should be as independent as possible. That's why we should use t => t == typeof(A) instead of t => true here.

It's fine if you want to unregister this convention at the end of the test. I just wanted to make the following points:

The test was not inherently wrong because the registered convention only applied to this test

I was surprised we even had a Remove method

The Remove method concerns me because the way serialization is supposed to work is that an application configures serialization at startup and then leaves it alone. Removing a convention at some later time sounds like it would destabilize the application because POCOs might be serialized differently depending on whether the POCO is mapped before or after the convention was removed.

I agree with all the points :)
I modified the predicate to be more restrictive, but kept the Remove to have a clean state after each test. I can agree also that maybe that method should not be there, but for now I'd say it's better to use it

rstam · 2024-12-01T00:14:04Z

.../MongoDB.Bson.Tests/Serialization/Conventions/ObjectSerializerAllowedTypesConventionTests.cs

+        }
+
+        [Fact]
+        public void Convention_should_be_applied_during_automapping()


As i commented above I'm surprised we even have a method to unregister a convention. You shouldn't be able to alter conventions in the middle of a program run.

We can either unregister it as you discovered (even though I didn't think you should be able to do that), or you can simply program the convention to only apply to the class in this test and just leave it registered forever.

rstam · 2024-12-01T00:15:25Z

.../MongoDB.Bson.Tests/Serialization/Conventions/ObjectSerializerAllowedTypesConventionTests.cs

+            var conventionName = Guid.NewGuid().ToString();
+
+            var subject = new ObjectSerializerAllowedTypesConvention(Assembly.GetExecutingAssembly());
+            ConventionRegistry.Register(conventionName, new ConventionPack {subject}, _ => true);


Can't we use something more restrictive that t => true?

If we think about a time in the future where we want to run our tests in parallel relying on unregistering will no longer work.

rstam · 2024-12-01T00:18:18Z

tests/MongoDB.Bson.Tests/Serialization/Conventions/MemberDefaultValueConventionTests.cs

        [Fact]
        public void TestMappingUsesMemberDefaultValueConvention()
        {
+            var conventionName = Guid.NewGuid().ToString();


This was not a problem because of t => t == typeof(A)

rstam

LGTM

papafe requested review from BorisDog and rstam November 15, 2024 12:25

rstam requested changes Nov 15, 2024

View reviewed changes

papafe marked this pull request as ready for review November 18, 2024 10:35

papafe requested a review from a team as a code owner November 18, 2024 10:35

papafe requested a review from rstam November 18, 2024 10:35

BorisDog requested changes Nov 19, 2024

View reviewed changes

papafe requested a review from BorisDog November 20, 2024 12:41

papafe commented Nov 20, 2024

View reviewed changes

BorisDog reviewed Nov 21, 2024

View reviewed changes

papafe commented Nov 21, 2024

View reviewed changes

papafe requested a review from BorisDog November 21, 2024 07:45

BorisDog reviewed Nov 21, 2024

View reviewed changes

rstam requested changes Nov 25, 2024

View reviewed changes

papafe added 14 commits November 26, 2024 10:34

CSHARP-4495: Add conventions and attributes to configure ObjectSerial…

a35b323

…izer AllowedTypes

Added basic functionality

446c778

Various corrections

5948efd

Added recursion

de1857a

Improved performance

a887d58

Small correction

738f196

Removed unused

8eaebc4

Small correction

ebc8e65

Small corrections

d64b390

Various corrections

5be94d4

Corrected spacing

d0a841c

Small improvements

11af5a7

Added default presets

b2e78fa

Added preset

2d100db

papafe added 2 commits November 26, 2024 10:34

Small corrections

6da9373

Corrections according to PR

60c4bea

papafe force-pushed the csharp4495 branch from a38c892 to 60c4bea Compare November 26, 2024 09:34

papafe requested a review from rstam November 26, 2024 09:35

papafe added 2 commits November 27, 2024 11:18

Final corrections

b61480e

Name correction

2841546

rstam requested changes Nov 27, 2024

View reviewed changes

Small corrections

8556a76

papafe requested a review from rstam November 28, 2024 11:09

papafe commented Nov 28, 2024

View reviewed changes

rstam requested changes Dec 1, 2024

View reviewed changes

Small corrections

e6571cf

papafe requested a review from rstam December 3, 2024 13:49

rstam approved these changes Dec 5, 2024

View reviewed changes

papafe merged commit 3fd8923 into mongodb:main Dec 6, 2024
26 of 30 checks passed

papafe deleted the csharp4495 branch December 6, 2024 12:21

CSHARP-4495: Add conventions and attributes to configure ObjectSerial… #1545

CSHARP-4495: Add conventions and attributes to configure ObjectSerial… #1545

Uh oh!

Conversation

papafe commented Nov 15, 2024

Uh oh!

papafe commented Nov 15, 2024

Uh oh!

rstam left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

papafe commented Nov 18, 2024

Uh oh!

BorisDog left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

papafe Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papafe Nov 22, 2024 •

edited

Loading