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.

Sign up for GitHub

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

Jump to bottom

Enable throwing better exceptions for specific cases of failed translation #20344

Closed
smitpatel opened this issue Mar 19, 2020 · 8 comments · Fixed by #20447
Closed

Enable throwing better exceptions for specific cases of failed translation #20344

smitpatel opened this issue Mar 19, 2020 · 8 comments · Fixed by #20447
Assignees
@smitpatel
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone
5.0.0

Comments

@smitpatel
Copy link
Contributor

If it cannot rewrite it should just return original tree. Let client eval deal with it.
@smitpatel
Copy link
Contributor Author

Related #20164

@ajcvickers ajcvickers changed the title Entity equality should not be throwing exceptions Enable throwing better exceptions for specific cases of client evaluation Mar 20, 2020
@ajcvickers ajcvickers changed the title Enable throwing better exceptions for specific cases of client evaluation Enable throwing better exceptions for specific cases of failed translation Mar 20, 2020
@ajcvickers
Copy link
Contributor

From an experience perspective, the more specific we can get about why something can't be translated the better. However, some of the expression visitors are designed not to throw. Therefore, we should look at how to propagate the more detailed information to the place that will throw.

@ajcvickers ajcvickers added this to the Backlog milestone Mar 20, 2020
@roji
Copy link
Member

roji commented Mar 26, 2020

One main point is to also avoid duplicating logic from the concerned visitor in order to identify specific patterns and throw the informative exception. If we end up doing this, it may be better to simply expose knobs in the expression visitor, telling it whether to throw or let the tree through. The visitor would throw by default, unless a provider has explicitly disabled that in order to handle things in translation.

@smitpatel smitpatel added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed consider-for-current-release needs-design labels Mar 28, 2020
@smitpatel smitpatel modified the milestones: Backlog, 3.1.x, 5.0.0 Mar 28, 2020
@smitpatel
Copy link
Contributor Author

All exception messages from scenarios Entity Equality was throwing has been migrated to new entity equality implementation.

smitpatel added a commit that referenced this issue Mar 28, 2020
@smitpatel
Query: Rewrite Entity Equality during translation phase
24718b0 
Resolves #15080
Implemented behavior:
- If any part of composite key is null then key is null.
- If comparing entity with null then check if "any" key value is null.
- If comparing entity with non-null then check if "all" key values are non null.

Resolves #20344
Resolves #19431
Resolves #13568
Resolves #13655
Since we already convert property access to nullable, if entity from client is null, make key value as null.

Resolves #19676
Clr type mismatch between proxy type and entity type is ignored.

Resolves #20164
Rewrites entity equality during translation

Part of #18923
smitpatel added a commit that referenced this issue Mar 28, 2020
@smitpatel
Query: Rewrite Entity Equality during translation phase
ba04dd0 
Resolves #15080
Implemented behavior:
- If any part of composite key is null then key is null.
- If comparing entity with null then check if "any" key value is null.
- If comparing entity with non-null then check if "all" key values are non null.

Resolves #20344
Resolves #19431
Resolves #13568
Resolves #13655
Since we already convert property access to nullable, if entity from client is null, make key value as null.

Resolves #19676
Clr type mismatch between proxy type and entity type is ignored.

Resolves #20164
Rewrites entity equality during translation

Part of #18923
@smitpatel smitpatel mentioned this issue Mar 28, 2020
Merged
smitpatel added a commit that referenced this issue Mar 28, 2020
@smitpatel
Query: Rewrite Entity Equality during translation phase
904a105 
Resolves #15080
Implemented behavior:
- If any part of composite key is null then key is null.
- If comparing entity with null then check if "any" key value is null.
- If comparing entity with non-null then check if "all" key values are non null.

Resolves #20344
Resolves #19431
Resolves #13568
Resolves #13655
Since we already convert property access to nullable, if entity from client is null, make key value as null.

Resolves #19676
Clr type mismatch between proxy type and entity type is ignored.

Resolves #20164
Rewrites entity equality during translation

Part of #18923
smitpatel added a commit that referenced this issue Mar 31, 2020
@smitpatel
Query: Rewrite Entity Equality during translation phase
b7360ca 
Resolves #15080
Implemented behavior:
- If any part of composite key is null then key is null.
- If comparing entity with null then check if "any" key value is null.
- If comparing entity with non-null then check if "all" key values are non null.

Resolves #20344
Resolves #19431
Resolves #13568
Resolves #13655
Since we already convert property access to nullable, if entity from client is null, make key value as null.

Resolves #19676
Clr type mismatch between proxy type and entity type is ignored.

Resolves #20164
Rewrites entity equality during translation

Part of #18923
smitpatel added a commit that referenced this issue Mar 31, 2020
@smitpatel
Query: Rewrite Entity Equality during translation phase
bb88898 
Resolves #15080
Implemented behavior:
- If any part of composite key is null then key is null.
- If comparing entity with null then check if "any" key value is null.
- If comparing entity with non-null then check if "all" key values are non null.

Resolves #20344
Resolves #19431
Resolves #13568
Resolves #13655
Since we already convert property access to nullable, if entity from client is null, make key value as null.

Resolves #19676
Clr type mismatch between proxy type and entity type is ignored.

Resolves #20164
Rewrites entity equality during translation

Part of #18923
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview3 Mar 31, 2020
@smitpatel smitpatel modified the milestones: 5.0.0-preview3, 5.0.0 Apr 1, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0, 5.0.0-preview4 Apr 20, 2020
@ajcvickers
Copy link
Contributor

@smitpatel What are examples of better exception messages that we now throw? Are they included in preview 4?

@smitpatel
Copy link
Contributor Author

This issue tracked only migrating existing exception messages of EntityEquality rewrite to translation pipeline. No new exception message were added.

@ajcvickers
Copy link
Contributor

@smitpatel Where are we tracking new exception messages?

@smitpatel
Copy link
Contributor Author

#20612

@ajcvickers ajcvickers mentioned this issue Jul 23, 2020
Closed
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview4, 5.0.0 Nov 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@smitpatel smitpatel

Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Milestone
5.0.0
Development

Successfully merging a pull request may close this issue.

Query: Rewrite Entity Equality during translation phase dotnet/efcore
3 participants
@ajcvickers @smitpatel @roji