Repository navigation
Difference in catch and reporting behavior of Coercing value errors vs literal errors #2811
Description
Describe the bug
I've recently noticed a difference in behavior (using release 18.0) between how exceptions are handled implementing a custom scalar which parses input fields values and literals.
As I understand them parseLiteral is used in the case where an input comes directly from a query. This works well as described in the docs when the custom Coercing throws a CoercingParseLiteralException exception. The error is handled by the execution engine and serialized with normal graphql errors according to the spec in the responses errors array.
However, when the same input comes from a query variable, parseValue is used and, as the as described in the docs mention, I throw CoercingParseValueException exception. In the case the execution engine does not catch and handle the exception and instead it results in a 500 error from my server rather than a 200 response with a graphql errors spec response.
below is an example stack trace if it helps
2022-04-23 17:27:49,743 - - - [jetty task-169] ERROR org.jooby.Err - execution of: POST/ resulted in exception
Route:
| Method | Path | Source | Name | Pattern | Consumes | Produces |
|--------|------|--------------------------------------------|------------|---------|----------|----------|
| POST | / | com.meetup.graphql.server.GraphqlServer:66 | /anonymous | / | [*/*] | [*/*] |
Stacktrace:
org.jooby.Err: Server Error(500)
<snip>
Caused by: graphql.schema.CoercingParseValueException: Variable 'startDateTime' has an invalid value: Try something like '2022-04-24T17:27' instead
at graphql.schema.CoercingParseValueException$Builder.build(CoercingParseValueException.java:46)
at graphql.execution.ValuesResolver.externalValueToInternalValueForVariables(ValuesResolver.java:420)
at graphql.execution.ValuesResolver.coerceVariableValues(ValuesResolver.java:94)
at graphql.analysis.QueryTraverser.coerceVariables(QueryTraverser.java:64)
at graphql.analysis.QueryTraverser.<init>(QueryTraverser.java:60)
at graphql.analysis.QueryTraverser.<init>(QueryTraverser.java:40)
at graphql.analysis.QueryTraverser$Builder.build(QueryTraverser.java:297)
at graphql.analysis.MaxQueryComplexityInstrumentation.newQueryTraverser(MaxQueryComplexityInstrumentation.java:134)
at graphql.analysis.MaxQueryComplexityInstrumentation.lambda$beginValidation$4(MaxQueryComplexityInstrumentation.java:85)
at graphql.execution.instrumentation.SimpleInstrumentationContext.onCompleted(SimpleInstrumentationContext.java:51)
at graphql.execution.instrumentation.ChainedInstrumentation$ChainedInstrumentationContext.lambda$onCompleted$1(ChainedInstrumentation.java:238)
at graphql.com.google.common.collect.ImmutableList.forEach(ImmutableList.java:422)
at graphql.execution.instrumentation.ChainedInstrumentation$ChainedInstrumentationContext.onCompleted(ChainedInstrumentation.java:238)
at graphql.GraphQL.validate(GraphQL.java:628)
at graphql.GraphQL.parseAndValidate(GraphQL.java:589)
at graphql.GraphQL.lambda$parseValidateAndExecute$10(GraphQL.java:553)
at com.meetup.graphql.server.GraphqlServer.lambda$new$0(GraphqlServer.java:59)
at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1134)
at java.base/java.util.Collections$SynchronizedMap.computeIfAbsent(Collections.java:2682)
at com.meetup.graphql.server.GraphqlServer.lambda$new$1(GraphqlServer.java:59)
at graphql.execution.preparsed.PreparsedDocumentProvider.getDocumentAsync(PreparsedDocumentProvider.java:45)
at graphql.GraphQL.parseValidateAndExecute(GraphQL.java:555)
at graphql.GraphQL.executeAsync(GraphQL.java:524)
at graphql.GraphQL.execute(GraphQL.java:450)
at com.meetup.graphql.server.GraphqlQueryExecutor.toExecutionResult(GraphqlQueryExecutor.java:93)
at com.meetup.graphql.server.GraphqlHandler.handle(GraphqlHandler.java:82)
at com.meetup.graphql.server.GraphqlServer.lambda$new$2(GraphqlServer.java:79)
at org.jooby.Route$Handler.handle(Route.java:1761)
at org.jooby.internal.RouteImpl.handle(RouteImpl.java:280)
at org.jooby.internal.RouteChain.next(RouteChain.java:262)
at org.jooby.Route$Chain.next(Route.java:2164)
at org.jooby.internal.HttpHandlerImpl.handle(HttpHandlerImpl.java:496)
... 19 more
Caused by: java.time.format.DateTimeParseException: Text '2022-05-01T05:30+400' could not be parsed, unparsed text found at index 16
at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2049)
at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1948)
at java.base/java.time.LocalDateTime.parse(LocalDateTime.java:492)
To Reproduce
Here is what my Coercing scalar impl looks like. note parse value errors are reported with CoercingParseValueException and parse literal errors are reported with CoercingParseLiteralException
when an invalid input is passed in a literal the execution engine does what I expect. when an invalid input is passed in through a variable, the exception bubbles all the way up to my server, resulting in a 500 error.
static java.time.LocalDateTime parseLocalDateTimeValue(Object input) {
String value =
input instanceof StringValue ? ((StringValue) input).getValue() : Objects.toString(input);
try {
if (input instanceof StringValue) {
return java.time.LocalDateTime.parse(value);
} else if (input instanceof String) {
return java.time.LocalDateTime.parse(value);
} else throw new IllegalArgumentException("Unexpected input type: " + input.getClass());
} catch (Exception e) {
throw new CoercingParseValueException(
"Try something like '"
+ java.time.LocalDateTime.now().truncatedTo(ChronoUnit.MINUTES).plusDays(1)
+ "' instead",
e);
}
}
static java.time.LocalDateTime parseLocalDateTimeLiteral(Object input) {
String value =
input instanceof StringValue ? ((StringValue) input).getValue() : Objects.toString(input);
try {
if (input instanceof StringValue) {
return java.time.LocalDateTime.parse(value);
} else if (input instanceof String) {
return java.time.LocalDateTime.parse(value);
} else throw new IllegalArgumentException("Unexpected input type: " + input.getClass());
} catch (Exception e) {
throw new CoercingParseLiteralException(
"Try something like '"
+ java.time.LocalDateTime.now().truncatedTo(ChronoUnit.MINUTES).plusDays(1)
+ "' instead.",
e);
}
}
public static final GraphQLScalarType LocalDateTime =
GraphQLScalarType.newScalar()
.name("LocalDateTime")
.description("A custom scalar that handles timestamps that exclude timezone information")
.coercing(
new Coercing<java.time.LocalDateTime, String>() {
@Override
public String serialize(Object dataFetcherResult) {
return null; // omitted as its not relevant to this issue
}
@Override
public java.time.LocalDateTime parseValue(Object input) {
return parseLocalDateTimeValue(input);
}
@Override
public java.time.LocalDateTime parseLiteral(Object input) {
return parseLocalDateTimeLiteral(input);
}
})
.build();
Activity
softprops commentedon Apr 23, 2022
update. I've narrowed down my case which I think might help point to the source of the bug (unless this is intentional behavior).
Note in the stack trace above, I'm using both
MaxQueryComplexityInstrumentationMaxQueryDepthInstrumentationin my instrumentation chain. It seems if you use either of these (I use both) you'll get the behavior I mentioned above. If I remove them entirely, I'll get the expected behavior of theCoercingParseValueExceptionbeing caught, handled, and serialized expected by the execution engine.It seems that there might a unhandled case with
MaxQueryComplexityInstrumentationandMaxQueryDepthInstrumentationthat's causing the issueI checked in case it was also of interested Im also using simple
PreparsedDocumentProvider. I was also curious if this contributed to the unexpected behavior but it did notbbakerman commentedon Apr 25, 2022
Thanks for the report - we have had other related problems with the MaxQueryComplexityInstrumentation where the new "value collector" code ran too early.
This PR changes that - instead of running the code immediately AFTER validation - it now waits for the engine to start and hence any co-erce exceptions should be caught and turned into errors which is what SHOULD happen.
The reason this change is that we "improved" the way "values" are collected in 18.x but it had a side effect for these instrumentations that we didn't see in testing.
#2773
If you are able to, we would appreciate you trying this PR and see if this solves the problem for you
softprops commentedon Apr 26, 2022
Thanks. I can probably take a look in the next few days.
Thanks for all your hard work on this amazing project
softprops commentedon May 16, 2022
I pulled in the latest release and it worked like a charm!