[feature/netcore] An System.AccessViolationException is raised when attempting to use $apply=aggregate

techniq commented 6 years ago

An System.AccessViolationException is raised when attempting to use $apply=aggregate

Assemblies affected

7.0.0-Nightly201801081323

Reproduce steps

Install Remotion.Linq version 2.2.0-alpha-005, which was recently released and implemented System.Linq.Queryable.AsQueryable, which in turn was needed to support OData's $apply=aggregate
Attempt to execute a query with $apply=groupby or $apply=aggregate, for example http://localhost:5000/odata/Employees?$apply=groupby((SupervisorId),aggregate(Id with countdistinct as Total))

Expected result

Result is returned

Actual result

dotnet process crashes. When debugging within Visual Studio on Windows, it appears the following exception is being raised: An unhandled exception of type 'System.AccessViolationException' occurred in Microsoft.AspNetCore.OData.dll Attempted to read or write protected memory. This is often an indication that other memory is corrupt.

Additional detail

More details can be found within https://github.com/aspnet/EntityFrameworkCore/issues/6132#issuecomment-359807356 and related to feature support of https://github.com/OData/WebApi/issues/1154

robward-ms commented 6 years ago

@techniq - Do you have a sample project or controller snippet you can share, specifically the integration with Remotion.Linq?

robward-ms commented 6 years ago

@techniq - I did see a similar issue when using only EFCore and the Aggregation E2E tests. I'll use this bug to track a fix for that. #1154 will track the larger EFCore compatibility issues.

techniq commented 6 years ago

@robward-ms thanks. Do you still need a sample project? It sounds like the E2E tests should show the same. If so, how should I provide it (where to upload, etc)?

Thanks for your help.

robward-ms commented 6 years ago

@techniq - The E2E won't test Remotion.Linq but it will test Aggregation and I did see the same exception you reported here. If you have a project, just push it to a branch/repo under your username and put the link here. Thanks - R

robward-ms commented 6 years ago

@techniq - There are a few errors popping up, let's address each one:

a.) EFCore 2.0 - Aggregation fails due to lack of AsQueryable support, the original for EF Core 6132.: This overload of the method 'System.Linq.Queryable.AsQueryable' is currently not supported.

b.) Remotion.Linq v2.2.0-alpha.5 and EFCore 2.0 - Aggregation fails with System.AccessViolationException in OData. Seen when using these assembly redirect; EF Core 2.0.0 was not released to work with Remotion.Linq v2.2.0. I'm inclined to think of this as a compatibility issues when those two versions.

c.) EF Core 2.1.0-preview2-30148 - This is designed to work with Remotion.Linq v2.2.0-alpha.5 or better and longer throws the NotImplemented exception as in the first case or AccessviolationException as in the second case but also does not return the correct result.

WebApi constructs LINQ queries to run against EF Core and those are failing at the moment. I have seen both invalid column ID and Object reference not set to an instance of an object thrown when enumerating the LINQ query WebApi constructed against EF Core.

So at the moment, I think EF Core 2.1.0 or better is required and there seems to be a bug in the LINQ generation for EF Core. I'll continue to investigate this last issue in the context of this GitHub issue, i.e. I'll not focus on the AccessViolationException or NotImplementedException, let me know if that sounds wrong to you.

techniq commented 6 years ago

That all makes since and sounds good to me. Thanks for digging into it.

On Tue, Feb 20, 2018, 8:07 PM Rob Ward notifications@github.com wrote:

@techniq https://github.com/techniq - There are a few errors popping up, let's address each one:

a.) EFCore 2.0 - Aggregation fails due to lack of AsQueryable support, the original for EF Core 6132. https://github.com/aspnet/EntityFrameworkCore/issues/6132: This overload of the method 'System.Linq.Queryable.AsQueryable' is currently not supported.

b.) Remotion.Linq v2.2.0-alpha.5 and EFCore 2.0 - Aggregation fails with System.AccessViolationException in OData. Seen when using these assembly redirect; EF Core 2.0.0 was not released to work with Remotion.Linq v2.2.0. I'm inclined to think of this as a compatibility issues when those two versions.

c.) EF Core 2.1.0-preview2-30148 - This is designed to work with Remotion.Linq v2.2.0-alpha.5 or better and longer throws the NotImplemented exception as in the first case or AccessviolationException as in the second case but also does not return the correct result.

WebApi constructs LINQ queries to run against EF Core and those are failing at the moment. I have seen both invalid column ID and Object reference not set to an instance of an object thrown when enumerating the LINQ query WebApi constructed against EF Core.

So at the moment, I think EF Core 2.1.0 or better is required and there seems to be a bug in the LINQ generation for EF Core. I'll continue to investigate this last issue in the context of this GitHub issue, i.e. I'll not focus on the AccessViolationException or NotImplementedException, let me know if that sounds wrong to you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OData/WebApi/issues/1221#issuecomment-367178345, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK1RIAQtHueJWRpuDLFBri6ry9t1uCfks5tW2xYgaJpZM4Rpv8v .

robward-ms commented 6 years ago

@techniq - After looking at it a bit more, I think what I'm now seeing could present itself as an AccessViolationExceptionso it's possible that case b.) and c.) are the same. Either way, I'll sort it out.

robward-ms commented 6 years ago

@techniq - OK, what I'm seeing is not something that would show up as an AV, it's caused by lack of lazy-loading in EFCore:

### Lazy loading
Lazy loading is not yet supported by EF Core. You can view the lazy loading item on our backlog to track [this feature](https://github.com/aspnet/EntityFramework/issues/3797).

In the AggregateNavigationPropertyWorks test, the Customers IQueryable<> is returned from the controller without any Order objects. When then EnableQueryAttribute class runs the query, there is no result since the Orders are null.

First thing I tried as forcing the result to include Orders, i.e. db.Customers.Include(c => c.Order), but that led to an exception about some object not implementing IComparable.

I was able to get the test to pass only by running the query over an in-memory IQueryable, i.e. db.Customers.Include(c => c.Order).ToList().AsQueryable(). This does not resemble a reasonable work-around.

I have a branch with the EFCore version of Aggregtion tests + solution here: https://github.com/robward-ms/WebApi/tree/NetCore-E2ECore-EFCore

divega commented 6 years ago

@robward-ms are you still seeing these issues with EF Core 2.1 RC1? BTW, we have support for lazy loading, but it needs to be enabled explicitly.

First thing I tried as forcing the result to include Order ... but that led to an exception about some object not implementing IComparable

I would expect this to work. Where is the exception coming from?

EF Core 2.1 also supports translating LINQ GroupBy with aggregates to GROUP BY in SQL. I am not super familiar with OData aggregate, but wouldn't it be ideal to leverage is rather than computing the count in memory?

techniq commented 6 years ago

Still crashing/killing the process when running on dotnet/EF Core/ASP.NET 2.1.1 with OData 7.0.0-beta4

kosinsky commented 5 years ago

Fix #1728 was merged. Could you validate with 7.2.x?

Nerevarin117 commented 4 years ago

I also had this issue and upgrading to 7.2.x fixed it, thanks!

OData / WebApi