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

#14 Include(...) after AsExpandable() #164

Merged

Conversation

doboczyakos
Copy link
Contributor

Fixed

@StefH
Copy link
Collaborator

StefH commented Jan 19, 2022

Hello @doboczyakos,

Thanks for this PR.

Is it possible that you create a console app which identifies this problem? And make a new PR for this on master.

Then we are sure that this PR does fix the issue.

And is there also some documentation which needs to be updated?

@doboczyakos
Copy link
Contributor Author

We identified the problem in our enterprise application and tested this fix with a forked package.
Are you sure you want a console application or would it be better to add a new testcase for this?
The only thing you have to document is that .AsExpandable().Include() is working with EF Core from now on... but it uses internal API, there's no other option to fix it, so it can cause build error with future versions of EF Core if they modify this API. Anyway it's the same in all EF Core versions until 6 so I don't expect breaking change.

@StefH
Copy link
Collaborator

StefH commented Jan 19, 2022

Are you sure you want a console application or would it be better to add a new testcase for this?

If a unit-test would be possible, that is preferred.

@doboczyakos
Copy link
Contributor Author

Microsoft.EntityFrameworkCore.Query.Internal is being used since 2016 in LinqKit so I don't think it's something we should be afraid of.
I'm committing a testcase to this PR today

@doboczyakos
Copy link
Contributor Author

doboczyakos commented Jan 19, 2022

Sorry, I create a new branch for the test to be able to test it with both versions

@sdanyliv
Copy link
Contributor

sdanyliv commented Jan 19, 2022

@doboczyakos, optionsBuilder.WithExpressionExpanding() is not suitable for you?

@davidnemeti
Copy link

@sdanyliv, although optionsBuilder.WithExpressionExpanding() works properly with .Include(); however we have several places where we use explicit .AsExpandable() in common code. In these common code we cannot apply optionsBuilder.WithExpressionExpanding(), because we do not have access to the concrete DbContext class there. Still, the common code use .Invoke() heavily, so .AsExpandable() is necessary; and we should not assume that the code that uses it is going to use optionsBuilder.WithExpressionExpanding().

Moreover, the situation now is very fragile, because as soon as a developer writes .AsExpandable().Include() explicitly in the code, then the .Include() behavior will not work, regardless the use of optionsBuilder.WithExpressionExpanding().

@sdanyliv
Copy link
Contributor

You are right. I don‘t know why Include code i so restrictive to IQueryable implementation.

@doboczyakos
Copy link
Contributor Author

@StefH #165

@davidnemeti
Copy link

@StefH , how is the merge of the pull request going? Can we help anything?

@StefH
Copy link
Collaborator

StefH commented Feb 9, 2022

@davidnemeti
I'll first merge the unit-test PR and then this one.

@StefH StefH merged commit 1ba5030 into scottksmith95:master Feb 19, 2022
@doboczyakos
Copy link
Contributor Author

What's going on with .netcoreapp 2.1?
"Testhost process exited with error: It was not possible to find any compatible framework version
The framework 'Microsoft.NETCore.App', version '2.1.0' (x64) was not found."

@doboczyakos
Copy link
Contributor Author

dotnet/core#6640

@StefH
Copy link
Collaborator

StefH commented Feb 19, 2022

@doboczyakos
I see.

I'll remove this test and replace it by other frameworks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants