Feature add allow batched http requests flag #5778

devkot · 2021-10-03T18:37:41Z

As per #5686, add the ability to opt out of batches queries in a single HTTP request. This defaults to true for backwards compatibility reasons.

This new feature has a few benefits such as performance as the response in a batch would be bounded by the slowest query. It can also be used as an increased security measure like preventing bad actors from using batching to validate 2FA pin codes by using different permutations.

apollo-cla · 2021-10-03T18:37:43Z

@devkot: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

devkot · 2021-10-03T18:47:54Z

packages/apollo-server-core/src/runHttpQuery.ts

@@ -298,6 +300,18 @@ export async function processHTTPRequest<TContext>(
        parseGraphQLRequest(httpRequest.request, requestParams),
      );

+      if (requests.length > 1 && options.allowBatchedHttpRequests === false) {


I couldn't figure out a nice way of setting defaults in the GraphQLServer options so I've left this conditional naive for now - I'd be keen to improve this

I think we should just block the array format entirely, whether or not there are multiple operations.

Let's say you write a client library that for simplicity always sends requests as an array. (I believe the Apollo Client batch HTTP link does this.) It would be better if every attempt to use the batch HTTP link against this server failed rather than only concurrent attempts.

glasser

This looks pretty good but I've got a few changes suggested. Let me know if you need my help doing them.

glasser · 2021-10-08T23:50:40Z

packages/apollo-server-core/src/runHttpQuery.ts

@@ -298,6 +300,18 @@ export async function processHTTPRequest<TContext>(
        parseGraphQLRequest(httpRequest.request, requestParams),
      );

+      if (requests.length > 1 && options.allowBatchedHttpRequests === false) {


I think we should just block the array format entirely, whether or not there are multiple operations.

Let's say you write a client library that for simplicity always sends requests as an array. (I believe the Apollo Client batch HTTP link does this.) It would be better if every attempt to use the batch HTTP link against this server failed rather than only concurrent attempts.

glasser · 2021-10-08T23:51:30Z

packages/apollo-server-core/src/runHttpQuery.ts

@@ -298,6 +300,18 @@ export async function processHTTPRequest<TContext>(
        parseGraphQLRequest(httpRequest.request, requestParams),
      );

+      if (requests.length > 1 && options.allowBatchedHttpRequests === false) {
+        return throwHttpGraphQLError(
+          500,


This is a client error like invalid JSON and should probably be 400, not a server error.

glasser · 2021-10-08T23:52:11Z

packages/apollo-server-core/src/runHttpQuery.ts

+          500,
+          [
+            new Error(
+              'GraphQL Query Batching is not allowed by Apollo Server, but the request contained multiple queries.',


This makes it sound like the AS project does not support batching, rather than the configuration. I would suggest something like "Operation batching disabled."

glasser · 2021-10-08T23:52:42Z

packages/apollo-server-integration-testsuite/src/index.ts

+              operationName: 'testX',
+            },
+          ]);
+        return req.then((res) => {


The test is already async so you can just const res = await request(app)....

glasser · 2021-10-08T23:53:26Z

packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts

+      request: new MockReq(),
+    };
+
+    it('succeeds when there are multiple queries in the request', async () => {


test name is inaccurate (though I'm also suggesting you change the logic)

glasser · 2021-10-08T23:53:56Z

packages/apollo-server-core/src/__tests__/runHttpQuery.test.ts

+      options: {
+        debug: false,
+        schema,
+        schemaHash: generateSchemaHash(schema),


Please rebase your PR and replace this line with schemaHash: 'deprecated' (I just changed the thing this was copied from).

glasser · 2021-10-08T23:54:42Z

docs/source/api/apollo-server.md

@@ -403,6 +403,16 @@ If this is set to any string value, use that value instead of the environment va
 </td>
 </tr>

+##### `allowBatchedHttpRequests`


This should be moved to the "Networking options" section from the "Debugging options" section.

glasser · 2021-10-08T23:55:41Z

docs/source/api/apollo-server.md

+</td>
+<td>
+
+Controls whether to allow batching multiple Queries in a single HTTP Request. Defaults to `true`. If the GraphQL Server has this flag set to `false` and a request comes in with more than one query, an error will be thrown.


You can link to https://www.apollographql.com/docs/apollo-server/requests/#batching (as ../requests/#batching) and probably mention that option on that page too.

devkot · 2021-10-12T22:13:31Z

This looks pretty good but I've got a few changes suggested. Let me know if you need my help doing them.

Thanks for the great feedback. It looks sensible and pretty straightforward - I'll let you know when it's ready for another review 😄

devkot · 2021-10-19T14:28:02Z

That's all the changes now made @glasser. Feel free to have another look whenever you're free 😁

Allow specifying allowBatchedHttpRequests in the ApolloServer constructor which will be used to disable requests that batch multiple queries in a single request.

Using the allowBatchedHttpRequests coming from the GraphQLServerOptions to limit requests to a single query per request. This defaults to True for backwards-compatibility reasons.

* Implement requested changes and update docs

glasser · 2021-11-02T23:40:40Z

packages/apollo-server-core/src/runHttpQuery.ts

@@ -298,6 +300,14 @@ export async function processHTTPRequest<TContext>(
        parseGraphQLRequest(httpRequest.request, requestParams),
      );

+      if (options.allowBatchedHttpRequests === false) {


I'm going to move this up a few lines — no use parsing the request if we're going to immediately throw.

glasser · 2021-11-02T23:53:24Z

Thanks @devkot! This should get released this week.

glasser · 2021-11-05T17:58:14Z

This is released in v3.5.0.

devkot force-pushed the feature_add_allowBatchedHttpRequests_flag branch from 38bbdab to 1c8176d Compare October 3, 2021 18:41

devkot commented Oct 3, 2021

View reviewed changes

glasser self-assigned this Oct 5, 2021

glasser added the 2021-10 label Oct 5, 2021

glasser requested changes Oct 8, 2021

View reviewed changes

devkot force-pushed the feature_add_allowBatchedHttpRequests_flag branch from 1c8176d to bf3bf21 Compare October 12, 2021 21:15

devkot force-pushed the feature_add_allowBatchedHttpRequests_flag branch from 53c868b to d14b37f Compare October 16, 2021 17:35

devkot requested a review from glasser October 16, 2021 17:50

devkot force-pushed the feature_add_allowBatchedHttpRequests_flag branch from d14b37f to 785528a Compare October 27, 2021 21:22

hwillson added 2021-11 and removed 2021-10 labels Nov 1, 2021

Stelios Kotanidis and others added 5 commits November 2, 2021 16:38

Add allowBatchedHttpRequests to the GraphQLServerOptions

ab79d18

Allow specifying allowBatchedHttpRequests in the ApolloServer constructor which will be used to disable requests that batch multiple queries in a single request.

Prevent batched queries in single request when specified

b5d8425

Using the allowBatchedHttpRequests coming from the GraphQLServerOptions to limit requests to a single query per request. This defaults to True for backwards-compatibility reasons.

Document allowBatchedHttpRequests

ae7331a

Disable any batch size when allowBatchedHttpRequests is disabled

5ca0feb

* Implement requested changes and update docs

CHANGELOG update

e0139ab

glasser force-pushed the feature_add_allowBatchedHttpRequests_flag branch from 785528a to e0139ab Compare November 2, 2021 23:39

glasser reviewed Nov 2, 2021

View reviewed changes

Tweaks from code review

14d1541

glasser merged commit 3b175dd into apollographql:main Nov 2, 2021

glasser mentioned this pull request Nov 2, 2021

Ability to disable batch request processing #5686

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature add allow batched http requests flag #5778

Feature add allow batched http requests flag #5778

devkot commented Oct 3, 2021

apollo-cla commented Oct 3, 2021

devkot Oct 3, 2021

glasser Oct 8, 2021

glasser left a comment

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

glasser Oct 8, 2021

devkot commented Oct 12, 2021

devkot commented Oct 19, 2021

glasser Nov 2, 2021

glasser commented Nov 2, 2021

glasser commented Nov 5, 2021

Feature add allow batched http requests flag #5778

Feature add allow batched http requests flag #5778

Conversation

devkot commented Oct 3, 2021

apollo-cla commented Oct 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devkot commented Oct 12, 2021

devkot commented Oct 19, 2021

Choose a reason for hiding this comment

glasser commented Nov 2, 2021

glasser commented Nov 5, 2021