CSHARP-5442: Atlas Search doesn't respect the correct serializer #1583

papafe · 2024-12-31T14:56:55Z

papafe · 2024-12-31T15:45:03Z

@BorisDog I've added you as a reviewer since it seems you were the one that worked on search, mostly. This is still a work in progress, I've broke some things in the process.
A couple of notes:

Probably we need to change the in and range search definitions as well to use the correct serializers.
This is effectively a behaviour breaking change, so we need to decide if we want include it in next version or wait for next major

papafe · 2025-01-02T11:20:23Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+
+                var renderedField = _arrayField.Render(args);
+
+                var serializer =


Incredibly ugly, but was just testing if it worked

@rstam this is what I was referring to.
Do you think it would be worth adding a new property to RenderArgs so that we can get directly the serializer we need (similar to RenderForElemMatch for instance)?

I think this is on the right track but can use a little tweaking to how the right serializer is found and to reduce duplication between how scalar and array fields are handled.

Something like this:

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) { IBsonSerializer<TValue> valueSerializer; if (_field != null) { var renderedField = _field.Render(args); valueSerializer = renderedField.FieldSerializer; } else { var renderedArrayField = _arrayField.Render(args); valueSerializer = (IBsonSerializer<TValue>)ArraySerializerHelper.GetItemSerializer(renderedArrayField.FieldSerializer); } var document = new BsonDocument(); using var bsonWriter = new BsonDocumentWriter(document); var context = BsonSerializationContext.CreateRoot(bsonWriter); bsonWriter.WriteStartDocument(); bsonWriter.WriteName("value"); valueSerializer.Serialize(context, _value); bsonWriter.WriteEndDocument(); return document; }

Perfect, this solves my doubts. That makes sense, I was mostly focusing on how to get the correct serializer before trying to refactor.

Regarding IBsonArraySerializer, the majority of times when we implement that interface we also implement IChildSerializerConfigurable, and that makes sense. It also seems that for this use case the result we'd get would be the same, so what would you think about using IChildSerializerConfigurable instead?
To be honest this is more of a generic question, I'm curious about if there's any preference on using one over the other, of course considering that TryGetItemSerializationInfo can potentially return more info (not in this case though).

IBsonArraySerializer and IChildSerializerConfigurable are orthogonal. We can't just implement IChildSerializerConfigurable instead of IBsonArraySerializer.

In this case we must implement IBsonArraySerializer.

It is true that in general whenever implementing IBsonArraySerializer we would likely also want to implement IChildSerializerConfigurable. However, that applies mostly to public serializers that a user might want to configure. This is an internal serializer, so I think implementing IBsonArraySerializer is sufficient.

rstam · 2025-01-03T20:35:42Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

@@ -118,38 +119,88 @@ private protected override BsonDocument RenderArguments(RenderArgs<TDocument> ar

    internal sealed class EqualsSearchDefinition<TDocument, TField> : OperatorSearchDefinition<TDocument>
    {
-        private readonly BsonValue _value;
+        private readonly TField _value;


Rename TField to TValue because that is what it is.

TField is the type of the field, that is also the type of the value, right?
Also if we do it here we'd need to make this change also in the other operators, and probably should propagate it as well.

TField and TValue are only the same when the field is not an array.

In the case of an array TField is IEnumerable<TValue>.

Did you remove the code you had earlier for _arrayField?

rstam · 2025-01-03T20:36:55Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+        public EqualsSearchDefinition(FieldDefinition<TDocument, TField> path, TField value, SearchScoreDefinition<TDocument> score)
+            : base(OperatorType.Equals, new SingleSearchPathDefinition<TDocument>(path), score)
+        {
+            ValidateType();


I question whether we need to have ValidateType at all. Looks to me like this was just a client-side limitation related to what .NET types ToBsonValue knew how to convert to BSON, but now that we are using the proper serializer this limitation should no longer exist.

I checked with Boris and he confirmed that it is a server limitation which types are supported.

But even so, we probably shouldn't be checking this client side. We should just send the value to the server and let it return an error if it doesn't support the data type.

That way our driver will work well with all supported versions of the server, which might have different limitations. And if future server versions support new value types we won't have to make any changes.

I also like this idea. I tried to keep it consistent with what was there before, but I agree that moving this check server side would allow us to be more free in what we do and don't support over time.
I need to remember to do the same with the other operators too.

rstam · 2025-01-03T20:38:34Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+
+                var renderedField = _arrayField.Render(args);
+
+                var serializer =


I think this is on the right track but can use a little tweaking to how the right serializer is found and to reduce duplication between how scalar and array fields are handled.

Something like this:

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) { IBsonSerializer<TValue> valueSerializer; if (_field != null) { var renderedField = _field.Render(args); valueSerializer = renderedField.FieldSerializer; } else { var renderedArrayField = _arrayField.Render(args); valueSerializer = (IBsonSerializer<TValue>)ArraySerializerHelper.GetItemSerializer(renderedArrayField.FieldSerializer); } var document = new BsonDocument(); using var bsonWriter = new BsonDocumentWriter(document); var context = BsonSerializationContext.CreateRoot(bsonWriter); bsonWriter.WriteStartDocument(); bsonWriter.WriteName("value"); valueSerializer.Serialize(context, _value); bsonWriter.WriteEndDocument(); return document; }

rstam · 2025-01-03T20:40:16Z

src/MongoDB.Driver/FieldValueSerializerHelper.cs

@@ -317,6 +317,8 @@ internal class IEnumerableSerializer<TItem> : SerializerBase<IEnumerable<TItem>>
        {
            private readonly IBsonSerializer<TItem> _itemSerializer;

+            public IBsonSerializer<TItem> ItemSerializer => _itemSerializer;
+


Instead of adding this new public property have the class implement IBsonArraySerializer and implement the TryGetItemSerializationIinfo method:

public bool TryGetItemSerializationInfo(out BsonSerializationInfo serialization { serializationInfo = new(elementName: null, _itemSerializer, typeof(TItem)); return true; }

All serializers for array-like values should implement IBsonArraySerializer. I don't know why this class did not already implement it.

rstam · 2025-01-03T20:43:15Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+                var renderedField = _arrayField.Render(args);
+
+                var serializer =
+                    (renderedField.ValueSerializer as FieldValueSerializerHelper.IEnumerableSerializer<TField>)


Instead of trying to cast to the very specific class FieldValueSerializerHelper.IEnumerableSerializer<TField> use the more general from (used everywhere else) to get the item serializer from an array-like serializer:

valueSerializer = (IBsonSerializer<TValue>)ArraySerializerHelper.GetItemSerializer(renderedArrayField.FieldSerializer);

This is why FieldValueSerializerHelper.IEnumerableSerializer<TItem> should implement IBsonArraySerializer (as all array-like serializers should). Not sure why it didn't already...

rstam · 2025-01-03T22:23:20Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+        public EqualsSearchDefinition(FieldDefinition<TDocument, TField> path, TField value, SearchScoreDefinition<TDocument> score)
+            : base(OperatorType.Equals, new SingleSearchPathDefinition<TDocument>(path), score)
+        {
+            ValidateType();


I checked with Boris and he confirmed that it is a server limitation which types are supported.

But even so, we probably shouldn't be checking this client side. We should just send the value to the server and let it return an error if it doesn't support the data type.

That way our driver will work well with all supported versions of the server, which might have different limitations. And if future server versions support new value types we won't have to make any changes.

papafe · 2025-01-06T09:29:32Z

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs

@@ -290,6 +290,8 @@ public void Equals_should_render_supported_type<T>(
                subject.Equals("x", value),
                $"{{ equals: {{ path: 'x', value: {valueRendered} }} }}");

+            //For the Guid we get (correctly): GuidSerializer cannot serialize a Guid when GuidRepresentation is Unspecified.


@rstam This test here is now failing because now we're using the default serializer and that doesn't have the GuidRepresentation specified.
Do you have any suggestion on how to fix this? The exception makes sense, and normally we'd probably suggest statically registering a serializer, but this would impact all the other tests here.
Should we ignore the Guid field for this test and make a separable test here that can only be run manually?
This would need to be done also for other operators.

That's because on line 292 this testuses the string form of the field name and specified "x" and there is no field called 'x" to pick up the correctly configured GuidSerializer from.

I would simply skip this part of the test for Guid:

// in this test there is no property called "x" to pick up a properly configured GuidSerializer from if (typeof(T) != typeof(Guid)) { AssertRendered( subject.Equals("x", value), $"{{ equals: {{ path: 'x', value: {valueRendered} }} }}"); var scoreBuilder = new SearchScoreDefinitionBuilder<BsonDocument>(); AssertRendered( subject.Equals("x", value, scoreBuilder.Constant(1)), $"{{ equals: {{ path: 'x', value: {valueRendered}, score: {{ constant: {{ value: 1 }} }} }} }}"); }

Yeah, I would have loved to avoid making special cases, but it seems we need to. I'll make some additional tests that can be run manually.

I'm fine with just skipping this test for Guids, because:

This test is against IMongoCollection<BsonDocument> and that is probably not used often

Anyone using BsonDocument should either not be using Guid or should take control of the conversion themselves

A manual test would involve registering a properly configured global GuidSerializer, but we already know that works (in part because the strongly typed version uses a properly configured GuidSerializer, though it's true it doesn't get it from the global registry).

papafe · 2025-01-07T10:40:52Z

src/MongoDB.Driver/FieldValueSerializerHelper.cs

@@ -141,6 +141,7 @@ public static IBsonSerializer GetSerializerForValueType(IBsonSerializer fieldSer
            return ConvertIfPossibleSerializer.Create(valueType, fieldType, fieldSerializer, serializerRegistry);
        }

+        //TODO Never used


Should we remove this?

papafe · 2025-01-07T10:48:32Z

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs

             new object[] { new[] { ObjectId.Empty, ObjectId.Parse("4d0ce088e447ad08b4721a37") }, new[] { "{ $oid: '000000000000000000000000' }", "{ $oid: '4d0ce088e447ad08b4721a37' }" } },
-             new object[] { new object[] { (byte)1, (short)2, (int)3 }, new[] { "1", "2", "3" } }


Do we want to support this? This does not work anymore because before we were using the serializer depending on the type of the value, while now we're trying to look at the serializer appropriate for the field.

papafe · 2025-01-07T10:49:13Z

src/MongoDB.Driver/Search/SearchDefinition.cs

@@ -123,9 +123,9 @@ private protected enum OperatorType
            QueryString,
            Range,
            Regex,
-            Search,
+            Search,  //TODO This is never used, should we remove them?


It looks like these two can be safely removed, should we do it?

papafe · 2025-01-07T10:50:24Z

src/MongoDB.Driver/Search/SearchDefinitionBuilder.cs

@@ -119,7 +119,7 @@ public SearchDefinition<TDocument> EmbeddedDocument<TField>(
        /// <param name="score">The score modifier.</param>
        /// <returns>An equality search definition.</returns>
        public SearchDefinition<TDocument> Equals<TField>(
-            FieldDefinition<TDocument, TField> path,
+            FieldDefinition<TDocument, TField> path,  //TODO We should try to uniform this to the In operator for next major (this should be SearchPathDefinition<TDocument>)


I can create a ticket for this if we want to go this way

rstam · 2025-01-07T17:21:00Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

@@ -118,38 +119,88 @@ private protected override BsonDocument RenderArguments(RenderArgs<TDocument> ar

    internal sealed class EqualsSearchDefinition<TDocument, TField> : OperatorSearchDefinition<TDocument>
    {
-        private readonly BsonValue _value;
+        private readonly TField _value;


TField and TValue are only the same when the field is not an array.

In the case of an array TField is IEnumerable<TValue>.

rstam · 2025-01-07T17:22:21Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

@@ -118,38 +119,88 @@ private protected override BsonDocument RenderArguments(RenderArgs<TDocument> ar

    internal sealed class EqualsSearchDefinition<TDocument, TField> : OperatorSearchDefinition<TDocument>
    {
-        private readonly BsonValue _value;
+        private readonly TField _value;


Did you remove the code you had earlier for _arrayField?

rstam · 2025-01-07T17:24:38Z

src/MongoDB.Driver/FieldValueSerializerHelper.cs

@@ -322,6 +323,12 @@ public IEnumerableSerializer(IBsonSerializer<TItem> itemSerializer)
                _itemSerializer = itemSerializer;
            }

+            public bool TryGetItemSerializationInfo(out BsonSerializationInfo serializationInfo)


Keep methods in alphabetical order?

rstam · 2025-01-07T17:26:19Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

+        {
+            var searchPathDefinition = (SingleSearchPathDefinition<TDocument>)_path;
+            var renderedField = searchPathDefinition.Field.Render(args);
+            var serializer = renderedField.FieldSerializer switch


Maybe rename serializer to valueSerializer.

To remove confusion about whether it is the field serializer or the value serializer (which are not the same in the case of array fields).

rstam · 2025-01-07T17:28:31Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

@@ -118,38 +121,35 @@ private protected override BsonDocument RenderArguments(RenderArgs<TDocument> ar

    internal sealed class EqualsSearchDefinition<TDocument, TField> : OperatorSearchDefinition<TDocument>
    {
-        private readonly BsonValue _value;
+        private readonly TField _value;

        public EqualsSearchDefinition(FieldDefinition<TDocument> path, TField value, SearchScoreDefinition<TDocument> score)


It is unfortunate that path is of type FieldDefinition<TDocument> instead of FieldDefinition<TDocument, TField>.

This is not type safe because it allows mismatches between the field type and the value type that are not caught at compile time.

In an earlier commit you had changed this but now it's removed. Can you explain why?

I tried to uniform the code between the Equals, In and Range search definition classes.
Unfortunately for In and Range we can't have the same constructor with FieldDefinition<TDocument, Field> because of the current public API of the operators in SearchDefinitionBuilder.
I agree that this is not type safe, but at least EqualsSearchDefinition is an internal class, and the operators in SearchDefinitionBuilder that use it have type safety (either through expressions or field definitions).

If this was a major version, I'd suggest to modify SingleSearchPathDefinition (the subclass of SearchPathDefinition for single fields) from using one generic parameter TDocument to two TDocument, TField, so we could be on the safer side.
Probably it would be worth actually removing SearchPathDefinition<TDocument> as an input parameter of the operators, and create multiple overrides for the various subclasses of SearchPathDefinition, so that we can use as many generic parameters as necessary.

I think this is something that would be worth investigating for next major in order to improve the API, but I don't think we can do much at the moment without making breaking changes.

(We also have MultiSearchPathDefinition, and that's another thing to consider when investigating what we can do with the API)

papafe · 2025-01-08T09:32:40Z

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs

            AssertRendered(
                subjectTyped.In(p => p.Hobbies, ["dance", "ski"]),
                "{ in: { path: 'hobbies', value: ['dance', 'ski'] } }");
        }

+        [Fact]


According to the mongo docs In should support wildcard paths, but we had no tests for that.

papafe added 5 commits December 31, 2024 10:42

Initial

40d8bd9

Various corrections

65567ee

Corrections

572ed5c

Small fix

1493813

Removed extra spaces

7f44c62

papafe requested a review from BorisDog December 31, 2024 15:29

papafe added 5 commits December 31, 2024 18:33

Small corrections

8dfa53f

Added tests and fixed

a5ba406

Missing statment

78490cd

Various corrections

845c7eb

Corrections

e283312

papafe commented Jan 2, 2025

View reviewed changes

papafe added 2 commits January 2, 2025 13:02

Small fix

dd89854

Small correction

7c7eafe

papafe requested a review from rstam January 2, 2025 16:36

rstam requested changes Jan 3, 2025

View reviewed changes

papafe commented Jan 6, 2025

View reviewed changes

papafe added 8 commits January 6, 2025 10:45

Moved testing to main file

2f1af5f

Corrected operator

f400017

Added missing parenthesis

5323c79

Removed unsupported test

b776914

Added support for In

1cb7876

Added support for range

e480108

Corrected tests

c27b824

Small corrections

5d47e61

papafe commented Jan 7, 2025

View reviewed changes

papafe requested review from rstam and sanych-sun and removed request for BorisDog January 7, 2025 11:42

papafe added 2 commits January 7, 2025 12:45

Small correction to tests

fdf5846

Test correction

bed20d9

papafe marked this pull request as ready for review January 7, 2025 15:32

papafe requested a review from a team as a code owner January 7, 2025 15:32

rstam requested changes Jan 7, 2025

View reviewed changes

papafe commented Jan 8, 2025

View reviewed changes

papafe added 3 commits January 8, 2025 10:44

Remove comment

60c4222

Moved method

0fe3915

Name corrections

269f0dc

papafe requested a review from rstam January 8, 2025 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSHARP-5442: Atlas Search doesn't respect the correct serializer #1583

CSHARP-5442: Atlas Search doesn't respect the correct serializer #1583

papafe commented Dec 31, 2024 •

edited

Loading

papafe commented Dec 31, 2024 •

edited

Loading

papafe Jan 2, 2025

papafe Jan 2, 2025 •

edited

Loading

rstam Jan 3, 2025

papafe Jan 6, 2025 •

edited

Loading

rstam Jan 6, 2025

rstam Jan 3, 2025

papafe Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 3, 2025

rstam Jan 3, 2025

papafe Jan 6, 2025 •

edited

Loading

rstam Jan 3, 2025

rstam Jan 3, 2025

rstam Jan 3, 2025

rstam Jan 3, 2025

papafe Jan 6, 2025 •

edited

Loading

rstam Jan 6, 2025

papafe Jan 6, 2025

rstam Jan 6, 2025

papafe Jan 7, 2025

papafe Jan 7, 2025

papafe Jan 7, 2025 •

edited

Loading

papafe Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 7, 2025

rstam Jan 7, 2025

papafe Jan 7, 2025 •

edited

Loading

papafe Jan 8, 2025


		var renderedField = _arrayField.Render(args);

		var serializer =

		new object[] { new[] { ObjectId.Empty, ObjectId.Parse("4d0ce088e447ad08b4721a37") }, new[] { "{ $oid: '000000000000000000000000' }", "{ $oid: '4d0ce088e447ad08b4721a37' }" } },
		new object[] { new object[] { (byte)1, (short)2, (int)3 }, new[] { "1", "2", "3" } }

CSHARP-5442: Atlas Search doesn't respect the correct serializer #1583

Are you sure you want to change the base?

CSHARP-5442: Atlas Search doesn't respect the correct serializer #1583

Conversation

papafe commented Dec 31, 2024 • edited Loading

papafe commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

papafe Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe commented Dec 31, 2024 •

edited

Loading

papafe commented Dec 31, 2024 •

edited

Loading

papafe Jan 2, 2025 •

edited

Loading

papafe Jan 6, 2025 •

edited

Loading

papafe Jan 6, 2025 •

edited

Loading

papafe Jan 6, 2025 •

edited

Loading

papafe Jan 7, 2025 •

edited

Loading

papafe Jan 7, 2025 •

edited

Loading