Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support $apply for EF Core #1728

Merged
merged 11 commits into from Jul 1, 2019

Conversation

kosinsky
Copy link
Contributor

@kosinsky kosinsky commented Jan 9, 2019

Issues

This pull request fixes issues #1154 and #1257. Contributes to #669

Description

We are generating expressions that look like

Next = new AggregationPropertyContainer() {
       Name = "Prop2",
       Value = $it.Prop2,
       Next = …
}                    

Value type is object, as a result proper .NET expression must look like as

Next = new AggregationPropertyContainer() {
       Name = "Prop2",
       Value = (object) $it.Prop2,
       Next = …
}    

or AccessViolationException will be thrown when executing on real objects. On other hand, casts to object isn't translatable by EF6 as a result skipping (object) in that case.
We used to do that adjustment for Linq to Objects, EF Core could execute some parts client side and uses linq to object as result.

Also had to tune how aggregation on top of IGrouping works. Linq to Objects/EF Core and EF 6 has different limitations

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

@kosinsky kosinsky changed the title Suuport $apply for EF Core Support $apply for EF Core Jan 9, 2019
@jannikbuschke
Copy link

@kosinsky @robward-ms is there anything stopping from releasing this fix? Im eagerly waiting for $apply 👍

@madansr7
Copy link
Contributor

@jannikbuschke, we have been working to release a few important updates in ODL so haven't reviewed this one yet. WebAPI's next release would be in march and i am hoping this PR will be part of that release.

@xuzhg
Copy link
Member

xuzhg commented Feb 19, 2019

It seems good to me. @mikepizzo @KanishManuja-MS Would you take a look?

@@ -30,7 +30,7 @@ internal class AggregationBinder : ExpressionBinderBase

private Type _groupByClrType;

private bool _linqToObjectMode = false;
private bool _classicEF = false;
Copy link
Member

@mikepizzo mikepizzo Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we effectively switch the default here, since _linqToObjectMode has the same default value, but the opposite behavior, of _classicEF. That's fine, as long as bind is called to explicitly set this value. I assume there are no scenarios where this value is used without first calling bind? #Closed

Copy link
Contributor Author

@kosinsky kosinsky Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New default behavior is correct form .NET point of view. Expression like: object a = 15; must look like object a = (object) 15; on MSIL to do proper boxing or AccessViolationException will be thrown. Unfortunately, EF6 can't translate that to SQL. So we have to do hack for EF6. The same for IEnumerable and IGroupingBy

#Closed

@kosinsky kosinsky force-pushed the users/kokosins/ApplyWithEfCore branch from 3c4e142 to dab67f1 Compare March 4, 2019 20:58
@KanishManuja-MS KanishManuja-MS added this to the 7.2 milestone Apr 3, 2019
@kosinsky
Copy link
Contributor Author

@mikepizzo do you have any other feedback to this PR?

@mikepizzo mikepizzo added the Ready for review Use this label if a pull request is ready to be reviewed label Jun 28, 2019
Copy link
Member

@mikepizzo mikepizzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@TehWardy
Copy link

TehWardy commented Aug 7, 2020

Is this part of the current version of Microsoft.AspNetCore.OData ?
I'm using version 7.4.1 at the moment and I can't seem to get any form aggregation / apply operation working.

@TehWardy
Copy link

TehWardy commented Aug 7, 2020

It looks like the limitation here may be in EF in my case ...

based on the above my query:

?$apply=groupby((DebtorName),aggregate($count as Count))

should be fine within odata, the exception given (as shown below) implies that this is a known issue with EF ...

2020-08-07 20:08:29,196 [9] ERROR  Api.IAppBuilderExtensions - Processing of the LINQ expression '(GroupByShaperExpression:
KeySelector: new GroupByWrapper{ GroupByContainer = new LastInChain{ 
        Name = (N'DebtorName'), 
        Value = (a.DebtorName) 
    }
     }
, 
ElementSelector:(EntityShaperExpression: 
    EntityType: ActiveTransaction
    ValueBufferExpression: 
        (ProjectionBindingExpression: EmptyProjectionMember)
    IsNullable: False
)
)' by 'RelationalProjectionBindingExpressionVisitor' failed. This may indicate either a bug or a limitation in EF Core. See https://go.microsoft.com/fwlink/?linkid=2101433 for more detailed information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants