Add nullable annotations to Authentication.Core & Authentication.Cookies #24307
Conversation
| /// <param name="properties">additional properties that can be consumed by the user or runtime.</param> | ||
| /// <param name="authenticationScheme">the authentication middleware that was responsible for this ticket.</param> | ||
| public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties? properties, string? authenticationScheme) | ||
| public AuthenticationTicket(ClaimsPrincipal principal, AuthenticationProperties? properties, string authenticationScheme) |
There was a problem hiding this comment.
Should authenticationScheme throw ArgumentNullException if null?
There was a problem hiding this comment.
We generally haven't added explicit null-checks to our code to express the non-nullness. IIRC, there was one code path that would result in an arg-null \ null-ref if the scheme was null, but I wasn't a 100% certain if this was the right default
| private async Task<AuthenticateResult> ReadCookieTicket() | ||
| { | ||
| var cookie = Options.CookieManager.GetRequestCookie(Context, Options.Cookie.Name); | ||
| var cookie = Options.CookieManager.GetRequestCookie(Context, Options.Cookie.Name!); |
There was a problem hiding this comment.
What happens if Options.Cookie.Name is null?
There was a problem hiding this comment.
In the default case, this is guaranteed to be non-null. The option configures it to a value and you cannot assign a null value to it.
| /// Calls success creating a ticket with the <see cref="Principal"/> and <see cref="Properties"/>. | ||
| /// </summary> | ||
| public void Success() => Result = HandleRequestResult.Success(new AuthenticationTicket(Principal, Properties, Scheme.Name)); | ||
| public void Success() => Result = HandleRequestResult.Success(new AuthenticationTicket(Principal!, Properties, Scheme.Name)); |
There was a problem hiding this comment.
What happens if Principal is null? Ok because AuthenticationTicket checks for null?
| { | ||
| byte[] Serialize(TModel model); | ||
|
|
||
| [return: MaybeNull] |
There was a problem hiding this comment.
This is fine for now, but we could switch MaybeNull/AllowNull attributes to use ? before .NET 5
There was a problem hiding this comment.
The version of the compiler we have doesn't support this as yet.
pranavkm
changed the title
3 participants
Contributes to #5680