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

Syntactic tweaks to models and schemas for compatibility with fastapi>0.66 #1131

Merged
merged 5 commits into from May 29, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Apr 25, 2022

This PR makes 2 main changes to ensure FastAPI compatibility going forward (see #887).

The issue is essentially that FastAPI broke OpenAPI specs generated from pydantic by adding extra = 'allow' to its base OpenAPI schema class (see tiangolo/fastapi#3745). For example, if you used Field(const=True) in pydantic, the const key would be added to the eventual OpenAPI schema, yet const does not exist as an OpenAPI field. The same then happened for our custom schema injections of support, queryable and unit.

This PR adds the following workarounds:

  • Replace all uses of const and pattern with just the pydantic regex (which checks the value with the regex and also injects pattern into the schema). typing.Literal, replacing constant values in the OpenAPI schema with single-valued enums. The other option here would be to remove the const check and replace pattern with regex so that pydantic applies the regex during validation.
  • Add wrappers to the underlying pydantic fields that prefix our custom injected fields, so that support becomes e.g. x-optimade-support. These need to be retained in the JSONSchema so that they can be used to set the OpenAPI nullable flag.
  • Finally this PR fixes an issue where one field was not wrapped in OptimadeField already.

@ml-evs ml-evs marked this pull request as draft April 25, 2022 14:40
@ml-evs
Copy link
Member Author

ml-evs commented Apr 26, 2022

Thanks for the review @JPBergsma, I've put this PR on hold as I think this is going to more complicated than I first thought.

@ml-evs ml-evs added schema Concerns the schema models server Issues pertaining to the example server implementation models For issues related to the pydantic models directly on-hold For PRs/issues that are on-hold for an unspecified time labels Apr 26, 2022
@CasperWA
Copy link
Member

CasperWA commented May 2, 2022

Concerning const, did you see that the value MUST be equal to the default value? https://pydantic-docs.helpmanual.io/usage/schema/#field-customization

@CasperWA
Copy link
Member

CasperWA commented May 2, 2022

Concerning const, did you see that the value MUST be equal to the default value? https://pydantic-docs.helpmanual.io/usage/schema/#field-customization

Ah, I see this was already the case. Never mind :)

@ml-evs
Copy link
Member Author

ml-evs commented May 25, 2022

Just FYI, I got a response on my FastAPI issue, it seems like there will not be a fix.

They accepted my PR adding a warning to the docs: https://fastapi.tiangolo.com/tutorial/body-fields/#add-extra-information

@ml-evs
Copy link
Member Author

ml-evs commented May 25, 2022

Thinking about this more, we might just have to bite the bullet and change how we test the OpenAPI schema. It looks like MP can't update to the latest OPTIMADE version atm as they are already using a more recent FastAPI. I'll see what I can do in time for the v0.18 release.

@ml-evs ml-evs marked this pull request as ready for review May 29, 2022 15:12
@codecov
Copy link

codecov bot commented May 29, 2022

Codecov Report

Merging #1131 (138fddb) into master (8d1edba) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1131      +/-   ##
==========================================
- Coverage   91.95%   91.84%   -0.12%     
==========================================
  Files          72       72              
  Lines        4265     4279      +14     
==========================================
+ Hits         3922     3930       +8     
- Misses        343      349       +6     
Flag Coverage Δ
project 91.84% <100.00%> (-0.12%) ⬇️
validator 91.23% <100.00%> (+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/optimade_json.py 95.78% <ø> (ø)
optimade/models/references.py 97.72% <ø> (ø)
optimade/models/structures.py 95.75% <ø> (ø)
optimade/server/mappers/entries.py 97.11% <ø> (ø)
optimade/server/routers/index_info.py 100.00% <ø> (ø)
optimade/server/routers/info.py 95.45% <ø> (ø)
optimade/models/baseinfo.py 95.23% <100.00%> (ø)
optimade/models/entries.py 97.77% <100.00%> (ø)
optimade/models/index_metadb.py 100.00% <100.00%> (ø)
... and 3 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 8d1edba...138fddb. Read the comment docs.

@ml-evs ml-evs removed the on-hold For PRs/issues that are on-hold for an unspecified time label May 29, 2022
@ml-evs ml-evs changed the title Remove use of const and manually prefix custom fields (FastAPI compatibility fixes) FastAPI>=0.66 compatibility updates to models May 29, 2022
@ml-evs ml-evs changed the title FastAPI>=0.66 compatibility updates to models Syntactic tweaks to models and schemas for compatibility with fastapi>0.66 May 29, 2022
@ml-evs ml-evs changed the title Syntactic tweaks to models and schemas for compatibility with fastapi>0.66 Syntactic tweaks to models and schemas for compatibility with fastapi>0.66 May 29, 2022
@ml-evs ml-evs merged commit 6c2e768 into master May 29, 2022
@ml-evs ml-evs deleted the ml-evs/fastapi_update branch May 29, 2022 19:32
"x-optimade-unit",
"x-optimade-queryable",
"x-optimade-support",
"x-optimade-unit",
Copy link
Contributor

Choose a reason for hiding this comment

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

You have listed "x-optimade-unit" unit twice here. That seems a bit strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm yes, this is a typo, though not a consequential one. I will remember to fix it in the next PR I make

setup.py Show resolved Hide resolved
JPBergsma added a commit that referenced this pull request Jun 1, 2022
As mentioned in PR #1131 "x-optimade-unit" occurs twice by mistake.
ml-evs pushed a commit that referenced this pull request Jun 1, 2022
As mentioned in PR #1131 "x-optimade-unit" occurs twice by mistake.
ml-evs pushed a commit that referenced this pull request Jun 1, 2022
As mentioned in PR #1131 "x-optimade-unit" occurs twice by mistake.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
models For issues related to the pydantic models directly schema Concerns the schema models server Issues pertaining to the example server implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants