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

Update to openapi-core 0.17 #206

Closed
wants to merge 8 commits into from
Closed

Update to openapi-core 0.17 #206

wants to merge 8 commits into from

Conversation

zupo
Copy link
Collaborator

@zupo zupo commented Mar 24, 2023

This is quite a breaking change.

We are dropping support for all older versions of openapi-core and for OpenAPI 3.0 (upgraded to 3.1).

Refs #203
Refs python-openapi/openapi-core#542

pyproject.toml Outdated Show resolved Hide resolved
validated = settings["request_validator"].validate(
settings["spec"], openapi_request
)
validated = settings["request_validator"].unmarshal(openapi_request)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec is now provided when settings["request_validator"] is instantiated during Pyramid bootup.

info:
version: "1.0.0"
title: Foo API
servers:
- url: /api/v1
paths:
/foo:
$ref: "paths.yaml#/foo"
# $ref: "paths.yaml#/foo"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't get refs to work. Needs further investigation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it seems refs work for everything but for paths?

Details: 9b080c1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this good enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@damonhook: can you pls have a look if you can get examples/splitfile to work with paths being in a separate paths.yaml file? I can probably take it from there then, fix the rests of the tests and such.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it is not just file references not working, but any reference at the paths level.
E.g: doing the following also breaks.

paths:
  /:
    $ref: "#/components/pathItems/todos"
components:
  pathItems:
    todos:
      get:
        ...
      post:
        ...

However, this seems to be an issue with the openapi-spec-validator library. I checked and ref's are allowed here in the OpenAPI 3.1 spec, and if I disable validation of the spec, openapi-core handles it correctly (I can make requests and the parameters pull through correctly).

Going to dig into the openapi-spec-validator library a bit and either create an issue/PR on that side

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that this openapi-spec-validator issue has been closed. The refs in the paths should hopefully work now

Example openapi.yaml

openapi: 3.1.0

info:
  title: Example API
  version: 1.0.0

paths:
  /:
    $ref: "#/components/pathItems/rootPath"

components:
  pathItems:
    rootPath:
      get:
        responses:
          200:
            description: Example Response⏎

openapi-spec-validator openapi.yaml

OK

Previous version of openapi-spec-validator was raising an error

Will give it a test with the examples/splitfile

"message": "'not-a-valid-uuid' does not match '^[0-9]{2}-[A-F]{4}$'",
# TODO: ideally, this response would include "field"
# but I don't know how I can achieve this ATM ¯\_(ツ)_/¯ # noqa: ENC100
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

💪

]
)
return True
def validate(self, name: str) -> bool:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same thing as before, just no longer needs to be wrapped in a class.

@@ -41,8 +41,8 @@ def excview_tween(request: Request) -> Response:
if "routes" in gsettings:
settings_key = gsettings["routes"][request.matched_route.name]
settings = request.registry.settings[settings_key]
result = settings["response_validator"].validate(
settings["spec"], request=openapi_request, response=openapi_response
result = settings["response_validator"].unmarshal(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We now have to use unmarshal to get the deserialized values, validate doesn't return anything.

In this light the request.openapi_validated should maybe be renamed to request.openapi_unmarshalled but that's such a mouthful.

@zupo
Copy link
Collaborator Author

zupo commented Mar 24, 2023

@am-on, @damonhook, @dz0ny, @miohtama: if you can spare some brain cycles, I'd love a review of the changes.

@blueracer-io
Copy link

blueracer-io bot commented Mar 24, 2023

Unit tests performance report: ❌

Whoa, hold your horses! This pull request is really slowing down your tests! Are you sure you want to merge it?

Branch Number of tests Suite duration Mean test duration
main 87 1.7s1 18.7ms1
update/openapi-core_0.172 87 (0%) 2.6s (53%) 30.1ms (61%)

Found 1 new test, this is how it compares to existing tests:

Name Duration Percentile Score Status
BadRequestsTests.test_missing_JWT_token 43ms slower than 77% of existing tests 👀

Footnotes

  1. The median/mean of the previous 15 runs. 2

  2. More specifically from commit 4b7879aa80f4d9a3595428b4f21f3253c895b780 in run #1.

Copy link
Collaborator

@damonhook damonhook left a comment

Choose a reason for hiding this comment

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

I noticed that with a later release of openapi-core, running the Spec.create(...) seems to run the validator https://github.com/python-openapi/openapi-core/blob/59996250c5f316c8fc64512115bfd18f587b78ee/openapi_core/spec/paths.py#L29

So we can likely remove the validate_spec() calls we make
here and here. Right now we are validating the spec twice

@zupo
Copy link
Collaborator Author

zupo commented Jun 19, 2023

Swagger UI 5 is released! https://github.com/swagger-api/swagger-ui/releases

@kskarthik
Copy link
Contributor

kskarthik commented Jun 21, 2023

Swagger UI 5 is released! https://github.com/swagger-api/swagger-ui/releases

do hey have multiple supported versions? I have observed 4.x series & 5.x series

Edit: previous were beta releases of 5.x didn't observe previously

Copy link
Collaborator

@damonhook damonhook left a comment

Choose a reason for hiding this comment

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

I see from the 0.17 release notes that Spec.create is deprecated.
https://github.com/python-openapi/openapi-core/releases/tag/0.17.0

While it will still work on 0.17, It does have a deprecation warning (and it has been removed in 0.18)

In add_spec_view

spec = Spec.create(spec_dict)

Should change to

spec = Spec.from_dict(spec_dict)

In add_spec_view_directory

spec = Spec.create(spec_dict, url=spec_url)

Should change to

spec = Spec.from_dict(spec_dict, spec_url=spec_url)

Note

In 0.18, spec_url will be changing to base_uri, but for 0.17 we need to still use spec_url

@zupo
Copy link
Collaborator Author

zupo commented Mar 7, 2024

The upgrade to openapi-core was done in #220.

This PR then remains a good basis to work on bringing in OpenAPI v3.1 support, but I imagine a new PR should be opened.

@zupo zupo closed this Mar 7, 2024
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.

3 participants