regression in GraphQLScalarType Coercing parseValue in 18.1

[softprops]

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

[softprops]

Additional details which may be helpful

I’m using both the max depth and query complexity limiting instrumentations provided by graphql java

[bbakerman]

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]

See https://github.com/graphql-java/graphql-java/releases/tag/v18.1

[softprops]

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]

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

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]

Thanks. I'll be cheering you on from the sidelines!

[softprops]

I just wanted to confirm the fix. I just upgraded to 18.2 in production without issue. Thank you so much for the fix!