TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
47.42k stars 10.35k forks source link

Improve 422 validation error responses #6050

Closed kevinansfield closed 6 years ago

kevinansfield commented 9 years ago

Our current validation error format (not sure if this is consistent across the app?) looks like this:

{
    "errors": [
        {
            "message": "Value in [posts.title] exceeds maximum length of 150 characters.",
            "errorType": "ValidationError"
        },
        {
            "message": "Value in [posts.meta_title] exceeds maximum length of 150 characters.",
            "errorType": "ValidationError"
        }
    ]
}

This has a couple of issues when attempting to consume on the client side:

Due to the above issues we don't currently have anything in our Ember Data adapters to take the error response and normalize it so that the errors are automatically picked up by Ember Data/DS.Errors and made available on the relevant model object.

There are two formats that are widely used with Ember Data and have built-in support:

ActiveModelSerializer format:

{
    "errors": {
        "title": ["Title cannot be longer than 150 characters."],
        "message": ["Meta Title cannot be longer than 150 characters."]
    }
}

JSON API format:

{
    "errors": [
        {
            "source": { "pointer": "/data/attributes/title" },
            "title":  "Value is too long",
            "detail": "Title cannot be longer than 150 characters."
        },
        {
            "source": { "pointer": "/data/attributes/meta_title" },
            "title": "Value is too long",
            "detail": "Meta Title cannot be longer than 150 characters."
        }
    ]
}

The JSON API format also allows for additional fields such as code for application-specific error codes or the meta field that can contain completely custom data.

i18n: If we are going to update our validation response format would it also be a good time to make accommodations for i18n support? To do so it would be best to provide a "key" for the localisation lookup, e.g. a generic validation.too_long or a more specific post.errors.title.too_long.

Perhaps the meta field would be best for that? That would allow us to both provide a localisation key and any other details that can be used for interpolation. For example:

"meta": {
    "i18n": {
        "key": "validation.too_long",
        "max_chars": 150
    }
}

Could map nicely to the translation key:

"validation.too_long": "cannot be longer than {{max_chars}} characters"


I'd like to get some discussion started on which direction we'd like move towards for two main reasons:

  1. Even though we've mostly eliminated hitting the server with invalid data from the admin client there are still some edge cases where automatic mapping would be useful.
  2. More importantly when we get to opening write-access through the public API and its out in the wild and in use it will be a lot more difficult to change.
ErisDS commented 8 years ago

I think we need to use something along the lines of the JSONAPI format. That's sort of where we were going but the spec didn't exist when we created what we have.

Validation errors are kind of a special case, and they do warrant some extra love.

Kind of neat that JSONAPI spec'd both 'parameter' and 'pointer' - not sure how easy implementing pointer will be, but we can definitely at least provide a 'property' name as the field a validation error belongs to not being returned is nothing more than an oversight in the formatHTTPErrors function: https://github.com/TryGhost/Ghost/blob/master/core/server/errors/index.js#L195.

I also very much want to start returning a custom error code with all of our errors. Need to come up with some sort of plan for what those look like. Any recommendations or resources for this would be great. I guess we should make use of our existing 'errorType' and perhaps do something like <errorType>-<endpoint>-<property/parameter>?-<code>

E.g. ValidationError-Posts-Title-TooLong

Harder to come up with something sane for general permissions errors etc:

E.g. NoPermissionError-Posts-C7B34DE4

As for i18n, I am not sure if we need to think about that now. JSONAPI allows the title itself to be translated, I think we can leave it for now and allow those working on that project to determine which strategy they prefer?

ErisDS commented 7 years ago

In terms of i18n, I think this issue can be closed in the same way as #5345 until we're ready to tackle #6525.

@kevinansfield is there any other part of this that is still needed or would be useful to split out into a separate issue? If not please close this 😁

kirrg001 commented 6 years ago

until we're ready to tackle #6525.

This reference seems to be wrong.


This issue contains two features/changes.

  1. Proper JSON API format for errors.
  2. i18n key improvements.

As for i18n, I am not sure if we need to think about that now. JSONAPI allows the title itself to be translated, I think we can leave it for now and allow those working on that project to determine which strategy they prefer?

I agree to that.

@kevinansfield is there any other part of this that is still needed or would be useful to split out into a separate issue? If not please close this 😁

Agree. I would like to suggest to rename the issue title to e.g. "JSON API format errors", remove the i18n suggestion and close with the later label.

cc @kevinansfield

kevinansfield commented 6 years ago

I would like to suggest to rename the issue title to e.g. "JSON API format errors" and close with the later label.

That's fine, the i18n note here was more of an aside and not meant to be the main focus of discussion.

I think this issue is still relevant, we've mostly worked around it in Ghost-Admin by detecting certain words in the error message to display the error in the right place on a per-error basis (not particularly future-proof) or by showing a generic error/alert (poorer UX). As we implement larger features that require forms with more validation this may become more relevant as it will be harder to work around without compromises.

kirrg001 commented 6 years ago

we've mostly worked around it in Ghost-Admin by detecting certain words in the error message to display the error

Yeah understood, but even with pointers/codes the admin client would need to differentiate the validation errors? Could you show an example of code reduction/optimisation if we return pointers or codes?

The issue is definitely relevant, but not sure about the priority and when we have time to tackle this πŸ™ˆ We could e.g. reference this issue in https://github.com/TryGhost/Team/issues/42 - just a suggestion.

kevinansfield commented 6 years ago

but even with pointers/codes the admin client would need to differentiate the validation errors

That's already built in to Ember Data, the errors would appear on the model.errors array for the right field automatically. At the moment we detect server-side validation errors manually and push errors on to the model in some cases or the error isn't picked up and ends up in a generic error alert.

kevinansfield commented 6 years ago

In any case, this can be closed with the later tag ready for when it becomes a more pressing problem.