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

Extract JSON request body validation to middleware #1588

Merged
merged 6 commits into from
Sep 18, 2022

Conversation

RobbeSneyders
Copy link
Member

First step towards #1525

This PR extracts the request body validation for json payloads into middleware. The body validator is pluggable by content type and implements the ASGI interface. This way the stream can be consumed and replayed as needed for each content type.

A couple of notes / high level ideas

  • We should transform the existing Operation classes into OperationSpec classes which provides an interface for an operation specification across Swagger 2 & OpenAPI 3. I don't think we should subclass the middleware operation classes for Swagger 2 & OpenAPI separately.
  • I think middleware operation classes should implement the ASGI interface and wrap the next app. This is already the case in the routing middleware, but not the security middleware. I'll fix this in a separate PR.
  • We have a lot of boilerplate across the middlewares. We should check if we can abstract this into some base classes.

@RobbeSneyders RobbeSneyders added this to the Connexion 3.0 milestone Sep 13, 2022
@RobbeSneyders RobbeSneyders force-pushed the feature/validation-middleware branch from 75c7aac to b1c1205 Compare September 13, 2022 21:02
@RobbeSneyders RobbeSneyders force-pushed the feature/validation-middleware branch from b1c1205 to cc29db5 Compare September 13, 2022 21:22
@coveralls
Copy link

coveralls commented Sep 13, 2022

Pull Request Test Coverage Report for Build 3063498594

  • 165 of 172 (95.93%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 94.765%

Changes Missing Coverage Covered Lines Changed/Added Lines %
connexion/validators.py 48 49 97.96%
connexion/middleware/validation.py 112 118 94.92%
Files with Coverage Reduction New Missed Lines %
connexion/decorators/validation.py 1 96.41%
Totals Coverage Status
Change from base Build 3046926983: 0.03%
Covered Lines: 2860
Relevant Lines: 3018

💛 - Coveralls

tests/fixtures/simple/openapi.yaml Show resolved Hide resolved

VALIDATOR_MAP = {
"parameter": ParameterValidator,
"body": {"application/json": JSONBodyValidator},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been struggling here with the distinction of parameter and body as there currently isn't a clear split: form parameters are validated by the ParameterValidator but also by the RequestBodyValidator. The request body also contains parameters, at least in Swagger 2 (link). In OpenAPI there is a clearer distinction between "parameters" and the "body" (link).

Seeing this, I think a way forward could be to move everything of formdata to a separate body validator as well. This also simplifies things wrt accessing the ASGI scope only vs also needing the send and receive callables. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that makes sense. Just let me rephrase to make sure I understand:

  • Move form validation into a separate body validator so it has the full ASGI interface (scope, send, receive) available and can decide if / how to consume the stream
  • Parameter validators will only have the scope available, possibly via a Request object which does not provide access to the stream.

Correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed. This to limit the amount of places in which the body stream could be accessed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 does this code mean that the JSONBodyValidator is string-literally matched? I ask because I was thinking about a case where an American business is insisting that a partner use their mm/dd/yyyy format for dates. They already use Connexion, and I'd sooner specify a separate accept & content-type for their weird expected output, and have everything else use, nice readable, standard-compliant ISO-8601 as per https://datatracker.ietf.org/doc/html/rfc3339#section-5.6 and https://swagger.io/docs/specification/data-models/data-types/#string.

@@ -368,31 +372,6 @@ def test_post_wrong_content_type(simple_app):
)
assert resp.status_code == 415

# this test checks exactly what the test directly above is supposed to check,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test is outdated as explained in comment.

tests/api/test_responses.py Outdated Show resolved Hide resolved
@Ruwann
Copy link
Member

Ruwann commented Sep 15, 2022

  • Agreed that there should be a Operation-like class that only serves to provide access to the Operation Spec. It would be nice to have a single interface for swagger 2 and openapi 3, but don't have a good view on whether that's actually feasible (some changes from 2 to 3 might be too big to unify again)
  • I think it makes sense indeed? At first sight, this would make it easier to reason about the send and receive callables, especially for the body validators. But I'll have to try it out myself to fully understand the impact.

@RobbeSneyders RobbeSneyders changed the title Feature/validation middleware Extract JSON request body validation to middleware Sep 16, 2022
@RobbeSneyders
Copy link
Member Author

@Ruwann @nielsbox The PR is cleaned up and all tests are passing, so if you approve, I'd propose to merge this already.

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

Successfully merging this pull request may close these issues.

5 participants