-
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
Update base URLs #155
Update base URLs #155
Conversation
Have OpenAPI schemas only contain a single version of the full API, i.e., the routers for `/optimade/vMAJOR.MINOR` and `/optimade/vMAJOR.MINOR.PATCH` are added after the OpenAPI schema creation. The version-less base URL has been removed and the standard is now `/optimade/vMAJOR`.
Due to the server tests, it is not straightforward to raise during creation of the top-level meta field, if the expected base URL prefix path is not found, e.g., `/optimade/v0`. Indeed, this should also issue an OPTiMaDe warning instead.
let me know once you're done |
Should be fine now since all checks have passed. |
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 - one question and one suggestion, otherwise looks all good
# path prefix could not be found, e.g., `/optimade/v0`. | ||
# However, due to the testing, this error cannot be raised anymore. | ||
# Instead, an OPTiMaDe warning should be issued. | ||
# Having this try and except is still good practice though. |
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.
So, what exactly would be excepting in the try
block?
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.
Yeah ... not much at this moment in time - but I hope to implement the OPTiMaDe warnings similarly to Python Warning
s, which are a sub-class of Exception
s. I.e., at some point again (hopefully soon) there will be raised something, which can be caught here and relayed as an OPTiMaDe warning
... But at the moment... yeah, not much.
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.
I don't quite follow - the except
block here would only be activated if there was a programming error in the try
block, i.e. in the preparation of the error response object.
In this case, one should not raise a Warning but an Error (since something would have gone seriously wrong).
The only difference I see between the response in try
and in except
is that the one in except
does not add all the optimade stuff.
Perhaps one can simply remove the try/except here?
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.
The except
block would be activated if the utility function meta_values
raises, for one reason or another. Within meta_values
is where the meta
->query
->representation
is created and returned, which is why i originally created the optional_base_urls
function - to return all 6 different path prefixes so they could be removed from the urlparse
d path.
I don't know if this makes sense ... but if it should happen that the URL path of the server, for any reason, does not have the prefixes we set for it (e.g., /optimade/v1
, which actually happens for the TestClient
used to test the server) it would in my original commit raise a HTTPException
. I tried to fix this in the test setup, but it eluded me, so instead I chose to backtrack, remove the HTTPException
raised in meta_values
and simply keep the try
and except
in this part - although it's technically not necessary (unless meta_values
should fail due to other unknown reasons).
97af883
to
141b412
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.
thanks for the fixes @CasperWA
looks good to go!
Looks like this has broken the heroku? https://optimade.herokuapp.com/optimade/info errors for me |
Sure. The "basic" base URL is now |
Ah okay (I see now from the description above that we removed the unversioned URL). The heroku badge in our README is still broken at least, will make a quick PR fixing that. |
Closes #122
The OpenAPI schemas now only provide information about a single base URL, i.e., the routers are not reproduced over and over.
After the schema creation, the separate, now OPTIONAL, versioned base URLs are created.
This means:
/optimade/vMAJOR,MINOR
/optimade/vMAJOR.MINOR.PATCH
For the regular server, and similar for the index meta-database, only it's starting with
/index/optimade/...
.The version-less base URL has been removed, the standard is now
/optimade/vMAJOR
or/index/optimade/vMAJOR
.representation
undermeta
->query
was wrongly showing part of the base URL, this has now been remedied.Also, it fixes #129