New instrumentation for limiting maximum operations in a batch request by vaibhav-gakhreja · Pull Request #3196 · graphql-java/graphql-java

vaibhav-gakhreja · 2023-04-20T10:04:32Z

The purpose of this PR is to limit the maximum number of batch queries that user is allowed to perform in a single call. That would restrict user from sending too many queries and abusing system resources. This would allow developers to have more accurate rate limiting of their APIs.

For example,
Lets say developers have configured a rate limit on their application like: 10 API calls/second, developers would be expecting that 10 graphQL operations (queries/mutations/subscription) would be executed in one second. But if users use graphQL batching then they may send 100 operations in one request and easily avoid this limit. Using this instrumentation developers can specify max number of batch operations that user is allowed to send in one request.

bbakerman · 2023-04-24T07:14:16Z

src/main/java/graphql/analysis/MaxBatchOperationsInstrumentation.java

+        }
+        if (log.isDebugEnabled()) {
+            log.debug("Request width info: {}", supplied_width);
+        }


Can we remove this debug log - we are moving away from logging in graphql-java (in a general sense) AND value of this log as it stands is low (eg just the wight)

bbakerman · 2023-04-24T07:16:15Z

src/main/java/graphql/analysis/RequestWidthInfo.java

+ * The request width info.
+ */
+@PublicApi
+public class RequestWidthInfo {


Naming is hard.

Can you justify why you called this Request Width - eg is it because the other instrumentations are request depth ?

I dont hate the name - just trying to understand the reasoning behind it

this use case was very similar to query depth instrumentation, so I thought keeping names similar to that would be better.

bbakerman · 2023-04-24T07:18:09Z

Thanks for this PR - its been in an excellent manner with a code style that resembles our other code. We appreciate this.

bbakerman · 2023-04-24T07:26:54Z

I am puzzled by the purpose of this PR. Here is the key code to it

if (!definitions.isEmpty()) {
            for (Definition definition : definitions) {
                OperationDefinition operationDefinition = (OperationDefinition) definition;
                SelectionSet selectionSet = operationDefinition.getSelectionSet();
                if (selectionSet != null && selectionSet.getSelections() != null) {
                    supplied_width += selectionSet.getSelections().size();
                }
            }
        }

That is for every operation in the document, sum the size of the 1st level selection set.

Now the fact is ONLY 1 operation is allowed to be executed. So this as written is confusing. It in theory is summing up selections sets that CANT be executed. They maybe redundant in a query text sense - but they wont be executed

So in your tests you ONLY test for a single operation - which I guess is what you mean by "max batch operation" aka MaxBatchOperationsInstrumentation

The naming here is wrong. An operation is the named query / mutation / subscription not top level fields

 {f1: foo {foo {foo {scalar}}} f2: foo { foo {foo {foo {foo{foo{scalar}}}}}} f3: foo { foo {foo {foo {foo{foo{scalar}}}}}} f4: foo { foo {foo {foo {foo{foo{scalar}}}}}} }

eg the above has many aliased type level fields BUT it has only 1 operation - the implied one.

So really I think this PR is about "max top level fields" - eg your aliased f1 / f2 / f3 top level fields.

Is the idea really to prevent someone using aliasing (onto the same field say) from abusing aliases to create a form of pagination or some such??

As it stands right now I am struggle to understand the purpose behind this instrumentation - eg what it can be be used to stop ?

bbakerman · 2023-04-24T07:28:38Z

src/test/groovy/graphql/analysis/MaxBatchOperationsInstrumentationTest.groovy

+        notThrown(Exception)
+    }
+
+    def "coercing null variables that are marked as non nullable wont blow up early"() {


Whats this test got to do with the code?? That validation works?

Your instrumentation kicks off at beginExecution its a garuteeed position

Since this use case was similar to max query depth instrumentation, i tried to keep everything including the tests similar to it.

As i understand it, this test is verifying that if the input validation fails then it should return errors at the correct place(validation should fail), and the instrumentation code should not interfere with it.

vaibhav-gakhreja · 2023-04-24T10:22:58Z

i added description to this PR to make its purpose clear, sorry for not doing that earlier.

You pointed out that only one operation is allowed to run even if user sends multiple, my code is not interfering with that flow, that behaviour is same as before, flow does not reach my code in that case.

So yes, it does not make sense to loop over all definitions, if you say so I can do the check on first definition in definitions list.

bbakerman · 2023-05-09T05:47:05Z

We want to thank you for making this PR - its well written and follows the code style of graphql-java really well

However have decided not to accept it. We feel its not general purpose enough to be included in graphql-java

As it stands this is a "maximum top level fields" counter.

In theory one could implement this via graphql.analysis.FieldComplexityEnvironment and the graphql.analysis.MaxQueryComplexityInstrumentation. eg count only top level fields and set a complexity score

While this PR is has some value, we dont think its valuable enough in a generic sense to be shipped with graphql-java to everyone.

added instrumentation for limiting maximum operations in a batch request

5ca09e8

bbakerman reviewed Apr 24, 2023

View reviewed changes

removing debug log

7d1fc3c

vaibhav-gakhreja requested a review from bbakerman April 25, 2023 05:25

addressing review comments.

1627887

bbakerman closed this May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New instrumentation for limiting maximum operations in a batch request#3196

New instrumentation for limiting maximum operations in a batch request#3196
vaibhav-gakhreja wants to merge 3 commits intographql-java:masterfrom
vaibhav-gakhreja:max_batch_operations_limit_instrumentation

vaibhav-gakhreja commented Apr 20, 2023 •

edited

Loading

Uh oh!

bbakerman Apr 24, 2023

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Uh oh!

bbakerman Apr 24, 2023

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Uh oh!

bbakerman commented Apr 24, 2023

Uh oh!

bbakerman commented Apr 24, 2023

Uh oh!

bbakerman Apr 24, 2023

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Uh oh!

vaibhav-gakhreja commented Apr 24, 2023

Uh oh!

bbakerman commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

vaibhav-gakhreja commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbakerman Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

bbakerman Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

bbakerman commented Apr 24, 2023

Uh oh!

bbakerman commented Apr 24, 2023

Uh oh!

bbakerman Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

vaibhav-gakhreja Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

vaibhav-gakhreja commented Apr 24, 2023

Uh oh!

bbakerman commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vaibhav-gakhreja commented Apr 20, 2023 •

edited

Loading