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

Query: We should throw when accessing non-nullable property over a null-value entity in the projection #20633

Closed
maumar opened this issue Apr 14, 2020 · 2 comments
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Milestone

Comments

@maumar
Copy link
Contributor

maumar commented Apr 14, 2020

query:

ctx.Gears.Select(g => g.AssignedCity.Name.Length);

assuming assigned city can be null, we should throw "nullable object must have a value" (or ideally something more meaningful).

Currently we return default(int) - it might not be what user expected.
Casting result to nullable will yield correct result (and would get rid of the exception in the future)

ctx.Gears.Select(g => (int?)g.AssignedCity.Name.Length);
@maumar maumar changed the title We should throw when accessing non-nullable property over a null-value entity in the projection Query: We should throw when accessing non-nullable property over a null-value entity in the projection Apr 14, 2020
@maumar
Copy link
Contributor Author

maumar commented Apr 14, 2020

Note: in 2.1 we would throw in this case, which we changed in 3.1. We want to go back to 2.1 behavior.

@ajcvickers
Copy link
Member

Notes from team discussion: we should do this alongside the other related change such that Sum queries still return correct values. Then I don't think this is a breaking change because it is only changing the result in places where it was previously wrong.

smitpatel added a commit that referenced this issue Jul 17, 2020
In 3.x when reading values from database, we started returning default value of expected type if the value was null in the database. That means if we needed to read int then database value was null then we got 0 back. Which is not correct as we should throw exception for the case.

The fix here is,
- All value read from databases are of nullable type as database can return null.
- Since all the bindings with database are nullable now,
  - When doing member access we add null check and return nullable of return type
  - When doing method call we add null check for instance. All parameters we convert to original types.
  - All other expressions, where they need to match the type to reconstruct the original tree, adds convert nodes.
- For translation of Single/SingleOrDefault (and friends)
  - For scalar subquery
    - We add coalesce with default value for SingleOrDefault
    - We don't add coalesce for Single case and assume there will be non-null value coming out of database which we will try to assign.
  - For non-scalar subquery
    - We add default value in client side in shaper for SingleOrDefault
    - We throw exception Sequence contains no elements for Single case
  - Above rules are only when not returning entity type. When returning entity type, we don't have enough information to differentiate the case. Also above behavior is just side-effect of assigning values to properties which cannot take null values.
- When doing DefaultIfEmpty over a non-reference type, navigation expansion will add client side coalesce so that we can move Selector after DefaultIfEmpty

Resolves #20633
Resolves #20959
Resolves #21002
smitpatel added a commit that referenced this issue Jul 18, 2020
In 3.x when reading values from database, we started returning default value of expected type if the value was null in the database. That means if we needed to read int then database value was null then we got 0 back. Which is not correct as we should throw exception for the case.

The fix here is,
- All value read from databases are of nullable type as database can return null.
- Since all the bindings with database are nullable now,
  - When doing member access we add null check for value types and return nullable of return type
  - When doing method call we add null check for instance. All parameters we convert to original types.
  - All other expressions, where they need to match the type to reconstruct the original tree, adds convert nodes.
- For translation of Single/SingleOrDefault (and friends)
  - For scalar subquery
    - We add coalesce with default value for SingleOrDefault
    - We don't add coalesce for Single case and assume there will be non-null value coming out of database which we will try to assign.
  - For non-scalar subquery
    - We add default value in client side in shaper for SingleOrDefault
    - We throw exception Sequence contains no elements for Single case
  - Above rules are only when not returning entity type. When returning entity type, we don't have enough information to differentiate the case. Also above behavior is just side-effect of assigning values to properties which cannot take null values.
- When doing DefaultIfEmpty over a non-reference type, navigation expansion will add client side coalesce so that we can move Selector after DefaultIfEmpty

Resolves #20633
Resolves #20959
Resolves #21002
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 18, 2020
@smitpatel smitpatel modified the milestones: 5.0.0, 5.0.0-preview8 Jul 18, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview8, 5.0.0 Nov 7, 2020
Liu233w added a commit to Liu233w/acm-statistics that referenced this issue Mar 5, 2021
If the summary does not exist, SummaryId can be null.

The bug has exist since the first day, but it had not been revealed until the fix in entity framework 5.

see dotnet/efcore#22517 and dotnet/efcore#20633
mergify bot pushed a commit to Liu233w/acm-statistics that referenced this issue Mar 5, 2021
If the summary does not exist, SummaryId can be null.

The bug has exist since the first day, but it had not been revealed until the fix in entity framework 5.

see dotnet/efcore#22517 and dotnet/efcore#20633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-bug
Projects
None yet
Development

No branches or pull requests

3 participants