Pylons / pyramid_openapi3

Pyramid addon for OpenAPI3 validation of requests and responses.
https://pypi.org/project/pyramid-openapi3/
MIT License
83 stars 46 forks source link

openapi_validated side effects #165

Open grinspins opened 2 years ago

grinspins commented 2 years ago

Factoring openapi_validated into a request method going from 0.11 to 0.12 comes with a few issues. I was wondering whether you would consider going back to the old implementation that set openapi_validated as an attribute on the request and accessing request.openapi_validated did not have side effects.
There are two problems that come to mind with the current implementation:

  1. Accessing request.openapi_validated in a route that has view_config(openapi=False) will break the route.

    @view_config(route_name="my_route", openapi=False)
    def my_route(request):
    request.openapi_validated
    return HTTPOk()

    request.openapi_validated is now always available on every request regardless of the openapi route setting. Accessing it will set the pyramid_openapi3.validate_response environ setting, breaking the route because it will attempt to validate the response, causing confusing errors. Of course there is no good reason to access request.openapi_validated directly in a view that has openapi=False set, but I can think of reasons why someone would want to access it upstream of the individual view callable in code that is shared across several views.

  2. Setting pyramid_openapi3.enable_request_validation to False will also deactivate response validation.

    
    def configure(config):
    config.registry.settings["pyramid_openapi3.enable_request_validation"] = False

@view_config(route_name="my_route", openapi=True) def my_route(request): return make_complicated_response()


In this example `request.openapi_validated` is not accessed in the view deriver because it bails out after seeing that request validation is disabled. Since it's also not explicitly accessed inside the view `pyramid_openapi3.validate_response` is never set in the request environ and response validation is skipped.  

I'm not too familiar with the reasoning behind moving to the new implementation (other than that it's certainly cleaner) and I may be missing problems that the old implementation had. 
zupo commented 2 years ago

Hey @grinspins, thanks for taking the time and providing a proper context.

Regarding 1.: to me this seems like a bug and we should raise a more descriptive error when people try to access request.openapi_validated on a view that has view_config(openapi=False).

But that would make your proposed use case even harder.

I can think of reasons why someone would want to access it upstream of the individual view callable in code that is shared across several views.

Could you please share some of the reasons to help me understand this use case better?

Regarding 2.: Sounds like a bug too. I'll look into it.

grinspins commented 2 years ago

Thanks for your reply. For the first issue an example I can think of is request parameter unpacking in APIs that have both openapi and non-openapi routes. E.g. something like the following would have worked until 0.11 and break starting in 0.12.
I didn't frame "upstream of the individual view callable" quite correctly in my initial statement. I suppose it's rather common code that's shared between openapi and non-openapi routes.

class Resource:
    def __init__(self, request):
        self.request = request

    def __call__(self):
        try:
            url_params = self.request.openapi_validated.parameters['path']
        except AttributeError:
            url_params = self.request.matchdict
        if self.request.request_method in ("GET", "DELETE"):
            try:
                params = self.request.openapi_validated.parameters['query']
            except AttributeError:
                params = self.request.params
        else:
            try:
                params = self.request.openapi_validated.body
            except AttributeError:
                params = self.request.json_body
       view_callable = getattr(self, self.request.request_method.lower())
       return view_callable(url_params=url_params, params=params)

@view_config(route_name="my_route", openapi=True)
class MyResource(Resource):
    def get(self, url_params, params):
        ...

    def post(self, url_params, params):
        ...

@view_config(route_name="my_other_route", openapi=False)
class MyOtherResource(Resource):
    def get(self, url_params, params):
        ...

    def post(self, url_params, params):
        ...
zupo commented 2 years ago

Ah, OK, this would be sufficiently covered by what I had in mind: raise a more descriptive error when accessing request.openapi_validated. AttributeError seems an appropriate error.

So, yes, good, we can do this!

Thanks for a taking the time to write an example with actual code!

zupo commented 2 years ago

Hey @grinspins, the issue 1. described above should be resolved with this PR: https://github.com/Pylons/pyramid_openapi3/pull/172

Could you take a look please?