graphql / graphiql

GraphiQL & the GraphQL LSP Reference Ecosystem for building browser & IDE tools.
MIT License
16.1k stars 1.72k forks source link

Allow to run multiple operations (for query batching) #1635

Open leoloso opened 4 years ago

leoloso commented 4 years ago

Executing multiple queries in a single query is not part of the spec yet, but it has been requested, and several servers already implement it (such as Apollo).

Then, GraphiQL should allow to execute multiple operations against the server. However, when there are multiple operations in the document, the Run button only allows to select either one, but not many of them, or all of them.

Please notice in this screenshot, the 2 operations are expected to be executed together, but that's not possible:

Screenshot 2020-08-07 at 10 46 55 PM

Proposal

Add a configuration option "runMultipleQueries" which, when true, if there is more than 1 operation in the document, also adds entry "Execute all" in the Run button dropdown:

Screenshot 2020-08-07 at 10 50 54 PM
acao commented 4 years ago

@leoloso since we are the official spec reference implementation, then someone who is advancing this spec is free to open a PR to graphiql, however it won't ship with the official GraphiQL preset until it's official spec!

acao commented 4 years ago

actually, i will re-open this to make sure we enable this at a relatively early iteration of plugin scope. this would be a VERY useful feature to help advance, but we are currently focused on proposals at more advanced stages right now.

more good news @leoloso - though we can't support this "officially" yet, and there is no plugin API yet, there are props that would allow you to do this, kinda. you'd still get the dropdown, but clicking any operation name would execute all of them if you want, as you can modify how the outbound http request is handled by GraphiQL

the FetcherParams you get when implementing the fetcher async function prop would allow you to implement this, and whatever request pattern you want. you can even use apollo client. when serializing the body for fetch as we normally do for graphiql, you could just specify operationNames instead of operationName, or no operationName to execute all operations, or however folks are implementing this spec

i wonder though, without implenting this in graphql-js, it may be that a lot of cases would fail validation

leoloso commented 4 years ago

That's great, thanks @acao.

To make it work, I've added a hack to my GraphQL server, where if the operationName is __ALL then it executes all the operations (except the one with name __ALL, of course).

I appreciate your suggestion of removing the operationName to make it work, but I guess that that might confuse the user a bit, since they may still want to run a single query and not all of them, so then that would not be possible anymore.

For instance, this is my GraphiQL working with my hack. You can run __ALL but you can also run GetUserName, the 1st query, and it also works.

I'm eager to see GraphiQL supporting this feature :)

acao commented 4 years ago

how is this supposed to be implemented exactly, I can't seem to find an exact payload example in the spec? thats why i suggested removing operationName, as i was unclear how to implement.

if it's either sending one operationName or all operationNames, then maybe I can add a feature to 1.x where you could override ExecuteButton (currently in 1.0.0 branch), and between that and fetch decide what kind of behaviors and request formats occur. some baked-in logic does expect a single operationName though, so it might have to wait until a 2.0.0 release when we can introduce some bigger breaking changes.

acao commented 4 years ago

IMO, it looks like a really useful feature to have, though i imagine graphql-js validation might have issues with fragments with operations like these?

for example, when you have f1, f2, f3 and q1 { ...f1 }, q2 { ...f2 ...f1 }, q3 { ...f2, ...f3 }, and then chose to execute only q1 and q2, it would consider that invalid, as f3 is an unused fragment. you could work around this in the fetcher, by rewriting the operation to only have the fragment it uses before executing the request, but more ideal would be a PR to graphql-js for that, unless there's already something in the works?

leoloso commented 4 years ago

Well, I actually created this issue as to start exploring for the best solution.

I think that sending operationNames would possibly not work, because it would really complicate the UI for GraphiQL. How do you let the user select which operations to run? Say you have a document with 17 operations, do they need to tick on 17 checkboxes? Looks like too much trouble.

That's why I thought about a simple "Run all". So you either send one specific operation through operationName, or you send some unique name as operationName to mean "all of them". It should use a forbidden character, say *, so GraphiQl can send it, but we can't input that in the editor.

leoloso commented 4 years ago

i imagine graphql-js validation might have issues with fragments with operations like these

You're right about this. But I think this is not a new issue, this should be happening right now. What happens if you have 3 operations in your document, and all of them have a fragment, and hit Run selecting one of them for operationName? The 3 of them will also send their fragments over to the server. But then only one operation is executed... what happens with the other 2 fragments?

How is this tackled currently? Are they validated? Is there an error?

acao commented 4 years ago

ah! i forgot. graphql-js is not what throws this error. i was thinking of vscode-graphql which uses apollo client@2 still. no need to rewrite the op!

acao commented 4 years ago

@leoloso to answer your question more clearly, graphql-js allows you to define as many unused fragments as you want when executing an operation, so they are essentially ignored. it is apollo-client that does not allow unused fragments in an operation

leoloso commented 4 years ago

Hence, I believe you've provided an answer to your own concern:

IMO, it looks like a really useful feature to have, though i imagine graphql-js validation might have issues with fragments with operations like these?

Then, implementing this feature in GraphiQL should not be really a problem, since it doesn't break anything from graphql-js.

And the community from Apollo client can adapt their code to make use of this feature (and I guess they will, since they provide query batching).

acao commented 4 years ago

@leoloso yes, I'm aware, thank you!

acao commented 4 years ago

I will be restructuring the repository here in the next few weeks after finishing up LSP/IDE extension efforts for the latest release, and then you can open a PR!

leoloso commented 4 years ago

Unfortunately I won't be able to open a PR, since JavaScript is not my domain, I'm a PHP developer. I posted this issue because I saw the need as a user of GraphiQL. I hope somebody else can jump in and help.

acao commented 4 years ago

for sure! otherwise, I'll be able to get to this in the next month realistically.

@danielrearden offhand, is this already possible with express-graphql?

danielrearden commented 4 years ago

@acao Nope, express-graphql does not currently support batching

acao commented 4 years ago

@leoloso looking into it some more, and though i think this would be a cool feature, i dont see it in the spec:

http://spec.graphql.org/draft/#sec-Executing-Requests

Let operation be the result of GetOperation(document, operationName).

leoloso commented 4 years ago

@acao Yes, you're right. The foundations to work with are not in the spec yet. The spec proposal change is in https://github.com/graphql/graphql-spec/issues/375:

GetOperations(document, operationNames)

    If operationName is null or empty:
        If document contains exactly one operation.
            Return the Operation contained in the document.
        Otherwise produce a query error requiring operationName.
    Otherwise:
        Let operations be the Operations named operationNames in document.
        If any of the operations are not found, produce a query error.
        Return operations.

Could a workaround that doesn't break the spec be considered? For instance, have operationName already behave as operationNames through some separator in that same string. , would be ideal, but I imagine it breaks the spec, right? If so, maybe _ could assume this role, by adding it in the GraphiQL configuration: operationNameSeparator: "_" and then can execute operationName: someOperation_anotherOperation

It looks ugly, I admit that.

Maybe we can take this conversation to that issue? Maybe it's possible to approve using operationNames even before approving that it is for the Multiple query execution feature? I.e. are there other potential features that could benefit from having operationNames?

leoloso commented 4 years ago

Actually, I can't find in the spec that , can't be part of operationName, even if we know that the operation name will not have a comma in its name.

So then, as a workaround, what do you think of operationName: someOperation,anotherOperation? Or is it too ugly?

acao commented 4 years ago

@leoloso workarounds would be less desirable. better to implement the intended spec. it looks like someone needs to champion this spec! i would suggest adding it to the agenda for the next working group meeting if you'd like to see it advanced further.

ideally, once we have an accepted plugin API RFC, people can provide plugins for earlier stage strawman proposals like these.

as a rule of thumb, I tend to implement Stage 3 draft proposals, using an opt-in approach if necessary.

if someone can make a PR to enable this via an opt-in prop, and use that prop to render an alternative BatchExecuteButton that provides a checklist or just executes all operations in the document, I believe that would be acceptable.

leoloso commented 4 years ago

It sounds great, thanks @acao. I'll keep track of the issue for the spec, contribute to the discussion. I won't be able to champion this issue though (I don't have the expertise needed to know it works with graphql-js).

danielrearden commented 4 years ago

@acao @leoloso This might make more sense as an addition to the GraphQL over HTTP spec instead.

acao commented 4 years ago

@danielrearden agreed, the last comment in the spec proposal said the same thing, however the spec itself does seem to require a single operation name per operation execution. so that part might need to be tweaked. but i think thats more of a discussion for the spec proposal than here.

as far as graphiql itself is concerned, I’d be happy to make this an opt-in capability that is disabled by default, though that means we will need to keep up with any modifications to this spec before it’s adopted

siddharthskulkarni commented 4 years ago

Hey @acao I would like to contribute to this! It would be great if you could brief me about the same. I have read the development and contribution guidelines and have gone through the codebase.

acao commented 4 years ago

that would be fantastic! a PR would be welcome.

let’s make it an opt-in feature using a top-level GraphiQL, and as i mentioned before in this thread, the prop could then determine which execute button component is rendered, or determine how it’s rendered.

the easiest approach would be for it to execute all operations in the editor. if you would like to provide a UX for selecting which operations, then that would be great. the list of chosen operationNames might be good to persist across sessions

siddharthskulkarni commented 4 years ago

Okay thanks, I will start working on the PR.

siddharthskulkarni commented 4 years ago

Hey @acao I have submitted the PR.