Custom scalar CoercionError clobbers parent messages

[dnd]

When a non-top level field has a custom scalar type that raises a coercion error. The coercion error message overwrites the parent's error message.

This gist shows the difference between a coercion returning nil, raising a CoercionError, and what the expected output would actually be.

https://gist.github.com/dnd/c9d6cbf18880f8fd916f962a7e10ca9e

This is from a block of code that looks like this:

class SigninUser < GraphQL::Function
  CredentialsInputObject = GraphQL::InputObjectType.define do
    name 'CredentialsInputObject'

    argument :num, Scalars::Date
  end

  argument :credentials, !CredentialsInputObject
 
  ...
end

Please ignore the non-sensical use of the name num. :)

It would also be nice if instead of only displaying the coercion error message, that the error message be appended to the default string describing which field actually failed. Because as far as I can tell there is no way to determine inside of the coercion what the specific field, or parent is such as num, or InputObject 'CredentialsInputObject'.

"Argument 'num' on InputObject 'CredentialsInputObject' has an invalid value. #{coercion_error.message}"

[eapache]

@swalkinshaw

[swalkinshaw]

I'm not really sure what the solution is here. valid_literal? gets called for the input object and results in the nested values being validated as well causing the exception to be raised and rescued in the context of the input object.

@rmosolgo any ideas here? I can't immediately see anything that would be easy 😔

[eapache]

Put a rescue at the bottom of LiteralValidator#validate where you pull out the error for the specific field and then reraise a generic coercion error for the parent?

Actually, I just remembered #160 which I filed a long time ago - do we even want an error on the parent field in this case? It's kind of redundant.

[swalkinshaw]

do we even want an error on the parent field in this case? It's kind of redundant.

I kind of agree. It's very redundant for these errors to bubble up to every parent input object.

[dnd]

I kind of think +1 to just having the error only for the field in question and no bubble up. The error message includes the full path array to the error. So if you wanted to just scan for a general failure of the parent object you could do so.

I think the only considerations would be that it's a potentially breaking change from current functionality, as well as is there ever a case where the parent would/could have a different and more specific message.

[rmosolgo]

Yeah, 👍 from me too to stop bubbling up 😬 For reference, here's the GraphQL-js behavior:

https://launchpad.graphql.com/w43v7938z

[modosc]

was there any movement on this? i'd be happy to help, i'm just not sure where to start (i assume this should be a configurable option to keep backwards compat)

[modosc]

how about changing LiteralValidator#validate to raise when a null, scalar, or enum isn't valid? then inside of ArgumentLiteralsAreCompatible and VariableDefaultValuesAreCorrectlyTyped we could rescue for that exception just like we already do for GraphQL::CoercionError except that we'd only append to context.errors if we're in a rescue block (instead of whenever valid is not true). seems like this would prevent the bubbling up and i think it should be possible to put this behavior behind a flag/setting so that there'e no backward compat issues.

edit: i just tried this approach and it seems to solve the bubbling issues we are seeing:

https://github.com/modosc/graphql-ruby/commit/ead5ef8804ac062957d2b085d03bf330071b2ad2

if this direction seems ok i'll finish it and do a pr.

[modosc]

ok, just updating with some notes/findings.

i had assumed that validation worked it's way from the bottom up, but it's actually the opposite. so consider {a: { b: { c: { d: 1 } } } } where d is invalid. we travers to a and ArgumentLiteralsAreCompatible validates a. this calls LiteralValidator first, which then recursively validates b, c, and d. at this point a is marked as invalid and a message is added to context.errors. now we traverse down, get to b, and ArgumentLiteralsAreCompatible is run on b (again), which recurses down to c and d (again) and finds d invalid. so now b is also marked as invalid, another message is added to context.errors, and we continue.

this is also why the error messages appear at the top level InputObject first, then continue to get more specific until they react the actual error object.

ideally we could cache the validations that are being done - there's no real reason to do these more than once, right?

anyway, this also made me change my approach to "solving" this problem. now i'm thinking that raised exceptions should include the ast_node that the error was found on. then inside of ArgumentLiteralsAreCompatible we can compare that node to the actual node we're attempting to validate. if the nodes match, then yes, this is an error that should be added to context.errors. if not, we still return false but we don't log the error (and here's where the switch would be to enable the current behavior, it would be much easier to do it here).

i'm currently working on something else right now but i expect to pick this up again next week. i'll also file a separate issue about caching the validations.