CSHARP-4656 Simplify A : "$A" to A : 1 only on find

[sanych-sun]

No description provided.

[rstam]

I expected this extra step to be in TranslateExpressionToFindProjection only.

[sanych-sun]

Right, fixed

[rstam]

I like your idea of replacing a regular expression projection definition with a FindExpressionProjectionDefinition when we know we're doing a Find.

[rstam]

Add copyright

[sanych-sun]

Done

[rstam]

Add copyright

[sanych-sun]

Done

[rstam]

I would refactor these two tests to be:

[Fact]
public void Projection_to_class_should_work()
    => AssertProjection(
        x => new Projection { A = x.A, X = x.B },
        "{ A : 1, X : '$B', _id : 0 }");

[Fact]
public void Projection_to_anonymous_type_should_work()
    => AssertProjection(
        x => new { x.A, X = x.B },
        "{ A : 1, X : '$B', _id : 0 }");

(Note shorter property names)

[sanych-sun]

Done

[rstam]

I would remove this test

[sanych-sun]

Done

[rstam]

Maybe add a test like this also (probably not in this file?):

[Fact]
public void Find_projection_should_simplify_as_expected()
{
    var collection = GetCollection<Document>();
    Expression<Func<Document, Projection>> expression = x => new Projection { A = x.A, X = x.B };
    var projectionDefinition = new ProjectExpressionProjection<Document, Projection>(expression, new ExpressionTranslationOptions());

    var find = collection.Find("{}").Project(projectionDefinition);

    var translatedProjection = TranslateFindProjection(collection, find);
    translatedProjection.Should().Be("{ A : 1, X : '$B', _id : 0 }");

    var results = find.ToList();
}

[sanych-sun]

Added similar test to FindFluentTests without interacting with the database to follow that test class way. And I agree we might want to have the integration test to cover find as well, but as we discussed it we might want to make a decision on how to share that utility methods before creating such test.

[rstam]

TSource can just be Document. It doesn't have to be a type parameter.

[sanych-sun]

Done

[rstam]

Probably just A and X for field names. One is the same as Document and the other is not.

[sanych-sun]

Done

[rstam]

Let me know if you want to have a Zoom meeting to discuss requested changes.

[rstam]

I don't think this is the best place to put this code. There are other ways to do a Find that don't involve the fluent find API.

I think we should add a new RenderForFind to ProjectionDefinition<TSource, TProjection>. Since this is a new method it won't be a breaking change to anyone using or deriving from this class.

This new method allows us to put in one place the logic for rendering for find instead of for $project.

/// <summary>
/// Renders the projection to a <see cref="RenderedProjectionDefinition{TProjection}"/> with additional simplifications to support Find on older servers.
/// </summary>
/// <param name="sourceSerializer">The source serializer.</param>
/// <param name="serializerRegistry">The serializer registry.</param>
/// <param name="linqProvider">The LINQ provider.</param>
/// <returns>A <see cref="RenderedProjectionDefinition{TProjection}"/>.</returns>
public RenderedProjectionDefinition<TProjection> RenderForFind(IBsonSerializer<TSource> sourceSerializer, IBsonSerializerRegistry serializerRegistry, LinqProvider linqProvider)
{
    if (this is ExpressionProjectionDefinition<TSource, TProjection> expressionProjectionDefinition)
    {
        var findExpressionProjectionDefinition = new FindExpressionProjectionDefinition<TSource, TProjection>(expressionProjectionDefinition.Expression);
        return findExpressionProjectionDefinition.Render(sourceSerializer, serializerRegistry, linqProvider);
    }

    return Render(sourceSerializer, serializerRegistry, linqProvider);
}

[rstam]

Note: I also think we should rename the ProjectExpressionProjection class to simply ExpressionProjectionDefinition.

The new approach is that projections are projections and it is only where they are used (Find vs $project) that determines how they are rendered. We no longer try to make that distinction based on class name alone (because that doesn't work).

[rstam]

Further note: the new RenderForFind methods is the ONE AND ONLY place in the whole code base where it is OK to new up an instance of FindExpressionProjectionDefinition.

[rstam]

The CreateFindOperation method in MongoCollectionImpl needs to be modified to call RenderForFind instead of Render.

[rstam]

Also, TranslateFindProjection in Linq3IntegrationTests needs to be modified to call RenderForFind instead of Render.

[sanych-sun]

Done, as it was discussed offline via virtual method.

[rstam]

This is probably not the best place to be testing this functionality, because there are other ways to do a Find that don't involve the fluent find API.

The fluent find API just ends up calling FindSync so there is probably a better place to test this.

[sanych-sun]

Done

[rstam]

There is still one call to:

new FindExpressionProjectionDefinition

in IFindFluentExtensions.cs.

I think it can be replaced with a call to:

new ExpressionProjectionDefinition

That would mean that the FindExpressionProjectionDefinition class is no longer used at all (and that's a good thing).

We can't really remove the FindExpressionProjectionDefinition class because it is public and we don't know if any users is creating it directly.

[rstam]

This class is no longer used.

Did you not find a new (better) place for the test you removed in this commit? (The test that used this class)

[sanych-sun]

Removed the unused class. Will create tests once we decide how to share that useful helper methods.

[rstam]

nit: for better readability I prefer that parameters either all be on one line or each on a separate line, as in:

internal override RenderedProjectionDefinition<TOutput> RenderForFind(
    IBsonSerializer<TInput> sourceSerializer,
    IBsonSerializerRegistry serializerRegistry,
    LinqProvider linqProvider)

[sanych-sun]

Done

[rstam]

This was a really good suggestion to make this method virtual!

We also discussed overriding this method in subclasses where it doesn't make sense and throw InvalidOperationException.

[sanych-sun]

Done

[rstam]

LGTM

[connorstorer-kbxcom]

There is still one call to:

new FindExpressionProjectionDefinition

in IFindFluentExtensions.cs.

I think it can be replaced with a call to:

new ExpressionProjectionDefinition

That would mean that the FindExpressionProjectionDefinition class is no longer used at all (and that's a good thing).

We can't really remove the FindExpressionProjectionDefinition class because it is public and we don't know if any users is creating it directly.

@rstam @sanych-sun Prior to 2.19.2, (2.19.1) this was valid below, where Projection = Builders<TDocumentEntity>.Projection.Expression returned public FindExpressionProjectionDefinition but now returns sealed ExpressionProjectionDefinition. What's your suggestion on how I can continue to Cast/Select/Project this. This is used within a test suite and I know there are a lot of missing pieces below, but I wanted to get your shot from the hip opinion? Much appreciated, no matter how brutal it is :D! This does seem to be addressed in one case #1146

public void ApplyProjections<TDocumentEntity, TSelectEntity>()
{
    Expression<Func<TDocumentEntity, TSelectEntity>> select = null;
    var findOpts = new FindOptions<TDocumentEntity, TSelectEntity>
    {
        Projection = Builders<TDocumentEntity>.Projection.Expression(select)
    };
    var projectedData = GetData<TDocumentEntity, TSelectEntity, TSelectEntity>(findOpts.Projection);
}

public async Task<List<TReturnEntity>> GetData<TDocumentEntity, TSelectEntity, TReturnEntity>(
    ProjectionDefinition<TDocumentEntity, TSelectEntity> select)
{
    var queryable = _instance.CollectionDocuments.AsQueryable();
    var additionalExp = GetSomeExpression();
    queryable = queryable.Where(additionalExp);
    var expSelect = select as FindExpressionProjectionDefinition<TDocumentEntity, TSelectEntity>;
    return queryable.Select(expSelect.Expression).Cast<TReturnEntity>().ToList();
}

[connorstorer-kbxcom]

Bypassing with some temporary reflection as noted in CSHARP-4860.