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

Difficult to debug request validation errors #49

Closed
davisagli opened this issue Nov 2, 2019 · 10 comments
Closed

Difficult to debug request validation errors #49

davisagli opened this issue Nov 2, 2019 · 10 comments

Comments

@davisagli
Copy link
Member

I'm excited to use this package, but am running into some trouble debugging a test and wonder if I'm missing something.

If I add a breakpoint in pyramid_openapi3.openapi_view.wrapper_view, I can see that my test is failing because I forgot to include one of the required properties in a webtest request to my POST endpoint:

(Pdb) request.openapi_validated.errors
[InvalidMediaTypeValue(original_exception=MissingSchemaProperty(property_name='slug'))]

However, when I run the test without the breakpoint, I get an error from webtest like this, which doesn't tell me what the actual problem was:

E           webtest.app.AppError: Bad response: 500 Internal Server Error (not 200)
E           500 Internal Server Error
E           
E           Response validation failed.
E           
E           
E           Unknown response http status: 400

It looks like pyramid_openapi3's response validation tween is kicking in since the 400 response generated from the request validator's exception does not match the specified status code for the endpoint -- which makes sense in a way, but I'd like to see that original 400 response, at least when testing.

I can figure something out but is there an established pattern or example for how to get more helpful output in this situation?

@davisagli
Copy link
Member Author

@zupo any suggestions?

@zupo
Copy link
Collaborator

zupo commented Nov 5, 2019

I'm still figuring out this one myself too. Note that the problem is mostly with response validation, not request validation. Request validation should result in readable output in webtests.

One idea I had is to use a config flag that disables response validation, so that I can see errors in response body. But then again, this would mean having extra code just for in-development support of which I'm not a huge fan.

The other thing I am doing is @exception_view_config(https://github.com/Pylons/pyramid_openapi3/issues/49) and logging all errors before rendering the 500 Internal Server Error so that I see the error in logs. This one is not great because you have to scroll up over the WebTest error to get to the underlying traceback.

Not entirely decided which one should be the recommended solution, eager to hear your thoughts.

@gweis
Copy link
Contributor

gweis commented Nov 15, 2019

The intention was to make the flag here https://github.com/Pylons/pyramid_openapi3/blob/master/pyramid_openapi3/__init__.py#L50 somehow configurable. E.g. something along the lines of:

When registering an openapi view you pass the openapi=True. This parameter could also take a tuple (True, False) or whatever to turn of response validation. (request.environ["pyramid_openapi3.validate_response"] = False).

I intended to create a pull request to that feature (didn't get around to do so yet), but maybe something else would be more helpful ?

@davisagli
Copy link
Member Author

Django is handling this with a flag on the test client: https://docs.djangoproject.com/en/3.0/releases/3.0/#tests (see first bullet)

@MatthewWilkes
Copy link
Contributor

I've been thinking about this issue a bit today, and I'm struggling a bit.

AIUI, the problem was that @davisagli had an endpoint that expected certain parameters and was defined to not return a 400 response code. When the parameters are missing, the openapi_view view deriver creates a 400 response with the validation error. That 400 response is then rejected by the excview_tween tween, which generates a 500 error.

David's issue is that he'd like a useful error message when he calls his view with incorrect parameters, as that would help with testing.

The problem seems to be that by defining an endpoint that doesn't allow 400 as a valid return code, we are all but guaranteeing that there will be no validation issues (the openapi3 spec says that documentation "is expected to cover successful responses and any known errors. By “known errors” we mean, for example, a 404 Not Found response for an operation that returns a resource by ID, or a 400 Bad Request response in case of invalid operation parameters").

I'm struggling to think of a use-case where we'd want users to have no access to errors and to be able to write assertions in tests against them, so I feel like logging should be sufficient. In particular, the raise_request_exception option in Django is quite different to this problem: that provides a way of tunnelling the final exception out of a request to re-raise it, but we are considering disabling an exception handler to get better information for debugging.

It seems to be that the following two actions would be good additions, but I'd be keen to hear opinions:

  1. The excview_tween should log validation errors that were discarded during validation.
  2. When configuring an API spec, we should consider checking if there are API endpoints that could raise validation errors that do not specify 400 as a valid response code, and raise a warning

@zupo
Copy link
Collaborator

zupo commented May 14, 2020

  1. The excview_tween should log validation errors that were discarded during validation.

Yep, sounds a good first step, non-invasive and potentially sufficient.

When configuring an API spec, we should consider checking if there are API endpoints that could raise validation errors that do not specify 400 as a valid response code, and raise a warning

#22 is supposed to do exactly this.

@MatthewWilkes
Copy link
Contributor

Nice, yeah, #22 looks good. Some documentation would bridge the gap so people know that's best practice. Works for me.

@mmerickel
Copy link
Member

I don't know if it helps but via #68 the response validation exception does contain the original response that triggered the error, so maybe you can do something with that.

@zupo
Copy link
Collaborator

zupo commented Jun 21, 2020

#68 and #80 help with minimizing the problem, and we have further work planned in #22, so I'm closing this one.

Please re-open if anyone feels this issue needs further attention.

@zupo zupo closed this as completed Jun 21, 2020
@davisagli
Copy link
Member Author

Thanks everyone!

zupo added a commit that referenced this issue Dec 12, 2020
A common pitfall when using this package is the following: you define
an endpoint that can result in 400 Bad Request, but you forget to list 400
in the `responses` section of your endpoint in openapi.yaml. This package
then instead returns 500 Internal Server error, because it keeps the promise
that only defined responses will be allowed (unless you set
`enable_request_validation` to `False`, that is).

With this commit, all endpoints, by default need to have 200, 400 and 500
on the list of `responses` in openapi.yaml, otherwise the app won't start. Additionally, all
endpoints that accept a parameter, also need to have 404 on the list of
`responses`.

You can skip this check by setting `enable_endpoint_validation` to `False`.

Refs #22
Refs #49 (comment)
Refs #36
zupo added a commit that referenced this issue Dec 12, 2020
A common pitfall when using this package is the following: you define
an endpoint that can result in 400 Bad Request, but you forget to list 400
in the `responses` section of your endpoint in openapi.yaml. This package
then instead returns 500 Internal Server error, because it keeps the promise
that only defined responses will be allowed (unless you set
`enable_request_validation` to `False`, that is).

With this commit, all endpoints, by default need to have 200, 400 and 500
on the list of `responses` in openapi.yaml, otherwise the app won't start. Additionally, all
endpoints that accept a parameter, also need to have 404 on the list of
`responses`.

You can skip this check by setting `enable_endpoint_validation` to `False`.

Refs #22
Refs #49 (comment)
Refs #36
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

5 participants