Implement Query Batching for apollo-runtime

[joaquim-verges]

Background

Hello Apollo team!

We, the Twitch Android team, have been using Apollo Android for 3+ years now, and have been very happy with it!

However, we recently ran into scalability issues in our app. Our most busy screen has exploded in features, where each feature is encapsulated and does its own GQL query. We've reached a point where that screen does 45 GQL queries (and therefore 45 HTTP calls) when it comes on screen 🤯 This is not only costly for the client, but also very costly for our backend services.

We explored grouping queries into larger ones, but we like the modularity and encapsulation that we have right now where each feature can do their fetching independently. We also know that our web team (who also uses Apollo) solved this problem by enabling batching for queries in the web Apollo client (and that our server supports it).

We decided to fork the Android library to implement the ability to batch multiple queries in a single HTTP call, similar to what the web client offers. We're about to test our implementation in production, and should will gather data in the next few weeks.

In the meantime, we thought it would be useful to push our implementation upstream and give back to the Apollo community!

Implementation

We followed the approach that the web Apollo client took, where batching is achieved by a "poller" that checks periodically for queries to be batched, packages them into a single request body and sends them to the server. On response, the response body is parsed to extract each sub-response and forwarded back to the corresponding callers.

The implementation below is the minimal one that we needed to get that approach working, and we're looking for early feedback before writing unit tests, samples and later port it to the apollo-runtime-kotlin artifact.

Let us know what you think!

[joaquim-verges]

similarly, adding the HTTP cache headers felt weird since a batch can potentially be different between 2 runs, as the queries that will be batched is timing dependent. I guess one could get lucky and get the exact same batch (maybe for single query screens), in which case HTTP caching could come into play. Let us know what you think.

[martinbonnin]

Agreed, it doesn't make a lot of sense to have HTTP cache + query batching. Ideally we could detect that HTTP + batching is used and throw an error. Maybe based on httpCachePolicy ?

[martinbonnin]

And I'd say we need same kind of thing with AutoPersistedQueries too.

edit: and File Upload too...😅. Well for these last two we can rely on users not trying to activate them at the same time as batching. I think that's a fair expectation to have.

[joaquim-verges]

we actually use APQ with batching and that works really well :)

Yeah I'll do the same for file uploads, throw an exception if used with batching with a clear message

[martinbonnin]

we actually use APQ with batching and that works really well :)

Nice. Good to know 👍

Yeah I'll do the same for file uploads, throw an exception if used with batching with a clear message

File Uploads are a bit weird and work with reflexion at the moment. It might be a bit awkward to detect when it's used. If it becomes too convoluted, I think it's fine to leave that as is.

[joaquim-verges]

actually, since we're only enabling batching for queries and not mutations, is file upload even possible with queries? (never used it so not sure)

[joaquim-verges]

and looking at http caching, since you can make non-batched queries with the same ApolloClient, would be annoying to have to turn it off entirely if you still want your single queries to take advantage of it.

I'm thinking of just not adding the http cache headers for batch calls, and just silently ignore it. Not sure if I should log something to let the caller know?

[martinbonnin]

is file upload even possible with queries?

In theory I think yes.. I don't see anything preventing someone to pass an Upload custom scalar as a field argument. For an example a service that could sign your files using a server-side private key:

query GetSignature($file: Upload) {
  signature(file: $file)
}

In practice, I've never seen anyone do that yet though...

we're only enabling batching for queries and not mutations

Would it make sense to enable batching for mutations? I don't think a theoretical reason to limit to queries?

looking at http caching, since you can make non-batched queries with the same ApolloClient, would be annoying to have to turn it off entirely if you still want your single queries to take advantage of it.

Right , makes sense 👍

I'm thinking of just not adding the http cache headers for batch calls, and just silently ignore it. Not sure if I should log something to let the caller know?

Could you do something per-query?

    if (batchPoller != null) {
      if (httpCachePolicy != null) {
        throw new IllegalArgumentException("Using httpCachePolicy and batching at the same time does not make sense");
      }
      interceptors.add(new ApolloBatchingInterceptor(batchPoller));

[martinbonnin]

Awesome contribution 😃 ! That'll make a lot of users happy and close the oldest open issue in this repo 🎉 .

A few comments but looks good overall 👍

PS: I just pushed the metalava signatures used to enforce API compatibility. You can regenerate them with

# run with Java8
./gradlew assemble metalavaGenerateSignature --console=plain

[joaquim-verges]

Adding some unit tests next.

[joaquim-verges]

@martinbonnin feeling pretty good about the latest implementation and tests, let me know what you think!

[martinbonnin]

Thanks for adding more tests! A couple of remaining comments:

• The testMultipleQueryBatchingError failed, I guess on timing issues. You can use a RxJava test subscriber (like here) or more recent tests use coroutines (like here) for synchronization.
• API: What about ApolloQueryCall.canBeBatched(boolean canBeBatched) instead of ApolloClient.batchQuery. This would be more consistent with the other per-query parameters like cacheHeaders, responseFetcher, etc..
• We did not settle on batching mutations. Just curious if you had any thoughts on that. I think it's ok to not include it in the first version as it can always be added later.

[joaquim-verges]

The testMultipleQueryBatchingError failed, I guess on timing issues. You can use a RxJava test subscriber (like here) or more recent tests use coroutines (like here) for synchronization.

Yeah I hesitated between controlling the time or testing the automatic trigger, I'll switch it to a more controlled test

API: What about ApolloQueryCall.canBeBatched(boolean canBeBatched) instead of ApolloClient.batchQuery. This would be more consistent with the other per-query parameters like cacheHeaders, responseFetcher, etc..

Great suggestion, should have thought of it earlier!

We did not settle on batching mutations. Just curious if you had any thoughts on that. I think it's ok to not include it in the first version as it can always be added later.

Still torn on that, it should work fine I think but since mutations are usually slower and more isolated one-offs in general, it feels a bit weird to mix them with queries? But yeah as you mentioned, can easily be enabled later if needed.

[joaquim-verges]

@martinbonnin should I add canBeBatched only to ApolloQueryCall.Builder ? or also add one ApolloQueryCall directly ? I see a bunch of them are deprecated in favor of the builder one?

[martinbonnin]

@martinbonnin should I add canBeBatched only to ApolloQueryCall.Builder ? or also add one ApolloQueryCall directly ? I see a bunch of them are deprecated in favor of the builder one?

Yup, you're right 👍 . ApolloQueryCall.Builder is the way to go now because of this. It's more verbose than I'd ideally like to and 3.0 will change that but for consistency let's put everything in ApolloQueryCall.Builder in main.

[joaquim-verges]

running metalava locally is changing way more signatures that it should for some reasons, mind running it for me?

[martinbonnin]

running metalava locally is changing way more signatures that it should for some reasons, mind running it for me?

Done. Not sure why but metalava signatures depend on the local JVM. I have to use JDK8 to get signatures matching the CI.

[martinbonnin]

LGTM!

[martinbonnin]

Thanks @joaquim-verges for the awesome contribution!

[joaquim-verges]

Glad I could help!