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.

Sign up for GitHub

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

Jump to bottom

Refactor Operation into AbstractOperation and Swagger2Operation #639

Merged
merged 5 commits into from
Aug 23, 2018
Merged

Refactor Operation into AbstractOperation and Swagger2Operation #639

merged 5 commits into from
Aug 23, 2018

Conversation

dtkav
Copy link
Collaborator

@dtkav dtkav commented Jul 25, 2018 •
edited
Loading

Relates to #420

In an attempt to break up the OpenAPI3 PR, this includes all of the refactor that went into that branch, but leaves out other openapi3 support.

Changes proposed in this pull request:

  • Establish interface for what an AbstractOperation does
  • Implements interface with Swagger2Operation
  • Defaults to Swagger2URIParser (not AlwaysMultiURIParser)

Plan for OpenAPI integration:
Once this is merged, the OAS3 branch should basically contain

  1. The OpenAPIOperation class
  2. The OpenAPIURIParser class
  3. requestBody form validation
  4. modifications to apis/abstract to get the spec version, and instantiate the correct operation class

@coveralls
Copy link

coveralls commented Jul 25, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 99.683% when pulling a514e56 on dtkav:swagger2_oas3_refactor into 07bcd0e on zalando:dev-2.0.

This was referenced Jul 25, 2018
Closed
Merged
@dtkav
Copy link
Collaborator Author

dtkav commented Jul 26, 2018

Landing #641 should make this diff a little smaller.
I'll remove the WIP from this PR once that's in.

@dtkav dtkav force-pushed the swagger2_oas3_refactor branch 2 times, most recently from b891e8e to 1a11cc6 Compare July 27, 2018 05:06
@dtkav dtkav mentioned this pull request Jul 30, 2018
Merged
@dtkav dtkav changed the title WIP: Create AbstractOperation ABC in prep for openapi version 3 Create AbstractOperation ABC in prep for openapi version 3 Jul 30, 2018
@dtkav dtkav changed the title Create AbstractOperation ABC in prep for openapi version 3 Refactor Operation into AbstractOperation and Swagger2Operation Jul 30, 2018
@dtkav dtkav force-pushed the swagger2_oas3_refactor branch 2 times, most recently from 02c03d0 to edfe260 Compare July 30, 2018 08:05
@dtkav dtkav mentioned this pull request Aug 1, 2018
Merged
@jmcs
Copy link
Contributor

jmcs commented Aug 2, 2018

@dtkav can you resolve the conflicts?

@dtkav dtkav force-pushed the swagger2_oas3_refactor branch 4 times, most recently from 9ddd931 to 2eba7bc Compare August 2, 2018 06:23
@dtkav
Copy link
Collaborator Author

dtkav commented Aug 2, 2018

I thought of another way to slim down this PR. I'll put up a PR that is just the change from swagger-spec-validator to openapi-spec-validator.

@dtkav dtkav force-pushed the swagger2_oas3_refactor branch from 2eba7bc to 2fb2a90 Compare August 2, 2018 06:37
@dtkav dtkav mentioned this pull request Aug 2, 2018
Merged
dtkav
dtkav commented Aug 2, 2018
View reviewed changes
tests/api/test_bootstrap.py Outdated
@@ -41,10 +41,10 @@ def test_app_with_different_server_option(simple_api_spec_dir, spec):


def test_app_with_different_uri_parser(simple_api_spec_dir):
from connexion.decorators.uri_parsing import Swagger2URIParser
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'll factor this out into a separate PR

@dtkav dtkav mentioned this pull request Aug 2, 2018
Merged
@dtkav dtkav force-pushed the swagger2_oas3_refactor branch from 2fb2a90 to f97a7a4 Compare August 7, 2018 04:56
@dtkav
Copy link
Collaborator Author

dtkav commented Aug 7, 2018

#653 will make this smaller (factors out switch to openapi_spec_validator)

@dtkav dtkav force-pushed the swagger2_oas3_refactor branch 4 times, most recently from 50c453f to 653745b Compare August 8, 2018 07:21
@dtkav dtkav mentioned this pull request Aug 8, 2018
Merged
@dtkav dtkav force-pushed the swagger2_oas3_refactor branch from 653745b to d25f6b6 Compare August 10, 2018 01:32
@dtkav dtkav mentioned this pull request Aug 10, 2018
Closed
@dtkav dtkav force-pushed the swagger2_oas3_refactor branch from 34ef804 to a514e56 Compare August 23, 2018 02:44
JuxhinDB
JuxhinDB reviewed Aug 23, 2018
View reviewed changes
connexion/operations/swagger2.py
@@ -0,0 +1,291 @@
import logging
Copy link

Choose a reason for hiding this comment

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

Curious, was this just stripped out from an older file into its own file to ensure backwards compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, the swagger2.py file is for backwards compatibility with the swagger2 spec. The idea with this PR is to create an Abstract Base Class (abc), which is essentially the interface that describe what an "Operation" is. Previously we only had one implementation of this interface (swagger 2 Operation), so it wasn't well defined. Now, after this refactor, the OpenAPIOperation can implement the exact same interface as a Swagger2Operation. Together they encapsulate the differences between the Swagger2 spec and the OpenAPI3 spec. Hope that makes sense.

@jmcs
Copy link
Contributor

jmcs commented Aug 23, 2018

👍

@jmcs jmcs merged commit 99ac95f into spec-first:dev-2.0 Aug 23, 2018
dtkav added a commit to dtkav/connexion that referenced this pull request Aug 24, 2018
@dtkav
Refactor Operation into AbstractOperation and Swagger2Operation (spec…
0621fb8 
…-first#639)

* refactor Operation into AbstractOperation ABC and SwaggerOperation

* remove defaults validation from swagger2.py (now handled in validator)

* update README 'new in 2.0' section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuxhinDB JuxhinDB JuxhinDB approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtkav @coveralls @jmcs @JuxhinDB