profusion / sgqlc

Simple GraphQL Client
https://sgqlc.readthedocs.io/
ISC License
513 stars 85 forks source link

Operation output graphql-core compatible Document AST #231

Closed taylor-cedar closed 1 year ago

taylor-cedar commented 1 year ago

➕ Enhancement Request

Is there any way to output/convert an Operation to a graphql-core Document ast? I use Strawberry and Graphene which use this library

Problem related to enhancement request

Sought-for solution

Considered alternatives

Additional context

barbieri commented 1 year ago

By "document AST" you mean a JSON-like AST? Currently we just output the GraphQL DSL to send to servers...

AFAIU Strawberry and Graphene are servers, so they are expected to receive the GraphQL DSL as query field. Then yes, they will parse.

So I'm wondering what are you trying to achieve, if you want to bypass sending the GraphQL DSL as query or something else.

taylor-cedar commented 1 year ago

Sorry about not being clear. Let me try do describe my problem a bit more.

Graphene and Strawberry have the ability to execute a GraphQL query locally (same process in Python. No network). In order to do this the function needs 2 things: the query and the local Python schema definition (the python code that resolves the query).

See this code for an example in Graphene: https://docs.graphene-python.org/en/latest/execution/execute/#executing-a-query

As you can see it takes a query/mutation as a string. Internally the execute() method uses graphql-core library to parse the query (as a string) and execute it. See this code in graphql-core https://github.com/graphql-python/graphql-core/blob/main/src/graphql/graphql.py#L106

Basically it parses the string into a Document ast here. https://github.com/graphql-python/graphql-core/blob/main/src/graphql/language/ast.py#L445

Basically what I am asking is whether there is any way this library can output a Document above instead of a string. That way I could avoid the string generation and the parsing from a string (slower). Maybe it makes sense for this library to use the same AST classes graphl-core uses for parsing? It should be possible to use the same classes for serialization. Since the 2 big libraries (Stawberry and Graphene) use it, it might make sense. Interested to hear your thoughts.

barbieri commented 1 year ago

the reason we don't use the classes is because we wanted to be zero-deps, but it proved cumbersome for the input types/serialization, then it's used in that case.

However, given your example, one could serialize to their classes by walking sgqlc types/classes. It doesn't need to be in the lib itself, but we could include such helper/sibling file.

BUT, I really think it won't save much performance, since the queries are usually small and parsing is straightforward... what usually takes much more time is the actual resolver execution.

taylor-cedar commented 1 year ago

Thanks for your thoughts @barbieri . Really helpful. I think for now I will just use the string serialization. If it becomes an issue of performance (sounds like resolvers are more of an issue), I will look into creating a 3rd party library to do it.

Since it's already dependency, would you use them today if someone refactored? I don't have time to do it, just wondering.

barbieri commented 1 year ago

no need to be a third party lib, you can provide a patch here, no issues. I'd just not refactor the existing code to generate the AST to then serialize it, as it would slow (and add more memory pressure) to the most common path.

But adding a sibling __to_ast__() would be acceptable.

taylor-cedar commented 1 year ago

@barbieri Makes sense. Yes, I definitely wasn't proposing serializing to AST and then serializing to a string. That would definitely not be a good idea. I was thinking about using the AST classes as an inherited class in the generated code in some way . Not sure it's possible, it was just a thought. Basically there would be no AST serialization because the operation already is an AST.

That may not make sense or work in practice. It was just an idea.

barbieri commented 1 year ago

likely not feasible as the classes are already hugely overloaded, so they can work as a "builder" and then as an "interpreter", creating the selection list, handling the aliases/fragments... in a specific way that is far away from the graphql-core, meant for a different purpose.

taylor-cedar commented 1 year ago

Thanks. Closing for now