Skip to content

Handle an array of GraphQL queries #171

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

Merged
merged 6 commits into from
Jun 8, 2018
Merged

Conversation

foxfriends
Copy link
Contributor

Adds support for batched queries, as they are sent by apollo-link-batch-http, and probably some other libraries as well.

They are handled in post requests to the Iron integration, and when parsing from a request body for the Rocket integration.

Added a test for it, and all seems to be working when used in my personal project, both with batching enabled and disabled.

@codecov-io
Copy link

codecov-io commented May 1, 2018

Codecov Report

Merging #171 into master will increase coverage by 0.35%.
The diff coverage is 88.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   90.01%   90.37%   +0.35%     
==========================================
  Files          96       96              
  Lines       18723    18890     +167     
==========================================
+ Hits        16854    17071     +217     
+ Misses       1869     1819      -50
Impacted Files Coverage Δ
juniper_rocket/src/lib.rs 95.28% <82.85%> (+1.87%) ⬆️
juniper_iron/src/lib.rs 76.14% <93.75%> (+8.73%) ⬆️
juniper_codegen/src/derive_object.rs 0% <0%> (ø) ⬆️
juniper/src/http/graphiql.rs 0% <0%> (ø) ⬆️
juniper_codegen/src/derive_input_object.rs 0% <0%> (ø) ⬆️
juniper_codegen/src/derive_enum.rs 0% <0%> (ø) ⬆️
juniper/src/parser/value.rs 96.47% <0%> (+0.04%) ⬆️
juniper/src/schema/model.rs 88.66% <0%> (+0.15%) ⬆️
juniper/src/schema/meta.rs 81.64% <0%> (+0.19%) ⬆️
juniper_codegen/src/lib.rs 4.54% <0%> (+0.19%) ⬆️
... and 5 more

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 7933bf9...a1c53a4. Read the comment docs.

@theduke
Copy link
Member

theduke commented May 2, 2018

Mhm.

This functionality is not in the spec and there isn't even any discussion about it on the repo about it.
Supporting Apollo is important of course, but I feel very ambiguous about integrating this so tightly into Juniper when it's not defined anywhere "official".

@foxfriends
Copy link
Contributor Author

Yeah, fair points. I did question the official-ness too. I could try and make it less tightly integrated (just in the Iron/Rocket integrations), if that would be preferable? Otherwise no worries, I can continue depending on my fork, just thought I'd share :)

@LegNeato
Copy link
Member

LegNeato commented May 4, 2018

How about we have an optional, off by default feature like apollo_extensions or nonstandard?

@LegNeato
Copy link
Member

@OinkIguana did you want to put this behind a flag? Otherwise we should just close.

@foxfriends
Copy link
Contributor Author

Oh yeah, I could do that. Been a bit busy sorry. Will try to do soon!

@LegNeato
Copy link
Member

Thanks! Absolutely no rush. I would imagine we want the flag to have something to do with "Apollo", as they are popular and also often add on features.

@theduke
Copy link
Member

theduke commented May 23, 2018

I totally open to this being a nice wrapper functionality in the iron/rocket crates, and also something like execute_batch on the executor, which takes an array of queries/mutations, but this should be relatively un-invasive IMO and not contain all the type changes.

So without the Executable and response traits you added.

@foxfriends
Copy link
Contributor Author

Should be good to go now. The main juniper crate is now untouched, knowing nothing of the non-standard features. The integrations have the new 'apollo-extensions' feature that can conditionally enable batch support. Tested the Iron integration on my other project, and the feature behaves as intended, and everything is working as before!

@LegNeato
Copy link
Member

LegNeato commented May 24, 2018

Awesome!

Now that the changes aren't in juniper proper I'm not so sure we need the feature flag anymore...the way you have it implemented the flag doesn't really gate much? AFAICT the only thing it is changing is the ability to use GraphQLBatchResponse::Batch, but the rest of the batch code is still compiled in. With the feature off one still needs to use GraphQLBatchResponse to call GraphQLBatchResponse::Single so I am not sure what the benefit is to the flag anymore.

I think gating made sense when we didn't want to put non-standard stuff by default in juniper, but I don't see any downsides to just blindly supporting multiple queries in the integrations....would there ever be a case where a server explicitly didn't want to respond to well-formed multiple queries instead of just handling them? Thoughts?

@theduke should probably weigh in here as well.

@LegNeato
Copy link
Member

LegNeato commented May 24, 2018

Also, it would be great to add this to the integration tests to make sure we do not regress...it looks like we are still only testing the Single path.

@LegNeato
Copy link
Member

LegNeato commented Jun 1, 2018

@theduke how does this look to you now?

@LegNeato LegNeato merged commit e841672 into graphql-rust:master Jun 8, 2018
@LegNeato
Copy link
Member

LegNeato commented Jun 8, 2018

Going to merge this and let @theduke back it out if he wants something different. Thanks so much for the patch and dealing with all the back and forth! 🍻

LegNeato added a commit to LegNeato/graphql-rust.github.io that referenced this pull request Jul 24, 2018
LegNeato added a commit to graphql-rust/graphql-rust.github.io that referenced this pull request Sep 13, 2018
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