-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Make TokenExtensions.UpdateTokenValue() tokenValue parameter nullable #62482
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was tempted to say we should make a breaking change and start throwing for null token values rather than continue putting null values in the properties dictionary, but I don't think it's worth it. The behavior isn't that much different than updating a token value with the empty string.
Can you update https://github.com/dotnet/aspnetcore/blob/main/src/Http/Authentication.Abstractions/src/PublicAPI.Unshipped.txt to include the following? That should fix the RS0016 errors.
#nullable enable
*REMOVED*static Microsoft.AspNetCore.Authentication.AuthenticationTokenExtensions.UpdateTokenValue(this Microsoft.AspNetCore.Authentication.AuthenticationProperties! properties, string! tokenName, string! tokenValue) -> bool
static Microsoft.AspNetCore.Authentication.AuthenticationTokenExtensions.UpdateTokenValue(this Microsoft.AspNetCore.Authentication.AuthenticationProperties! properties, string! tokenName, string? tokenValue) -> bool
Thanks!
If you're not against making a potentially-observable change, a better approach would be to remove the token entry when the value is aspnetcore/src/Http/Authentication.Abstractions/src/AuthenticationProperties.cs Lines 178 to 193 in 5098e3e
aspnetcore/src/Http/Authentication.Abstractions/src/AuthenticationProperties.cs Lines 129 to 144 in 5098e3e
|
Summary
This PR updates the
TokenExtensions.UpdateTokenValue()
method signature to make thetokenValue
parameter nullable (string?
instead ofstring
), addressing the oversight mentioned in #62257.Description
The
tokenValue
parameter inTokenExtensions.UpdateTokenValue()
is expected to be nullable as it:GetTokenValue()
which returnsstring?
This change eliminates CS8604 compiler warnings when passing nullable values to the method.
Changes
UpdateTokenValue()
method signature to acceptstring?
fortokenValue
parameterStoreTokens_StoresTokensWithNullValues
- Verifies storing tokens with null valuesUpdateTokenValue_WorksWithNullGetTokenValueResult
- Tests updating with null fromGetTokenValue()
GetTokens_ExcludesTokensWithNullValues
- Confirms null-valued tokens are excluded fromGetTokens()
Customer Impact
This is a compile-time only change that:
Example of fixed scenario:
Fixes #62257