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

Allow FromSql on IQueryable<T> #18720

Closed
Neme12 opened this issue Nov 2, 2019 · 8 comments
Closed

Allow FromSql on IQueryable<T> #18720

Neme12 opened this issue Nov 2, 2019 · 8 comments

Comments

@Neme12
Copy link

Neme12 commented Nov 2, 2019

EF Core 3.0 made a change to remove FromSql on IQueryable<T> and replace it with FromSql* methods on DbSet<T> itself, so it's no longer composable. Why is this no longer supported?

My use case was:
I was overriding ASP.NET Core's UserStore like this:

public override IQueryable<ApplicationUser> Users => base.Users.Include(u => u.UserProfiles);

to make sure that the UserProfiles collection of ApplicationUser is always included, because we are using it everywhere we're using ApplicationUser itself.

In a controller, we implemented search manually using SQL because EF Core did a poor job translating the LINQ we attempted to write:

var users = searchText is null
    ? _userManager.Users
    : _userManager.Users.FromSql($"...");

Now with EF Core 3.0, I can't use the UserManager abstraction anymore and i'm forced to drill down to the DbContext to get the DbSet itself and copy the Include here. This seems like a regression 😕

@ajcvickers
Copy link
Member

@smitpatel This seems to work correctly in 2.2. That is, an IQueryable that is not a DbSet can have FromSql used on it and the results we get back still honor the Include. If FromSql was allowed on IQueryable, would this work with the 3.0 query pipeline?

@smitpatel
Copy link
Member

If FromSql was allowed on IQueryable, would this work with the 3.0 query pipeline?

Yes, it will work. I believe, when adding the restriction on DbSet, we explicitly decided that user would have to specify FromSql before Include. Are we changing it?

@ajcvickers
Copy link
Member

@smitpatel No decision to change it yet; we will discuss.

@ajcvickers
Copy link
Member

Discussed in triage and given that FromSql always replaces the query root, even if it is not called at the root, we think that it makes sense to keep enforcing that it only be applied to the root. This can break certain ways of writing repositories (or similar) that are valid, like here, but these can generally be refactored to inject the root rather than expose the root as a simple property.

@Neme12
Copy link
Author

Neme12 commented Nov 17, 2019

@ajcvickers Then should it also be disallowed to use FromSql on a DbSet that it mapped to a query (using ToQuery()) for consistency? That seems like it would let people bypass this limitation and have the same effect as using FromSql on an IQueryable created using LINQ.

@ajcvickers
Copy link
Member

@Neme12 This is an interesting point, probably related to #18903 and #17270 which I referenced in another response to you.

Reopening to discuss in triage.

@ajcvickers ajcvickers reopened this Nov 19, 2019
@smitpatel
Copy link
Member

That is a bug which would go away when fixing #18903

@ajcvickers
Copy link
Member

Discussed in triage and, as Smit says, we will handle this as part of #18903.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants