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 types to align with OpenAPI #480

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Aug 31, 2020

Use conlist instead of Tuples

Fixes #478

Due to the way pydantic converts types to JSON types, Tuples are not converted to OpenAPI-compliant JSON types.
Instead, conlist can be used to limit the length of the list and still comply with the desired effects in the actual use of the models.

Fields with constant values

Fixes #479

These fields were not listed as required, nor was it known what the constant value was from the OpenAPI.
This has been updated by adding the pattern attribute to Field, specifying the value wrapped in ^ and $. pattern is used instead of regex, because there is no need to add an extra validator.

These fields mainly include type, but also id.

Also, it seems from the pydantic documentation that the value of the const attribute of Field should be the same as the supplied default value. So this has been updated for all const attribute values.

Warning model better represented in OpenAPI

The status property is not part of the OPTIMADE Warning model, this has now been removed in the OpenAPI specification.


Possible additions to this PR (or another PR):

  • Add OpenAPI validator to CI
    I think we essentially already have this, in terms of the openapi-spec-validator, but it seems it's not good enough?

    Edit: This will be added in another PR (Validate OpenAPI specification in CI #481).

Swagger validation of the 70e8651 commit (it shows that all is good).

* Use `conlist` instead of `Tuple`s:

  Due to the way pydantic converts types to JSON types, Tuples are not
  converted to OpenAPI-compliant JSON types.
  Instead, conlist can be used to limit the length of the list and still
  comply with the desired effects in the actual use of the models.

* Fields with constant values:

  These fields were not listed as required, nor was it known what the
  constant value was from the OpenAPI.
  This has been updated by adding the `pattern` attribute to Field,
  specifying the value wrapped in `^` and `$`.
  `pattern` is used instead of `regex`, because there is no need to add
  an extra validator.

  These fields mainly include `type`, but also `id`.

  Also, it seems from the pydantic documentation that the value of the
  `const` attribute of Field should be the same as the supplied
  `default` value. So this has been updated for all `const` attribute
  values.

* Warning model better represented in OpenAPI:

  The `status` property is not part of the OPTIMAD Warning model, this
  has now been removed in the OpenAPI specification.
@CasperWA CasperWA added schema Concerns the schema models OpenAPI OpenAPI / Swagger related issue labels Aug 31, 2020
@CasperWA CasperWA requested review from ml-evs and shyamd August 31, 2020 13:56
@CasperWA
Copy link
Member Author

Ah, idea: We use the Swagger validator? Just like I have done in the link above to the Swagger validation of commit 70e8651. It will naturally fail if Swagger is down, but otherwise, it might not be a bad choice?
Perhaps we could just add it to the pre-commit instead of the CI? This way it can be more easily by-passed if it should be down?

@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #480 into master will increase coverage by 0.02%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   91.65%   91.67%   +0.02%     
==========================================
  Files          60       60              
  Lines        2828     2848      +20     
==========================================
+ Hits         2592     2611      +19     
- Misses        236      237       +1     
Flag Coverage Δ
#unittests 91.67% <96.66%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/models/links.py 100.00% <ø> (ø)
optimade/models/references.py 97.67% <ø> (ø)
optimade/models/optimade_json.py 95.74% <88.88%> (-0.77%) ⬇️
optimade/models/baseinfo.py 95.23% <100.00%> (ø)
optimade/models/index_metadb.py 100.00% <100.00%> (ø)
optimade/models/jsonapi.py 91.96% <100.00%> (+0.96%) ⬆️
optimade/models/structures.py 95.62% <100.00%> (ø)
optimade/validator/validator.py 77.10% <0.00%> (ø)
... and 4 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 878d932...482ea75. Read the comment docs.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @CasperWA!

(I think we can have the other discussions re: pre-commit, in the other PR)

@CasperWA CasperWA merged commit c70050e into Materials-Consortia:master Aug 31, 2020
@CasperWA CasperWA deleted the openapi_updates branch August 31, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenAPI OpenAPI / Swagger related issue schema Concerns the schema models
Projects
None yet
2 participants