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

Throw better exception message for client eval #17247

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

ajcvickers
Copy link
Member

Fixes #16133 and also partial fix for #15937

This is far from perfect and needs some further work. For example, see #17236

No issues reference #14935 any more; new issues cover things not fixed here but which were refeceing that issue.

roji
roji previously approved these changes Aug 19, 2019
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

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

LGTM.

There seem to be some cases where tests for something that is currently untranslatable have been changed to assert on translation failure - it seems better to keep them skipped (and ideally reference an issue).

RemoveNewLines(
Assert.Throws<InvalidOperationException>(
() => context.Orders
.Where(o => o.OrderID > new Random().Next())
Copy link
Member

Choose a reason for hiding this comment

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

Could be translated? Also other random-exercising tests below

Copy link
Member Author

@ajcvickers ajcvickers Aug 19, 2019

Choose a reason for hiding this comment

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

Likewise

Copy link
Contributor

Choose a reason for hiding this comment

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

Not possible to translate for all providers. We should revisit in future, how do we provide tests which are totally conditionally in provider. No suitable relational available. (or throwing can be relational translation)


In reply to: 315191515 [](ancestors = 315191515)

@@ -3496,86 +3496,48 @@ public class Configuration9468

#region Bug10635

[ConditionalFact(Skip = "issue #14935")]
[ConditionalFact]
public void Include_with_order_by_on_interface_key()
Copy link
Member

Choose a reason for hiding this comment

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

Should be translatable (in the future)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Likewise

Copy link
Contributor

Choose a reason for hiding this comment

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

Translatable. I think @ajcvickers is already working on issue.


In reply to: 315192777 [](ancestors = 315192777)

@smitpatel
Copy link
Contributor

  • Use null returning and base throwing would simplify code & also allow to print better message.
  • Every exception in query should not be printed as translation failed. Translation failure should only be printed when we fail to translate. We should discuss this.
  • I did not look at all the tests, but are all tests which are not throwing exception right now which were disabled with client eval in past are enabled in this PR?

@smitpatel smitpatel dismissed roji’s stale review August 19, 2019 16:25

Requires follow-up discussion

@smitpatel
Copy link
Contributor

Unable to process Expression in visitor. + thing about using AsEnumerable etc.

@ajcvickers
Copy link
Member Author

Going to merge this now, then work on the feedback in a separate PR.

@ajcvickers ajcvickers force-pushed the NoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNo0816 branch from f53e234 to b6614de Compare August 19, 2019 18:41
Fixes #16133 and also partial fix for #15937

This is far from perfect and needs some further work. For example, see #17236

No issues reference #14935 any more; new issues cover things not fixed here but which were refeceing that issue.
@ajcvickers ajcvickers force-pushed the NoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNo0816 branch from b6614de to 76bd379 Compare August 19, 2019 18:42
@ajcvickers ajcvickers merged commit aca4958 into release/3.0 Aug 19, 2019
@ghost ghost deleted the NoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNoNo0816 branch August 19, 2019 18:46
@ajcvickers
Copy link
Member Author

ajcvickers commented Aug 19, 2019

@smitpatel @divega Messages for the two general cases:

The LINQ expression '{expression}' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly, for example by by inserting a call to either AsEnumerable(), AsAsyncEnumerable(), ToList(), or ToListAsync(), if this is appropriate. See https://go.microsoft.com/fwlink/?linkid=2101038 for more detailed information.

Processing of the LINQ expression '{expression}' failed. This may indicate either a bug or a limitation in EF Core. See https://go.microsoft.com/fwlink/?linkid=2101038 for more detailed information.

With two different fwlinks?

Thoughts?

@smitpatel
Copy link
Contributor

Also include visitor name in 2nd exception message. LGTM otherwise.

.Where(o => o.OrderID > new Random().Next(0, 10))
.ToList();
Assert.Equal(
CoreStrings.TranslationFailed("(o) => o.OrderID > new Random().Next( minValue: 0, maxValue: 10)"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to improve this printing. cc: @maumar

@smitpatel
Copy link
Contributor

@roji - I commented on all the why not translated comments. If you see any other test which you think we should translate then, please file an issue with the test name and what should be the translation.

@divega
Copy link
Contributor

divega commented Aug 19, 2019

The new fwlink for the "processing the linq expression failed" case is https://go.microsoft.com/fwlink/?linkid=2101433.

@roji
Copy link
Member

roji commented Aug 19, 2019

@smitpatel realistically we should probably just do a pass later and search for translation failure assertions in tests, I don't think there's anything urgent in there for 3.0 (at least not that I can tell).

@smitpatel
Copy link
Contributor

Nothing urgent. I meant to say whenever you decide to do pass or see something out of place. That is true even without regards of client eval changes.

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

Successfully merging this pull request may close these issues.

5 participants