Nullable annotation fixes for consistency #41
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #40.
Issue #40 indicated that the
ITenantIdentificationStrategy.TryIdentifyTenant
was allowing for a null return value if the method returnedtrue
.While I think this is a super edge case, technically speaking we do have the concept of a "default tenant" in the system that is indicated by a
null
tenant ID. Thus, it is technically possible someone may want to say, "I looked, and I intentionally have determined that we should use the default tenant ID, thus I am intentionally setting the return value tonull
."I found that we weren't correctly handling all nullable annotations in
MultitenantContainer
, so that has been updated. I've also added some documentation to explain the potential intentionalnull
; and some unit tests to illustrate more of the notion of intentional identification-as-null.The other option here would be to make a change to the
TryIdentifyTenant
method definition such that it's not allowed to returnnull
if the tenant is identified. I think it's possible this is a breaking change given that sort of behavior is currently allowed and we've definitely seen people doing every possible edge case out there, even if it's not the 80% case. If we did that, it may be that we need to have a public/singleton for the default tenant ID such that an intentional identification as the default tenant will result in a non-null tenant ID. Not a huge lift, but not just a nullable annotation change, either.