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

✨ NEW: Initial implementation of GraphQL mutations #24

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jun 15, 2021

@ltalirz @NinadBhat this is a rough initial implementation of the mutation (see https://graphql.org/learn/queries/#mutations and https://docs.graphene-python.org/en/latest/types/mutations/) to create a group, e.g.:

mutation {
  groupCreate(label: "hallo", description: "heya") {
    created
    group {
      id
      uuid
      label
      type_string
      description
    }
  }
}

returns:

{
  "data": {
    "groupCreate": {
      "created": true,
      "group": {
        "id": 19,
        "uuid": "7e345722-892d-4764-b820-b4f12921dc67",
        "label": "hallo",
        "type_string": "core",
        "description": "heya"
      }
    }
  }
}

@codecov-commenter
Copy link

codecov-commenter commented Jun 15, 2021

Codecov Report

Merging #24 (08ed301) into master (d115033) will decrease coverage by 0.72%.
The diff coverage is 82.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   92.09%   91.36%   -0.73%     
==========================================
  Files          25       25              
  Lines         746      799      +53     
==========================================
+ Hits          687      730      +43     
- Misses         59       69      +10     
Flag Coverage Δ
pytests 91.36% <82.75%> (-0.73%) ⬇️

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

Impacted Files Coverage Δ
aiida_restapi/graphql/main.py 74.19% <60.00%> (-25.81%) ⬇️
aiida_restapi/graphql/plugins.py 92.50% <94.44%> (+1.19%) ⬆️
aiida_restapi/graphql/groups.py 97.43% <95.00%> (-2.57%) ⬇️

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 d115033...08ed301. Read the comment docs.

@ltalirz
Copy link
Member

ltalirz commented Jun 15, 2021

Thanks @chrisjsewell

Hm... that does indeed look quite a bit more complicated than simply posting a JSON {"label": "hallo", "description": "heya"} to /groups.

  • Do we have to create custom "mutation methods" for creating / updating / ... each entity that a user has to learn or can one get around this?
  • While I understand that it is convenient to have the get_or_create functionality, I would probably first try to keep the API simple, i.e. go for group creation only and implement the get_or_create only later if we really need it

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 15, 2021

that does indeed look quite a bit more complicated than simply posting a JSON

yes but the point is you get structured schema validation, and control exactly the return data you receive. for /groups or whatever POST method, you have to go to the documentation and find out what is going to return, this is a lot more explicit.
Plus, as with queries, you can perform multiple mutation in a single request.

You can always just write mutation { groupCreate(label: "hallo", description: "heya") { created } }, if you want to be more concise, but the point is the client controls what they receive, and knows by the structure of the request exactly what the structure of the return JSON will be.

each entity that a user has to learn or can one get around this?

what do they have to learn it, its explicitly in the schema: https://aiida-restapi--24.org.readthedocs.build/en/24/user_guide/graphql.html#the-graphql-schema (see RootMutation) 😉

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 15, 2021

Do we have to create custom "mutation methods" for creating / updating /

you can create whatever mutation you want, but again IMO explicit is better than implicit (although note currently the get_or_create means that groupCreate is both a create and update mutation)

I'm not sure why this would be any different to deciding what POST methods to create though?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Jun 15, 2021

In 4b6005f I have added a skeleton implementation of middleware (see https://docs.graphene-python.org/en/latest/execution/middleware/). This allows you to interject before each query resolution / mutation, to run operations such as authentication.
IMO this is a better approach than having to remember to decorate every one of your requests with some authentication function (and expecting plugins to do the same), i.e. it consolidates the "business logic" in a single place and separates it from the query/mutation logic (see https://graphql.org/learn/thinking-in-graphs/#business-logic-layer)

(see also https://www.starlette.io/graphql/#accessing-request-information)

@chrisjsewell
Copy link
Member Author

and my final pitch (lol) is that, as with queries, mutations are fully pluggable

@chrisjsewell
Copy link
Member Author

FYI I added some links to the initial comment

@ltalirz
Copy link
Member

ltalirz commented Jun 15, 2021

Thanks for the links to the docs @chrisjsewell

It seems this is indeed the canonical way in graphql...
Are there conventions for the created field? I see the graphene example using ok.
Should we include a field like this for the create methods of every entity type?

Another question that comes to mind here is how to handle files.
E.g. say you post a request that creates two SinglefileData nodes - is there some policy for passing the files with the request that associated them to the right mutation?

@chrisjsewell
Copy link
Member Author

Another question that comes to mind here is how to handle files.

I thought exactly the same thing. Graphene specifically mentions https://pypi.org/project/graphene-file-upload/ but not satisfied on the best approach yet

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