Skip to content

Controller's with CompletableFuture return bad error messages or no error at all #479

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

Closed
JamesPeters98 opened this issue Sep 2, 2022 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@JamesPeters98
Copy link

JamesPeters98 commented Sep 2, 2022

I can't tell if this is bad API use on my end, as I'm not an expert with Reactor/Async etc.

But my controllers that return a Completable future of a List type e.g

  @QueryMapping
  public CompletableFuture<List<String>> jobNumbers(Instant timestamp, DataFetchingEnvironment env) {
    return CompletableFuture.completedFuture(List.of("123"));
  }

return bad error messages when there are any exceptions outside of the CompletableFuture. In this case, I missed the @Argument annotation on the timestamp parameter but only got this generic message:

n.graphql.execution.ExecutionStrategy    : Can't resolve value (/jobNumbers) : type mismatch error, expected type LIST got class reactor.core.publisher.MonoError

Or if the type is a String it simply just gets passed through to the final query like this:

{
    "data": {
        "jobNumbers": "MonoError"
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 2, 2022
@JamesPeters98 JamesPeters98 changed the title Controller's with CompletableFutur return bad error messages or no error at all Controller's with CompletableFuture return bad error messages or no error at all Sep 4, 2022
@rstoyanchev
Copy link
Contributor

I could not easily reproduce this. There is something less obvious going on. Could you create a sample through https://start.spring.io/?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Sep 8, 2022
@JamesPeters98
Copy link
Author

Ah, yes it turns out it seems to only happen when graphql-java-extended-validation is added into the runtime wiring:

 @Bean
  RuntimeWiringConfigurer runtimeWiringConfigurer() {
    var validationRules = ValidationRules.newValidationRules()
      .build();

    return wiring -> {
      wiring.directiveWiring(new ValidationSchemaWiring(validationRules));
    };
  }

I can't tell if this is a Spring issue or GraphQLJava? Or if the configuration is wrong for use with Spring, I've just used the standard directive wiring on the github page.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 8, 2022
@JamesPeters98
Copy link
Author

Looks like FieldValidatorDataFetcher causes the DataFetcher to bypass ContextDataFetcherDecorator in GraphQlMetricsInstrumentation somewhere.

Not sure if this relates to changes made in, or is similar to, this issue #240

@JamesPeters98
Copy link
Author

if (dataFetcher.getClass().getPackage().getName().startsWith("graphql.")) {
    return TraversalControl.CONTINUE;
}

This is where the problem happens in ContextDataFetcherDecorator.java:119

Since FieldValidatorDataFetcher is in the graphql package it doesn't get added as a ContextDataFetcherDecorator

@rstoyanchev
Copy link
Contributor

That does sound like a problem. I'll schedule it to have a closer look. We'll probably have refine how we decide if a DataFetcher should be decorated or not.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Sep 8, 2022
@rstoyanchev rstoyanchev added this to the 1.0.2 milestone Sep 8, 2022
@rstoyanchev rstoyanchev self-assigned this Sep 8, 2022
@JamesPeters98
Copy link
Author

Thanks for the quick fix, when's the next release cycle for 1.0.2 likely to be?

@rstoyanchev
Copy link
Contributor

Next week. It's set for Sep 20 now.

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

No branches or pull requests

3 participants