introduce RequestValidationError and ResponseValidationError exceptions #26
Conversation
remove add_validation_error_view config directive which can be replaced by standard pyramid custom exception views re-raise any exceptions thrown in view (to keep view semantics of returning vs throwing HTTPExceptions) updated tests
|
Seems like I overlooked that circle-ci is running the examples. I'll update |
There was a problem hiding this comment.
The initial quick look: I like the PR a lot. But please give me a few days to test it out in our big-ish private codebase and also on https://github.com/niteoweb/pyramid-realworld-example-app, to make sure all edge cases are covered.
…nge / render a response like pyramid's default exception view have a chance to do their work and the final response then get's validated split out code base into smaller modules (tween, exceptions, etc..) fix example applications
|
I have updated the pull request and changed the behaviour a fair bit. I have moved the response validation code into a tween, so that other tweens like pyramid_exception_tween to handle the exception and generate a response before response validation kicks in. There are probably still a few rough edges, but now it covers pretty much all my use cases. |
|
Great, I'll have a look!
This is fine, I'm planning to provide a way to enforce common responses soon-ish. |
|
No changes required on pyramid-realworld-example-app: https://travis-ci.com/niteoweb/pyramid-realworld-example-app/builds/115003852 |
That's great. The only changes that may be needed are replacing In a future pull/feature request, I'd like to make the request/response validation optional. E.g.: by configuring the view with a |
As long as |
|
Still need to test this against our private codebase that is currently the biggest user of pyramid_openapi3. |
Yes, that is the intention. No backwards incompatible changes necessary. (and barely a performance hit, as this would be parsed on startup). |
|
I've gone over the changes here and it looks really good to me. My only concern was a minor readability item in the tests. |
| def __init__(self, *args, errors, **kwargs) -> None: | ||
| super().__init__(*args, **kwargs) | ||
| self.errors = errors | ||
| self.detail = self.message = "\n".join(str(e) for e in errors) |
There was a problem hiding this comment.
You mean the exception view handler? The pyramid default exception view handler does not handle exceptions with an error attribute (or any other custom attribute for that matter). Are you suggesting to add a default exception handler to this library?
There was a problem hiding this comment.
No, the exact line self.detail = self.message = "\n".join(str(e) for e in errors) you are creating object in memory that are not even needed yet.
There was a problem hiding this comment.
I was trying to keep the pyramid httpexception contract up here with some useful information from validation errors, and not re-implementing all the templating that's implemented in pyramid http exception/response.
Because the validation exceptions are subclasses of pyramids HTTPException class, the validation details can't be put into the message or detail easily. They are instance fields set in the base class __init__ method. The only way I can see to delay the generation of an error message is by turning the comment field into @property ro even @refiy descriptor. Happy to change that, but I think detail / message are more appropriate fields for that.
| # If there is no exception view, we also see request validation errors here | ||
| except ResponseValidationError: | ||
| exc_info = sys.exc_info() | ||
| try: |
There was a problem hiding this comment.
Not sure whether it is good practice to re-use private functions?
There was a problem hiding this comment.
I've run it on an internal project and it seems it breaks badly because exception handling is reimplemented. Will have more info in a couple of days. It's specific to us because we enforce response spec (with some additional info).
The second one is debug_toolbar:
Traceback (most recent call last):
File ".venv/lib/python3.7/site-packages/pyramid_debugtoolbar/toolbar.py", line 258, in toolbar_tween
toolbar.status_int = response.status_int
AttributeError: 'ResponseValidationResult' object has no attribute 'status_int'
There was a problem hiding this comment.
Maybe there is somewhere a custom exception view handler that returns the ResponseValidationResult instead of a Response / HTTPException instance? Otherwise I can't see why there would be a ResponseValidationResult returned by the debug_toolbar tween handler.
|
@gweis: quick update: got the private project's build green, now need to get it through review and pushed to production. |
| def excview_tween(request) -> Response: | ||
| try: | ||
| response = handler(request) | ||
| if not request.environ.get("pyramid_openapi3.validate_response"): |
There was a problem hiding this comment.
pyramid_openapi3.validate_response infers that only response will not be validated, but the code actually ignores the request too.
There was a problem hiding this comment.
That's a bit of a source for confusion here... the Request is validated in the view wrapper, but the response is validated after the exception view handler ran (if there is any)
|
@gweis Can you explain a bit what were your motivations for re-implementing error handler and not having just standard tween? I don't understand why is this better? |
|
@dzony The original problem was, that within pyramid apps where not every view is an openapi3 view, You need some way of deciding when to validate the request. That's done in this pkg via the view wrapper. There is also some discussion in #15 |
| if result.errors: | ||
| raise ResponseValidationError(errors=result.errors) | ||
| # If there is no exception view, we also see request validation errors here | ||
| except ResponseValidationError: |
There was a problem hiding this comment.
I think a test is missing here for if not request.environ.get('pyramid_openapi3.validate_response') incase the handler raises one of these errors for some reason on a view that's not protected by the spec.
There was a problem hiding this comment.
Not quite sure whether I follow here, but I have added another test. 48b8049
Otherwise I think a non openapi view should not raise a ResponseValidationError. Maybe we should think about that in case there is a real use case for it, otherwise I'd consider throwing random ResponseValidationError instances in user code as a bug.
| try: | ||
| response = request.invoke_exception_view(exc_info) | ||
| except HTTPNotFound: | ||
| reraise(*exc_info) |
There was a problem hiding this comment.
a finally is missing here to del exc_info to avoid cycles on exc_info stack frames referencing the current stack frame... I will say I'm sensitive to this because of python 2 and I'm not positive it's an issue in python 3 - but I wanted to bring it up to make sure someone double checks :-)
There was a problem hiding this comment.
I don't think a del exc_info is needed here, but I am not really sure, so I have added it anyway (can't hurt ).
make sure we don't keep exc_info around
enable python 3.6 support via pipenv
|
Had to update version pins for Pipfile, otherwise it would not build on travis. |
|
@gweis: I finally found enough time to give this PR another rundown and test it in our private codebase. It seems to work exactly as advertised , so I merged it. I'm terribly sorry it took me this long! Thanks a million for your contribution, the package is so much better off with this change! |
|
I've cut a new release that includes this PR: https://pypi.org/project/pyramid-openapi3/0.4.0/ |
remove add_validation_error_view config directive which can be replaced by standard pyramid custom exception views
re-raise any exceptions thrown in view (to keep view semantics of returning vs throwing HTTPExceptions)
updated tests