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 v3 support #420

Closed
tomas-fp opened this issue Mar 22, 2017 · 88 comments
Closed

OpenAPI v3 support #420

tomas-fp opened this issue Mar 22, 2017 · 88 comments
Milestone

Comments

@tomas-fp
Copy link

Just curious about OpenAPI 3 support plans.

See related post about Swagger 2 and OpenAPI 3 introduction:
https://blog.readme.io/an-example-filled-guide-to-swagger-3-2/

@rafaelcaricio
Copy link
Collaborator

I have this blog post in my read buffer. 😄

To be honest I haven't thought too much about the OpenAPI 3 in Connexion so far. But definitely it is time to start considering it. I will follow up in this thread later when I give this topic some more thought. Thank you for opening this issue.

If anyone have ideas/suggestions or is willing to help in this I will be glad to support.

@tomas-fp
Copy link
Author

Cool. I'm open to help in a near future, especially when in my team we can start considering OpenAPI 3 adoption across our services.

@danballance
Copy link
Contributor

I need to get up to speed on version 3 and would then like to help.

@JuxhinDB
Copy link

JuxhinDB commented Apr 4, 2017

Just went over the new OpenAPI 3.0 which looks quite nice. I'm also curious as to when we may consider supporting it in Connexion. :-)

@danballance
Copy link
Contributor

Will we try to simultaneously support both versions 2 and 3? Although more work, my assumption is this would be the way to go, otherwise we will break a lot of APIs currently built with Connexion.

@rafaelcaricio
Copy link
Collaborator

@danballance Yes, we will support both. We will try to reuse as much as possible the codebase we have.

As side note, I am working on a sketch of the support for OpenAPI v3.

@JuxhinDB
Copy link

Hey @rafaelcaricio just wanted to ask if you guys have made any progress on this and if you might need any form of testing.

@rafaelcaricio
Copy link
Collaborator

@JuxhinDB Unfortunately no. I'm currently busy with some personal stuff and with little free time to take care of this right now. :(

@JuxhinDB
Copy link

That's fine, @rafaelcaricio! Feel free to ping me in the future should you need any help with this.

@hjacobs hjacobs changed the title OpenAPI v3 support plans OpenAPI v3 support Aug 7, 2017
@smith-m
Copy link

smith-m commented Aug 9, 2017

support for 3.0 would be awesome. Parameter and anchor references are much flexible and it can mean less repeated code to document the same API.

@bhearsum
Copy link

bhearsum commented Aug 9, 2017

The OpenAPI 3.0 spec was finalized and released a couple of weeks ago: https://www.openapis.org/blog/2017/07/26/the-oai-announces-the-openapi-specification-3-0-0

hellais added a commit to ooni/api that referenced this issue Sep 4, 2017
public API endpoints

This achieves the following goals:

* It's possible to programmatically generate the docs from the swagger
spec (see: https://github.com/Rebilly/ReDoc)

* It's easier to enforce better request argument normalisation and avoid
stupid type casting bugs at the source

* It reduces the overall amount of boilerplate needed to do common API
tasks

Currently I use Open API 2.0 instead of 3.0 because the connexion (the
library that implements the swagger spec doesn't support 3.0 yet. See:
spec-first/connexion#420)
@JuxhinDB
Copy link

JuxhinDB commented Oct 26, 2017

Hi, if anyone is willing to guide me through with an email or two on the codebase, I may try to help with some progress on OpenAPIv3 support. If so feel free to ping me. :-)

@advance512
Copy link

Any roadmap for support for v3?
e.g. having oneOf on types.

@martsa1
Copy link

martsa1 commented May 3, 2018

Any update on this? This would be awfully handy!

@dtkav
Copy link
Collaborator

dtkav commented May 22, 2018

I've been making some progress on a branch https://github.com/dtkav/connexion/tree/oas3
I'd love to chat about it if anyone is keen to help. There are a handful of design decisions to figure out.
My plan is to get the unit tests passing, and all of the examples, and then open a pull request for discussion.

@hjacobs
Copy link
Contributor

hjacobs commented May 22, 2018

@dtkav sounds great!

@JuxhinDB
Copy link

@dtkav -- Some time next week I would love to check out the work and see if I can provide you with any help, even if it's just testing related. :-)

@dtkav
Copy link
Collaborator

dtkav commented May 23, 2018

I'm looking for feedback on ways to approach the problem of supporting swagger2 and oas3 simultaneously.

I can imagine a few approaches:

  1. add a bunch of if/else statements all over the place, get it working, then refactor.
  • pros:
    -- very incremental
    -- leverage existing tests
  • cons:
    -- increases code complexity dramatically
    -- generally yucky
  1. refactor the operations class into classes that deal with the different specs (resolve refs, path/global defaults), and then slim out the connexion operation class to focus on the webserver side of the operation.
  • pros:
    -- allows for a refactor first before adding oas3 support
    -- relatively incremental
  • cons:
    -- crams oas3 support into swagger2 data structures, negating a lot of the benefits of oas3
  1. convert all code to oas3 style data structures, then convert swagger2 spec -> oas3 spec immediately upon loading the spec.
  • pros:
    -- reap benefits of simplified spec, new features
    -- oas3 is completely backwards compatible, so conversion should be lossless
    -- separation of concerns (spec differences are captured in the converter only)
  • cons:
    -- lose benefits of lots of existing test coverage
    -- not very incremental
    -- no existing swagger2 -> oas3 spec converter in python (that I know of)

I've put a little bit of time into options 1, and 2. I'll probably give 3 a shot next and report back.
The main pain points so far have been around requestBody vs. body/form parameters, and content mimetypes vs. consumes/produces.

Let me know what you all think! I'm currently leaning toward option 3 because it would delegate the responsibility of backwards compatibility and keep the code clean. Unfortunately that strategy isn't very incremental, so I imagine it will be tough to upstream.

@hjacobs
Copy link
Contributor

hjacobs commented May 23, 2018

@dtkav I would be fine with option 1 (incremental changes) as it would leverage existing tests and add new tests for OpenAPI v3. This then would be a solid base to do a bigger refactor like proposed with option 3. Maybe I'm too pragmatic and not so much into the code anymore, so take my opinion with a grain of salt 😏

@JuxhinDB
Copy link

JuxhinDB commented May 24, 2018

Just to add my 2c -- given the man power for this, I also feel option 1 makes most sense where we can leverage as much of the current framework as we can to build on top of. I will dedicate some time next week to help out with implementation/testing where possible.

@dtkav
Copy link
Collaborator

dtkav commented May 24, 2018

cool - thanks team!
I've gotten the unit tests passing again for swagger2, and I'm able to support some converted os3 examples here: https://github.com/dtkav/connexion/tree/oas3 (definitely a very messy work in progress!)

@JuxhinDB thanks - I'm keen to have someone to bounce ideas off of - little things keep coming up.
for example -- in oas3 the requestBody doesn't have a name (vs swagger2 body/form parameters).

with swagger2:
the body is passed to the handler function by name (ex. def put_pet(pet_id, pet) where pet is the deserialized/validated body).
with oas3 there are a few options, but none of them seem great:

  1. Unpack the body (ex. def put_pet(pet_id, id, animal_type, name)).
    However, currently the operation params are unpacked so they have the potential for collision!
  2. Always call the body 'body' (ex. def put_pet(pet_id, body)).
    However, this limits the ability to name any params "body"!
  3. Pack the params as well (ex. def put_pet(params, body))
    However, the function signature is a lot less useful.
  4. Make some variable available like the request object in Flask that is implicitly accessible in the request context. ex:
def put_pet(pet_id):
   pet = cnx.body
   # do stuff

However, this is a significant workflow difference between swagger2 and oas3, and is potentially difficult to support for the different webservers (flask, aiohttp, etc).

Let me know what you think!

@hjacobs
Copy link
Contributor

hjacobs commented May 24, 2018

@dtkav I would propose passing the body as a parameter (option 2) and would try to make it smart, i.e. it should work for existing function signatures (last argument is the body parameter) and we could make it configurable via some x-.. extension in the YAML to define the body parameter name.

@dtkav
Copy link
Collaborator

dtkav commented Aug 23, 2018

@JuxhinDB if you can click the "review changes" button next time, that would record your review and approval with the commit.

@JuxhinDB
Copy link

@dtkav -- Done, cool feature that I got to learn. :-)

Later today I'll go over the other bigger PR and give my thoughts.

@viralanomaly
Copy link

This is probably an awful hack, but I just put this in my file that I run with pip install -r:

# connexion
git+https://github.com/dtkav/connexion.git@oas3#egg=connexion

@dtkav
Copy link
Collaborator

dtkav commented Aug 24, 2018

@jmcs thanks for the merge!
It looks like there are some things that are in master that haven't made it into dev-2.0. I've merged the latest master into dev-2.0. I've pushed the previous state to https://github.com/zalando/connexion/tree/dev-2.0-bak in case I messed that up.

@JuxhinDB thanks a lot for the review of #639 !
Now #621 should be a pretty reasonable size, and ready for review! Let me know if you can think of ways to pull out features to make the diff smaller.

@dtkav
Copy link
Collaborator

dtkav commented Aug 27, 2018

I just found an new edge case with form parameters in OpenAPI 3 from a test case that had I added to master a while back. I need to work out array handling for form parameters that come in through the requestBody in oas3, so I'm going to put the WIP tag back on the diff. I'll update again when It's ready for review.

@JuxhinDB
Copy link

Sounds good @dtkav -- feel free to ping me if you would like eyes on certain diffs.

@dtkav
Copy link
Collaborator

dtkav commented Aug 29, 2018

Thanks @JuxhinDB I really appreciate it.
AFAICT I've fixed up the problems related to form data in oas3, so I've removed the WIP status from #621 . I think it should be ready for early eyes on, but it's a bit big so I'll try to break out smaller pieces if I can in parallel. I would love your review!

@dtkav
Copy link
Collaborator

dtkav commented Sep 3, 2018

To anyone who is able - please review #621 - it adds support for OpenAPI 3 specs!
Once this is landed I'll have much more energy to address other issues. Thanks!
@JuxhinDB @rafaelcaricio @coltenkrauter @viralanomaly @positron96 @ioggstream @jmcs @hjacobs

@JuxhinDB
Copy link

JuxhinDB commented Sep 3, 2018

I went over the PR and overall I didn't notice any glaring issues. That said, it would be best if someone with Swagger2/OpenAPI3 experience went over it from a spec perspective

@dtkav
Copy link
Collaborator

dtkav commented Sep 4, 2018

Thanks @JuxhinDB ! Do you mind going through the review process (click the review button, leave comments, etc)?

@dtkav
Copy link
Collaborator

dtkav commented Sep 6, 2018

Wholehearted thanks for the review @JuxhinDB - I'm going to leave the PR open for the rest of the week to give others a chance. By default I'll merge this PR into dev-2.0 on Monday unless:
a) Someone else requests changes
b) Someone asks for more time to review

@lihan
Copy link

lihan commented Sep 7, 2018

Some validations are not working yet.
One of the best new feature in OpenAPI spec 3.0 is the 'oneOf' which lets you do polymorphism.

In a post request body


requestBody:
  description: Event
  content:
    application/json:
       schema:
         oneOf:
            - $ref: '#/components/schemas/Event1'
            - $ref: '#/components/schemas/Event2'

It will fail the validation when bootstrapping the app.
It prompts only recognise these two types.

{'oneOf': [
{'$ref': '#/definitions/requestBody'},
{'$ref': '#/definitions/reference'}]}

This is the issue from python-openapi/openapi-spec-validator#46 anyway.

@dtkav
Copy link
Collaborator

dtkav commented Sep 7, 2018

hey @lihan - thanks for opening that issue. I'll take a look over on the openapi-spec-validator project.

@dtkav dtkav closed this as completed Sep 7, 2018
@dtkav dtkav reopened this Sep 7, 2018
@dtkav
Copy link
Collaborator

dtkav commented Sep 7, 2018

whoops, I pressed the wrong button, sorry!

@dtkav
Copy link
Collaborator

dtkav commented Sep 7, 2018

@lihan which version of jsonschema are you using? I'm on jsonschema==2.6.0
The below example works for me:

# added required fields to your spec
openapi: 3.0.0
info:
  version: 0.0.0
  title: pets
# added required fields to your spec

paths:
  /pets:
    patch:
      operationId: lihan.patch
      requestBody:
        content:
          application/json:
            schema:
              oneOf:
                - $ref: '#/components/schemas/Cat'
                - $ref: '#/components/schemas/Dog'
      responses:
        '200':
          description: Updated
components:
  schemas:
    Dog:
      type: object
      properties:
        bark:
          type: boolean
        breed:
          type: string
          enum: [Dingo, Husky, Retriever, Shepherd]
      required:
       - bark
       - breed
    Cat:
      type: object
      properties:
        hunts:
          type: boolean
        age:
          type: integer
      required:
       - hunts
       - age
> curl -X PATCH "http://localhost:5000/pets" -H  "accept: */*" -H  "Content-Type: application/json" -d "{\"age\":5,\"hunts\":true}"
"OK"

function args: ()
function kwargs: {'body': {'age': 5, 'hunts': True}}

@dtkav
Copy link
Collaborator

dtkav commented Sep 7, 2018

While debugging your issue, I discovered that discriminator is not part of jsonschema, so I'll probably need to add that to the validator somehow. Feel free to contribute tests cases for important use cases that you have.

@dtkav
Copy link
Collaborator

dtkav commented Sep 11, 2018

I've merged #621 (OpenAPI 3.0.0 Support) into the dev-2.0 branch 🎉
Next up will be the examples. I should be able to polish those up for review tomorrow.

@dtkav
Copy link
Collaborator

dtkav commented Sep 12, 2018

Next up are:

@dtkav
Copy link
Collaborator

dtkav commented Sep 13, 2018

Next step is to finalize the feature set for the 2.0 release and ship it!! (#619) 🚢
Also, I'm working on fixing up the codacy warnings for the branch, but some of them are bogus (#673).

Please comment on #678 if you think there are any major features that should be in the 2.0 release!

@hjacobs
Copy link
Contributor

hjacobs commented Nov 5, 2018

Closing with the release of 2.0 (https://github.com/zalando/connexion/releases/tag/2.0.0) 😄

@hjacobs hjacobs closed this as completed Nov 5, 2018
@ioggstream
Copy link
Contributor

Thanks to @dtkav and all the team for this great release!

@dtkav
Copy link
Collaborator

dtkav commented Nov 5, 2018

Thanks for everyone's hard work and collaboration!

@JuxhinDB
Copy link

JuxhinDB commented Nov 6, 2018

Big well done guys, loved the collaboration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.