MultiTenantContext can be null?

[Bouke]

It seems that if no tenant was resolved, MultiTenantContextAccessor<T> will return null for MultiTenantContext. So should this warrant a nullable reference?

https://github.com/Finbuckle/Finbuckle.MultiTenant/blob/65507bfe47ff05ca99f015dec527d6c9951ff76a/src/Finbuckle.MultiTenant/Abstractions/IMultiTenantContextAccessor.cs#L13

[AndrewTriesToCode]

hi @Bouke There is currently interest in improving null reference behavior in the library and making use of null reference types from C#8. Issue #487 is the most recent item on the topic. My general view lately is that MultiTenantContext itself should probably always be returned, but some of its properties would be nullable, e.g. TenantInfo. What do you think?

[Bouke]

There is currently interest in improving null reference behavior in the library and making use of null reference types from C#8. Issue #487 is the most recent item on the topic.

That's great! We're in the process of converting our code as well. It takes some time to get all the nullable annotations right, but after they are in place they are a great help in reducing NRE and removing if null checks that somebody sprinkled over their code "just in case".

What do you think?

That would make sense, yes.

[fossbrandon]

My general view lately is that MultiTenantContext itself should probably always be returned, but some of its properties would be nullable, e.g. TenantInfo. What do you think?

@AndrewTriesToCode I'm currently doing the NRT implementation and reached the same question. My initial reaction is to label it as nullable to match its existing functionality. I was also worried that returning a non-nullable MultiTenantContext with nullable properties might hide a bigger issue from a user. I think it would be best to fail early by letting them know that something went wrong with getting the context instead of leading them to believe it's an issue with a property. If you think it would be better to use your suggestion, let me know and I can switch the code to match it.

[Bouke]

In my application code, I'm interested in TenantInfo only. So having to go through the MultiTenantContext property is mostly noise in my code anyway. So I'm all for having TenantInfo available as a property on the Accessor directly. W.r.t. the nullability I don't really care if it is Accessor.MultiTenantContext?.TenantInfo.Identifier or Accessor.MultiTenantContext.TenantInfo?.Identifier, whatever makes most sense to the framework. However I would suggest not to have both nullable: Accessor.MultiTenantContext?.TenantInfo?.Identifier.

[AndrewTriesToCode]

@fossbrandon

I agree with your thoughts there. I might reconsider for a future major version breaking release but for the initial changeover I prefer to preserve current behavior while being nullable compliant.

[AndrewTriesToCode]

@Bouke I agree with you that both as nullable is annoying -- I think for the next major release I'll have MultiTenantContext be non-nullable. No set timeframe for that yet though.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Bouke]

Ugh stale bot 👎.

[AndrewTriesToCode]

Yeah I know, sorry. By the way I am planning for a major release later this year where MultiTenantContext will never be null which I think will be a friendlier design.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Bouke]

Ugh stale bot 👎.