Nullable annotations for metadata 2 #23147
Conversation
| { | ||
| _typeMapping = typeMapping; | ||
| // TODO-NULLABLE: null check conflicts with [NotNull] annotation above, which seems like it may be incorrect. |
There was a problem hiding this comment.
The annotation is correct, the check should be removed
There was a problem hiding this comment.
HasTypeMapping should also have [NotNull] and callers should add a check.
There was a problem hiding this comment.
So no way to unset a type mapping, right? Making sure because the docs on HasTypeMapping explicitly document accepting null to remove type mapping.
The only usage in our codebase seems to be in TypeMappingConvention.ProcessModelFinalizing, which we'll annotate a bit later - should that throw if FindMapping returns null?
In the meantime, changed HasTypeMapping (and CanHasTypeMapping) to have [NotNull].
There was a problem hiding this comment.
I will likely obsolete these methods when #22031 is done
There was a problem hiding this comment.
@AndriySvyryd as pointed by @smitpatel in #23147 (comment), SetTypeMapping returns CoreTypeMapping but that isn't documented. Since the parameter is now [NotNull], it seems they only ever return the supplied parameter. So these can probably be changed to return void, but if you're planning to obsolete them anyway for 6.0...
There was a problem hiding this comment.
We can say that it returns the type mapping that was set
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.Expressions.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
|
@smitpatel addressed your comments |
roji
force-pushed
the
MoreMetadataNullability
branch
from
eb4bb1c
to
971a8ce
Compare
| @@ -88,7 +90,7 @@ public static IEnumerable<IConventionKey> GetContainingKeys([NotNull] this IConv | |||
| /// <param name="property"> The property. </param> | |||
| /// <param name="typeMapping"> The <see cref="CoreTypeMapping" /> for this property. </param> | |||
| /// <param name="fromDataAnnotation"> Indicates whether the configuration was specified using a data annotation. </param> | |||
| public static CoreTypeMapping SetTypeMapping( | |||
There was a problem hiding this comment.
Is this method missing in XML docs?
There was a problem hiding this comment.
You mean in the published docs? I think this is a new method in 5.0 so not yet published, no?
There was a problem hiding this comment.
In XML docs in source code. The method is not void but still does not specify tag.
There was a problem hiding this comment.
Right, will fix this in the next nullability PR, thanks.
There was a problem hiding this comment.
Actually, this now always returns the provided parameter (#23147 (comment)), I'm embarrassed to document the return type as "always identical to the parameter" :)
There was a problem hiding this comment.
Here too we can say that it returns the type mapping that was set
roji
force-pushed
the
MoreMetadataNullability
branch
from
971a8ce
to
216b034
Compare
|
Hello @roji! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Annotates Model, Property, Navigation, Key, Index, and all their extensions. Part of #19007
roji
force-pushed
the
MoreMetadataNullability
branch
from
216b034
to
71f418c
Compare
ghost
merged commit 
Am just going to drop this here, sorry @AndriySvyryd... This annotates Model, Property, Navigation, Key, Index, and all their extensions.
IPropertyBase.ClrType is nullable (because of skip navigations), but IProperty overrides it to be non-nullable.Part of #19007