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

openapi_validated side effects #165

Open
grinspins opened this issue May 17, 2022 · 4 comments
Open

openapi_validated side effects #165

grinspins opened this issue May 17, 2022 · 4 comments

Comments

@grinspins
Copy link

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.

  1. 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
Copy link
Collaborator

zupo commented May 18, 2022

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

grinspins commented May 18, 2022

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

zupo commented May 18, 2022

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 added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Refs #165
@zupo
Copy link
Collaborator

zupo commented May 27, 2022

Hey @grinspins, the issue 1. described above should be resolved with this PR: #172

Could you take a look please?

zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Also a fix to make response validation still work even if request validation
is turned off.

Refs #165
zupo added a commit that referenced this issue May 27, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Also a fix to make response validation still work even if request validation
is turned off.

Finally, test_request_validation_disabled was a moot test: it would pass even
if validation was enabled, because the request was valid. Made sure both this
test and test_response_validation_disabled first verify the request/response
is invalid, then disable validation and test again.

Refs #165
zupo added a commit that referenced this issue May 30, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Also a fix to make response validation still work even if request validation
is turned off.

Finally, test_request_validation_disabled was a moot test: it would pass even
if validation was enabled, because the request was valid. Made sure both this
test and test_response_validation_disabled first verify the request/response
is invalid, then disable validation and test again.

Refs #165
zupo added a commit that referenced this issue Nov 28, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Also a fix to make response validation still work even if request validation
is turned off.

Finally, test_request_validation_disabled was a moot test: it would pass even
if validation was enabled, because the request was valid. Made sure both this
test and test_response_validation_disabled first verify the request/response
is invalid, then disable validation and test again.

Refs #165
zupo added a commit that referenced this issue Nov 28, 2022
Accessing `request.openapi_validated` in a route that has
`view_config(openapi=False)` will no longer break the route.

Also a fix to make response validation still work even if request validation
is turned off.

Finally, test_request_validation_disabled was a moot test: it would pass even
if validation was enabled, because the request was valid. Made sure both this
test and test_response_validation_disabled first verify the request/response
is invalid, then disable validation and test again.

Refs #165
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

2 participants