Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Disallowing null values in ContextSnapshot #102

Merged
merged 10 commits into from
May 25, 2023
Merged

Disallowing null values in ContextSnapshot #102

merged 10 commits into from
May 25, 2023

Conversation

chemicL
Copy link
Collaborator

@chemicL chemicL commented May 24, 2023

This change takes care of guaranteeing @NonNullApi package annotation is in effect for ThreadLocalAccessor calls when dealing with ContextSnapshot. When a source context Object provided null for a key, the API would not have held its guarantees. Now we explicitly make sure the backing Map has no mappings to null.

@chemicL chemicL added this to the 1.0.3 milestone May 24, 2023
@chemicL chemicL self-assigned this May 24, 2023
@chemicL chemicL changed the base branch from main to 1.0.x May 24, 2023 14:24
@chemicL chemicL changed the title Non null api Disallowing null values in ContextSnapshot May 24, 2023
@rstoyanchev
Copy link
Collaborator

rstoyanchev commented May 24, 2023

A bit more context from offline conversations.

When capturing ThreadLocal values, we already prevent null from going into a ContextSnapshot, and with a ThreadLocal it's impossible to differentiate between a ThreadLocal with null vs no ThreadLocal at all. Likewise, the Reactor Context does not allow null values, so there is no way for null values to enter a ContextSnapshot from a Reactor Context either.

Therefore chances are that a ContextSnapshot never contains null values. Nevertheless, ContextSnapshot is a Map, it is possible for some other type of context and its ContextAccessor to insert null values. From there the possibility of null values in the ContextSnapshot raises questions about what to do when setting ThreadLocal values, i.e. whether to clear it or set it to null. There are no good answers, and the distinction again between no ThreadLocal and ThreadLocal with null is meaningless. Rather than trying to come with with ways to deal with the possibility of null values, it is better to close the loophole, and ensure that a ContextSnapshotnever containsnull` values.

Note that we also have static methods on ContextSnapshot to set ThreadLocal values directly through a ContextAccessor, and in that case we also already ignore null values, and there wouldn't be a way in any case for ContextAccessor#readValue to differentiate between no value and null value.

So all this PR does is to provide a consistent treatment of null values by preventing them from making their way into the ContextSnapshot and updating the contracts accordingly with such guidance.

Copy link
Collaborator

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, good changes!

  1. I would like to see a Javadoc updates that settles this topic more formally in writing now that we've looked at it in more detail and made concrete decisions. Starting with ContextSnapshot, clarify that a snapshot should not contain null values since those are not useful for setting ThreadLocal values anyway, and that all implementations should ensure that null values are ignored, and never passed into ContextAccessor or ThreadLocalAccessor. Similar updates could be made in ContextAccessor, e.g. on readValues to explain that any null values put into the Map will be filtered out, and likewise on setValues to clarify the source Map will never have null values. Make similar updates in ThreadLocalAccessor too in the setValue and restore(String) methods.
  2. Around this line I think it would be more clear to get the value and either check and/or assert that it is not null. Otherwise, if null finds its way in somehow (bug?), it goes into the setValue method of TLA where it may or may not be handled well. In short, we should assert our assumption that there is no null value.

@chemicL chemicL requested a review from rstoyanchev May 25, 2023 15:02
Copy link
Collaborator

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the updates. Looks good, just one minor Javadoc update below.

…tSnapshot.java

Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
@chemicL
Copy link
Collaborator Author

chemicL commented May 25, 2023

@rstoyanchev thank you for the comprehensive review!

@chemicL chemicL merged commit e443fe9 into 1.0.x May 25, 2023
@chemicL chemicL deleted the non-null-api branch May 25, 2023 17:54
@chemicL
Copy link
Collaborator Author

chemicL commented May 26, 2023

For future reference: the merge commit (e443fe9) has been squashed (failure to squash+merge on my part) into c5998ba

@chemicL chemicL added the enhancement A general enhancement label Jun 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants