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

ServerRequestContext.currentRequest() is empty when enable-automatic-context-propagation: true #495

Closed
vad3x opened this issue Jan 23, 2024 · 2 comments · Fixed by #512
Closed
Assignees
Labels
type: bug Something isn't working

Comments

@vad3x
Copy link

vad3x commented Jan 23, 2024

Expected Behavior

ServerRequestContext.currentRequest() should not be empty.

Actual Behaviour

ServerRequestContext.currentRequest() is empty when enable-automatic-context-propagation: true which breaks main services such as SecurityService.

Steps To Reproduce

  1. add implementation("io.micronaut.reactor:micronaut-reactor") to gradle.
  2. add implementation("io.micrometer:context-propagation").
  3. Run the app.
  4. ServerRequestContext.currentRequest() is empty for DataFetchers.

Data fetcher example:

@Singleton
public class GraphQLDataFetchers {
    private static final Logger LOG = LoggerFactory.getLogger(GraphQLDataFetchers.class);

    private final DbRepository dbRepository;

    public GraphQLDataFetchers(DbRepository dbRepository) {
        this.dbRepository = dbRepository;
    }

    public DataFetcher<Book> getBookByIdDataFetcher() {
        return dataFetchingEnvironment -> {
            // should not be empty, but empty
            LOG.info("currentRequest={}", ServerRequestContext.currentRequest());

            String bookId = dataFetchingEnvironment.getArgument("id");
            return dbRepository.findAllBooks()
                    .stream()
                    .filter(book -> book.getId().equals(bookId))
                    .findFirst()
                    .orElse(null);
        };
    }
}

Setting enable-automatic-context-propagation: false makes ServerRequestContext.currentRequest() to be not empty.

reactor:
  enable-automatic-context-propagation: false

The workaround I found is to avoid using io.micronaut.core.async.publisher.Publishers.* library.
For example, If I override DefaultGraphQLInvocation.invoke with:

    @Override
    public Publisher<ExecutionResult> invoke(
            GraphQLInvocationData invocationData,
            HttpRequest httpRequest,
            @Nullable MutableHttpResponse<String> httpResponse) {
        ExecutionInput.Builder executionInputBuilder = ExecutionInput.newExecutionInput()
                .query(invocationData.getQuery())
                .operationName(invocationData.getOperationName())
                .variables(invocationData.getVariables());
        if (dataLoaderRegistry != null) {
            executionInputBuilder.dataLoaderRegistry(dataLoaderRegistry.get());
        }
        ExecutionInput executionInput = executionInputBuilder.build();

        return Flux.from(graphQLExecutionInputCustomizer.customize(executionInput, httpRequest, httpResponse))
                // replaced Publishers.fromCompletableFuture with Mono.fromFuture
                .flatMap(customizedExecutionInput -> Mono.fromFuture(() -> {
                    try {
                        return graphQL.executeAsync(customizedExecutionInput);
                    } catch (Throwable e) {
                        CompletableFuture future = new CompletableFuture();
                        future.completeExceptionally(e);
                        return future;
                    }
                }));
    }

And DefaultGraphQLExecutionInputCustomizer.customize

    @Override
    public Publisher<ExecutionInput> customize(ExecutionInput executionInput, HttpRequest httpRequest,
            @Nullable MutableHttpResponse<String> httpResponse) {
        // replaced Publishers.just with Flux.just
        return Flux.just(executionInput);
    }

it now starts working well (ServerRequestContext.currentRequest() is NOT empty for DataFetchers).

Environment Information

  • Java v17.0.9
  • Gradle v8.4
  • Micronaut 4.2.3

Example Application

No response

Version

5.2.3

@sdelamo sdelamo added the type: bug Something isn't working label Feb 28, 2024
@jeremyg484
Copy link
Contributor

@vad3x Thanks for reporting. Just want to clarify, did you also have

implementation("io.micrometer:context-propagation")

specified in your original non-working version?

@vad3x
Copy link
Author

vad3x commented Mar 5, 2024

Hi @jeremyg484, yes, I have implementation("io.micrometer:context-propagation") in both versions.

UPD: updated Steps To Reproduce to reflect it.

jeremyg484 added a commit that referenced this issue 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 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
sdelamo pushed a commit that referenced this issue 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
sdelamo pushed a commit that referenced this issue 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 a pull request may close this issue.

3 participants