Nullable partial work #183

bgavrilMS · 2020-06-01T12:24:32Z

This is most of the work needed to conform to the nullable reference types. This makes the API more expressive, by showing which input / output types can be null or not.

https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

This is NOT a breaking change.

Please read through my PR comments and decide if this is an avenue you wish to pursue. To sum up:

PROs:

better semantics (you know when to pass null or not)
will help us discover bugs
much less chances of NULL refs
better for customers who also use this feature, as it will help the compiler analyze NULLs as well

CONs:

you still have to program defensively
about ~2h more of work (I am happy to finish it if)
one more things to understand

bgavrilMS · 2020-06-01T12:44:48Z

src/Microsoft.Identity.Web/AccountExtensions.cs

@@ -17,7 +17,7 @@ public static class AccountExtensions
        /// </summary>
        /// <param name="account">The IAccount instance.</param>
        /// <returns>A <see cref="ClaimsPrincipal"/> built from IAccount.</returns>
-        public static ClaimsPrincipal ToClaimsPrincipal(this IAccount account)
+        public static ClaimsPrincipal? ToClaimsPrincipal(this IAccount? account)


Public API changes are backwards compatible.

Here and in similar methods, we can also use attributes like `[return: NotNullIfNotNull("account")]

bgavrilMS · 2020-06-01T12:46:26Z

src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

@@ -16,7 +16,7 @@ public static class ClaimsPrincipalExtensions
        /// </summary>
        /// <param name="claimsPrincipal">Claims principal.</param>
        /// <returns>A string corresponding to an account identifier as defined in <see cref="Microsoft.Identity.Client.AccountId.Identifier"/>.</returns>
-        public static string GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)
+        public static string? GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)


If a consuming application also enforces the "nullable reference" feature, then they will get warnings when they try to pass in a NULL, for example a null ClaimsPrincinpal here.

If the consuming application does not enforce this feature, they will not get warnings.

How do we decide if we should change the parameter to nullable or keep it non-nullable and throw the ArgumentNullException?

In this case, a null claimsPrincipal can return a null ID value and non-null claimsPrincipal returns non-null ID value.

At line 23 we throw an ArgumentNullEx if the claimsPrincipal is null, we don't return null.

bgavrilMS · 2020-06-01T12:46:58Z

src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

@@ -16,7 +16,7 @@ public static class ClaimsPrincipalExtensions
        /// </summary>
        /// <param name="claimsPrincipal">Claims principal.</param>
        /// <returns>A string corresponding to an account identifier as defined in <see cref="Microsoft.Identity.Client.AccountId.Identifier"/>.</returns>
-        public static string GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)
+        public static string? GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)
        {
            if (claimsPrincipal == null)


From a code perspective, we still need to make null checks and program defensively, because NULLs can still be passed everywhere.

bgavrilMS · 2020-06-01T12:48:03Z

src/Microsoft.Identity.Web/Microsoft.Identity.Web.csproj

+    <SignAssembly>true</SignAssembly>
+    <AssemblyOriginatorKeyFile>../../build/MSAL.snk</AssemblyOriginatorKeyFile>
+    <GenerateDocumentationFile>true</GenerateDocumentationFile>
+    <Nullable>enable</Nullable>


This is how the feature is enabled. It needs C# 8, which is available with .net core 3.1 or with .net class 4.smth (4.7.2?)

bgavrilMS · 2020-06-01T12:48:48Z

src/Microsoft.Identity.Web/Resource/RegisterValidAudience.cs

@@ -61,7 +61,12 @@ internal class RegisterValidAudience
            SecurityToken securityToken,
            TokenValidationParameters validationParameters)
        {
-            JwtSecurityToken token = securityToken as JwtSecurityToken;
+            JwtSecurityToken? token = securityToken as JwtSecurityToken;
+            if (token == null)


This is the kind of bugs you discover when you turn this feature on. THe compiler does NULL analysis and points out areas of the code that are potentially unsafe.

src/Microsoft.Identity.Web/Resource/RegisterValidAudience.cs

bgavrilMS · 2020-06-01T12:49:45Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

@@ -446,9 +444,9 @@ private async Task<IConfidentialClientApplication> BuildConfidentialClientApplic
        /// <param name="tenant"></param>
        private async Task<string> GetAccessTokenOnBehalfOfUserFromCacheAsync(


You can see how the methods become more expressive - you now know which input params can be null and which ones can't.

bgavrilMS · 2020-06-01T12:49:53Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

            IEnumerable<string> scopes,
-            string tenant)
+            string? tenant)
        {
            if (scopes == null)


But you still have to code defensively...

bgavrilMS · 2020-06-01T12:51:22Z

src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs


        /// <summary>
        /// Key section on the configuration file that holds the scope value.
        /// </summary>
-        public string ScopeKeySection { get; set; }
+        public string? ScopeKeySection { get; set; }

        /// <summary>
        /// Handles the MsalUiRequiredException.
        /// </summary>
        /// <param name="context">Context provided by ASP.NET Core.</param>
        public override void OnException(ExceptionContext context)


I rewrote this method a bit, but all changes are non-breaking (done with analyzers)

bgavrilMS · 2020-06-01T12:52:02Z

src/Microsoft.Identity.Web/AuthorizeForScopesAttribute.cs

@@ -128,9 +124,13 @@ private bool CanBeSolvedByReSignInOfUser(MsalUiRequiredException ex)
                 OidcConstants.ScopeProfile,
            };

+            // TODO: scopes can actually be null here - how do we treat this case? 


This is the kind of bugs that can be discovered. In this particular case I am not sure how to handle it - please advise.

yes...good point. This is for incremental consent, so we technically should only be here if we have scopes, because that's the point, that additional scopes are required. So I think in OnException(), we should make sure we have something. @jmprieur thoughts?

In reply to: 433215274 [](ancestors = 433215274)

pmaytak · 2020-06-01T16:27:01Z

Thanks @bgavrilMS. Is there an official Microsoft doc that talks about this pattern, plus when to use it or not, etc.?

jennyf19 · 2020-06-01T17:00:43Z

Thanks @bgavrilMS I will try to have a look later today. I think this is definitely worth exploring and thanks for you time thus far.

jennyf19 · 2020-06-01T17:01:36Z

@pmaytak here's a doc on it

bgavrilMS · 2020-06-01T17:02:26Z

@pmaytak - sorry I didn't add the official guidance - https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

jmprieur · 2020-06-02T09:48:58Z

Thanks @bgavrilMS for starting this conversation.
This is BTW related to GitHub issue #15 requested by a customer.

I like that the API shows what can be null and not, and that's not a breaking change (except a renaming you've done)
I also like that this enables to find out possible null reference exceptions

Did you notice that the branch does not build?

Do you think customers will understand the subtlety?
@jennyf19 @pmaytak @henrik-me : how do you feel?

bgavrilMS · 2020-06-02T15:07:39Z

@jmprieur - I broke a unit test related to json handling...

jmprieur · 2020-06-02T09:43:05Z

src/Microsoft.Identity.Web/ClientInfo.cs

-            return (T)JsonSerializer.Create().Deserialize(reader, typeof(T));
+            using MemoryStream stream = new MemoryStream(jsonByteArray);
+            using StreamReader reader = new StreamReader(stream, Encoding.UTF8);
+            return (ClientInfo)JsonSerializer.Create().Deserialize(reader, typeof(ClientInfo));


I'm not sure I understand why you replaced the generic here (T) by ClientInfo?

JSON deserialization can always return NULL (since you don't know what you deserialize). But a nullable value type is not the same as a nullable reference type. So general purpose serialization helpers that just take a "T" won't work. I think @pmaytak 's suggestions (where T : notnull) might do it, I didn't research this enough.

But from a code perspective, why have a general purpose deserialization method in a class that only deserializes ClientInfo? Makes the code hard to read...

jmprieur · 2020-06-02T09:43:29Z

src/Microsoft.Identity.Web/Microsoft.Identity.Web.csproj

+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+
+  <PropertyGroup Label="Package Metadat">


Metadat? or Metadata?

src/Microsoft.Identity.Web/Resource/RegisterValidAudience.cs

jmprieur · 2020-06-02T09:44:39Z

src/Microsoft.Identity.Web/TokenAcquisition.cs

@@ -199,7 +197,7 @@ internal class TokenAcquisition : ITokenAcquisition, ITokenAcquisitionInternal
        /// OpenIdConnectOptions.Events.OnAuthorizationCodeReceived.</remarks>
        public async Task<string> GetAccessTokenForUserAsync(
            IEnumerable<string> scopes,
-            string tenant = null)
+            string? tenantId = null)


this is a breaking change. But that's probably ok. We'd document it.

Ah, you mean the rename? Didn't realize, but you are right. I have an analyzer that flaged it up because the name of the param on the interface != name of the param on the class. That analyzer is SQ btw...

jennyf19

jennyf19 · 2020-06-02T17:13:18Z

Looks fine to me, bogdan. thanks for all the effort. You still have a few hours of this? thanks again.

In reply to: 637603887 [](ancestors = 637603887)

pmaytak

I think we should move forward with this.

From the docs below, .NET 5 will fully implement Nullable Reference Types (NRTs). The advisory for libraries is to start implementing NRTs before .NET 5 is released. And there's a possibility that NRTs will be the default for new Visual Studio projects starting with .NET 5.

I think the good thing about NRTs is that it forces the developer to think about the intent of the potential null values in code and does add more expressiveness to the public API.

Along with that we do have to focus more on the intent of our code as related to nulls - do we use nullable or non-nullable return values and parameters, do we throw Argument Null Exceptions, do we use NRT attributes for nuanced behaviors. Related quote from one of the articles:

Updating your library for nullable references requires more than sprinkling ? on some of the variables and type names. The preceding example shows that you need to examine your APIs and consider your expectations for each input argument. Consider the guarantees for the return value, and any out or ref arguments upon the method's return.

One question that I still have is how do we decide if we should change the parameter to nullable or keep it non-nullable and throw the ArgumentNullException?

More resources:
Embracing nullable reference types
Update libraries to use nullable reference types and communicate nullable rules to callers
Reserved attributes contribute to the compiler's null state static analysis
Try out Nullable Reference Types

pmaytak · 2020-06-02T21:08:04Z

src/Microsoft.Identity.Web/AccountExtensions.cs

@@ -17,7 +17,7 @@ public static class AccountExtensions
        /// </summary>
        /// <param name="account">The IAccount instance.</param>
        /// <returns>A <see cref="ClaimsPrincipal"/> built from IAccount.</returns>
-        public static ClaimsPrincipal ToClaimsPrincipal(this IAccount account)
+        public static ClaimsPrincipal? ToClaimsPrincipal(this IAccount? account)


Here and in similar methods, we can also use attributes like `[return: NotNullIfNotNull("account")]

pmaytak · 2020-06-02T21:16:50Z

src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

@@ -16,7 +16,7 @@ public static class ClaimsPrincipalExtensions
        /// </summary>
        /// <param name="claimsPrincipal">Claims principal.</param>
        /// <returns>A string corresponding to an account identifier as defined in <see cref="Microsoft.Identity.Client.AccountId.Identifier"/>.</returns>
-        public static string GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)
+        public static string? GetMsalAccountId(this ClaimsPrincipal claimsPrincipal)


How do we decide if we should change the parameter to nullable or keep it non-nullable and throw the ArgumentNullException?

In this case, a null claimsPrincipal can return a null ID value and non-null claimsPrincipal returns non-null ID value.

pmaytak · 2020-06-02T21:24:42Z

src/Microsoft.Identity.Web/ClientInfo.cs

-            return DeserializeFromJson<ClientInfo>(Base64UrlHelpers.DecodeToBytes(clientInfo));
-        }
-
-        internal static T DeserializeFromJson<T>(byte[] jsonByteArray)


I would keep DeserializeFromJson. I think you can also leave the generic parameter and add where T : notnull.

pmaytak · 2020-06-02T21:28:35Z

src/Microsoft.Identity.Web/HttpContextExtensions.cs

@@ -24,7 +24,7 @@ internal static void StoreTokenUsedToCallWebAPI(this HttpContext httpContext, Jw
        /// </summary>
        /// <param name="httpContext">Http context associated with the current request.</param>
        /// <returns><see cref="JwtSecurityToken"/> used to call the Web API.</returns>
-        internal static JwtSecurityToken GetTokenUsedToCallWebAPI(this HttpContext httpContext)
+        internal static JwtSecurityToken? GetTokenUsedToCallWebAPI(this HttpContext httpContext)


In this method (and similar ones), we expect httpContext to have a value here. However, if null is passed in, we get an NPE. So we either need to return null or throw.

For public APIs, we should always check for NULLs and throw NullArgumentException.
For internal APIs like this - use your best judgement - I think in most cases we can add a null check, but this is where you know best. If you can't decide, my advice would be throw, i.e. be stingy with returning NULLs.

i.e. be stingy with returning NULLs.

👍

pmaytak · 2020-06-02T21:34:56Z

src/Microsoft.Identity.Web/InstanceDiscovery/Metadata.cs

@@ -15,19 +15,19 @@ internal class Metadata
        /// Preferred alias.
        /// </summary>
        [JsonProperty(PropertyName = "preferred_network")]
-        public string PreferredNetwork { get; set; }
+        public string? PreferredNetwork { get; set; }


This case is similar to Late-initialized properties, Data Transfer Objects and nullability. What is our intent - value is required for all these properties or optional?

Very good point, I missed this one!

Good thing about this work and PR is that it starts the discussions about the intent of our code. 👍

jennyf19 · 2020-06-12T18:28:58Z

@bgavrilMS what's the status on this? just concerned the longer it sits the more out of sync it will get w/master. and the more work ultimately to address merge conflicts.

bgavrilMS · 2020-06-15T09:03:22Z

Folks, I apologise but this is probably more work that I can handle for the next few weeks, especially as @pmaytak makes an excellent point about how to handle JSON + nullables, which requires more investment.

jennyf19 · 2020-06-15T16:40:13Z

Thanks @bgavrilMS ... @pmaytak is going to finish the work

Nullable partial work

3bb9b1d

bgavrilMS commented Jun 1, 2020

View reviewed changes

src/Microsoft.Identity.Web/Resource/RegisterValidAudience.cs Show resolved Hide resolved

bgavrilMS commented Jun 1, 2020

View reviewed changes

bgavrilMS requested review from jmprieur, pmaytak and jennyf19 and removed request for jmprieur and pmaytak June 1, 2020 12:54

jmprieur reviewed Jun 2, 2020

View reviewed changes

jennyf19 approved these changes Jun 2, 2020

View reviewed changes

pmaytak reviewed Jun 2, 2020

View reviewed changes

pmaytak mentioned this pull request Jun 23, 2020

Implemented C# 8 Nullable Reference Type standard. #231

Merged

jmprieur closed this Jun 25, 2020

jmprieur deleted the bogavril/nullable branch June 28, 2020 15:26

		@@ -446,9 +444,9 @@ private async Task<IConfidentialClientApplication> BuildConfidentialClientApplic
		/// <param name="tenant"></param>
		private async Task<string> GetAccessTokenOnBehalfOfUserFromCacheAsync(

Nullable partial work #183

Nullable partial work #183

Conversation

bgavrilMS commented Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmaytak commented Jun 1, 2020

jennyf19 commented Jun 1, 2020

jennyf19 commented Jun 1, 2020

bgavrilMS commented Jun 1, 2020

jmprieur commented Jun 2, 2020

bgavrilMS commented Jun 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyf19 left a comment

Choose a reason for hiding this comment

jennyf19 commented Jun 2, 2020

pmaytak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmaytak Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennyf19 commented Jun 12, 2020

bgavrilMS commented Jun 15, 2020

jennyf19 commented Jun 15, 2020

bgavrilMS commented Jun 1, 2020 •

edited

Loading

pmaytak left a comment •

edited

Loading

pmaytak Jun 3, 2020 •

edited

Loading