NonNullableValueCoercedAsNullException being thrown in QueryTraversal #2761

Activity

1. bbakerman commented on Mar 13, 2022

   

   bbakerman

   on Mar 13, 2022

   MemberAuthor

   More actions

   Ok I have had a look at this

   What is happening is that during the QueryTraversal creation ValuesResolver tries to turn the input variables into the internal representation for the runtime.

   During one of these calls, something is returning null when the input type is says its non null

           if (isNonNull(graphQLType)) {
               Object returnValue =
                       externalValueToInternalValue(fieldVisibility, unwrapOne(graphQLType), value);
               if (returnValue == null) {
                   throw new NonNullableValueCoercedAsNullException("", emptyList(), graphQLType);
               }
               return returnValue;
           }

   Now it's possible that the starting value here was non null but the call to a scalar say ended up returning a null value perhaps.

   I see a couple of problems in the code. First off tjhe lack of "field name or type" in the error message ios a mistake. We should fix that.

   The other problem (debatable) is the re-use of NonNullableValueCoercedAsNullException I think this is a mistake in that yes its a nullable value on a non null type but should we have a specific exception for output variables versus input variables to help with semantic understanding. Anyway that can be discussed and debated.

   I think we should

   • improve the error message generation
   • make the variable coercing lazy in QueryTraversal
   • debate on using a different exception here

   As to the root cause, I suspect a scalar is breaking contract here.

       /**
        * Called to resolve an input from a query variable into a Java object acceptable for the scalar type.
        * <p>
        * Note : You should not allow {@link java.lang.RuntimeException}s to come out of your parseValue method, but rather
        * catch them and fire them as {@link graphql.schema.CoercingParseValueException} instead as per the method contract.
        *
        * @param input is never null
        *
        * @return a parsed value which is never null
        *
        * @throws graphql.schema.CoercingParseValueException if value input can't be parsed
        */
       @NotNull I parseValue(@NotNull Object input) throws CoercingParseValueException;

   The parseValue call is never meant to turn null - as the input is never null. It's either CoercingParseValueException or non null value.

   @slisaasquatch do you have any custom scalars in place? Could one of them be breaking contract?
2. slisaasquatch commented on Mar 13, 2022

   

   slisaasquatch

   on Mar 13, 2022

   More actions

   Thanks a lot for looking into this. Yes, I have quite a few custom scalars, and I'll take a look at them.
3. slisaasquatch commented on Mar 13, 2022

   

   slisaasquatch

   on Mar 13, 2022

   More actions

   I checked every single one of my custom scalar implementations, and I don't think it's possible for any of the parseValue methods to return null. Also, the original query in question didn't actually use any custom scalars. Is it possible that something else can trigger this error as well?
4. bbakerman commented on Mar 14, 2022

   

   bbakerman

   on Mar 14, 2022

   Last edited by bbakerman

   MemberAuthor

   More actions

   Is it possible that something else can trigger this error as well?

   Yeah of course - it could be a bug. We need to do a reproduction. Can you send us in the schema and query for the exception above please.

   We have the variables (or we can make them up) but we don't have the original query nor the backing schema that might lead to this. If you can send this in direct or use our anonymizer if you want to protect you type names

   https://github.com/graphql-java/graphql-anonymizer
5. slisaasquatch commented on Mar 14, 2022

   

   slisaasquatch

   on Mar 14, 2022

   More actions

   Sounds good. I've attached the schema and the query in a gist here since they are quite large. The operation name used is UpsertUserMutation.
6. slisaasquatch commented on Mar 14, 2022

   

   slisaasquatch

   on Mar 14, 2022

   More actions

   Hi, after doing more digging, it looks like it's my own fault, because the variables was actually

   {
     "accountId": "133...63",
     "userId": "133...63",
     "userInput": {
       "accountId": "133...63",
       "id": "133...63",
       "referredByCodes": [null]
     }
   }

   And the referredByCodes field got lost in our logging framework, and it's now obvious that that's what's causing the problem.
7. johnloven commented on Mar 15, 2022

   

   johnloven

   on Mar 15, 2022

   More actions

   I get this

   Exception in thread "graphql-exec-1" graphql.execution.NonNullableValueCoercedAsNullException: Variable 'input' has coerced Null value for NonNull type 'UpdateMatrixInput!'
   	at graphql.execution.ValuesResolver.externalValueToInternalValueForVariables(ValuesResolver.java:407)
   	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.MaxQueryDepthInstrumentation.newQueryTraverser(MaxQueryDepthInstrumentation.java:92)
   	at graphql.analysis.MaxQueryDepthInstrumentation.lambda$beginValidation$2(MaxQueryDepthInstrumentation.java:57)
   	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:405)
   	at graphql.execution.instrumentation.ChainedInstrumentation$ChainedInstrumentationContext.onCompleted(ChainedInstrumentation.java:238)
   	at graphql.GraphQL.validate(GraphQL.java:600)
   	at graphql.GraphQL.parseAndValidate(GraphQL.java:562)
   	at graphql.GraphQL.lambda$parseValidateAndExecute$10(GraphQL.java:531)
   	at graphql.execution.preparsed.NoOpPreparsedDocumentProvider.getDocument(NoOpPreparsedDocumentProvider.java:15)
   	at graphql.GraphQL.parseValidateAndExecute(GraphQL.java:533)
   	at graphql.GraphQL.executeAsync(GraphQL.java:502)
   	at graphql.kickstart.execution.GraphQLInvoker.executeAsync(GraphQLInvoker.java:37)
   	at graphql.kickstart.execution.GraphQLInvoker.execute(GraphQLInvoker.java:28)
   	at graphql.kickstart.servlet.HttpRequestInvokerImpl.invoke(HttpRequestInvokerImpl.java:164)
   	at graphql.kickstart.servlet.HttpRequestInvokerImpl.lambda$invokeAndHandleAsync$2(HttpRequestInvokerImpl.java:79)
   	at org.springframework.security.concurrent.DelegatingSecurityContextRunnable.run(DelegatingSecurityContextRunnable.java:82)
   	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
   	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
   	at java.base/java.lang.Thread.run(Thread.java:829)
   
   

   when sending

   {
     "variables":{},
     "query":"mutation ($input: UpdateMatrixInput!) { updateMatrix(input: $input) { id } }"
   }

   with a schema containing

   type Mutation {
       updateMatrix(input: UpdateMatrixInput!): Matrix!
   }
   

   which makes sense.

   The problem is that this exception doesn't trigger anything to be returned to the client, so the client waits for a response until it times out.

   ---

   Having the input inline instead

   {
     "query":"mutation { updateMatrix { id } }"
   }

   results in an error being returned to the client immediately

   {
     "errors":[
       {
         "message":"Validation error of type MissingFieldArgument: Missing field argument input @ 'updateMatrix'",
         "locations":[
           {
             "line":1,
             "column":12
           }
         ],
         "extensions":{
           "classification":"ValidationError"
         }
       }
     ],
     "data":null
   }

   which is what I would want in the first case as well.

   Is there a way to return the error to the client in the first case as well?
8. bbakerman commented on Mar 16, 2022

   

   bbakerman

   on Mar 16, 2022

   MemberAuthor

   More actions

   The problem is that this exception doesn't trigger anything to be returned to the client, so the client waits for a response until it times out.

   You are using kickstart as the framework around graphql-java. I am not sure how it handles exceptions so cant comment on the "timeout" issue.

   But I do want to have a look at the coercing problem via MaxQueryDepthInstrumentation - I suspect we have case where it should not be running as is
9. bbakerman commented on Mar 16, 2022

   

   bbakerman

   on Mar 16, 2022

   MemberAuthor

   More actions

   I have replicated this problem in a test here: #2763

   I am yet unsure what we will do here... This is still in the investigation phase
10. dondonz commented on Mar 18, 2022

    

    dondonz

    on Mar 18, 2022

    Member

    More actions

    I'll fix up this exception so it's more useful

    throw new NonNullableValueCoercedAsNullException("", emptyList(), graphQLType);
    
11. dondonz commented on Jun 4, 2022

    

    dondonz

    on Jun 4, 2022

    Last edited by dondonz

    Member

    More actions

    Fixed by #2773
12. dondonz

    closed this as completedon Jun 4, 2022