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

Finalize 2.0 Release #678

Closed
dtkav opened this issue Sep 13, 2018 · 28 comments
Closed

Finalize 2.0 Release #678

dtkav opened this issue Sep 13, 2018 · 28 comments
Assignees
Milestone

Comments

@dtkav
Copy link
Collaborator

dtkav commented Sep 13, 2018

Description

We created the dev-2.0 branch to introduce a handful of major breaking changes to connexion.
https://github.com/zalando/connexion/milestone/10

At the time, the two major changes were:

Both of these changes are now in dev-2.0

Next Steps

What other things should be in the 2.0 release? Is it large enough as is? What's could be better?
It would be great to finalize the feature set for Connexion 2.0 and get it shipped! 🚢

Let me know what you think!

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 13, 2018

The PR for merging dev-2.0 -> master is python-jsonschema/jsonschema#619

@ioggstream

This comment has been minimized.

@hjacobs
Copy link
Contributor

hjacobs commented Sep 13, 2018

@dtkav thanks for the great work!

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 14, 2018

Thanks for the bug report @ioggstream ! I'll follow that up in a separate ticket. I'd like to keep this one a bit more of a high-level discussion on what features / breaking changes should make it into Connexion 2.0. (I've hidden your comment, now that its' in python-jsonschema/jsonschema#681 - hope you don't mind!)

@hjacobs and @jmcs , I would love your thoughts especially on high-level next steps!

Thanks to everyone in the community who helped make these features possible ❤️

@hjacobs
Copy link
Contributor

hjacobs commented Sep 14, 2018

@dtkav I think the 2.0 release should not be bigger as the OpenAPI 3 support is already a monster change. Let's get some feedback from people trying it out and then releasing it with any additional blocker bug fixes we encounter.

@jmcs WDYT? Do we know who could/would try it out and give feedback?

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 14, 2018

@hjacobs - I tend to agree. The only other thing I can think might be useful (and a breaking change) is to validate requests/responses depending on mimetype, and not only if the endpoint is purely json (i.e. python-jsonschema/jsonschema#593).
Everything else I can think of can be incremental improvement.

@cziebuhr
Copy link
Contributor

I'm a quite new user of connexion, but I really like the project.
OpenAPI 3 uses JSON Schema Specification Wright Draft 00 (= Draft 5), which is mostly compatible to JSON-Schema Draft 4.
But it points out that "$ref" should only be used wherever a schema is expected. This allows "$ref" to be a valid key for the "properties" keyword. The new resolve_refs function breaks that feature.
Future versions of OpenAPI may use Draft 6 or later, which introduce more incompabilities, e.g. exclusiveMinimum/exclusiveMaximum being a number instead of a boolean.
I therefore want to discuss if it makes sense to have a validator_map per api (add parameter to add_api) instead of having it globally.

In our setup, we already have some extensions to Draft4Validator, regarding x-nullable and readOnly (the first one with a different implementation than here). I will open seperate issues/PRs for discussion about these, but that shouldn't block the 2.0 release.

@ioggstream
Copy link
Contributor

@dtkav I'm running the specs against some files. I'll file separate issues like you did with the yaml stuff, ok?

PS: Thank You for the flash fix!

@cziebuhr
Copy link
Contributor

Can you have a look at dev-2.0...cziebuhr:nullable ? According to OpenAPI 3, nullable "allows sending a null value for the defined schema", so it should also work outside of properties.

@cziebuhr
Copy link
Contributor

Depending on using swagger 2 / openapi 3, ConnexionOptions sets swagger_ui_local_path to swagger_ui_2_path / swagger_ui_3_path. IMHO this is only the major version of swagger-ui, swagger-ui 3 supports both swagger 2 and openapi 3.

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 17, 2018

Can you have a look at dev-2.0...cziebuhr:nullable ? According to OpenAPI 3, nullable "allows sending a null value for the defined schema", so it should also work outside of properties.

@cziebuhr please make a pull request! Those changes look reasonable to me.

Depending on using swagger 2 / openapi 3, ConnexionOptions sets swagger_ui_local_path to swagger_ui_2_path / swagger_ui_3_path. IMHO this is only the major version of swagger-ui, swagger-ui 3 supports both swagger 2 and openapi 3.

I couldn't quite tell from your comment - are you suggesting we always serve swagger-ui-3? I'm open to that. I am aware that swagger-ui-3 is backwards compatible, but I purposefully tried to avoid changing too much about the Swagger2 development experience when adding OpenAPI3 support. Since the path is easily override-able, my thinking was to not force a major-version bump by default.

I'm totally open to feedback, and happy to make the change based on what the community thinks is best.

@cziebuhr
Copy link
Contributor

PR for nullable is at #684.
I changed it a bit, so I don't need to overwrite iter_errors.

And I have another suggestion, regarding changes in security handling: dev-2.0...cziebuhr:security
It addresses some issues I had when trying to use connexion for a production-ready product.

  • Use token_info['sub'] instead of token_info['uid'] to better align with RFC7662 (oauth token introspection)
  • x-tokenInfoUrl is only kept for compability reasons. IMHO it's only useful for a small amount of people, because it hardcodes bearer authentication and the response format is not standardized. I can add an example later on how to use RFC7662 with x-tokenInfoFunc.
  • validate_token_info was not customizable and simply compared two sets of strings.
    In our setup, we want to add more logic, e.g. scope 'ressource' is superior to 'ressource:read'. One can now set a custom scope validator.
  • If security definitons don't match the connexion implementation (only one definition supported), connexion silently ignors ALL definitions and allows full access to the api
  • Basic authentication had to be done manully as function decorator instead of using the security definitions from the swagger definition. Now one can define callbacks similar to x-tokenInfoFunc.
    Bonus: If both, oauth and basic auth is specified for an operation, the new x-basicInfoFunc also receives the required oauth scopes to use them for authorization.
  • Also added support for apiKey for completeness, I don't use it myself

Sorry for "misusing" the dev-2.0 issue here, but I think it would be huge benefit for all connexion users and would fit into a major version bump.
It's not yet fully tested and documentation/examples are not updated yet, but I would like to get some general feedback on that approach.

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 18, 2018

Thank @cziebuhr - I'll try to take a look at python-jsonschema/jsonschema#684 tonight.
Can you make a pull request for your other proposed changes as well? That's the best way to get specific feedback. We'll likely need some more in-depth review from Zalando folks because your proposal changes the security flow.

@cziebuhr
Copy link
Contributor

Opened #686 for the security rework

@cbornet
Copy link

cbornet commented Sep 18, 2018

Speaking in the name of OpenAPI-generator, a community fork of swagger-codegen, we are very eager to see the v2 of connexion. Our current flask-connexion generator is broken because we can only output a v3 version of the spec in the templates (OpenAPITools/openapi-generator#323).
So 💯 to release as soon as possible. Do you have an ETA for the release or for a RC ?

@Lawouach
Copy link

It's a big plus one for us as well! Looking forward to it :)

@dtkav
Copy link
Collaborator Author

dtkav commented Sep 21, 2018

A bit of an update:
PR's merged:

  1. Remove the DEFAULT_TYPES and all non-TypeChecker-related type modification for Validators python-jsonschema/jsonschema#681 allows references to yaml files (thanks @ioggstream )
  2. Cast to json schema python-jsonschema/jsonschema#683 adds x-openapi-router-controller for OpenAPI-generator compatibility. (thanks @wheelsandmetal)
  3. Still struggling with local "$ref"s python-jsonschema/jsonschema#684 added (proper) support for jsonschema nullable, readOnly, and writeOnly (great work @cziebuhr )
  4. [[False], [0]] should be considered unique python-jsonschema/jsonschema#686 changes the security flow, so probably needs proper review from someone at zalando (Thanks @cziebuhr @jmcs and @hjacobs)
  5. Enhance jsonschema.exceptions.best_match to prefer deeper errors python-jsonschema/jsonschema#698 setting base_url does not update the spec that is served to swagger-ui (thanks @czchen @JuxhinDB @jmcs)

Issues on hold:

  1. Custom error message instead of "xx is a required property" python-jsonschema/jsonschema#691 oneOf bug (thanks @czchen)
    I don't think this should block the release (what do you think?), because:
    1. The bug is pretty well understood now.
    2. It will take some time to fix, but won't be a breaking change.

Thanks to everyone who has been testing the dev-2.0 branch and reporting issues.
Everyone please test the dev-2.0 branch if you can!

It's easy to install it with:

pip install git+https://github.com/zalando/connexion.git@dev-2.0#egg=connexion[swagger-ui]

@jmcs
Copy link
Contributor

jmcs commented Oct 1, 2018

I released version 2.0.0rc1 this can be installed with pip --pre ... (or pipenv install --pre ...).

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 4, 2018

FYI: Due to python-jsonschema/jsonschema#707 if you want to install RC1 with pip install --pre, you'll need to prevent jsonschema from installing their alpha release as well:
pip install --pre connexion[swagger-ui] 'jsonschema<3.0.0'

@jmcs
Copy link
Contributor

jmcs commented Oct 4, 2018

I released version 2.0.0rc2 with the jsonschema version fix.

@jmcs
Copy link
Contributor

jmcs commented Oct 5, 2018

I released version 2.0.0rc3 with #712

@lsorber
Copy link

lsorber commented Oct 10, 2018

Feature request: it would be great to have JWT support in v2.0, which seems to have been asked many times before (#607, python-jsonschema/jsonschema#389, python-jsonschema/jsonschema#124) and is part of the OpenAPI 3 spec.

@hjacobs hjacobs added this to the 2.0 milestone Oct 18, 2018
@dtkav
Copy link
Collaborator Author

dtkav commented Oct 18, 2018

hey @lsorber , feel free to make a pull request! I'm happy to review it.

We've already closed the merge window on 2.0, and I think we should get it out without any more changes (unless they are bugfixes). JWT support should be backwards compatible though right? No reason why we can't merge it in for 2.1 or some other minor release.

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 27, 2018

FYI @lsorber , @krise3k put up this great PR (#732) for JWT support.

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 29, 2018

@jmcs has released 2.0.0rc5 🎉
pip install --pre connexion[swagger-ui] to install it.

@JuxhinDB
Copy link

Big well done guys--sorry I haven't been active but I'm definitely keeping an eye. @dtkav Would you consider rc5 to be somewhat stable? I can start using it soon if so

@dtkav
Copy link
Collaborator Author

dtkav commented Oct 29, 2018

@JuxhinDB yes, I think it's fairly stable. Please bang on it and see if you find any issues. I've recommended that we push RC5 as the 2.0 release (and put JWT support in 2.1, which could easily be soon after).

@hjacobs
Copy link
Contributor

hjacobs commented Nov 5, 2018

2.0.0 was released 🎉 https://github.com/zalando/connexion/releases/tag/2.0.0

Thanks to all contributors!

@hjacobs hjacobs closed this as completed Nov 5, 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

No branches or pull requests

9 participants