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

WIP: support openapi spec version 3 #591

Closed
wants to merge 100 commits into from
Closed

Conversation

dtkav
Copy link
Collaborator

@dtkav dtkav commented May 31, 2018

This is a work in progress and is not ready for merge!

Fixes #420

Changes proposed in this pull request:

Changes in behavior for OAS3 specs:

  • The request body is now passed to the function handler as an object (called body by default, with x-body-name override).

There are a lot of new files in this diff. This is because I vendored swagger-ui-3 (this will change with #614)

The test suite is mostly the same, except that it has been parameterized to run with both versions of each spec. Test fixtures were added in #616

@dtkav dtkav mentioned this pull request Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment Jun 5, 2018
@spec-first spec-first deleted a comment from dtkav Jun 5, 2018
@coveralls
Copy link

coveralls commented Jun 6, 2018

Coverage Status

Coverage decreased (-1.7%) to 98.105% when pulling 3bf0a4c on dtkav:oas3 into 0c352cd on zalando:master.

from ..operation import Operation
from ..options import ConnexionOptions
from ..resolver import Resolver
from ..utils import Jsonifier

try:
from urllib.parse import urlparse
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use six here?

from six.moves.urllib.parse import urlparse

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah great - thanks!

if data is None and len(request.body) > 0 and not self.is_null_value_valid:
# the body has contents that were not parsed as JSON
return problem(415,
"Unsupported Media Type",
Copy link
Contributor

Choose a reason for hiding this comment

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

Being working on major changes, I'd avoid typographical changes as they increase the review work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, thanks!

if data is None and len(request.body) > 0 and not self.is_null_value_valid:
# complain about no data?
pass
data.update({k: '' for k in dict(request.files)}) # validator expects string..
Copy link
Contributor

@ioggstream ioggstream Jun 18, 2018

Choose a reason for hiding this comment

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

does this work?

data.update(dict.fromkeys(request.files, ''))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL - thanks!

@spec-first spec-first deleted a comment Jun 18, 2018
@spec-first spec-first deleted a comment Jun 19, 2018
@spec-first spec-first deleted a comment Jun 19, 2018
if parameter['in'] == 'formData'}

# openapi3 body
if body_name is None and body_schema is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if body_name is blank?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think body_name can be blank in swagger2 as "name" is a required field(https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#parameterObject). It is set to None in oas3 because requestBody replace the in["body"] parameters of swagger2, so body_parameters = [{}].

I find all this interspersed logic really yucky, so I'm open to ideas for how to split it out by spec version.
This file is especially bad 😞
related: see my other branch that does this with the Operation class dtkav/connexion@18a7c28...dtkav:oas3_class_separation#diff-f4018bfc9e3bdfe01ab150a7069d72ceL183

else:
body_properties = {}

query_defns = {sanitize_param(parameter['name']): parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and beyond,

{... for parameter in parameters}
{... for param in parameters}

be consistent for readability (I prefer short stuff in comprehension) eg {... for p in parameters}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I totally agree

@dtkav
Copy link
Collaborator Author

dtkav commented Jul 11, 2018

The new plan is to stage changes for a swagger 2.0 release on the dev-2.0 branch.
Closing this in favor of PR #621 against that branch.

@dtkav dtkav closed this Jul 11, 2018
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.

OpenAPI v3 support
5 participants