Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages for invalid input types #3133

Closed

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Sep 4, 2020

Error messages when recursively validating a field argument's type don't actually include the input type's name which makes them a bit confusing. Consider the following invalid schema:

type MyType {
  myField(myArg: MyInput)
}

input MyInput {
  # This input field is invalid because it has an output type
  someInput: MyOtherType
}

type MyOtherType {
  foo: String
}

This generates the following error message that doesn't mention the MyInput type:

Argument myArg on MyType.myField is invalid: argument "someInput" type must be a valid input type (Scalar or InputObject), not GraphQL::ObjectType (MyOtherType)

With the proposed changes the error message becomes:

MyInput is invalid: argument "someInput" type must be a valid input type (Scalar or InputObject), not GraphQL::ObjectType (MyOtherType)

@rmosolgo
Copy link
Owner

rmosolgo commented Sep 7, 2020

👍 This change is good by me, I just want to point out that it won't apply to class-based types using the interpreter (which will someday be the only way to run graphql-ruby). I don't think it uses traversal.rb in that case. I recently added #3120 for a similar case, but maybe you're still using .define?

@jturkel
Copy link
Contributor Author

jturkel commented Sep 8, 2020

Doh. I first noticed the problem when adding validation that deprecated arguments aren't required in #3015. The tests in question use the new class based API but I guess many of them call GraphQL::Schema.graphql_definition which triggers the legacy validation. I can't wait till the legacy APIs are removed in 2.0 😄

I'm going to close this PR since we're not using the legacy API. I'll open up a new PR that properly handles validation of required deprecated arguments in the class based API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants