Skip to content

Move towards JSON API standard for responses#18

Open
ltalirz wants to merge 3 commits into
masterfrom
adopt-json-api
Open

Move towards JSON API standard for responses#18
ltalirz wants to merge 3 commits into
masterfrom
adopt-json-api

Conversation

@ltalirz

@ltalirz ltalirz commented Jun 3, 2021

Copy link
Copy Markdown
Member

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"
         }
      }
    ]
  },
}

ltalirz added 2 commits June 3, 2021 23:05
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

ltalirz commented Jun 3, 2021

Copy link
Copy Markdown
Member Author

@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
Copy Markdown

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

ltalirz commented Jun 3, 2021

Copy link
Copy Markdown
Member Author

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

chrisjsewell commented Jun 3, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
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

Comment thread .pre-commit-config.yaml
- fastapi~=0.65.1
- uvicorn[standard]>=0.12.0,<0.14.0
- pydantic~=1.8.2
- pydantic-jsonapi==0.11.0

Copy link
Copy Markdown
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 *

@chrisjsewell chrisjsewell Jun 3, 2021

Copy link
Copy Markdown
Member

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?)

@chrisjsewell chrisjsewell left a comment

Copy link
Copy Markdown
Member

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

@edan-bainglass

Copy link
Copy Markdown
Member

@ltalirz thanks for initiating this. I recently wrapped up several major updates in this PR that build on the pydantic models of aiida-core (see aiidateam/aiida-core#6990). My plan is to now do what you aimed to do here. However, it is likely easier to implement it from scratch, considering the changes. As such, I will open a PR superseding this one and tag you there.

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.

4 participants