app-sre / qenerate

Code Generator for GraphQL Query and Fragment Data Classes
Apache License 2.0
10 stars 7 forks source link

Mutations support #71

Open hoefling opened 1 year ago

hoefling commented 1 year ago

Hi, first of all kudos on opensourcing this great project! I ran qenerate on a fairly complex GraphQL schema and a set of ~200 different queries without any issues in no time, very cool! Do you have any plans on supporting mutations as well?

fishi0x01 commented 1 year ago

Hi, thanks for the nice feedback!

We currently do not use mutations in our projects, thus qenerate doesn't support mutations (yet). However, I will take a look.

Further, out of curiosity, which gql client are you using? Did you use the generated convenience query() function?

fishi0x01 commented 1 year ago

I've just added a test with a mutation based on github's gql schema.

Does the generated code look useful for mutations? Basically I'm trying to understand the use-case, i.e., will the data classes be filled with content and then sent to the server via query method? Should the convenience query method look different for mutations?

hoefling commented 1 year ago

Hi @fishi0x01, sorry for the late reply!

Further, out of curiosity, which gql client are you using? Did you use the generated convenience query() function?

I'm still in the evaluation phase, but I guess integrating gql would be pretty easy, smth like

import gql
from generated_query_module import query as qenerate_query

class MyClient(gql.Client):
    def execute(self, query: str, *args, **kwargs):
        return super().execute(gql.gql(query), *args, **kwargs)

client = MyClient(...)
result = qenerate_query(client.execute)

I especially like the query() wrapper, as it makes possible to avoid the imports of autogenerated data model classes in the business logic. This way, the boilerplate modules can easily be regenerated on demand, without touching the handwritten code.

will the data classes be filled with content and then sent to the server via query method?

Yep, that's the use case exactly.

Does the generated code look useful for mutations?

:+1:

Should the convenience query method look different for mutations?

TBH I have no preference regarding this, as long as there is a convention for it, I am happy :sunglasses:

fishi0x01 commented 1 year ago

will the data classes be filled with content and then sent to the server via query method?

Yep, that's the use case exactly.

I think to make that work for mutations we also need to make input variables available:

mutation CreateReviewForEpisode($ep: Episode!, $review: ReviewInput!) {
  createReview(episode: $ep, review: $review) {
    stars
    commentary
  }
}

should yield a structure for the input variables

{
  "ep": "JEDI",
  "review": {
    "stars": 5,
    "commentary": "This is a great movie!"
  }
}

Currently we fully neglect input variables from the query definitions, as they are redundant for the data structures - filtering happens in the server. Different story for mutations though. Im looking into how to make these input variables best accessible, like providing a data structure for these input variables that could be used by the convenience function. Else you would need to craft a (nested) dictionary to send it to the server - you cannot use the generated classes for that. You could only use them to pass in the server response.

j30ng commented 1 year ago

Hi all. This improvement sounds very exciting, because the lack of support for mutations was the one reason hindering me from completely moving to using this library - as I'm pretty sure lots of people would feel the same. It's been around a month from the last update. When do you expect this to be merged? If you are busy and need some extra hand, maybe I could provide some help.

fishi0x01 commented 1 year ago

The input variables would still need to be crafted manually, as described in my above comment:

Given,

mutation CreateReviewForEpisode($ep: Episode!, $review: ReviewInput!) {
  createReview(episode: $ep, review: $review) {
    stars
    commentary
  }
}

the required input variable structure is

{
  "ep": "JEDI",
  "review": {
    "stars": 5,
    "commentary": "This is a great movie!"
  }
}

I am still trying to figure out a good way to parse input variables in order to generate proper data structures for them too.

@j30ng and @hoefling if that is no big issue for you, then I can go ahead and merge https://github.com/app-sre/qenerate/pull/72, as this would at least generate the structures for the returned data. Does that already help you?

j30ng commented 1 year ago

Sounds good to me. Parsing input variables and generating data structures for them can be done in a separate PR, imo.

fishi0x01 commented 1 year ago

@j30ng I adjusted the generated code a little to better distinguish between mutation and query. Please have a look at this generated test file for a mutation. Does that look convenient or do you think there should be some adjustments?

It could be used the following way:

from my.generated.class import mutate
from gql import Client

# This needs to be enhanced in a future step. Ideally, we also have
# generated classes for input variables
input_data = {'my': 'variable'}

gql_client = Client(...)
typed_response = mutate(mutation_func=gql_client.execute, variables=input_data)
j30ng commented 1 year ago

I think it looks good. I also find the naming "MutationResponse" quite fitting.

fishi0x01 commented 1 year ago

@j30ng @hoefling release 0.5.0 is available now on pypi. It contains the changes discussed in this issue. Please let me know if you stumble across any issues.

I will keep this issue open until there is a solution for the input variables too.

idank commented 1 year ago

Are you sure 0.5.0 supports mutations? I can't get it to generate anything for this file:

# qenerate: plugin=pydantic_v1

mutation CreateCart($input: CartInput) {
  cartCreate(input: $input) {
    cart {
      ...Cart
    }
    userErrors {
      code
      field
      message
    }
  }
}
fishi0x01 commented 1 year ago

@idank you are right - the code command did not pick up the mutation type yet. I Fixed it now in 0.5.1. Does it work for you now?

idank commented 1 year ago

Yeah seems to work now, thanks!

So what's the plan for input variables?

fishi0x01 commented 1 year ago

Glad it works now!

I would like to generate dataclasses for input variables too. However, we do not use mutations right now in our projects, so the time I will invest for this is limited. I'm not sure yet when I will be able to add it, but basically one needs an extra AST parser for the variable definitions in mutations ASTs.

idank commented 1 year ago

That's understandable. I can stitch things together manually for now. Thanks for releasing this tool!