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

Correct nullability annotation for ReferenceEntry #25591

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

roji
Copy link
Member

@roji roji commented Aug 19, 2021

Unfortunately, the situation here is complicated, since ReferenceEntry APIs sometimes returns nullable for TProperty, sometimes not:

  • ReferenceEntry.CurrentValue can return nulls if the reference navigation property was nullable.
  • ReferenceEntry.TargetEntry returns a nullable EntityEntry, so the TProperty it wraps is never null. Query also return IQueryable<TProperty> where TProperty should never be null.

I don't think there's a perfect solution, here are some options with what they mean for the return values of ReferenceEntry.TargetEntry and CurrentValue:

TargetEntry for nullable property TargetEntry for non-nullable property CurrentValue for nullable property CurrentValue for non-nullable property
1. Only change EntityEntry.Reference to accept nullable property (no change to ReferenceEntry) EntityEntry<BlogDetails>?, good EntityEntry<BlogDetails>?, good BlogDetails, really bad BlogDetails, good
2. (1) + change ReferenceEntry.CurrentValue to return TProperty? EntityEntry<BlogDetails>?, good EntityEntry<BlogDetails>?, good BlogDetails?, good BlogDetails?, bad
3. (1) + change ReferenceEntry to accept class? constraint EntityEntry<BlogDetails?>?, bad EntityEntry<BlogDetails>?, good BlogDetails?, good BlogDetails, good
  • It's always better to return non-nullable as nullable (only forcing an extra check) rather than to return nullable as non-nullable (causing unexpected NRE) - so option (1) is the worst.
  • Option (3) also requires changing EntityEntry's constraint to class?, since ReferenceEntry.TargetEntry returns that; this means EntityEntry.Entry would return nullable when reached through a nullable reference navigation - not ideal. It would also make ReferenceEntry.Query return IQueryable<TProperty?>, which isn't good.
  • So option (2) seems the least bad - the only downside is that users need an extra annoying null check when accessing ReferenceEntry.CurrentValue. This is what this PR implements.

Fixes #24659

@AndriySvyryd
Copy link
Member

Consider merging this to rc1 since it impacts public API

@roji roji merged commit c833579 into release/6.0 Aug 19, 2021
@roji roji deleted the roji/ReferenceEntryNullability branch August 19, 2021 17:26
roji added a commit that referenced this pull request Aug 19, 2021
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.

Should EntityEntry's Expression-based parameters be nullable-friendly?
2 participants