Skip to content

fix!: oai schema validation false negatives (vendored openapi_schema_pydantic update) #426

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

Conversation

p1-ra
Copy link
Contributor

@p1-ra p1-ra commented May 13, 2021

The vendored openapi_schema_pydantic seems to be old? It have some bugs that are resolved on its last up to date version.

As for exemple, the following oai content -- headers definition will raise a false negative oai validation error:

(...)
      requestBody:
        content:
          multipart/related:
            encoding:
              binaryPayload:
                contentType: application/vnd.3gpp.sms
                headers:
                  Content-Id:
                    schema:
                      type: string
              jsonData:
                contentType: application/json
            schema:
              properties:
                binaryPayload:
                  format: binary
                  type: string
                jsonData:
                  $ref: '#/components/schemas/SmsRecordData'
              type: object
        required: true
(...)

The actual vendored version wrongly only accept reference as header definition:
https://github.com/triaxtec/openapi-python-client/blob/0f58cfd4fc985ba871b3d2694d861c7f595dcb81/openapi_python_client/schema/openapi_schema_pydantic/encoding.py#L25
Resolved:
https://github.com/P1sec/openapi-python-client/blob/607a22109c93c885e24b20c0847436a5b32fa184/openapi_python_client/schema/openapi_schema_pydantic/encoding.py#L25

This PR update the vendored version and backport to it tiny changes that was made to it.

Some fixes bring internal breaking changes, as exemple, schema oneOf, anyOf that was defaulting to a shared list reference:
https://github.com/triaxtec/openapi-python-client/blob/0f58cfd4fc985ba871b3d2694d861c7f595dcb81/openapi_python_client/schema/openapi_schema_pydantic/schema.py#L244
Are now defaulting to None value:
https://github.com/kuimono/openapi-schema-pydantic/blob/0836b429086917feeb973de3367a7ac4c2b3a665/openapi_schema_pydantic/v3/v3_0_3/schema.py#L243

I also consider it to bring external breaking change, since the OAI validation is now way more strict. Existing OAI definitions that was accepted may now raise validation errors.

@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch 3 times, most recently from da4ddec to b312cd0 Compare May 13, 2021 13:45
@p1-ra p1-ra changed the title fix!: oai schema validation false positives (vendored openapi_schema_pydantic update) fix!: oai schema validation false negatives (vendored openapi_schema_pydantic update) May 13, 2021
@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch 8 times, most recently from 3810aa5 to 986bf2b Compare May 13, 2021 15:13
  - anyOf, allOf, oneOf now default to `None` instead
    of `[]`
@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch 3 times, most recently from bbdc1f2 to f1cdd88 Compare May 13, 2021 15:41
@codecov
Copy link

codecov bot commented May 13, 2021

Codecov Report

Merging #426 (e58f790) into main (0f58cfd) will decrease coverage by 0.12%.
The diff coverage is 97.36%.

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #426      +/-   ##
===========================================
- Coverage   100.00%   99.87%   -0.13%     
===========================================
  Files           47       48       +1     
  Lines         1548     1586      +38     
===========================================
+ Hits          1548     1584      +36     
- Misses           0        2       +2     
Impacted Files Coverage Δ
..._client/schema/openapi_schema_pydantic/callback.py 80.00% <80.00%> (ø)
..._client/schema/openapi_schema_pydantic/encoding.py 92.85% <83.33%> (-7.15%) ⬇️
...penapi_python_client/parser/properties/__init__.py 100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py 100.00% <100.00%> (ø)
..._client/schema/openapi_schema_pydantic/__init__.py 100.00% <100.00%> (ø)
...lient/schema/openapi_schema_pydantic/components.py 100.00% <100.00%> (ø)
...n_client/schema/openapi_schema_pydantic/contact.py 100.00% <100.00%> (ø)
...nt/schema/openapi_schema_pydantic/discriminator.py 100.00% <100.00%> (ø)
...n_client/schema/openapi_schema_pydantic/example.py 100.00% <100.00%> (ø)
.../openapi_schema_pydantic/external_documentation.py 100.00% <100.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f58cfd...e58f790. Read the comment docs.

Copy link
Contributor Author

@p1-ra p1-ra left a comment

Choose a reason for hiding this comment

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

PR ready for review @dbanty

Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! I think we should make a few changes though, as I'm not sure what exists upstream is necessarily what's best for this project (which is why I vendored it). Adding Callback and fixing Headers seem like the main two benefits here, maybe we can just keep those.

@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch 2 times, most recently from bba6e33 to b0dd28a Compare May 13, 2021 17:06
@p1-ra p1-ra requested a review from dbanty May 13, 2021 17:30
@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch from ac61bda to b552181 Compare May 13, 2021 17:55
@p1-ra p1-ra force-pushed the fix/oai-schema-validation-false-positives branch from ff7950f to e58f790 Compare May 13, 2021 23:06
@dbanty dbanty added this to the 0.11.0 milestone Dec 18, 2021
@dbanty
Copy link
Collaborator

dbanty commented Jan 18, 2022

Finally getting back around to this so I can add it to the upcoming breaking release. Sorry it took so long 😅 but the changes are over in #568 now so I can update with the latest from main.

@dbanty dbanty closed this Jan 18, 2022
dbanty added a commit that referenced this pull request Jan 19, 2022
BREAKING CHANGE: Validation of OpenAPI documents is now more strict.
Co-authored-by: Nementon <nementon@badwolf.fr>
Co-authored-by: p1-ra <aurelien.roose@p1sec.com>
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