Query: Top projection should fully materialize and track entities before they are passed to client methods

[RoshanJeewantha]

EFCore throws DetachedLazyLoading exception when I try something like below,

var   employeeDtos = (from device in context.Set<EmployeeDevice>()
                                select DtoFactory.CreateEmployeeDto(device)).ToList();

Is this by design? This is a big problem for us as we are trying to migrate an existing code-base with Linq2Sql (this was a common pattern through out that code-base).

Exception message:

System.InvalidOperationException: 'Error generated for warning 'Microsoft.EntityFrameworkCore.Infrastructure.DetachedLazyLoadingWarning: An attempt was made to lazy-load navigation property 'Employee' on detached entity of type 'EmployeeDevice'. Lazy-loading is not supported for detached entities or entities that are loaded with 'AsNoTracking()'.'. This exception can be suppressed or logged by passing event ID 'CoreEventId.DetachedLazyLoadingWarning' to the 'ConfigureWarnings' method in 'DbContext.OnConfiguring' or 'AddDbContext'.'

Stack trace:

   at Microsoft.EntityFrameworkCore.Diagnostics.EventDefinition`2.Log[TLoggerCategory](IDiagnosticsLogger`1 logger, WarningBehavior warningBehavior, TParam1 arg1, TParam2 arg2, Exception exception)
   at Microsoft.EntityFrameworkCore.Internal.CoreLoggerExtensions.DetachedLazyLoadingWarning(IDiagnosticsLogger`1 diagnostics, DbContext context, Object entityType, String navigationName)
   at Microsoft.EntityFrameworkCore.Internal.LazyLoader.ShouldLoad(Object entity, String navigationName, NavigationEntry& navigationEntry)
   at Microsoft.EntityFrameworkCore.Internal.LazyLoader.Load(Object entity, String navigationName)
   at TestHostForCastException.EmployeeDevice.get_Employee() in C:\EFCoreTestBed\DataContext.cs:line 56
   at TestHostForCastException.DtoFactory.CreateEmployeeDto(EmployeeDevice device) in C:\EFCoreTestBed\DtoFactory.cs:line 7
   at lambda_method(Closure , QueryContext , EmployeeDevice )
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ProjectionShaper.TypedProjectionShaper`3.Shape(QueryContext queryContext, ValueBuffer& valueBuffer)
   at Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.ProjectionShaper.TypedProjectionShaper`3.Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal.IShaper<TOut>.Shape(QueryContext queryContext, ValueBuffer& valueBuffer)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.Enumerator.BufferlessMoveNext(DbContext _, Boolean buffer)
   at Microsoft.EntityFrameworkCore.Storage.Internal.NoopExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded)
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.Enumerator.MoveNext()
   at Microsoft.EntityFrameworkCore.Query.Internal.LinqOperatorProvider.ExceptionInterceptor`1.EnumeratorExceptionInterceptor.MoveNext()

Steps to reproduce

Repro included at https://github.com/avinash-phaniraj-readify/EFCoreTestBed/tree/DetachedNavProperty

Further technical details

EF Core version: 2.1
Database Provider: EntityFrameworkCore.SqlServerCompact35
Operating system: Windows 10.0.17134
IDE: Visual Studio 2017 15.7.4

[ErikEJ]

Maybe just rephrase to:

var   employeeDtos = (from device in context.Set<EmployeeDevice>().ToList()
                            select DtoFactory.CreateEmployeeDto(device)).ToList();

[RoshanJeewantha]

Thanks @ErikEJ for your response. Yes, indeed we could do that. But our problem is the massive (millions of LOC) legacy code-base which does not have full test coverage. We can try to fix places we know, but there is a good chance of missing many other places.

Is there any possibility that EFCore will support this kind of usage in the future?

[ajcvickers]

@RoshanJeewantha Have you tried disabling the exception as detailed in the message?

[optiks]

@ajcvickers The data needs to be loaded, so suppressing the exception won’t help.

In the query above, is it possible to force device to be tracked somehow? If required, we could do query rewriting to include an additional method (e.g. TrackProjectionInputs()).

[smitpatel]

You are using a static method DtoFactory.CreateEmployeeDto which is not of type Expression<Func<>> hence it is opaque to query translation pipeline. That requires us to call the client code (your method) with passing in right parameter. Since it is inside client code while executing queries, the navigations won't be populated in it. Only navigations which are visible to EF Core in Expression tree are expanded and filled in appropriately.

You can convert your method to be of type Expression<Func<>> then EF Core can use the method inside ExpressionTree. Or as @ErikEJ posted, you need to call ToList() before writing custom select like that, (which has no perf penalty since we are going to do the same under the hood for opaque methods). And to have navigations populated before calling into your method, you can use Include before calling first ToList.

[optiks]

@smitpatel I appreciate the predicament, but the reality is that LINQ to SQL dealt with this scenario. I acknowledge the workarounds, I'm just wondering if there's a more global way of solving this.

It seems to me that, logically, we're asking for the LazyLoader.ShouldLoad() code to effectively be:

if (!entityIsTracked && !entityIsReferencedInProjection) {
   // add DetachedLazyLoadingWarning
}

How you'd determine entityWasProjected... I don't know. I'd imagine if you don't want to treat the entity as tracked (which is fair enough), that the entity could still be recorded as projected.

I could be totally misunderstanding, but as I said, LINQ to SQL was able to solve this problem (and I get that they're totally different libraries).

Looking forward to hearing your thoughts.

[optiks]

I'm thinking about doing this via expression re-writing. Is there a way of identifying whether a given expression is going to result in client-side execution?

[divega]

@optiks, @smitpatel and I talked about this and we are not sure there is a way to workaround it by only rewriting the expression.

Given the current behavior of EF Core, the entity needs to be tracked in order for lazy loading to work, so you need to make sure the entity is returned in the top-most projection that EF Core executes. Also, you need to invoke the method after EF Core has finished materialized that row. I.e. each of the IEnumerable.MoveNext() methods from EF Core need to return before you can invoke the client method.

That leaves you with something like this:

var employees = from device in context.Set<EmployeeDevice>() select device; 
var employeeDtos = employees
    .AsEnumerable() 
    .Select(device => DtoFactory.CreateEmployeeDto(device))
    .ToList();

Of course if you know beforehand what navigation properties CreateEmployeeDto is going to dereference, you may choose to use Include() to make sure they are loaded, but that is an orthogonal.

Anyway, I would like to clarify that I agree that some of the design choices we made in the EF Core implementaiton (FWIW, a lot of the complexity derives from the idea that that devices should not be tracked because it isn't in the results of the final projection) make dealing with this particular scenario a bit difficult. We are going to keep this scenario in mind for some thinking we are doing to improve (and hopefully simplify) the design of our LINQ implementation in EF Core 3.0, which we are tracking at #12795.

[optiks]

@divega, just to clarify, the expression re-writing would be to transparently insert the .ToList().AsQueryable().

Where it gets tricky is that some methods are evaluated and then passed into the resultant SQL query as parameters. e.g. .Where(x => x.SomeDate < DateUtility.Now()). In that case we don’t want to force materialisation, because it’ll result in an inefficient query, which is why I was wondering if there’s a way of checking whether a given expression would result in client-side execution.

Cheers.

[divega]

@optiks Besides a few cases explained in #12672, and unless x.SomeDate is a client-only property (e.g. a property that isn't mapped to column in the database) a query like this:

var q = xs.Where(x => x.SomeDate < DateUtility.Now());

Should cause DateUtility.Now() to be evaluated on the client then the value passed to a parameter. I.e. the query should still have a WHERE clause and be efficient.

For the cases in which a filter predicate is really evaluated on the client, EF Core produces client-evaluation warnings. You should definitively check your logs to see if those can occur in cases in which a large number of rows could be returned and filtered on the client. You may even decide to turn those wranings into errors. That is documented at https://docs.microsoft.com/en-us/ef/core/querying/client-eval.

I don't think you need to be concerned as much with that case, because if you are going to try to workaround this you should only be looking at what happens in the top-most Select() anyway.

Besides that, you are not going to be able to resolve this introducing a call to ToList().AsQueryable() in the expression three and then passing that to EF Core's LINQ provider. ToList() in that possition in the expression tree is not supported by EF Core.

Assuming you are already wrapping EF Core's query provider, what you will have to do is is rewrite the query to remove the top Select() from the query expression that you pass to EF Core, then evaluate that projection in memory on your own.

[optiks]

@divega Here's what I was thinking: https://github.com/avinash-phaniraj-readify/EFCoreTestBed/tree/spike/AutomagicMaterialization

Obviously it needs to be generalised, but it appears to work. Can you see foresee any issues with this approach?

[optiks]

@divega When using .ConfigureWarnings(warnings => warnings.Throw(RelationalEventId.QueryClientEvaluationWarning)), it'd be great to be able to not throw for specific usages. Is this possible? Otherwise if client evaluation is required in even one case, throwing would need to be removed.

I'm thinking something like:

var query = 
   context.Set<Device>()
          .Config(c => c.Warnings.Ignore(RelationalEventId.QueryClientEvaluationWarning))
          .Select(SomeFactoryMethod);

[divega]

@optiks I have been also thinking about how to provide more finely-grained control over client evaluation. However my conclusion was that an extension method that you can include in the query (e.g. just to give you an idea AsServerOnly() or AsEvaluateOn(QueryEvaluationEnum)) would be much more approachable. Thoughts on this?

Edit: filed #12844 for this.

[divega]

Here's what I was thinking...

@optiks yes, I think that approach implements the same thing I had in mind. I misunderstood you when you said you would rewrite the expression tree because I assumed that you would then try to pass it to our query provider, but you are actually evaluating ToList().AsQueryable().Select(...) using LINQ to Objects, and only the rest of the query with our provider.

Keep in mind that when you call expression.Compile().Invoke() this is triggering the evaluation of ToList() and that triggers the evaluation of the EF Core side of the query.

[optiks]

@divega, I've now integrated the spike into our code base, and it's looking good 👍 .

[divega]

Cool. Also, consider restricting this approach to only the top-most projection. AFAIR, that should be sufficient for LINQ to SQL parity.

[optiks]

@divega, yep, done. I've got some test cases if you're interested.

[divega]

@optiks I have been thinking a bit more about the workaround and wondering about its performance impact. As usual, measuring is key, avoid premature optimization, etc., but if the impact is significant, and if you are not doing this already, consider that you shouldn't need to apply the workaround to all queries. You can probably add logic to recognize the pattern to decide whether you need it.

[optiks]

Thanks @divega. I've emailed some more context around our specific workaround.

[optiks]

We ended up giving up on this approach, as we lost the EF Core null-coalescing benefits. We've now also increased the scope of our transformation to deal with existing performance issues, which means we now Include() any required navigations up-front.

This is still a design issue though IMO.