ITenantIdentificationStrategy.TryIdentifyTenant nullable annotation incomplete

[bbarry]

Describe the Bug

Nullable annotation on ITenantIdentificationStrategy.TryIdentifyTenant is incorrect. It is missing an attribute [NotNullWhen(true)] to follow common try-get-* patterns seen commonly in .NET.

Steps to Reproduce

public void DoSomething(ITenantIdentificationStrategy strategy)
{
    if (strategy.TryIdentifyTenant(out var id))
    {
        Foo(id);
    }
}
public void Foo(object input) { }

Expected Behavior

No compiler warning.

Actual Behavior

CS8604: Possible null reference argument for parameter 'input'

Additional Info

https://github.com/dotnet/csharplang/blob/main/meetings/2019/LDM-2019-05-15.md#conditional-postconditions

Mscorelib offers some guidelines intended to be specific to that library but that turn out to be in general good to follow: The api guidelines for that repo say https://github.com/dotnet/runtime/blob/e12e2fa6cbdd1f4b0c8ad1b1e2d960a480c21703/docs/coding-guidelines/api-guidelines/nullability.md?plain=1#L88 and from this the following bullet seems to apply:

• DO annotate non-generic out reference type arguments on Try methods as being nullable and with [NotNullWhen(true)] (e.g. TryCreate([NotNullWhen(true)] out Semaphore? semaphore)). For non-generic arguments, the argument should be nullable because a failed call will generally store null into the argument, and it should be [NotNullWhen(true)] because a successful call will store non-null.

[tillig]

There's an interesting bit here because this existed long before nullable annotations.

While I do see I missed the ReconfigureTenant annotation to allow a null tenant ID to be passed in, we actually do allow you to say "the current tenant ID is null" to indicate that you've explicitly said, "I identified the tenant, and I want it to be the default tenant settings."

That, at least at the time, made some sense. Changing that may actually be considered breaking, even if it is a bit of an edge case.

This is probably a good opportunity to take a look at how null tenant IDs are handled all the way through the stack and at least make sure they're dealt with consistently. The nullable annotation thing is a good trigger to investigate it, and possibly add some more tests around that if they're missing.

[tillig]

I submitted the PR for this here and just merged it. Since we can technically have a null tenant ID found, I updated the annotations to make things consistently allow that. That does mean you'll still get a potential null coming out of TryIdentifyTenant, but I think about the logic more like this:

Let's say you have IDictionary<string, object?>. If you do dict.TryGetValue("key", out var value)), the key was found, but the value may, in fact, be null.

In this case, the tenant ID was determined, but it was determined to intentionally be null.

Anyway, I got the other methods updated to properly allow null tenant IDs to get passed in so things should be consistent. I also added a couple of unit tests to show how that intentionally-null tenant IDs are possible/supported and ensure that doesn't break in the future.

I'll try to cut a v8.0.3 release shortly.

[bbarry]

That comment handles it reasonably. The additional annotations as well as a few small similar changes in my code should fix my warnings also.

Thanks.