Skip to content

Provide nicer validation errors? #160

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

Closed
eapache opened this issue Jun 3, 2016 · 6 comments
Closed

Provide nicer validation errors? #160

eapache opened this issue Jun 3, 2016 · 6 comments

Comments

@eapache
Copy link
Contributor

eapache commented Jun 3, 2016

@rmosolgo cc @dylanahsmith @lreeves

Right now if I run a mutation with invalid input (e.g. from a custom scalar definition):

mutation {
  doTheThing(input: { myField: "someInvalidValue" }) {
    myField
  }
}

I'll get back:

{
  "errors": [
    {
      "message": "Argument 'input' on Field 'doTheThing' has an invalid value",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
    },
    {
      "message": "Argument 'myField' on InputObject 'DoTheThingInput' has an invalid value",
      "locations": [
        {
          "line": 2,
          "column": 21
        }
      ]
    }
  ]
}

There are two things that would be nice here:

  • The first error complaining about input is, as far as I can tell, entirely redundant and occurs because the input for myField is invalid. It would be good if this was not returned since we already have a more precise error for myField.
  • If the error for myField also returned fields: [String!] in the form ["doTheThing", "input", "myField"] for callers to be able to easily (and programatically) determine which field was malformed.

I'm willing to work on this but a quick dive into argument_literals_are_compatible.rb didn't make either change look easy so I thought I'd ask for opinions and maybe hints first.

@rmosolgo
Copy link
Owner

rmosolgo commented Jun 4, 2016

I agree with your suspicion that the first error is "cascading"!

Agreed, it would be nice to include a human-readable path to the validation error instead of just the line & col.

Feel free to take a crack at it if you're so inclined, but I think StaticValidation is ready for an overhaul. If you can survive with it for another month or so, I'll consider this a "feature request" at that time! Otherwise, if you add a spec for the desired behavior, I'll be sure not to break it when I get around to it.

@rmosolgo
Copy link
Owner

rmosolgo commented Aug 4, 2016

Adding path to validation errors in #198

@rmosolgo
Copy link
Owner

working on a rewrite now ...

@gjtorikian
Copy link
Contributor

working on a rewrite now ...

Just jumping in, did this happen in a branch somewhere, @rmosolgo ?

@rmosolgo
Copy link
Owner

Yes, #268 , but I gave up. I took some parts from there into master, including the part that made infinite loops 😆

When I got to the part about making up new errors, I realized it was going to be impossible to preserve backwards compatibility.

In retrospect, I think the right thing to've done would've been to target GraphqQL-CATS error messages.

@rmosolgo
Copy link
Owner

rmosolgo commented Mar 3, 2020

This specific error is a bit better now:

image

  • It no longer makes two errors
  • It includes the problematic value and expected type
  • It includes the path

I don't have short-term plans to work more on validation errors, so if there are other specific validation errors to improve, please open a new issue!

@rmosolgo rmosolgo closed this as completed Mar 3, 2020
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

No branches or pull requests

3 participants