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

Move towards JSON API standard for responses #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ltalirz
Copy link
Member

@ltalirz ltalirz commented Jun 3, 2021

Start moving towards the JSON API standard for responses of the API.

In particular, the actual data is now returned in a data field.

We are not fully adopting the JSON API standard here just yet, since the responses of the aiida-core REST API are not fully compatible.

aiida-core REST API example:

{
  "data": {
    "nodes  ": [
      {
        "ctime": "Sun, 21 Jul 2019 11:45:52 GMT",
        "full_type": "data.dict.Dict.|",
        "id": 102618,
        "label": "",
        "mtime": "Sun, 21 Jul 2019 11:45:52 GMT",
        "node_type": "data.dict.Dict.",
        "process_type": null,
        "user_id": 4,
        "uuid": "a43596fe-3d95-4d9b-b34a-acabc21d7a1e"
      }
    ]
  },
}

How a JSON API conforming response would look like

{
  "data": {
    "nodes  ": [
      {
        "type": "node",
         "id": 102618,       
          "attributes": {
            "ctime": "Sun, 21 Jul 2019 11:45:52 GMT",
            "full_type": "data.dict.Dict.|",
            "label": "",
            "mtime": "Sun, 21 Jul 2019 11:45:52 GMT",
            "node_type": "data.dict.Dict.",
            "process_type": null,
            "user_id": 4,
            "uuid": "a43596fe-3d95-4d9b-b34a-acabc21d7a1e"
         }
      }
    ]
  },
}

using models from optimade python tools
@ltalirz ltalirz changed the title Adopt json api Move towards JSON API standard for responses Jun 3, 2021
@ltalirz
Copy link
Member Author

ltalirz commented Jun 3, 2021

@chrisjsewell I've explicitly excluded the vendored json_api.py from the mypy checks, yet the pre-commit still complains about it.
Can you please tell me how to fix this?

@codecov-commenter
Copy link

Codecov Report

Merging #18 (f65ab59) into master (9abe7f7) will increase coverage by 0.86%.
The diff coverage is 89.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   88.88%   89.75%   +0.86%     
==========================================
  Files           6        9       +3     
  Lines         153      205      +52     
==========================================
+ Hits          136      184      +48     
- Misses         17       21       +4     
Flag Coverage Δ
pytests 89.75% <89.39%> (+0.86%) ⬆️

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

Impacted Files Coverage Δ
aiida_restapi/models/json_api.py 75.00% <75.00%> (ø)
aiida_restapi/routers/users.py 96.55% <94.73%> (+9.59%) ⬆️
aiida_restapi/models/__init__.py 100.00% <100.00%> (ø)
aiida_restapi/models/entities.py 93.02% <100.00%> (ø)
aiida_restapi/models/responses.py 100.00% <100.00%> (ø)

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 9abe7f7...f65ab59. Read the comment docs.

@ltalirz
Copy link
Member Author

ltalirz commented Jun 3, 2021

We'll also need to decide whether to make the response fully JSON API compliant - would be straightforward but would definitely mean having to edit all response processing of existing clients (since the data moves from data to data -> attributes).
Perhaps we start without it for the time being...

@chrisjsewell
Copy link
Member

chrisjsewell commented Jun 3, 2021

@chrisjsewell I've explicitly excluded the vendored json_api.py from the mypy checks, yet the pre-commit still complains about it.

So I imagine you want something like this in the mypy.ini:

[mypy-aiida_restapi.json_api]
check_untyped_defs = False

(it doesn't matter that you've excluded it, because it follows imports)

@chrisjsewell
Copy link
Member

would be straightforward but would definitely mean having to edit all response processing of existing clients (since the data moves from data to data -> attributes).
Perhaps we start without it for the time being...

Well I guess if we are going to make breaking changes, lets not be shy about it lol. I think it is better if possible to have one major change that clients can deal with in one go, rather than multiple stages of changes

@@ -51,6 +52,7 @@ repos:
- fastapi~=0.65.1
- uvicorn[standard]>=0.12.0,<0.14.0
- pydantic~=1.8.2
- pydantic-jsonapi==0.11.0
Copy link
Member

Choose a reason for hiding this comment

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

why pinned to a single version?

"""
# pylint: disable=too-few-public-methods

from .entities import *
Copy link
Member

@chrisjsewell chrisjsewell Jun 3, 2021

Choose a reason for hiding this comment

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

Err, I'm not really a fan of * imports, I think explicit is better. Like we've seen with aiida-core, you can end up with things like sub-classes clobbering namespaces.
I'm open to a discussion on what the best practice is though (is there some PEP that advocates for this?)

Copy link
Member

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

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

thanks @ltalirz, looks like a good standardised approach 👌
just a few minor questions

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.

3 participants