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

Ensure reactive context is propagated #512

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Conversation

jeremyg484
Copy link
Contributor

@jeremyg484 jeremyg484 commented Mar 5, 2024

DefaultGraphQLInvocation and DefaultGraphQLExecutionInputCustomizer are updated to use Mono and Flux in their internal implementation in order to ensure that the Micronaut PropagationContext is carried appropriately through the invocation flow. Previous use of the bare io.micronaut.core.async.publisher.Publishers API in DefaultGraphQLExecutionInputCustomizer was causing the context not to be propagated as desired when including the Micrometer Context Propagation library.

A test is added to verify the context propagation works as desired.

Some additional cleanup is done throughout the test suite to reduce the scope of included Micronaut beans to the specific tests in order to make it easier to test different setups.

Resolves #495

DefaultGraphQLInvocation and DefaultGraphQLExecutionInputCustomizer are updated
to use Mono and Flux in their internal implementation in order to ensure that
the Micronaut PropagationContext is carried appropriately through the invocation
flow. Previous use of the bare io.micronaut.core.async.publisher.Publishers API
in DefaultGraphQLExecutionInputCustomizer was causing the context not to be
propagated as desired when including the Micrometer Context Propagation library.

A test is added to verify the context propagation works as expected.

Some additional cleanup is done throughout the test suite to reduce the scope
of included Micronaut beans to the specific tests in order to make it easier to
test different setups.

Resolves #495
@jeremyg484 jeremyg484 self-assigned this Mar 5, 2024
Copy link
Contributor

@sdelamo sdelamo left a comment

Choose a reason for hiding this comment

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

Looking good.

Can we rewrite the snippets:

    ApplicationContext context = new DefaultApplicationContext(Environment.TEST)
        context.environment.addPropertySource(PropertySource.of(
                ["spec.name": GraphQLConfigurationSpec.simpleName,
                 "graphql.graphiql.enabled": false]
        ))
        context.start()

with:

ApplicationContext context = ApplicationContext.run(["spec.name": GraphQLConfigurationSpec.simpleName,  "graphql.graphiql.enabled": false])

I don't think we should be using DefaultApplicationContext class directly which is an internal class.

timyates
timyates previously approved these changes Mar 6, 2024
@jeremyg484
Copy link
Contributor Author

I don't think we should be using DefaultApplicationContext class directly which is an internal class.

Agreed, good point. Done in 5ebb76d

@jeremyg484 jeremyg484 requested a review from sdelamo March 6, 2024 16:14
Copy link

sonarcloud bot commented Mar 6, 2024

@sdelamo sdelamo requested a review from timyates March 7, 2024 14:49
@sdelamo sdelamo added the type: bug Something isn't working label Mar 7, 2024
@sdelamo sdelamo merged commit 9d73b52 into master Mar 7, 2024
23 checks passed
@sdelamo sdelamo deleted the context-propagation branch March 7, 2024 14:51
sdelamo pushed a commit that referenced this pull request Mar 7, 2024
DefaultGraphQLInvocation and DefaultGraphQLExecutionInputCustomizer are updated
to use Mono and Flux in their internal implementation in order to ensure that
the Micronaut PropagationContext is carried appropriately through the invocation
flow. Previous use of the bare io.micronaut.core.async.publisher.Publishers API
in DefaultGraphQLExecutionInputCustomizer was causing the context not to be
propagated as desired when including the Micrometer Context Propagation library.

A test is added to verify the context propagation works as expected.

Some additional cleanup is done throughout the test suite to reduce the scope
of included Micronaut beans to the specific tests in order to make it easier to
test different setups.

* Avoid direct instantiation of DefaultApplicationContext

Resolves #495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ServerRequestContext.currentRequest() is empty when enable-automatic-context-propagation: true
3 participants