-
Notifications
You must be signed in to change notification settings - Fork 42
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
(Cosmetic) updates to models #195
(Cosmetic) updates to models #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @CasperWA
Looks fine to me - although I didn't fully get the reasoning behind the set => list change.
One thing to keep in mind is that list introduces an ordering that people may start to rely on, while set has no ordering.
We should just make sure to use what makes most sense here.
If there is a technical issue to make this change that should be reverted later, keep track of this in an issue.
It's also not clear.
True for Python (and other languages). Not true for OpenAPI / JSON. Here only
I agree. And the
I think I will try and fix it here. |
Will revert change from Set
to List
Moved all non-response models/classes to optimade_json. This mainly includes Warnings and ResponseMeta and all their used/internal models.
Testing this further I have come to the conclusion that The solution can be to do the following: Another solution would be to try and fix this in |
For the attributes/fields that should indeed have unique items, the OpenAPI attribute `uniqueItems` is manually set to `True`. Otherwise, the implementation has to make sure the items are indeed unique.
5844765
to
f8d190f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't see anything wrong with this, thanks @CasperWA!
I think it's worth raising an issue with pydantic for the Set
issue, at least for clarification.
Up to v0.6.0. **New features**: - GitHub Action validator that runs `optimade_validator` for a locally running OPTiMaDe server (#191, @CasperWA, tested by @ml-evs) - Support filter queries for `HAS ALL`, `HAS ANY` and `HAS ONLY` and value lists on MongoDB backends (#173, @ml-evs) Note: `OPERATOR` use in value lists are still _not_ supported. - Debug mode. Start the server in debug mode to enable `debug` log-level in `uvicorn` and get a full Python traceback in the JSON response (#190, @CasperWA) - Add testing of mandatory `filter` queries to `optimade_validator` (#205, @ml-evs) **Updates**: - Allow Cross-Origin requests from anywhere (`*`), i.e., enable CORS for both servers (#194, @CasperWA) - Updates to models (correct misspelling, more transparent model class naming, streamline models with respect to python class inheritance, update field descriptions) (#195, @CasperWA) - API change: Rename `optimade.models.toplevel.py` to `optimade.models.responses.py` (#195, @CasperWA) - Update dependencies to newest versions (as of 02.03.2020) (#202, #196, @CasperWA) - Move imports from `starlette` to `fastapi`, where possible (#202, @ml-evs) - Remove custom middleware to redirect slashed URLs in favor of `starlette` implementation (#202, @ml-evs) - CI tests are now performed with a real MongoDB in the backend. CI tests are also performed with a `mongomock` backend for the tests in `server/test_middleware.py`, `server/test_server_validation.py`, and `server/test_config.py` (#196, @ml-evs, additional testing by @CasperWA ) - Remove `/optimade` from base URLs. This was especially important for the OpenAPI schema (#201, #216, @CasperWA, @ml-evs) - Check integrity of query part of the raw URL using a custom middleware (#209, @CasperWA) - New `optimade/server/exceptions.py` to contain custom `HttpException`s, moved `BadRequest` here (#209, @CasperWA) - Pattern and regex testing for `data.available_api_versions` parts in `/info` endpoint and fix tests for the same (#211, @CasperWA) - Restructure import of test data for regular server (#212, @shyamd) **Bug fixes**: - New retrieval URL for Materials-Consortia's list of providers (#187, @CasperWA) - Skip local `HAS ONLY` tests with a `mongomock` backend, since v3.19.0 does not support these (#206, @ml-evs) - Resource ID's can now contain slashes (`/`) (#183, @ml-evs, @CasperWA) - Remove _valid_ version part of base URL in `meta.query.representation` (#201, @CasperWA)
This branch was originally created to test #192, however. Unfortunately it does not fix it, however, I found some minor mistakes in the models and have fixed them here:
BaseRelationshipMeta
.Error
model from JSON API toOptimadeError
.errors
inErrorResponse
, which is the same as it's parent model classFailure
.Set
toList
(this may not be prudent, but fails forErrorResponse
. So it should either be fixed and returned to its status ofSet
or not).Note, this changes the OpenAPI parameter
uniqueItems
fromtrue
tofalse
(or rather, just removes it).Due to an error in
pydantic
, this is necessary (see my comment here), to keep the OpenAPI schemas reflecting it should indeed be unique items in the respective lists, theuniqueItems
attribute is added "manually" throughpydantic
'sField
.meta
attributes in response models.New "major" change:
toplevel.py
in models toresponses.py
and moved all non-"final"-response models tooptimade_json.py
.