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();