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

Add support for /versions endpoint #54

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

rartino
Copy link
Contributor

@rartino rartino commented Jan 5, 2021

The index meta-database isn't exempt from the mandatory /versions endpoint in the OPTIMADE specification v1.0.0 section 5.2 (right?).

This PR adds this support when hosing via netlify.

Edit: Realized we need the same support also for the hosted index meta-databases. I think I've added it (but it isn't yet tested - I'll check the netlify deploy as soon as it is finished). -- yes, it now seems to work in the netlify deploy.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

All good for me, especially with the latest testing 👍 Thanks @rartino !

I am wondering whether we need to also add support for the api_version query parameter? (If that's what it's called, can't remember now.)
I hope not, but we should check in the specification and fix it there if it says so, I guess?

Otherwise, I think you can merge-at-will :)

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.

Just a comment from me, the version negotiation for this repo doesn't work according to the specification, which dictates that 553 Version Not Supported must be returned when requesting e.g. http://deploy-preview-54--optimade-providers.netlify.app/v2 ... other than that, everything looks fine on the deploy preview!

@ml-evs
Copy link
Member

ml-evs commented Jan 5, 2021

Perhaps the validator could be run in the CI for this repo, or somewhere else?

$ optimade-validator --index -vv http://deploy-preview-54--optimade-providers.netlify.app/v1
Testing entire implementation at http://deploy-preview-54--optimade-providers.netlify.app/v1
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/info - received expected response: <Response [200]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/info - deserialized correctly as object of type <class 'optimade.models.responses.IndexInfoResponse'>
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1 - successfully found available entry types in baseinfo
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/versions - received expected response: <Response [404]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/v123123 - received expected response: <Response [404]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/links - received expected response: <Response [200]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/v1/links - deserialized correctly as object of type <class 'optimade.validator.utils.ValidatorLinksResponse'>


Passed 6 out of 6 tests.
Additionally passed 1 out of 1 optional tests.

and similarly for the hosted dbs:

$ optimade-validator --index -vv http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1
Testing entire implementation at http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/info - received expected response: <Response [200]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/info - deserialized correctly as object of type <class 'optimade.models.responses.IndexInfoResponse'>
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1 - successfully found available entry types in baseinfo
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/versions - received expected response: <Response [404]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/v123123 - received expected response: <Response [404]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/links - received expected response: <Response [200]>.
✔: http://deploy-preview-54--optimade-providers.netlify.app/index-metadbs/oqmd/v1/links - deserialized correctly as object of type <class 'optimade.validator.utils.ValidatorLinksResponse'>


Passed 6 out of 6 tests.
Additionally passed 1 out of 1 optional tests.

@rartino
Copy link
Contributor Author

rartino commented Jan 5, 2021

Support for /versions is on MUST level. After this PR, according to my reading, we only break SHOULD level requirements, rights?

@CasperWA So, trying to decipher exactly what we've written about api_hint. Any serving at all of the API on unversioned endpoints is on MAY level according to Section 3.1.2. The section on version negotiation then says that serving on the unversioned base url SHOULD take api_hint into account. But, clearly it must still be an option to simply not serve anything on the unversioned URL, and in that case api_hint will be ignored. So I think we are good with respect to api_hint as long as we don't serve anything on the unversioned URL.

@ml-evs Indeed, the failure to return 553 on /v2 does break the SHOULD level requirement in 3.1.1.. Thinking about it, I think this can actually be fixed with some more Netlify configuration. But we can do that in a separate PR.

@CasperWA
Copy link
Member

CasperWA commented Jan 5, 2021

Perhaps the validator could be run in the CI for this repo, or somewhere else?

I thought that was what the "Scheduled validator / OPTIMADE validator" did? But maybe I'm wrong? :)

@ml-evs
Copy link
Member

ml-evs commented Jan 5, 2021

Perhaps the validator could be run in the CI for this repo, or somewhere else?

I thought that was what the "Scheduled validator / OPTIMADE validator" did? But maybe I'm wrong? :)

That seems to be running some more manual tests that just check for deserialization (which is fine)... using the actual validator can definitely wait :)

@ml-evs ml-evs self-requested a review January 5, 2021 19:43
@rartino rartino merged commit 2307338 into Materials-Consortia:master Jan 5, 2021
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.

None yet

3 participants