PawelGerr / Thinktecture.EntityFrameworkCore

These libraries extend Entity Framework Core by a few features to make it easier to work with EF and for easier integration testing or to get more performance in some special cases.
https://dev.azure.com/pawelgerr/Thinktecture.EntityFrameworkCore
BSD 3-Clause "New" or "Revised" License
66 stars 17 forks source link

Can't cast inside OrderBy clause - throws exception #17

Closed virzak closed 2 years ago

virzak commented 2 years ago

Here is a failing test case, which should pass

[Fact]
public void Throws_if_RowNumber_contains_NewExpression_with_Cast()
{
    ArrangeDbContext.TestEntities.Add(new TestEntity { Id = new Guid("4883F7E0-FC8C-45FF-A579-DF351A3E79BF"), Name = "1", Count = 1 });
    ArrangeDbContext.TestEntities.Add(new TestEntity { Id = new Guid("18C13F68-0981-4853-92FC-FB7B2551F70A"), Name = "1", Count = 2 });
    ArrangeDbContext.SaveChanges();

    var query = ActDbContext.TestEntities
                            .Select(e => new
                            {
                                e.Count,
                                RowNumber = EF.Functions.RowNumber(new { e.Name, e.Count },
                                                                               EF.Functions.OrderBy(e.Name).ThenBy((long)e.Count))
                            });
    query.Invoking(q => q.ToList()).Should().Throw<NotSupportedException>()
         .WithMessage("The EF function 'RowNumber' contains some expressions not supported by the Entity Framework. One of the reason is the creation of new objects like: 'new { e.MyProperty, e.MyOtherProperty }'.");
}

Looks like it is caused by the null mappings in the following line:

return new RowNumberExpression(partitionBy, orderings.Orderings, RelationalTypeMapping.NullMapping);

The exception is thrown here:

https://github.com/dotnet/efcore/blob/70cd92974e09e938a92384e91b5c3179f4773725/src/EFCore.Relational/Query/QuerySqlGenerator.cs#L681

PawelGerr commented 2 years ago

Hi, the reason was something else. EF is not adding the type mappings to conversions (like (long)e.Count ) if the target type (long) has a "default mapping".

/* From RelationalSqlTranslatingExpressionVisitor.cs */

// Introduce explicit cast only if the target type is mapped else we need to client eval
if (unaryExpression.Type == typeof(object)
    || Dependencies.TypeMappingSource.FindMapping(unaryExpression.Type, Dependencies.Model) != null)
{
     sqlOperand = _sqlExpressionFactory.ApplyDefaultTypeMapping(sqlOperand);

     return _sqlExpressionFactory.Convert(sqlOperand!, unaryExpression.Type, typeMapping: null); // <-- the root cause
}

Now, I check whether the conversion has a type mapping and if not then I add it explicitly.

The version 4.0.0-beta06 is on the way.

virzak commented 2 years ago

Thanks for the prompt response and the commit. Would you say that this is a bug in EF Core?

PawelGerr commented 2 years ago

I can't say. Probably EF is handling this case later during the query translation.

virzak commented 2 years ago

Filed this behaviour at dotnet/efcore#27075

virzak commented 2 years ago

The workaround only good if the conversion happens at the top level expression. Here is another failing test case.

.ThenBy((long)e.ConvertibleClass + 1)

Doesn't seem like it should be on the plugin developer to recursively adjust type mappings.

PawelGerr commented 2 years ago

Yes, less work for me :)

PawelGerr commented 2 years ago

Everything should be working now.