Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add ContextSnapshot.captureFromMany variants #98

Closed
wants to merge 2 commits into from

Conversation

bclozel
Copy link
Contributor

@bclozel bclozel commented May 9, 2023

Prior to this commit, ContextSnapshot would only allow to capture from
a single context, unless the captureAll variants which involve
ThreadLocal capture.

This commit introduces new captureFromMany methods for this, since
introducing varargs variants for captureFrom would break existing
applications.
This new method allows to capture a context snapshot from multiple
contexts (without capturing from threadlocals). If a particular
key is present in multiple contexts, it will be overriden as values are
extracted.

Prior to this commit, `ContextSnapshot` would only allow to capture from
a single context, unless the `captureAll` variants which involve
`ThreadLocal` capture.

This commit introduces new `captureFromMany` methods for this, since
introducing varargs variants for `captureFrom` would break existing
applications.
This new method allows to capture a context snapshot from multiple
contexts (without capturing from threadlocals). If a particular
key is present in multiple contexts, it will be overriden as values are
extracted.
@bclozel bclozel requested a review from rstoyanchev May 9, 2023 13:58
@bclozel
Copy link
Contributor Author

bclozel commented May 9, 2023

This PR is in draft mode currently - there is a use case for this in Spring for GraphQL instrumentation.

@rstoyanchev
Copy link
Collaborator

rstoyanchev commented May 10, 2023

To clarify the scenario, the GraphQL Java DataFetchingEnvironment has a "main", per-request GraphQLContext object in addition to any number of local ones for specific fields (and sub-fields below them). Until now only the main context was used, but there is a need to overlay that with local contexts.

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 a straight forward change. In retrospect, the captureFrom methods should have been vaarg methods from the start, just like captureAll. Be it as it may, we can either double the number of captureFrom~ methods for a total of 6, or we can correct via deprecations. My thought is to rename the proposed captureFromMany to captureFromContext, and deprecate captureFrom eventually to be removed.

What do you think @chemicL, @marcingrzejszczak ?

This commit introduces `captureFromContext` method variants and
deprecates all `captureFrom` methods; this allows to introduce
consistent ordering of method parameters and variants with varargs for
supporting multiple contexts.
@bclozel
Copy link
Contributor Author

bclozel commented May 10, 2023

I've just pushed a second commit that aligns with Rossen's thoughts so you can see what it would look like.

@chemicL
Copy link
Collaborator

chemicL commented May 11, 2023

Looks good to me, thanks for aligning the method names, too.

rstoyanchev pushed a commit that referenced this pull request May 11, 2023
Prior to this commit, `ContextSnapshot` would only allow to capture from
a single context, unless the `captureAll` variants which involve
`ThreadLocal` capture.

This commit introduces `captureFromContext` method variants with varargs
to support multiple contexts and deprecates all `captureFrom` methods.

See gh-98
@rstoyanchev rstoyanchev added the enhancement A general enhancement label May 11, 2023
@rstoyanchev rstoyanchev added this to the 1.0.3 milestone May 11, 2023
@bclozel bclozel deleted the manycontexts branch May 11, 2023 12:32
@chemicL chemicL removed this from the 1.0.3 milestone Jun 7, 2023
@bclozel
Copy link
Contributor Author

bclozel commented Jun 19, 2023

Replaced by #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants