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.

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

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 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.

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.

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.

Done.

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.

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

Tests moved to FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs

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.

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.

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.

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?

Tests moved to FieldExistsOrIsMissingMethodToAggregationExpressionTranslatorTests.cs

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.

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.

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?

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

Need to adjust the namespace as well.

Good catch.

field exists

Good catch

null or missing?

Good catch

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

Done (in this file)