Repository navigation
regression in GraphQLScalarType Coercing parseValue in 18.1 #2819
Description
Describe the bug
I just hit a bug in production when deploying an upgrade from 18.0 to 18.1 that I think relates to some of the work in resolving #2811
I noticed the following in my production logs
...
Caused by: graphql.schema.CoercingParseValueException: Variable 'memberDues' has an invalid value: Failed to parse input value 2022-05-16T19:52:37Z as ZonedDateTime
...
Caused by: java.lang.IllegalArgumentException: Unexpected input type: class java.time.ZonedDateTimeat com.meetup.graphql.wiring.CustomScalars.parseDateTimeValue(CustomScalars.java:38)at com.meetup.graphql.wiring.CustomScalars$1.parseValue(CustomScalars.java:71)at com.meetup.graphql.wiring.CustomScalars$1.parseValue(CustomScalars.java:63)at graphql.execution.ValuesResolver.externalValueToInternalValueForScalar(ValuesResolver.java:578)at graphql.execution.ValuesResolver.externalValueToInternalValue(ValuesResolver.java:510)at graphql.execution.ValuesResolver.externalValueToInternalValue(ValuesResolver.java:498)at graphql.execution.ValuesResolver.externalValueToInternalValueForObject(ValuesResolver.java:565)at graphql.execution.ValuesResolver.externalValueToInternalValue(ValuesResolver.java:517)at graphql.execution.ValuesResolver.externalValueToInternalValueForObject(ValuesResolver.java:565)at graphql.execution.ValuesResolver.externalValueToInternalValue(ValuesResolver.java:517)at graphql.execution.ValuesResolver.externalValueToInternalValue(ValuesResolver.java:498)at graphql.execution.ValuesResolver.externalValueToInternalValueForVariables(ValuesResolver.java:410)
note that the string 2022-05-16T19:52:37Z is actually parsable as ZonedDateTime however I believe the issue is previously parseValue was receiving either a String or a StringValue and after this upgrade it started receiving the value that I was attempting to coerce it into, in this case a ZonedDateTime
is this new expected behavior or was this previous expected behavior that was underspecified until now?
Here's an abbreviated version of a custom scaler for parsing out ZonedDateTime values which is roughly based on the docs
static ZonedDateTime parseDateTimeValue(Object input) {
try {
if (input instanceof StringValue) {
return ZonedDateTime.parse(((StringValue) input).getValue());
} else if (input instanceof String) {
return ZonedDateTime.parse((String) input);
} else throw new IllegalArgumentException("Unexpected input type: " + input.getClass()); <-- change in behavior causes this line to get evaluated
} catch (Exception e) {
throw new CoercingParseValueException(
"Failed to parse input value " + input + " as ZonedDateTime", e);
}
}
public static final GraphQLScalarType DateTime =
GraphQLScalarType.newScalar()
.name("DateTime")
.description("A custom scalar that handles timestamps")
.coercing(
new Coercing<ZonedDateTime, String>() {
@Override
public String serialize(Object dataFetcherResult) {
return // doesnt matter
}
@Override
public ZonedDateTime parseValue(Object input) {
return parseDateTimeValue(input);
}
@Override
public ZonedDateTime parseLiteral(Object input) {
return // doesnt matter
}
})
.build();To Reproduce
Please provide a code example or even better a test to reproduce the bug.
see above
Activity
softprops commentedon May 17, 2022
Additional details which may be helpful
I’m using both the max depth and query complexity limiting instrumentations provided by graphql java
bbakerman commentedon May 19, 2022
This has been fixed in a 18.1 release in this PR : #2773
It was specific to a new way to co-erce variables combined with either of those two instrumentations
bbakerman commentedon May 19, 2022
See https://github.com/graphql-java/graphql-java/releases/tag/v18.1
softprops commentedon May 20, 2022
To clarify this is new behavior only after upgrading to 18.1 from 18.0. I’m familiar with the previous issue. Something changes in the fix for that that broke. Existing coercers now seemed passed arguments that were already coerced. I’m not sure if that is new expected behavior or not
bbakerman commentedon May 22, 2022
Sorry I didnt read the bug report well enough - yes we have found a place where values are getting double co-erced. This is not new - the double co-ercion was always there - however what happens during co-ercion IS new and hence the existing double co-ercion bug is now hurting.
We are looking at this
softprops commentedon May 23, 2022
Thanks. I'll be cheering you on from the sidelines!
softprops commentedon Jun 28, 2022
I just wanted to confirm the fix. I just upgraded to 18.2 in production without issue. Thank you so much for the fix!
Update graphql-java