search

Finbuckle / Finbuckle.MultiTenant

Finbuckle.MultiTenant is an open-source multitenancy middleware library for .NET. It enables tenant resolution, per-tenant app behavior, and per-tenant data isolation.
https://www.finbuckle.com/multitenant
Apache License 2.0
1.26k stars 258 forks source link

MultiTenantContext can be null? #491

Open Bouke opened 2 years ago

Bouke commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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] commented 2 years ago

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 commented 2 years ago

Ugh stale bot 👎.

AndrewTriesToCode commented 2 years ago

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] commented 1 year ago

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 commented 1 year ago

Ugh stale bot 👎.

codymullins commented 1 month ago

@AndrewTriesToCode - it's still possible for MultiTenantContext to be null, e.g. when there is no Authorize attribute on an endpoint and you are using the Claims strategy.

Not having the property be nullable ends up with a really mysterious null reference exception that drove me crazy this week. I'd propose we make this nullable in the next major version. Thoughts?

AndrewTriesToCode commented 1 month ago

@codymullins thanks for reaching out. Sounds like that was a difficult one to debug on your side. My preference would be to fix this error so that that MultiTenantContext itself is never null -- the ITenantInfo property may be, but the context itself should always be there. It cleans up a lot of code that would have to otherwise check for null on the context.

Do you have an example or a test project that can reproduce the null context that I can take a look at?

codymullins commented 1 month ago

I think I have a fairly decent fix for this -- one is relying more heavily on the accessor in the data context and the other is an early return in the EnforceMultiTenant method. Let me play around more and see if it resolves everything.

I'll try to wiggle some time to get a light repro, but it is really consistent with our specific flow (IDP provider + an app, both multi-tenant)