Add nullability to remaining middleware #26098

pranavkm · 2020-09-19T15:00:29Z

Adds nullable annotations to the remaining middleware in the repo (except SpaServices)

pranavkm · 2020-09-19T15:01:31Z

src/Http/Http.Features/src/ISession.cs

@@ -43,7 +44,7 @@ public interface ISession
        /// <param name="key"></param>
        /// <param name="value"></param>
        /// <returns></returns>
-        bool TryGetValue(string key, out byte[] value);
+        bool TryGetValue(string key, [MaybeNullWhen(false)] out byte[] value);


I'll see if I can port this change to 5.0. It is more restrictive so we might as well try and get it right.

Ideally we would have:

Suggested change

bool TryGetValue(string key, [MaybeNullWhen(false)] out byte[] value);

bool TryGetValue(string key, [NotNullWhen(true)] out byte[]? value);

This might be more problematic with how distributed session works. I think it can return null while returning true.

This might be more problematic with how distributed session works. I think it can return null while returning true.

I wouldn't expect that. Where do you see it happening?

pranavkm · 2020-09-19T15:04:52Z

src/Middleware/StaticFiles/src/Helpers.cs

@@ -29,7 +29,7 @@ internal static bool IsGetOrHeadMethod(string method)

        internal static bool PathEndsInSlash(PathString path)
        {
-            return path.Value.EndsWith("/", StringComparison.Ordinal);
+            return path.HasValue && path.Value!.EndsWith("/", StringComparison.Ordinal);


This seems like a bug. PathSting.HasValue is annotated with MemberNotNullWhen but it doesn't work here. @sharwell any idea why that might?

Now in gif!

pranavkm · 2020-09-19T15:06:51Z

src/Middleware/WebSockets/src/WebSocketsDependencyInjectionExtensions.cs

@@ -11,6 +11,11 @@ public static class WebSocketsDependencyInjectionExtensions
    {
        public static IServiceCollection AddWebSockets(this IServiceCollection services, Action<WebSocketOptions> configure)
        {
+            if (configure is null)


services.Configure will throw if configure is null. Might as well throw it here and make it consistent with all of our other extension methods.

src/Middleware/Session/src/NoOpSessionStore.cs

sharwell · 2020-09-19T20:02:57Z

src/Middleware/Session/src/SessionMiddleware.cs

@@ -108,7 +108,7 @@ public async Task Invoke(HttpContext context)
            }
            finally
            {
-                context.Features.Set<ISessionFeature>(null);
+                context.Features.Set<ISessionFeature?>(null);


❔ Can you link me to the definition of Set<T>?

aspnetcore/src/Http/Http.Features/src/FeatureCollection.cs

Lines 101 to 104 in 15f341f

public void Set<TFeature>(TFeature instance)

{

this[typeof(TFeature)] = instance;

}

sharwell · 2020-09-19T20:03:42Z

src/Middleware/Session/src/SessionFeature.cs

@@ -8,6 +8,6 @@ namespace Microsoft.AspNetCore.Session
 {
    public class SessionFeature : ISessionFeature
    {
-        public ISession Session { get; set; }
+        public ISession Session { get; set; } = default!;


💡 Consider using this instead:

Suggested change

public ISession Session { get; set; } = default!;

[DisallowNull]

public ISession? Session { get; set; }

Ooh, what does this do?

Hmm, I don't think this works for this case. In the ordinary case, features are instantiated by the framework and initialize these properties. If the feature exists, user code does not see a null value. Ideally we would have created initialized this in the ctor, but that ship has sailed.

src/Middleware/ResponseCompression/src/ResponseCompressionBody.cs

src/Middleware/Rewrite/src/RewriteOptions.cs

sharwell · 2020-09-19T20:19:54Z

src/Middleware/Session/src/DistributedSession.cs

@@ -105,7 +106,7 @@ private byte[] IdBytes
                    _sessionIdBytes = new byte[IdByteCount];
                    RandomNumberGenerator.Fill(_sessionIdBytes);
                }
-                return _sessionIdBytes;
+                return _sessionIdBytes!;


:\ Not a huge fan of this suppression, but I'm not exactly sure what the alternative would be. You could put MemberNotNullWhen on the IsAvailable property and mark this property as nullable, but I'm not sure it's an improvement over what you have here. Here are all the potential options I see:

Applying MemberNotNullWhen to IsAvailable

Changing the property type to byte[]?

Returning _sessionIdBytes ?? Array.Empty<byte>()

Returning _sessionIdBytes ?? throw new InvalidOperationException(...)

Keeping the suppression

@Tratcher thoughts?

This code could be restructured. Load always sets _sessionIdBytes or _sessionId so if IdBytes is called then it will return a non-null value. How about just removing the IsAvailable check here?

Ok. I've removed the check here. I've added a new exception to Serialize which would have previously resulted in a null-ref.

Tratcher · 2020-09-25T15:36:01Z

src/Http/Http.Features/src/ISession.cs

@@ -43,7 +44,7 @@ public interface ISession
        /// <param name="key"></param>
        /// <param name="value"></param>
        /// <returns></returns>
-        bool TryGetValue(string key, out byte[] value);
+        bool TryGetValue(string key, [MaybeNullWhen(false)] out byte[] value);


This might be more problematic with how distributed session works. I think it can return null while returning true.

I wouldn't expect that. Where do you see it happening?

src/Middleware/Session/src/NoOpSessionStore.cs

src/Middleware/StaticFiles/src/StaticFileContext.cs

pranavkm

🆙 📅

pranavkm · 2020-10-16T23:41:34Z

src/Middleware/Session/src/DistributedSession.cs

@@ -105,7 +106,7 @@ private byte[] IdBytes
                    _sessionIdBytes = new byte[IdByteCount];
                    RandomNumberGenerator.Fill(_sessionIdBytes);
                }
-                return _sessionIdBytes;
+                return _sessionIdBytes!;


Ok. I've removed the check here. I've added a new exception to Serialize which would have previously resulted in a null-ref.

pranavkm · 2020-10-17T00:15:44Z

src/Middleware/Session/src/SessionFeature.cs

@@ -8,6 +8,6 @@ namespace Microsoft.AspNetCore.Session
 {
    public class SessionFeature : ISessionFeature
    {
-        public ISession Session { get; set; }
+        public ISession Session { get; set; } = default!;


Hmm, I don't think this works for this case. In the ordinary case, features are instantiated by the framework and initialize these properties. If the feature exists, user code does not see a null value. Ideally we would have created initialized this in the ctor, but that ship has sailed.

src/Middleware/Session/src/NoOpSessionStore.cs

src/Middleware/Session/src/DistributedSession.cs

pranavkm requested review from jkotalik and Tratcher as code owners September 19, 2020 15:00

pranavkm requested a review from JamesNK September 19, 2020 15:00

pranavkm added this to the 6.0.0-alpha1 milestone Sep 19, 2020

pranavkm commented Sep 19, 2020

View reviewed changes