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

Error's path on validation errors #166

Open
l3x4 opened this issue Apr 14, 2021 · 7 comments
Open

Error's path on validation errors #166

l3x4 opened this issue Apr 14, 2021 · 7 comments
Labels
enhancement New feature or request

Comments

@l3x4
Copy link

l3x4 commented Apr 14, 2021

Question

For validation errors it will be very helpful to specify the field where the validation failed (to highlight it on the form for example).

Currently the situation looks like this:

  1. The path contains just the mutation name, for example for the empty email it's like so: "path": ["userSignUp"]. Would be great to have "path": ["userSignUp", "email"]
  2. The attempt to build the error manually with rescue_from ActiveModel::ValidationError { ... } in the schema doesn't seem to be catching this.

What's the proper way to do it?

Thanks!

@mcelicalderon
Copy link
Member

Hey @l3x4! So yes, the error won't come in the path and I'm not sure if we want that level of detail for the arguments or if even if that is a part of the GQL (I'll check). What we do right now for ActiveRecord errors is send all validation errors in an array as part of the extension key. Take a look at this spec so you can see how the errors are returned in the response.

@l3x4
Copy link
Author

l3x4 commented Apr 14, 2021

@mcelicalderon Thanks for the explanation!

I did have a look at that spec, and tried it out myself on my setup too.

Yes, there's a message that the email is missing for example, but there's no signs even in the extensions what field is it.
It seems impossible to automatically tell the empty email error from mismatching passwords error.

If it was just some other mutation we could rescue from ActiveModel::ValidationError and analyse error.model.errors, building GraphQL::ExecutionError with the path containing the attribute and adding it to the context.

But using GraphqlDevise::SchemaPlugin this approach doesn't seem to work.

@mcelicalderon
Copy link
Member

Mmm, you mean something like this:

{
  : errors => [{
    : message => "Argument 'email' on Field 'userSignUp' has an invalid value (1). Expected type 'String!'.",
    : locations => [{
      : line => 2,
      : column => 11
    }],
    : path => ["mutation", "userSignUp", "email"],
    : extensions => {
      : code => "argumentLiteralsIncompatible",: typeName => "Field",: argumentName => "email"
    }
  }]
}

I passed the wrong type to the mutation just to try it, but I guess doing the same for invalid data would be possible. I see how right now the only way to display the errors is on a list, separate from the fields that originated the arguments.

I can think of an easy way to implement something that would help identify the arguments, detailed_errors could look like this:

{
  : data => {
    : userSignUp => nil
  },: errors => [{
    : message => "User couldn't be registered",
    : locations => [{
      : line => 2,
      : column => 11
    }],
    : path => ["userSignUp"],
    : extensions => {
      : code => "USER_ERROR",: detailed_errors => {
        : email => ["can't be blank"]
      }
    }
  }]
}

That would be simple to change, of course we would have to make it optional via an initializer, maybe eventually the default. These error keys come from active_record, so they might not match the arguments in the request, depending on the validations you used.

I will check if there's a way to customize the path on the returned errors, but even then I think it might be hard to match with the actual arguments of the mutation instead of the error keys from active record.

@mcelicalderon
Copy link
Member

Actually the GQL gem's docs say something about it, maybe this is the best approach https://graphql-ruby.org/mutations/mutation_errors

@l3x4
Copy link
Author

l3x4 commented Apr 15, 2021

Yep, that's this part in the doc, which adds the attributes to the path:

def resolve(id:, attributes:)
  post = Post.find(id)
  if post.update(attributes)
    {
      post: post,
      errors: [],
    }
  else
    # Convert Rails model errors into GraphQL-ready error hashes
    user_errors = post.errors.map do |attribute, message|
      # This is the GraphQL argument which corresponds to the validation error:
      path = ["attributes", attribute.to_s.camelize(:lower)]
      {
        path: path,
        message: message,
      }
    end
    {
      post: post,
      errors: user_errors,
    }
  end
end

And the result is:

{
  "data" => {
    "createPost" => {
      "post" => nil,
      "errors" => [
        { "message" => "Title can't be blank", "path" => ["attributes", "title"] },
        { "message" => "Body can't be blank", "path" => ["attributes", "body"] }
      ]
    }
  }
}

The path is not exactly as I mentioned above (["userSignUp", "email"]), but the variant from documentation works perfectly well, specifying which attribute had the error.

Also, I'd expect the errors to be on the same level as data, but maybe I'm missing some concepts here.

@mcelicalderon
Copy link
Member

Yes, the author of the gem recommends these kind of errors to be at the same level as data and not make them top level errors, but not sure if that's his personal preference or if that's actually somewhere in the GQL spec. We are still discussing with @00dav00 how to move forward on this one, but we'll definitively implement something that allows to identify the field of the error. Thank you for bringing this up.

@mcelicalderon mcelicalderon added the enhancement New feature or request label Apr 16, 2021
@mcelicalderon
Copy link
Member

Just as an update, we have decided to implement mutation level errors field, details still under discussion, but we will probably add a setting to disable top-level errors so you can actually query the new errors field.

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

No branches or pull requests

2 participants