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

Consider changing error status to 422 #132

Open
juliasilge opened this issue Aug 12, 2022 · 3 comments
Open

Consider changing error status to 422 #132

juliasilge opened this issue Aug 12, 2022 · 3 comments
Labels
feature a feature request or enhancement

Comments

@juliasilge
Copy link
Member

We could change the error that folks see on predicting with bad data from 400 to 422. This would involve doing something like this within vetiver_pr_post():

pr <- plumber::pr_set_error(pr, handler_error_422)

where handler_error_422() would be something like:

handler_error_422 <- function(req, res, err) {
    res$status <- 422
    list(error = "422 - Unprocessable Entity", message = err$message)
}

One issue that I haven't resolved yet is that I think as I have this laid out ⬆️ this will change all errors to 422, even ones that aren't predicting with valid but not endpoint-appropriate JSON. I would need to add more careful checking somehow.

This is motivated by FastAPI returning 422 in the situation where valid JSON is posted but it doesn't match the Pydantic model. @isabelizimm have I stated this correctly? Is this still the case? It do think it is a good idea to harmonize the error status codes between R and Python.

In reading the spec for the 422 code and various discussions, I'm not 100% sure that switching from 400 to 422 is best or right just based on what we're doing. Maybe we can get more feedback 400 vs. 422 for the context of model predictions?

@juliasilge juliasilge added the feature a feature request or enhancement label Aug 12, 2022
@isabelizimm
Copy link

That is correct! 422 is what is used when the data POSTed does not match the Pydantic model. From those discussions, it looks like 422 or 400 are both not incorrect, but I agree we should align the errors if possible (without forcing all errors to be 422/400!). With 422, I think we want to be careful that it's clear the unprocessable entity is at the model level, not necessarily that the API cannot physically process it.

@juliasilge
Copy link
Member Author

Here's a nice outline of how to approach this from @atheriel:
https://unconj.ca/blog/structured-errors-in-plumber-apis.html

@atheriel
Copy link

You may also be interested in the package I wrote that pushes those ideas further: httpproblems.

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

No branches or pull requests

3 participants