Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/MongoDB.Driver/FieldValueSerializerHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public static IBsonSerializer GetSerializerForValueType(IBsonSerializer fieldSer
return ConvertIfPossibleSerializer.Create(valueType, fieldType, fieldSerializer, serializerRegistry);
}

//TODO Never used
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove this?

public static IBsonSerializer GetSerializerForValueType(IBsonSerializer fieldSerializer, IBsonSerializerRegistry serializerRegistry, Type valueType, object value)
{
if (!valueType.GetTypeInfo().IsValueType && value == null)
Expand Down Expand Up @@ -313,7 +314,7 @@ public override void Serialize(BsonSerializationContext context, BsonSerializati
}
}

internal class IEnumerableSerializer<TItem> : SerializerBase<IEnumerable<TItem>>
internal class IEnumerableSerializer<TItem> : SerializerBase<IEnumerable<TItem>>, IBsonArraySerializer
{
private readonly IBsonSerializer<TItem> _itemSerializer;

Expand Down Expand Up @@ -351,6 +352,12 @@ public override void Serialize(BsonSerializationContext context, BsonSerializati
bsonWriter.WriteEndArray();
}
}

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

internal class NullableEnumConvertingSerializer<TFrom, TTo> : SerializerBase<Nullable<TFrom>> where TFrom : struct where TTo : struct
Expand Down
1 change: 0 additions & 1 deletion src/MongoDB.Driver/IAggregateFluent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,6 @@ IAggregateFluent<TNewResult> Lookup<TForeignDocument, TAsElement, TAs, TNewResul
IAggregateFluent<BsonDocument> SetWindowFields<TWindowFields>(
AggregateExpressionDefinition<ISetWindowFieldsPartition<TResult>, TWindowFields> output);

//TODO If I add a parameter here, then this would be a binary breaking change
/// <summary>
/// Appends a $search stage to the pipeline.
/// </summary>
Expand Down
153 changes: 78 additions & 75 deletions src/MongoDB.Driver/Search/OperatorSearchDefinitions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,12 @@
using System.Collections.Generic;
using System.Linq;
using MongoDB.Bson;
using MongoDB.Bson.IO;
using MongoDB.Bson.Serialization;
using MongoDB.Bson.Serialization.Serializers;
using MongoDB.Driver.Core.Misc;
using MongoDB.Driver.GeoJsonObjectModel;
using MongoDB.Driver.Linq.Linq3Implementation.Misc;

namespace MongoDB.Driver.Search
{
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename TField to TValue because that is what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remove the code you had earlier for _arrayField?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@papafe papafe Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

: base(OperatorType.Equals, path, score)
{
_value = ToBsonValue(value);
_value = value;
}

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) =>
new("value", _value);

private static BsonValue ToBsonValue(TField value) =>
value switch
private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args)
{
var searchPathDefinition = (SingleSearchPathDefinition<TDocument>)_path;
var renderedField = searchPathDefinition.Field.Render(args);
var valueSerializer = renderedField.FieldSerializer switch
{
bool v => (BsonBoolean)v,
sbyte v => (BsonInt32)v,
byte v => (BsonInt32)v,
short v => (BsonInt32)v,
ushort v => (BsonInt32)v,
int v => (BsonInt32)v,
uint v => (BsonInt64)v,
long v => (BsonInt64)v,
float v => (BsonDouble)v,
double v => (BsonDouble)v,
DateTime v => (BsonDateTime)v,
DateTimeOffset v => (BsonDateTime)v.UtcDateTime,
ObjectId v => (BsonObjectId)v,
Guid v => new BsonBinaryData(v, GuidRepresentation.Standard),
string v => (BsonString)v,
null => BsonNull.Value,
_ => throw new InvalidCastException()
null => BsonSerializer.LookupSerializer<TField>(),
IBsonArraySerializer => ArraySerializerHelper.GetItemSerializer(renderedField.FieldSerializer),
_ => renderedField.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;
}
}

internal sealed class ExistsSearchDefinition<TDocument> : OperatorSearchDefinition<TDocument>
Expand Down Expand Up @@ -225,7 +225,7 @@ private protected override BsonDocument RenderArguments(RenderArgs<TDocument> ar

internal sealed class InSearchDefinition<TDocument, TField> : OperatorSearchDefinition<TDocument>
{
private readonly BsonArray _values;
private readonly TField[] _values;

public InSearchDefinition(
SearchPathDefinition<TDocument> path,
Expand All @@ -234,36 +234,38 @@ public InSearchDefinition(
: base(OperatorType.In, path, score)
{
Ensure.IsNotNullOrEmpty(values, nameof(values));
var array = new BsonArray(values.Select(ToBsonValue));

var bsonType = array[0].GetType();
_values = Ensure.That(array, arr => arr.All(v => v.GetType() == bsonType), nameof(values), "All values must be of the same type.");
_values = values.ToArray();
}

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) =>
new("value", _values);
private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args)
{
IBsonSerializer valueSerializer;

private static BsonValue ToBsonValue(TField value) =>
value switch
if (_path is SingleSearchPathDefinition<TDocument> searchPathDefinition)
{
bool v => (BsonBoolean)v,
sbyte v => (BsonInt32)v,
byte v => (BsonInt32)v,
short v => (BsonInt32)v,
ushort v => (BsonInt32)v,
int v => (BsonInt32)v,
uint v => (BsonInt64)v,
long v => (BsonInt64)v,
float v => (BsonDouble)v,
double v => (BsonDouble)v,
decimal v => (BsonDecimal128)v,
DateTime v => (BsonDateTime)v,
DateTimeOffset v => (BsonDateTime)v.UtcDateTime,
string v => (BsonString)v,
ObjectId v => (BsonObjectId)v,
Guid v => new BsonBinaryData(v, GuidRepresentation.Standard),
_ => throw new InvalidCastException()
};
var renderedField = searchPathDefinition.Field.Render(args);
valueSerializer = renderedField.FieldSerializer switch
{
null => new ArraySerializer<TField>(BsonSerializer.LookupSerializer<TField>()),
IBsonArraySerializer => renderedField.FieldSerializer,
_ => new ArraySerializer<TField>((IBsonSerializer<TField>)renderedField.FieldSerializer)
};
}
else
{
valueSerializer = new ArraySerializer<TField>(BsonSerializer.LookupSerializer<TField>());
}

var document = new BsonDocument();
using var bsonWriter = new BsonDocumentWriter(document);
var context = BsonSerializationContext.CreateRoot(bsonWriter);
bsonWriter.WriteStartDocument();
bsonWriter.WriteName("value");
valueSerializer.Serialize(context, _values);
bsonWriter.WriteEndDocument();

return document;
}
}

internal sealed class MoreLikeThisSearchDefinition<TDocument, TLike> : OperatorSearchDefinition<TDocument>
Expand Down Expand Up @@ -361,8 +363,6 @@ internal sealed class RangeSearchDefinition<TDocument, TField> : OperatorSearchD
where TField : struct, IComparable<TField>
{
private readonly SearchRange<TField> _range;
private readonly BsonValue _min;
private readonly BsonValue _max;

public RangeSearchDefinition(
SearchPathDefinition<TDocument> path,
Expand All @@ -371,34 +371,37 @@ public RangeSearchDefinition(
: base(OperatorType.Range, path, score)
{
_range = range;
_min = ToBsonValue(_range.Min);
_max = ToBsonValue(_range.Max);
}

private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args) =>
new()
private protected override BsonDocument RenderArguments(RenderArgs<TDocument> args)
{
var searchPathDefinition = (SingleSearchPathDefinition<TDocument>)_path;
var renderedField = searchPathDefinition.Field.Render(args);
var valueSerializer = renderedField.FieldSerializer switch
{
{ _range.IsMinInclusive ? "gte" : "gt", _min, _min != null },
{ _range.IsMaxInclusive ? "lte" : "lt", _max, _max != null },
null => BsonSerializer.LookupSerializer<TField>(),
IBsonArraySerializer => ArraySerializerHelper.GetItemSerializer(renderedField.FieldSerializer),
_ => renderedField.FieldSerializer
};

private static BsonValue ToBsonValue(TField? value) =>
value switch
var document = new BsonDocument();
using var bsonWriter = new BsonDocumentWriter(document);
var context = BsonSerializationContext.CreateRoot(bsonWriter);
bsonWriter.WriteStartDocument();
if (_range.Min is not null)
{
sbyte v => (BsonInt32)v,
byte v => (BsonInt32)v,
short v => (BsonInt32)v,
ushort v => (BsonInt32)v,
int v => (BsonInt32)v,
uint v => (BsonInt64)v,
long v => (BsonInt64)v,
float v => (BsonDouble)v,
double v => (BsonDouble)v,
DateTime v => (BsonDateTime)v,
DateTimeOffset v => (BsonDateTime)v.UtcDateTime,
null => null,
_ => throw new InvalidCastException()
};
bsonWriter.WriteName(_range.IsMinInclusive? "gte" : "gt");
valueSerializer.Serialize(context, _range.Min);
}
if (_range.Max is not null)
{
bsonWriter.WriteName(_range.IsMaxInclusive? "lte" : "lt");
valueSerializer.Serialize(context, _range.Max);
}
bsonWriter.WriteEndDocument();

return document;
}
}

internal sealed class RegexSearchDefinition<TDocument> : OperatorSearchDefinition<TDocument>
Expand Down
4 changes: 2 additions & 2 deletions src/MongoDB.Driver/Search/SearchDefinition.cs
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ private protected enum OperatorType
QueryString,
Range,
Regex,
Search,
Search, //TODO This is never used, should we remove them?
Copy link
Contributor Author

@papafe papafe Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Span,
Term,
Term, //TODO This is never used, should we remove them?
Text,
Wildcard
}
Expand Down
2 changes: 2 additions & 0 deletions src/MongoDB.Driver/Search/SearchPathDefinitionBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,8 @@ internal sealed class SingleSearchPathDefinition<TDocument> : SearchPathDefiniti
{
private readonly FieldDefinition<TDocument> _field;

public FieldDefinition<TDocument> Field => _field;

public SingleSearchPathDefinition(FieldDefinition<TDocument> field)
{
_field = Ensure.IsNotNull(field, nameof(field));
Expand Down
Loading