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.

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

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.

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

src/MongoDB.Driver/FieldValueSerializerHelper.cs

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

src/MongoDB.Driver/Search/SearchDefinition.cs

src/MongoDB.Driver/Search/SearchDefinitionBuilder.cs

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

src/MongoDB.Driver/FieldValueSerializerHelper.cs

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

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

rstam

I'm not too familiar with the Search API but as far as I can tell this looks OK.

You should get one more LGTM before merging to main though.

papafe · 2025-01-14T10:30:54Z

I'm not too familiar with the Search API but as far as I can tell this looks OK.

You should get one more LGTM before merging to main though.

@sanych-sun can you take a look too?

papafe · 2025-01-28T19:52:10Z

@BorisDog another thing to consider is that this is a behaviour breaking change, so we need to consider if we want to merge this now or wait. I think it could also be seen as a bug fix to be honest, but we should take a decision.
The difference is that now the correct registered serializers are used to serialize the values for the equals, in and range operators, while before default conversions were used, ignoring the registered serializers.

src/MongoDB.Driver/FieldValueSerializerHelper.cs

src/MongoDB.Driver/Search/SearchDefinition.cs

BorisDog · 2025-02-04T21:07:04Z

src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs

-            value switch
+        private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args)
+        {
+            var searchPathDefinition = (SingleSearchPathDefinition<TDocument>)_path;


It's a bit inefficient that we need to render the path twice.
Also it would be good to extract this to a single helper method.

What do you and @rstam think about having new internal Render method in SearchPathDefinition, something like

(BsonValue, RenderedFieldDefinition) Render(...)

And passing field serializer in OperatorSearchDefinition.RenderArguments() ?

I see your point. I also think it's a little complicated to extract a method like that to SearchPathDefinition, since (BsonValue, RenderedFieldDefinition) Render(...) would make sense for a SingleSearchPathDefinition, but not for MultiSearchPathDefinition or WildcardSearchPathDefinition.
The issue here is that operators support various degrees of SearchPathDefinition. I'm trying to think of a more general solution that does not make breaking changes in the public API, but so far it all feels kinda clunky.

Yeah I meant that the returned RenderedFieldDefinition would be null in some cases. I agree that it's not the greatest design, but I don't have any better suggestion as of now. I does have the upside of not rendering the path twice.
As long as all the API changes are internal, we can evolve them in the future if we come up with a better idea (the new suggested Render method should be internal).

In 60c1d57, I've added a fix to avoid rendering the path twice. It's not super nice, and I'm not sure it's worth it given that probably the network call of the atlas search would be much greater than the time needed to render the path twice.

I think that if we want to have special cases depending on the path we could also specialize OperatorSearchDefinition.RenderArguments and produce a different method for each path type, for example OperatorSearchDefinition.RenderArgumentsForSingleSearchPath where we can pass additional arguments. This would require quite some changes, but could also allow to control which kind of path can be used with certain kinds of operators.

papafe · 2025-02-06T20:26:20Z

@BorisDog I've done some corrections following your suggestions, and it looks much better than before, thank you 😄
I'm a little bit unsure about the names though, they feel a little redundant

BorisDog · 2025-02-07T00:14:33Z

src/MongoDB.Driver/Search/SearchPathDefinition.cs

+
+        internal virtual (BsonValue, IBsonSerializer) RenderAndGetFieldSerializer(RenderArgs<TDocument> args) => (Render(args), null);
+
+        internal (string renderedPath, IBsonSerializer fieldSerializer) RenderFieldWithSerializer(FieldDefinition<TDocument> fieldDefinition, RenderArgs<TDocument> args)


RenderFieldWithSerializer might be sound like "Render the field with provided serializer"
Would RenderFieldAndGetFieldSerializer be more accurate?

Yes, I think it makes sense

BorisDog · 2025-02-07T03:27:20Z

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

-            AssertRendered(
-                subject.Equals("x", value),
-                $"{{ equals: {{ path: 'x', value: {valueRendered} }} }}");
+            //There is no property called "x" where to pick up a properly configured GuidSerializer for the tests


I'm not sure I fully understand this exclusion. Is the default guid serializer different from the one used by serialization?

I think I need to improve the comment, I agree it's not super clear.
The issue here is that when we use a string path, we're going to get the default serializer for the value, so GuidSerializer. In v3.0 we decided to have GuidRepresentation.Unspecified as the default Guid representation, and this will throw an exception when trying to serialize/deserialize. Normally a user would go over this by registering a default GuidSerializer with the proper representation (BsonSerializer.Register...(GuidSerializer)), but we can't do it because the registered serializers are global and this would be a problem for all the other tests.

For the typed path on line 305 it's not an issue since we get the default field serializer from the class, and the Guid property has the BsonGuidRepresentation attribute on it.

This is more of a testing issue and should be solved when we can pass down the serialization domain 😁

BorisDog · 2025-02-07T03:31:27Z

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

@@ -313,29 +319,55 @@ public void Equals_should_render_supported_type<T>(
            new object[] { (float)1, "1", Exp(p => p.Float), nameof(Person.Float) },
            new object[] { (double)1, "1", Exp(p => p.Double), nameof(Person.Double) },
            new object[] { DateTime.MinValue, "ISODate(\"0001-01-01T00:00:00Z\")", Exp(p => p.Birthday), "dob" },
-            new object[] { DateTimeOffset.MaxValue, "ISODate(\"9999-12-31T23:59:59.999Z\")", Exp(p => p.DateTimeOffset), nameof(Person.DateTimeOffset) },
+            new object[] { DateTimeOffset.MaxValue, """{ "DateTime" : { "$date" : "9999-12-31T23:59:59.999Z" }, "Ticks" : 3155378975999999999, "Offset" : 0 }""", Exp(p => p.DateTimeOffset), nameof(Person.DateTimeOffset) },


So does this mean that we are changing the defaults also for non typed queries (SearchDefinitionBuilder<BsonDocument>)?

If so, that might be considered a bug and a breaking change.

Yes it will change also for non typed queries. They would need to convert the DateTime themselves if they want to keep the same behaviour.

It's probably a breaking change, but I'm not sure this can be considered a bug. If you're working directly with Bson documents then I assume that you would like to control how to the values are serialized in any case.

papafe · 2025-02-07T07:02:01Z

@BorisDog, as an additional note. I've tried making an untyped query (using BsonDocument) and passing a DateTime value for the Equal operator. No error returned but zero results as expected.

rstam · 2025-02-27T19:27:38Z

src/MongoDB.Driver/Search/SearchDefinition.cs

@@ -135,6 +160,8 @@ private protected enum OperatorType
        protected readonly SearchPathDefinition<TDocument> _path;
        protected readonly SearchScoreDefinition<TDocument> _score;

+        protected bool _useConfiguredSerialization = false;


I'm not convinced this is a good idea.

Might as well call this _useTheWrongSerializer.

You're not convinced about the naming or the actual API/solution?

I'm not convinced this bool should even exist.

I somewhat tongue in cheek suggested a name that illustrates why this makes no sense to me.

We have a bug: we are using the wrong serializer.

We are fixing the bug: we now use the correct serializer.

But somehow we think a user would like to use the wrong serializer???

I understand your point, but I suppose that some developers could already use this feature as is, and this would be considered a breaking change (even though it was not correct). The idea here would be to support this behaviour until next major and then get rid of it.
@BorisDog am I right?

While this sounds like a valid concern, I can't figure out any scenario in which it would actually be valid.

In essence we are replacing are "hand rolled" serializer (the ToBsonValue method) with using the correct serializer.

There are only two scenarios:

ToBsonValue and the configured serializer return the same serialized values

Or they return different values

In the first case there is no problem.

In the second case the value returned by ToBsonValue is wrong and I don't see how the query could have worked, and therefore why we would keep the existing behavior.

Can you point me to a test that passes when _useConfiguredSerialization is false and fails when it is true?

My concern is with pipelines using BsonDocument (unfortunately this happens more often than we'd like).
For example DateTimeOffset is serialized by default as document, while users with BsonDocument pipelines might be relying on it to be serialized as BsonDate.

I think in case we should introduce the "correct" behavior in the next major version.

@papafe can you point me to a test that passes when _useConfiguredSerialization is false and fails when it is true?

I want to confirm that this is not just a hypothetical concern, and if real, whether there isn't a better way to address than turning off the fix entirely by default.

@rstam sorry for the late reply.
If you look at the tests Equals_should_render_supported_type (it's already in main) and Equals_with_configured_serializer_should_render_supported_type, they are essentially the same test but with different test data (EqualsSupportedTypesTestData and EqualsWithConfiguredSerializerSupportedTypesTestData). The only difference between the two sets of data is the line with DateTimeOffset, and that's because in the current implementation that is serialized as a BsonDate while the default DateTimeOffsetSerializer uses a document representation.

@rstam did you have the change to look at this?

rstam · 2025-02-27T23:42:24Z

src/MongoDB.Driver/Search/SearchDefinition.cs

@@ -135,6 +160,8 @@ private protected enum OperatorType
        protected readonly SearchPathDefinition<TDocument> _path;
        protected readonly SearchScoreDefinition<TDocument> _score;

+        protected bool _useConfiguredSerialization = false;


I'm not convinced this bool should even exist.

I somewhat tongue in cheek suggested a name that illustrates why this makes no sense to me.

We have a bug: we are using the wrong serializer.

We are fixing the bug: we now use the correct serializer.

But somehow we think a user would like to use the wrong serializer???

rstam · 2025-02-28T16:04:56Z

src/MongoDB.Driver/Search/SearchDefinition.cs

@@ -135,6 +160,8 @@ private protected enum OperatorType
        protected readonly SearchPathDefinition<TDocument> _path;
        protected readonly SearchScoreDefinition<TDocument> _score;

+        protected bool _useConfiguredSerialization = false;


While this sounds like a valid concern, I can't figure out any scenario in which it would actually be valid.

In essence we are replacing are "hand rolled" serializer (the ToBsonValue method) with using the correct serializer.

There are only two scenarios:

ToBsonValue and the configured serializer return the same serialized values

Or they return different values

In the first case there is no problem.

In the second case the value returned by ToBsonValue is wrong and I don't see how the query could have worked, and therefore why we would keep the existing behavior.

Can you point me to a test that passes when _useConfiguredSerialization is false and fails when it is true?

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

papafe commented Jan 2, 2025

View reviewed changes

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

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs Outdated Show resolved Hide resolved

papafe commented Jan 7, 2025

View reviewed changes

src/MongoDB.Driver/FieldValueSerializerHelper.cs Outdated Show resolved Hide resolved

papafe commented Jan 7, 2025

View reviewed changes

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs Show resolved Hide resolved

papafe commented Jan 7, 2025

View reviewed changes

src/MongoDB.Driver/Search/SearchDefinition.cs Outdated Show resolved Hide resolved

papafe commented Jan 7, 2025

View reviewed changes

src/MongoDB.Driver/Search/SearchDefinitionBuilder.cs Outdated Show resolved Hide resolved

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

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

tests/MongoDB.Driver.Tests/Search/SearchDefinitionBuilderTests.cs Show resolved Hide resolved

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

rstam approved these changes Jan 10, 2025

View reviewed changes

papafe requested a review from BorisDog January 28, 2025 19:52

BorisDog requested changes Feb 4, 2025

View reviewed changes

papafe requested a review from BorisDog February 6, 2025 20:26

BorisDog requested changes Feb 7, 2025

View reviewed changes

papafe force-pushed the charp5442 branch from 1dc887d to ba81027 Compare February 7, 2025 06:46

rstam self-requested a review February 10, 2025 22:19

papafe added 25 commits February 27, 2025 15:39

Added support for In

8e53214

Added support for range

ca05584

Corrected tests

f402b0e

Small corrections

b67443a

Small correction to tests

0cae022

Test correction

c672881

Remove comment

8796216

Moved method

a8571e5

Name corrections

640abef

Fixes according to PR

ed54bae

Small fixes

b19e5e6

Corrected TField to TValue for better clarity

53bb632

Small fix

d84f290

Correction to avoid path rendering twice

993832b

Small fix

a99e785

Small fix

4afcaef

Fixes according to PR

166edbf

Improvement

eb4cb3f

Small corrections according to PR

82e2675

Fixed indentation

1f29e6a

Corrections according to PR

484fa7e

Naming correction

2450326

Added extension method to support old and new behaviour

6e2d349

Small correction again

eaa16c4

Corrected serializer

8522ef3

papafe force-pushed the charp5442 branch from 39a9744 to 8522ef3 Compare February 27, 2025 14:48

Name correction

44a8aa8

rstam requested changes Feb 27, 2025

View reviewed changes

rstam requested changes Feb 28, 2025

View reviewed changes


		var renderedField = _arrayField.Render(args);

		var serializer =


		internal virtual (BsonValue, IBsonSerializer) RenderAndGetFieldSerializer(RenderArgs<TDocument> args) => (Render(args), null);

		internal (string renderedPath, IBsonSerializer fieldSerializer) RenderFieldWithSerializer(FieldDefinition<TDocument> fieldDefinition, RenderArgs<TDocument> args)

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

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

rstam left a comment

Choose a reason for hiding this comment

papafe commented Jan 14, 2025

papafe commented Jan 28, 2025

Choose a reason for hiding this comment

papafe Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

BorisDog Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

papafe commented Feb 6, 2025

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 commented Feb 7, 2025

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

Choose a reason for hiding this comment

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 Feb 5, 2025 •

edited

Loading

BorisDog Feb 5, 2025 •

edited

Loading