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

Basic FromSql() support in new pipeline #15752

Merged
merged 1 commit into from
May 27, 2019
Merged

Basic FromSql() support in new pipeline #15752

merged 1 commit into from
May 27, 2019

Conversation

roji
Copy link
Member

@roji roji commented May 18, 2019

  • Parameterization not implemented (see QueryRewrite: parameter support for FromSql() #15750)
  • FromSqlRaw() and FromSqlInterpolated() are now defined over DbSet<>,
    not IQueryable<>. FromSql() is still defined over IQueryable<> but
    throws if not directly on a DbSet<>.

Closes #15704

(rebased and pushed last minute before boarding a plane, it may be somewhat broken :))

@roji roji requested a review from smitpatel May 18, 2019 23:44
Expression.Constant(sql.Format),
Expression.Constant(sql.GetArguments())));
}

internal static IQueryable<TEntity> FromSqlBackingMethod<TEntity>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this method is internal rather than private?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just so that we can do nameof() on it to identify it (see here).

@roji
Copy link
Member Author

roji commented May 20, 2019

@smitpatel addressed your comments, AFAIK we're now waiting only for @divega's sign-off on the exception message.

@smitpatel
Copy link
Contributor

Microsoft.EntityFrameworkCore.SqliteComplianceTest.All_test_bases_must_be_implemented [FAIL]
�[m�[37m
�[m�[37m -- Missing derived classes for --
�[m�[37m Microsoft.EntityFrameworkCore.Query.AsyncFromSqlQueryTestBase`1
�[m�[37m Expected: False
�[m�[37m Actual: True

@roji
Copy link
Member Author

roji commented May 21, 2019

@smitpatel sorry for the runaround, async tests should now be fully enabled and passing.

@roji
Copy link
Member Author

roji commented May 23, 2019

@divega ping, we need your sign-off on the new exception message

@smitpatel anything remaining? If not can you approve?

@roji
Copy link
Member Author

roji commented May 23, 2019

Rebased on latest master and squashed

@roji roji force-pushed the FromSql branch 2 times, most recently from 6ab73ff to 244613b Compare May 27, 2019 11:30
@roji roji requested a review from dougbu as a code owner May 27, 2019 11:30
* Parameterization not implemented (see dotnet#15750)
* FromSqlRaw() and FromSqlInterpolated() are now defined over DbSet<>,
  not IQueryable<>. FromSql() is still defined over IQueryable<> but
  throws if not directly on a DbSet<>.

Closes dotnet#15704
@khellang
Copy link
Member

Ugh, just updated to Preview 8 and this broke me pretty horribly. Why is

FromSqlRaw() and FromSqlInterpolated() now defined over DbSet<>, not IQueryable<>.

Did I do something wrong earlier, cause it seemed to work fine with IQueryable<>?

@khellang
Copy link
Member

Specifying FromSql anywhere other than on a DbSet had no added meaning or added value, and could cause ambiguity in certain scenarios.

Yes, it had both meaning and added value. It used to work fine 🤦‍♂ I guess I'll have to find a way to build up the query in a different order then...

@smitpatel
Copy link
Contributor

@khellang - If you have valid use case then please file a new issue with details why you need to apply FromSql on something other than query root.

@ajcvickers
Copy link
Member

@khellang To add to what @smitpatel said, this could be something we revisit if we have misunderstood how it is valuable, but time is running short to make changes, so the sooner you can give us the feedback the more chance it will be actionable.,

@khellang
Copy link
Member

I managed to work around it by breaking a couple of APIs in my library.

I get that the old APIs made it easier to shoot yourself in the foot, but it just seemed like a totally unnecessary change for something that already worked fine.

I guess that's just part of the pain of moving to new major versions 😔

@smitpatel
Copy link
Contributor

it just seemed like a totally unnecessary change for something that already worked fine.

Worked fine for some cases, but not for all. It is entirely true that for your case it was fine. But there were cases in which people actually tried to use it incorrectly and "shot" themselves in the foot.

@roji roji deleted the FromSql branch December 9, 2019 18:05
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.

5 participants