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 /links #95

Merged
merged 8 commits into from Dec 2, 2019
Merged

Add /links #95

merged 8 commits into from Dec 2, 2019

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Nov 28, 2019

Closes #89

Several things are happening here:

  • Centralize the mapper method map_back in the parent class. This should provide an idea of going forward and perhaps removing the sub-classed mappers or similar.
  • Add the /links endpoint with a pseudo index parent implementation and all the providers from providers.json.

Comments:
I am unsure on how to treat the /links endpoint concerning whether /links/{entry_id} is valid. And why is it demanded that only the SingleEntryQueryParams are valid query parameters for the endpoint, (see here)?
I think some of these questions should be put to the consortium and changes should be suggested to the spec.

Missing:

  • Tests for the /links endpoint. (Added by @ml-evs)

Since map_back is the same for several endpoints, it has been moved to
ResourceMapper and will only be modified (at this point) for the /links
endpoint mapper.
TODO: Turn providers.json into proper MongoDB entries
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #95 into master will decrease coverage by 0.75%.
The diff coverage is 74.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   85.74%   84.99%   -0.76%     
==========================================
  Files          38       39       +1     
  Lines        2315     2352      +37     
==========================================
+ Hits         1985     1999      +14     
- Misses        330      353      +23
Impacted Files Coverage Δ
optimade/server/deps.py 100% <ø> (ø) ⬆️
optimade/server/mappers/references.py 100% <ø> (+3.57%) ⬆️
optimade/server/mappers/structures.py 100% <ø> (+6.45%) ⬆️
optimade/server/config.py 79.54% <100%> (ø) ⬆️
optimade/server/mappers/__init__.py 100% <100%> (ø) ⬆️
optimade/server/utils.py 77.09% <100%> (+3.64%) ⬆️
optimade/models/toplevel.py 100% <100%> (ø) ⬆️
optimade/validator/validator.py 61.77% <30.76%> (-2.25%) ⬇️
optimade/server/entry_collections.py 81.88% <40%> (-1.86%) ⬇️
optimade/server/main.py 74.03% <50%> (-4.62%) ⬇️
... and 4 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 1b67038...8ce3e80. Read the comment docs.

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.

Thanks @CasperWA! LGTM but made a few comments below.

optimade/server/deps.py Show resolved Hide resolved
optimade/server/entry_collections.py Show resolved Hide resolved
Comment on lines +182 to +196
def get_links(request: Request, params: EntryListingQueryParams = Depends()):
for str_param in ["filter", "sort"]:
if getattr(params, str_param):
setattr(params, str_param, "")
for int_param in [
"page_offset",
"page_page",
"page_cursor",
"page_above",
"page_below",
]:
if getattr(params, int_param):
setattr(params, int_param, 0)
params.page_limit = CONFIG.page_limit
return u.get_entries(links, LinksResponse, request, params)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you ran into the same problems as me, there must be a better way of doing this!

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there is in pydantic v1 maybe? Here's hoping.

optimade/server/mappers/entries.py Outdated Show resolved Hide resolved
optimade/server/mappers/entries.py Show resolved Hide resolved
optimade/server/utils.py Outdated Show resolved Hide resolved
@ml-evs
Copy link
Member

ml-evs commented Nov 28, 2019

I've just added two validator changes to this PR: one that checks the new links endpoint, and one minor change that stops the validator from failing if there are no references in the database.

@CasperWA CasperWA mentioned this pull request Nov 29, 2019
@CasperWA CasperWA requested a review from ml-evs November 29, 2019 10:35
ml-evs
ml-evs previously approved these changes Dec 1, 2019
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.

Thanks for this @CasperWA , looks good. I'll accept and let you merge, could you just clarify what the new local_index_openapi.json is for?

@ml-evs ml-evs mentioned this pull request Dec 1, 2019
Better documentation of `map_back` and _only_ use `task_id` for
structures ids.
@CasperWA
Copy link
Member Author

CasperWA commented Dec 2, 2019

Thanks for this @CasperWA , looks good. I'll accept and let you merge, could you just clarify what the new local_index_openapi.json is for?

This should not have been in this PR and has now been removed. It's purpose is for the upcoming "extra" server, serving the index meta-database. A PR will be created for this after this and #99 have been merged.

@CasperWA CasperWA requested a review from ml-evs December 2, 2019 09:41
@ml-evs ml-evs merged commit bcd2b72 into master Dec 2, 2019
@CasperWA CasperWA deleted the close_89_add_links branch December 2, 2019 11:12
@ltalirz ltalirz mentioned this pull request Dec 2, 2019
16 tasks
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.

Server is missing /links endpoint
2 participants