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

Implement include query parameter #163

Conversation

CasperWA
Copy link
Member

@CasperWA CasperWA commented Feb 5, 2020

Closes #94

This PR also changes the API slightly, in the sense that it renames optimade/server/deps.py to optimade/server/query_params.py, as was first suggested in #162.

Note: Support for providing relationship paths to include is not implemented, only endpoint names/entry types, e.g., "references".

@CasperWA CasperWA marked this pull request as ready for review February 5, 2020 19:26
@CasperWA
Copy link
Member Author

CasperWA commented Feb 5, 2020

Should we include child, parent, etc. types in the "allowed" types to pass to the include parameter? Now it is the keys of optimade/server/routers/__init__.py:ENTRY_COLLECTIONS, i.e., links, references, and structures, as well as the "empty" values: "" and ''.

@CasperWA CasperWA mentioned this pull request Feb 5, 2020
10 tasks
@CasperWA CasperWA force-pushed the close_94_handle_include_query_param branch from bf7f80b to b60e7b5 Compare February 6, 2020 14:31
Added to both multi and single entry query parameters.
A 400 Bad Request exception has been added, this may be collected in a
separate file with other custom exceptions that mainly sub-classes
FastAPI's `HTTPException`.

To provide an "empty" `include` parameter, the value has to be either ""
or ''.
If a resource not specified in the keys of
optimade/server/routers/__init__.py's ENTRY_COLLECTIONS is provided in
the value of `include` (and it's not an "empty" value) a 400 Bad Request
error will be raised and returned.
@CasperWA CasperWA force-pushed the close_94_handle_include_query_param branch from b60e7b5 to 801086a Compare February 6, 2020 23:19
@CasperWA
Copy link
Member Author

CasperWA commented Feb 6, 2020

I started to play around with using a regex or Enum for include, but ended up not doing this, since if caught in the validation, it will respond with a 500 status code, but the spec specifically asks for a 400 status code response if the include does not validate properly.
This is handled further down the line in the code, so it will be left "open" in the query parameter classes.

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.

This looks solid, thanks @CasperWA. Works nicely at my end.

@CasperWA
Copy link
Member Author

CasperWA commented Feb 7, 2020

What is your position on including other entry types, such as child and so on? Or should we just keep it for references and structures (and links)?

@ml-evs
Copy link
Member

ml-evs commented Feb 7, 2020

What is your position on including other entry types, such as child and so on? Or should we just keep it for references and structures (and links)?

I guess parent, child, and provider should all be allowed as they are valid types (if not entry types).

@CasperWA
Copy link
Member Author

CasperWA commented Feb 7, 2020

Hmm. Looking further into the code on how relationships are handled in general it's a bit messy. The models only allow references and structures at the moment, but the routers also allow links.
Furthermore, the JSON API infers resource types, not endpoints, but in OPTiMaDe we have decided to make it only endpoints (or is it types? - I am confused about that, although for references and structures it doesn't matter, since it's the same), and one can even supply nested relationships to include, which is not at all implemented in our repo. And I don't know exactly how we would do that either just now.
So I think I will just let the query parameter validate against what the router utility function is also validating against, i.e., links, references and structures. I wonder what happens if links is provided now?

@CasperWA
Copy link
Member Author

CasperWA commented Feb 7, 2020

(...) I wonder what happens if links is provided now?

It works fine with links, but that should only be true as long as noone actually starts adding links relationships to the data - since then the models shouldn't allow the relationships. Even though the router utility function technically is fine with it ...

Anyway, leaving the include parameter to validate against the same variable as the utility function handling included relationships seems the best approach now. And then it can be updated together at a later point.

@CasperWA CasperWA merged commit 44b1b9b into Materials-Consortia:master Feb 8, 2020
@CasperWA CasperWA deleted the close_94_handle_include_query_param branch February 8, 2020 21:29
@CasperWA CasperWA mentioned this pull request Feb 13, 2020
CasperWA added a commit that referenced this pull request Feb 13, 2020
Bump to v0.5.0

Changes:
- Possibility for Docker deployment for both the index meta-database
  server as well as the regular server (#140, @ltalirz, @CasperWA)
- Test building and starting Docker images with GitHub Actions CI
  (#140, @CasperWA, @ml-evs, @ltalirz)
- Remove `/index` from the index meta-database's base URL
  (#140, @ltalirz, @CasperWA)
- `include` query parameter (#163, @CasperWA)
- Rename `optimade/server/deps.py` to `optimade/server/query_params.py`
  (#163, @CasperWA)
- Human-readable landing page for versioned base URLs, as well as for
  `/optimade` (#172, @ml-evs)
- Move mapper aliases to config file and out of mapper classes
  (#175, @ml-evs)

Bug fixes:
- Properly build versioned base URLs (#178, @CasperWA)
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.

Handle include standard JSON API query parameter
2 participants