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.TryIdentifyTenantwas 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
nulltenant 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
TryIdentifyTenantmethod definition such that it's not allowed to returnnullif 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.