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 422 validation error responses #6050

Closed
kevinansfield opened this issue Nov 5, 2015 · 7 comments
Closed

Improve 422 validation error responses #6050

kevinansfield opened this issue Nov 5, 2015 · 7 comments
Labels
affects:api Affects the Ghost API later [triage] Things we intend to work but are not immediate priority needs:info [triage] Blocked on missing information server / core Issues relating to the server or core of Ghost

Comments

@kevinansfield
Copy link
Contributor

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:

  • It's necessary to extract the invalid property name from the message string
  • The message is not suitable for display within in-line validations

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
Copy link
Member

ErisDS commented Dec 7, 2015

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?

@kirrg001 kirrg001 added the server / core Issues relating to the server or core of Ghost label May 12, 2017
@kirrg001 kirrg001 added the needs:info [triage] Blocked on missing information label Sep 8, 2017
@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2017

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 pushed a commit that referenced this issue Nov 9, 2017
refs #8143

Sets soft limits for certain db fields:

- `posts`:
	- `title`: 255 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
- `users`:
	- `bio`: 200 chars (current hard limit: 65,535 chars)
	- `location`: 150 chars (current hard limit: 65,535 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
- `tags`:
	- `description`: 500 chars (current hard limit: 65,535 chars)
	- `meta_title`: 300 chars (current hard limit: 2,000 chars)
	- `meta_description`: 500 chars (current hard limit: 2,000 chars)

- same error message for isLength validator as for hard limits (more improvements are comming with #6050)
- added more tests for importer
- added dynamic translation key handling for validators errors (isLength is only supported atm)
@kirrg001
Copy link
Contributor

kirrg001 commented Jan 2, 2018

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

@kirrg001 kirrg001 added later [triage] Things we intend to work but are not immediate priority and removed i18n labels Jan 2, 2018
@kevinansfield
Copy link
Contributor Author

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
Copy link
Contributor

kirrg001 commented Jan 2, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API later [triage] Things we intend to work but are not immediate priority needs:info [triage] Blocked on missing information server / core Issues relating to the server or core of Ghost
Projects
None yet
Development

No branches or pull requests

3 participants