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

Filter out extension endpoints before validation #794

Merged
merged 3 commits into from May 11, 2021
Merged

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented May 7, 2021

Closes #793.

@ml-evs ml-evs requested a review from CasperWA as a code owner May 7, 2021 13:49
@codecov
Copy link

codecov bot commented May 7, 2021

Codecov Report

Merging #794 (14917c9) into master (37e3c97) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #794   +/-   ##
=======================================
  Coverage   92.71%   92.72%           
=======================================
  Files          68       68           
  Lines        3666     3669    +3     
=======================================
+ Hits         3399     3402    +3     
  Misses        267      267           
Flag Coverage Δ
project 92.72% <100.00%> (+<0.01%) ⬆️
validator 92.72% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
optimade/validator/config.py 100.00% <100.00%> (ø)
optimade/validator/validator.py 82.39% <100.00%> (+0.03%) ⬆️

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 37e3c97...14917c9. Read the comment docs.

@ml-evs ml-evs added priority/medium Issue or PR with a consensus of medium priority validator Related to the OPTIMADE validator labels May 11, 2021
available_json_entry_endpoints = [
endp
for endp in available_json_entry_endpoints
if not endp.startswith("extensions/")
Copy link
Member

Choose a reason for hiding this comment

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

If there are custom endpoints not under extensions/, then they are still not accounted for.
I think we should instead turn this upside down and only test a specific set of endpoints and leave everything else alone? Or do we have requirements for custom endpoints in the specification? Stuff like what they have to return and such?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only constraint is that they have to be served from extensions/<type>, so anything that does not start with extensions/ is fair game (this obviously relates to #793 (comment)).

I think your suggestion makes sense to only validate {"structures", "references", "calculations"} (where "/calculations" will only be checked as a valid entry listing endpoint).

Copy link
Member

@CasperWA CasperWA May 11, 2021

Choose a reason for hiding this comment

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

I believe we at some point agreed in the specification that providers can create custom endpoints both at the root as well as under extensions/, hence my idea to instead focus on the entry endpoints we know about, i.e., the ones you list here.

Edit: It seems someone (perhaps myself) may have trumped through that they have to be served under extensions/, but I remember there was a discussion with @rartino and @merkys (as well as yourself) about this. But never mind. If we just instead focus on the endpoints we know of, i.e., again the ones you list here, then we can check if they are listed or available and only validate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, imagine this is something that will be discussed again this year; let's just fix this as suggested for now.

CasperWA
CasperWA previously approved these changes May 11, 2021
optimade/validator/config.py Outdated Show resolved Hide resolved
@ml-evs ml-evs merged commit 7742d1b into master May 11, 2021
@ml-evs ml-evs deleted the ml-evs/closes_#793 branch May 11, 2021 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/medium Issue or PR with a consensus of medium priority validator Related to the OPTIMADE validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not validate extension endpoints
2 participants