CSHARP-4858: Add IsMissing and IsNullOrMissing MongoDBFunctions. #1224

rstam · 2023-11-28T22:09:59Z

No description provided.

rstam · 2023-11-28T22:11:33Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+            var collection = GetCollection();
+
+            var queryable = collection.AsQueryable()
+                .Select(x => MongoDBFunctions.IsMissing(x.S));


Damien thinks MongoDBFunctions should not be a static class and that IsMissing should be an extension method instead.

To the user the difference would be that they would write

MongoDB.Functions.IsMissing(x.S)

Note the period between MongoDB and Functions.

@JamesKovacs let's discuss when you get back.

Damien would also like to see the LINQ provider support

EF.Functions.IsMissing(x.S)

even though the C# driver knows nothing about the type DbFunctions (that's the type of the EF.Functions static property) or anything about any extension methods defined on DbFunctions.

The question from the perspective of the C# driver is whether we want to start writing EF Core provider specific code (so far we have avoided doing so).

Ideally we would avoid having EF Core specific code in the C# driver. From a pragmatic viewpoint we might be willing to do some of that.

Besides the question of whether this class should be static or not we should also think hard about naming.

We currently have a few places custom LINQ related methods live:

Various extensions methods on existing .NET classes (e.g. DateTime, String, NullableDateTime)

MongoDBMath

LinqExtensions

MongoEnumerable (similar to .NET Enumerable)

MongoQueryable (similar to .NET Queryable)

MongoDBFunctions is named similarly to EF.Functions, but I don't think we need to follow EF Core naming conventions in the C# driver. Maybe a simpler name would be possible.

Hopefully we don't end up with too many new places for custom LINQ methods to live.

Additional point to consider:
As we are planning to support trimming in CSHARP-4840, it would be great to ensure that at least all new code supports trimming.
In this PR the reflection is an immediate suspect for trimming compatibility issues. If it can be avoided that would be preferable.

What exactly in this PR is preventing trimming? (I need to read up on trimming but maybe you can help me out here).

Looks like a false alarm. I've checked this in a sample project, and those methods are not getting trimmed.
So please ignore my previous comments.

Thanks for checking that.

What is the problem with using MongoDbFunctions.IsNullOrMissing in EF? It is a MongoDB-specific extension that other providers won't understand.

While we could write it as an extension method, we end up with field.IsNullOrMissing() - which while possible would suggest we are calling a method on a potentially null value. Not a huge deal as it is an extension method. No different than writing someString.IsNullOrEmpty(), which is simply defined as:

public class StringExtensions { public static bool IsNullOrEmpty(this string str) { return String.IsNullOrEmpty(str); } }

Totally valid C# but still quirky as it looks like you're calling a method on a potentially null value but not throwing.

Is there an existing EF.Functions.IsMissing? Why wouldn't we simply use MongoDbFunctions.IsMissing directly?

The larger question of whether we should merge the various extensions is a 3.0 driver question. It would be nice to simply using MongoDB.Driver.Extentions (or whatever we call it) and have them all available. But that sounds like a task for another ticket targeted at the 3.0 driver.

I would vote for implementing this as extensions methods (empty class that exists only to be extended). Because if we would ever came to the point when our LINQ provider would be extendable from outside it would be nice extension point to inject 3rd-party functions. And they would be available same way as built-in.

Two comments:

1. Trimming - Given these are going to be static methods (either extension or regular) I don't think they're going to cause a problem for trimming. While reflection has limitations I haven't seen anything that says the compiler is incapable of identifying references inside expression trees that the compiler builds during compilation.

2. Naming - Let's go with whatever works best for the LINQ provider for now. EF doesn't require them to be in EF.Functions so let's see if it causes confusion or not. Worse case at some later point we duplicate the static class/stubs and either translate it from one to the other in the EF provider or recognize both in the V3 provider.

rstam · 2023-12-01T15:45:32Z

src/MongoDB.Driver/Linq/Linq3Implementation/Reflection/MongoDBFunctionsMethod.cs

+        static MongoDBFunctionsMethod()
+        {
+            __isMissing = ReflectionInfo.Method((object field) => MongoDBFunctions.IsMissing(field));
+            __isNullOrMissing = ReflectionInfo.Method((object field) => MongoDBFunctions.IsNullOrMissing(field));


Don't these two static references to IsMissing and IsNullOrMissing avoid any trimming concerns?

JamesKovacs

LGTM. See my inline comment for my discussion points.

JamesKovacs · 2023-12-07T00:32:52Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+            var collection = GetCollection();
+
+            var queryable = collection.AsQueryable()
+                .Select(x => MongoDBFunctions.IsMissing(x.S));


What is the problem with using MongoDbFunctions.IsNullOrMissing in EF? It is a MongoDB-specific extension that other providers won't understand.

While we could write it as an extension method, we end up with field.IsNullOrMissing() - which while possible would suggest we are calling a method on a potentially null value. Not a huge deal as it is an extension method. No different than writing someString.IsNullOrEmpty(), which is simply defined as:

public class StringExtensions { public static bool IsNullOrEmpty(this string str) { return String.IsNullOrEmpty(str); } }

Totally valid C# but still quirky as it looks like you're calling a method on a potentially null value but not throwing.

Is there an existing EF.Functions.IsMissing? Why wouldn't we simply use MongoDbFunctions.IsMissing directly?

The larger question of whether we should merge the various extensions is a 3.0 driver question. It would be nice to simply using MongoDB.Driver.Extentions (or whatever we call it) and have them all available. But that sounds like a task for another ticket targeted at the 3.0 driver.

rstam · 2023-12-24T19:53:36Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+            var collection = GetCollection();
+
+            var queryable = collection.AsQueryable()
+                .Select(x => MongoDBLinq.Functions.IsMissing(x.S));


I first thought of using MongoDB.Functions (patterned after EF.Functions) but we can't have a class named MongoDB because it conflicts with the root of our namespaces which is also MongoDB.

I suppose we could consider MDB.Functions but we normally avoid abbreviations.

MongoDBLinq.Functions.IsMissing (and friends) isn't readily discoverable. Make sure you mark this as requiring documentation. I would have said the same thing about MongoDBFunctions. It's not the name change. It's the fact that it won't appear readily in Intellisense.

rstam · 2023-12-24T19:56:53Z

The latest commit explores Alex's suggestion to define IsMissing and IsNullOrMissing as extension methods against a dummy MongoDBFunctions class. This idea comes from how EF Core does something similar.

This is just a prototype of such an approach. I need you to review the approach and the naming even more than reviewing the code itself. If we use this approach we are committing to it for future methods.

rstam · 2023-12-26T16:26:18Z

The latest commit adds an Exists method to test for the existence of a field.

Using Exists is more convenient than !IsMissing.

sanych-sun

LGTM

JamesKovacs

LGTM

JamesKovacs · 2024-01-11T00:41:12Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+            var collection = GetCollection();
+
+            var queryable = collection.AsQueryable()
+                .Select(x => MongoDBLinq.Functions.IsMissing(x.S));


MongoDBLinq.Functions.IsMissing (and friends) isn't readily discoverable. Make sure you mark this as requiring documentation. I would have said the same thing about MongoDBFunctions. It's not the name change. It's the fact that it won't appear readily in Intellisense.

damieng · 2024-01-11T08:09:06Z

MongoDBLinq.Functions.IsMissing (and friends) isn't readily discoverable. Make sure you mark this as requiring documentation. I would have said the same thing about MongoDBFunctions. It's not the name change. It's the fact that it won't appear readily in Intellisense.

This is unfortunately the case and has been since LINQ was introduced. There's no way to bring a function like this into scope and be discoverable only inside expressions so functions like SqlFunctions (EF 4) and EF.Functions (EF Core) end up being introduced.

While it's not that discoverable there are at least similarities elsewhere in .NET like the Math static class with it's less-regularly-used math operations rather than them existing on the numeric data types themselves.

JamesKovacs

Renaming LGTM.

BorisDog · 2024-01-19T17:49:05Z

src/MongoDB.Driver/Linq/Mql.cs

+    /// Contains methods that can be used to access MongoDB specific functionality in LINQ queries.
    /// </summary>
-    public static class MongoDBLinqFunctionsExtensions
+    public static class Mql


Should Mql be under Linq namespace? Would it possible to use with Mql with Builders, even though it uses LINQ machinery?

Originally I would have said yes to putting this in the Linq namespace.

This was discussed and others felt it shouldn't be in the Linq namespace because you can use this with builders (and the LINQ machinery is incidental).

What do others think about which namespace this should be in?

Sorry I misunderstood your question.

It currently IS under the MongoDB.Driver.Linq namespace.

Should we move it up one level?

I'd go with bringing it up one level. Even when they are doing LINQ they may well have not brought the MongoDB.Driver.Linq namespace in.

It seems that it should be moved up, to be more easily discoverable for Builder users. Unless it can be used only with Linq.

BorisDog · 2024-01-19T18:46:31Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+                    _ => throw new InvalidOperationException("Unexpected method."),
+                }; ;
+
+                return new AggregationExpression(expression, ast, BooleanSerializer.Instance);


Does Is/IsOneOf invocation result in simple comparison or in reflection call to GetGenericMethodDefinition, which might not be not ideal to do twice?
Consider refactoring to eliminate second call to Is and remove the __isMissingMethods field.

public static AggregationExpression Translate(TranslationContext context, MethodCallExpression expression) { var method = expression.Method; var arguments = expression.Arguments; var ast = method switch { _ when method.Is(MqlMethod.Exists) => AstExpression.Ne(GetFieldExpression(), "missing"), _ when method.Is(MqlMethod.IsMissing) => AstExpression.Eq(GetFieldExpression(), "missing"), _ when method.Is(MqlMethod.IsNullOrMissing) => AstExpression.In(GetFieldExpression(), new BsonArray { "null", "missing" }), _ => throw new ExpressionNotSupportedException(expression) }; return new AggregationExpression(expression, ast, BooleanSerializer.Instance); static AstExpression GetFieldExpression() { ... } }

We haven't worried to much about this in other translators.

The initial test is to ensure it's one of the methods that this translator knows how to handle.

In other translators that would be it, but in this one there are slight variations depending on which method is being translated.

I will refactor a slightly different way.

BorisDog · 2024-01-19T18:48:39Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+
+namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
+{
+    public class CSharp4858Tests : Linq3IntegrationTest


Maybe MqlTests/MqlMethodsTest?
Seems that new functionality is significant enough to justify its own test class.

Tests moved to FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs

BorisDog · 2024-01-19T18:54:36Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+
+namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.MethodTranslators
+{
+    internal static class FieldExistsOrIsMissingMethodToAggregationExpressionTranslator


Looks like Exits and IsMissing* are invoked from different code paths, and the only common thing is the usage of MqlMethods.
Should this translator be called something like MqlMethodsTranslator\ FieldMqlMethodsTranslator. Or split to Exists and IsMissing translators?

We name these classes after the method(s) they translate.

We wouldn't want to group ALL of them into a single MqlMethodsTranslator.

We could separate translation of Exists and IsMissing but they have so much in common that there is good opportunity to share code here.

Got it.
So any new method in MqlMethods, would require to implement a new translator, and could not take advantage of code reuse here? Or we don't expect new methods for the same form?

Sharing a translator for multiple methods is only reasonable when all the methods have something in common. In this case the thing they have in common is that they all do some kind of existence tests on a field.

The usual case is that a new method requires a new translator.

Just trying to think what happens when a method that has similar translation will be added to Mql, and FieldExistsOrIsMissingMethodToAggregationExpressionTranslator would be the most natural place for it.

Which might be the case for many potential methods in Mql, or is it unlikely at this point?

In general when we add new MQL methods they won't have anything in common with these three.

There will be all sorts of combinations of return types, number of arguments, argument types, argument interpretations (i.e. in this case they must idenitfy a field) and all kinds of different translations.

I don't anticipate supporting any new methods in this translator.

rstam · 2024-01-19T19:09:41Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+
+namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.MethodTranslators
+{
+    internal static class FieldExistsOrIsMissingMethodToAggregationExpressionTranslator


We name these classes after the method(s) they translate.

We wouldn't want to group ALL of them into a single MqlMethodsTranslator.

We could separate translation of Exists and IsMissing but they have so much in common that there is good opportunity to share code here.

rstam · 2024-01-19T19:16:17Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+                    _ => throw new InvalidOperationException("Unexpected method."),
+                }; ;
+
+                return new AggregationExpression(expression, ast, BooleanSerializer.Instance);


We haven't worried to much about this in other translators.

The initial test is to ensure it's one of the methods that this translator knows how to handle.

In other translators that would be it, but in this one there are slight variations depending on which method is being translated.

I will refactor a slightly different way.

rstam · 2024-01-19T19:17:50Z

src/MongoDB.Driver/Linq/Mql.cs

+    /// Contains methods that can be used to access MongoDB specific functionality in LINQ queries.
    /// </summary>
-    public static class MongoDBLinqFunctionsExtensions
+    public static class Mql


Originally I would have said yes to putting this in the Linq namespace.

This was discussed and others felt it shouldn't be in the Linq namespace because you can use this with builders (and the LINQ machinery is incidental).

What do others think about which namespace this should be in?

rstam · 2024-01-19T19:20:14Z

tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Jira/CSharp4858Tests.cs

+
+namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira
+{
+    public class CSharp4858Tests : Linq3IntegrationTest


Tests moved to FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs

rstam · 2024-01-19T19:30:04Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+                {
+                    nameof(Mql.Exists) => AstExpression.Ne(AstExpression.Type(fieldAst), "missing"),
+                    nameof(Mql.IsMissing) => AstExpression.Eq(AstExpression.Type(fieldAst), "missing"),
+                    nameof(Mql.IsNullOrMissing) => AstExpression.In(AstExpression.Type(fieldAst), new BsonArray { "null", "missing" }),


It's safe to use string comparisons here because line 42 verified that it REALLY is one of the supported methods and not just one that is named alike.

damieng · 2024-01-19T20:22:36Z

src/MongoDB.Driver/Linq/Mql.cs

+        /// <returns>True if the field is null missing.</returns>
+        public static bool IsNullOrMissing<TField>(TField field)
+        {
+            throw new NotSupportedException("This method is not functional. It is only usable in MongoDB LINQ queries.");


Might be worth having the method name explicitly in the message, e.g.

static InvalidOperationException BuildException([CallerMemberName] string memberName = null) { return new InvalidOperationException($"Mql.{memberName} is a method for mapping queries to MongoDB and should not be called directly client-side."); }

EF goes an InvalidOperationException with the message:

The '{methodName}' method is not supported because the query has switched to client-evaluation. This usually happens when the arguments to the method cannot be translated to server. Rewrite the query to avoid client evaluation of arguments so that method can be translated to server.

This is a good suggestion. Maybe we need a separate JIRA ticket for it.

This exception was copied verbatim from existing custom methods.

Changing the exception type would normally be considered a breaking change. Changing the message is somewhat less breaking.

BorisDog · 2024-01-19T20:21:40Z

...anslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslator.cs

+
+namespace MongoDB.Driver.Linq.Linq3Implementation.Translators.ExpressionToAggregationExpressionTranslators.MethodTranslators
+{
+    internal static class FieldExistsOrIsMissingMethodToAggregationExpressionTranslator


Got it.
So any new method in MqlMethods, would require to implement a new translator, and could not take advantage of code reuse here? Or we don't expect new methods for the same form?

BorisDog · 2024-01-19T20:23:09Z

src/MongoDB.Driver/Linq/Mql.cs

+    /// Contains methods that can be used to access MongoDB specific functionality in LINQ queries.
    /// </summary>
-    public static class MongoDBLinqFunctionsExtensions
+    public static class Mql


It seems that it should be moved up, to be more easily discoverable for Builder users. Unless it can be used only with Linq.

BorisDog · 2024-01-19T20:24:59Z

...tors/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs

+using MongoDB.Driver.Linq;
+using Xunit;
+
+namespace MongoDB.Driver.Tests.Linq.Linq3Implementation.Jira


Need to adjust the namespace as well.

Good catch.

BorisDog

Few minor comments.

BorisDog · 2024-01-19T23:46:47Z

src/MongoDB.Driver/Mql.cs

+        /// </summary>
+        /// <typeparam name="TField">The type of the field.</typeparam>
+        /// <param name="field">The field.</param>
+        /// <returns>True if the field is missing.</returns>


field exists

BorisDog · 2024-01-19T23:47:02Z

src/MongoDB.Driver/Mql.cs

+        /// </summary>
+        /// <typeparam name="TField">The type of the field.</typeparam>
+        /// <param name="field">The field.</param>
+        /// <returns>True if the field is null missing.</returns>


null or missing?

BorisDog · 2024-01-19T23:48:12Z

src/MongoDB.Driver/Mql.cs

+        /// </summary>
+        /// <typeparam name="TField">The type of the field.</typeparam>
+        /// <param name="field">The field.</param>
+        /// <returns>True if the field is missing.</returns>


<c>true</c> in all comments?

Done (in this file)

BorisDog

LGTM

damieng · 2024-01-20T23:25:29Z

LGMT

sanych-sun

LGTM

JamesKovacs

LGTM

JamesKovacs

Just noticed that the patch is failing on Windows due to a path length issue.

[2024/01/19 17:16:15.096] LibGit2Sharp.LibGit2SharpException: path too long: 'C:/data/mci/98efc01bfb8b1056ce0fd6be52c3b4a2/mongo-csharp-driver/tests/MongoDB.Driver.Tests/Linq/Linq3Implementation/Translators/ExpressionToAggregationExpressionTranslators/MethodTranslators/FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs'

…when building on Evergreen.

rstam requested a review from a team as a code owner November 28, 2023 22:10

rstam requested review from adelinowona and removed request for a team and adelinowona November 28, 2023 22:10

rstam commented Nov 28, 2023

View reviewed changes

rstam requested review from JamesKovacs and damieng November 28, 2023 22:17

rstam commented Dec 1, 2023

View reviewed changes

JamesKovacs approved these changes Dec 7, 2023

View reviewed changes

rstam commented Dec 24, 2023

View reviewed changes

rstam requested review from BorisDog, JamesKovacs and sanych-sun December 24, 2023 19:57

rstam mentioned this pull request Dec 27, 2023

CSHARP-4540: Add full support for $dateFromString. #1242

Merged

sanych-sun approved these changes Dec 28, 2023

View reviewed changes

JamesKovacs approved these changes Jan 11, 2024

View reviewed changes

rstam requested review from JamesKovacs and sanych-sun January 11, 2024 16:35

JamesKovacs approved these changes Jan 11, 2024

View reviewed changes

rstam requested a review from JamesKovacs January 18, 2024 17:15

BorisDog requested changes Jan 19, 2024

View reviewed changes

rstam commented Jan 19, 2024

View reviewed changes

rstam requested a review from BorisDog January 19, 2024 19:30

damieng reviewed Jan 19, 2024

View reviewed changes

BorisDog requested changes Jan 19, 2024

View reviewed changes

rstam requested review from BorisDog and damieng January 19, 2024 21:34

BorisDog requested changes Jan 19, 2024

View reviewed changes

rstam requested a review from BorisDog January 20, 2024 00:01

BorisDog approved these changes Jan 20, 2024

View reviewed changes

damieng approved these changes Jan 20, 2024

View reviewed changes

sanych-sun approved these changes Jan 23, 2024

View reviewed changes

JamesKovacs approved these changes Jan 23, 2024

View reviewed changes

CSHARP-4858: Add IsMissing and IsNullOrMissing MongoDBFunctions.

1b1934d

JamesKovacs requested changes Jan 23, 2024

View reviewed changes

CSHARP-4858: Shorten long filename to work around Windows limitation …

370e8b0

…when building on Evergreen.

rstam force-pushed the csharp4858 branch from 8619937 to 370e8b0 Compare January 23, 2024 23:39

rstam merged commit 370e8b0 into mongodb:master Jan 24, 2024

CSHARP-4858: Add IsMissing and IsNullOrMissing MongoDBFunctions. #1224

CSHARP-4858: Add IsMissing and IsNullOrMissing MongoDBFunctions. #1224

Uh oh!

Conversation

rstam commented Nov 28, 2023

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!

sanych-sun Dec 7, 2023 • 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!

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!

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!

rstam commented Dec 24, 2023

Uh oh!

rstam commented Dec 26, 2023

Uh oh!

sanych-sun left a comment

Choose a reason for hiding this comment

Uh oh!

JamesKovacs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damieng commented Jan 11, 2024

Uh oh!

JamesKovacs 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!

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!

sanych-sun Dec 7, 2023 •

edited

Loading