Limit number of errors returned (when variables are used) #1034

bc-dima-pasieka · 2023-07-17T19:39:34Z

Follow up to #1017

The solution from the previous PR works only for cases where input is specified (hardcoded) as a part of the query but doesn't work/executed when variables are used. For example, the following query will still return an unlimited number of errors:

mutation createProduct($input: CreateProductInput!) {
  product {
    createProduct(input: $input) {
      ...
    }
  }
}

// variables
{
    "input":{
      "field_with_invalid_objects":[{},{},{},{},{}, ...]
    }
  }

modules/core/src/main/scala/sangria/execution/Executor.scala

bc-dima-pasieka · 2023-07-17T19:45:18Z

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

+      implicit um: InputUnmarshaller[In]): Vector[Violation] = {
+
+    // keeping track of the number of errors
+    var errors = 0


Using mutable variable was the only viable option here to avoid making huge refactoring and keeping the same signature of the original method (by returning Vector[Violation]).

The alternative 1 was to use ListBuffer[Violation] but it would require making signature changes (probably just to return Unit) + it didn't work well with .map(ListValueViolation(0, _, sourceMapper, Nil)).

The alternative 2 was to propagate the number of errors back to the recursion function but I was not able to make it work properly 🙂

bc-dima-pasieka · 2023-07-17T19:46:48Z

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

-        objTpe.fields.toVector.flatMap(f =>
-          isValidValue(f.fieldType, um.getMapValue(valueMap, f.name))
-            .map(MapValueViolation(f.name, _, sourceMapper, Nil)))
+    def isValidValueRec(tpe: InputType[_], in: Option[In])(implicit


I created a separate recursion function (basically the old method migrated here for the most part) to be called internally so it has easy access to mutable variable

bc-dima-pasieka · 2023-07-17T19:47:54Z

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

-        case other => scalar.coerceUserInput(other)
-      }
+          case (objTpe: InputObjectType[_], _) =>
+            addViolation(


So instead of returning Vector(violation), we call addViolation(violation) that mutates number of errors and returns the same Vector(violation)

bc-dima-pasieka · 2023-07-17T19:50:21Z

modules/core/src/main/scala/sangria/execution/ValueCollector.scala

    if (!um.isMapNode(inputVars))
      Failure(
        new ExecutionError(
          s"Variables should be a map-like object, like JSON object. Got: ${um.render(inputVars)}",
          exceptionHandler))
    else {
      val res =
-        definitions.foldLeft(Vector.empty[(String, Either[Vector[Violation], VariableValue])]) {
+        definitions.foldLeft(
+          (0, Vector.empty[(String, Either[Vector[Violation], VariableValue])])) {


I've changed the accumulator's signature from Vector to (Int, Vector) where Int is to keep track of the number of errors.

We need this change in addition to ValueCoercionHelper.scala, so we can limit the number of errors when multiple inputs are provided

bc-dima-pasieka · 2023-07-17T19:51:43Z

modules/core/src/main/scala/sangria/execution/ValueCollector.scala

-              case Left(violations) => acc :+ (varDef.name -> Left(violations))
+                  fromScalarMiddleware,
+                  // calculate the allowed number of errors to be returned (if any)
+                  errorsLimit.map(_ - accErrors)


propagate the remaining number of errors that are allowed to be returned

bc-dima-pasieka · 2023-07-17T19:53:05Z

modules/core/src/main/scala/sangria/validation/QueryValidator.scala

    val ctx = new ValidationContext(
      schema,
      queryAst,
      queryAst.sourceMapper,
      new TypeInfo(schema),
-      errorsLimit)
+      errorsLimitOverride)


It would take precedence over RuleBasedQueryValidator.errorsLimit so we could pass and use a single limit via execute method

bc-dima-pasieka · 2023-07-17T19:55:23Z

@yanns pls take a look when you have time 🙂

yanns · 2023-07-19T10:50:52Z

so that you're not surprised by the delay:
I'll have some time off. I'll start the review again after.

bc-dima-pasieka · 2023-08-09T20:24:04Z

Hey @yanns, just following up in case you are back 🙂

yanns · 2023-08-14T09:44:19Z

Do you think that we should revert part of the initial PR, and go into that direction: bc-dima-pasieka#2 ?

bc-dima-pasieka · 2023-08-23T21:06:04Z

@yanns

Do you think that we should revert part of the initial PR, and go into that direction: bc-dima-pasieka#2 ?

Probably yes, it might play better in the long run. Let me update the PR

bc-dima-pasieka · 2023-08-23T23:23:56Z

modules/core/src/main/scala/sangria/validation/QueryValidator.scala

@@ -11,7 +11,10 @@ import scala.collection.mutable.{ListBuffer, Map => MutableMap, Set => MutableSe
 import scala.reflect.{ClassTag, classTag}

 trait QueryValidator {
-  def validateQuery(schema: Schema[_, _], queryAst: ast.Document): Vector[Violation]
+  def validateQuery(


@yanns based on the suggestion from https://github.com/bc-dima-pasieka/sangria/pull/2/files#diff-f36cd7c72002ebd8d7f5fcc1f1b1026e4f54c67bfe9d7268425ba73ecd8a272bL14 - are we ok introducing a breaking change like this?

This PR introduces quite a lot of important breaking changes.
I'd suggest that we ignore them for now to focus on the feature.
Once we're happy with the result, we can check how to handle the breaking changes.
You can temporary remove the CI step about breaking changes so that we can focus on the tests.

I commented out a few checks. Let me know what would be the best next steps from here. It seems to be "ready" from my end

yanns

This is looking good to me.
You can add the exceptions for the binary compatibility checks:

    mimaBinaryIssueFilters ++= Seq(
      ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.execution.Executor.apply"),
      ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.execution.Executor.copy"),
      ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.execution.Executor.this"),
      ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.execution.Executor.execute"),
      ProblemFilters.exclude[DirectMissingMethodProblem]("sangria.execution.Executor.prepare"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.execution.QueryReducerExecutor.reduceQueryWithoutVariables"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.execution.ValueCoercionHelper.isValidValue"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.execution.ValueCoercionHelper.getVariableValue"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.execution.batch.BatchExecutor.executeBatch"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.schema.ResolverBasedAstSchemaBuilder.validateSchema"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.validation.QueryValidator.validateQuery"),
      ProblemFilters.exclude[ReversedMissingMethodProblem](
        "sangria.validation.QueryValidator.validateQuery"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.validation.RuleBasedQueryValidator.validateQuery"),
      ProblemFilters.exclude[DirectMissingMethodProblem](
        "sangria.validation.ValidationContext.this")
    ),

I've tried to minimize this, but it's a nightmare as the new argument is optional.
The source compatibility is good though.

Can you add those exceptions are enable the binary check again?

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala

yanns

Thanks a lot!

Use secure configuration by default Follow-up of #1034

Limit number of errors returned (when variables are used)

81914d3

bc-dima-pasieka mentioned this pull request Jul 17, 2023

DRAFT / WIP: Limit number of errors returned (when variables are used) #1022

Closed

bc-dima-pasieka commented Jul 17, 2023

View reviewed changes

modules/core/src/main/scala/sangria/execution/Executor.scala Show resolved Hide resolved

bc-dima-pasieka commented Jul 17, 2023

View reviewed changes

yanns self-requested a review July 17, 2023 20:50

Remove QueryValidator changes from the previous release

5deba0a

bc-dima-pasieka commented Aug 23, 2023

View reviewed changes

bc-dima-pasieka added 2 commits August 24, 2023 10:21

Comment out binary compatibility check temporarily

635db88

Comment out workflow up to date check temporarily

a7a9d6a

yanns reviewed Sep 4, 2023

View reviewed changes

modules/core/src/main/scala/sangria/execution/ValueCoercionHelper.scala Outdated Show resolved Hide resolved

bc-dima-pasieka added 2 commits September 6, 2023 14:10

Add the exceptions for the binary compatibility checks

3e5a563

Fix formatting

9e60707

yanns approved these changes Sep 7, 2023

View reviewed changes

yanns added this pull request to the merge queue Sep 7, 2023

Merged via the queue into sangria-graphql:main with commit 80d7e09 Sep 7, 2023
4 checks passed

yanns added a commit that referenced this pull request Sep 10, 2023

use a max number of errors by default

c8155a3

Use secure configuration by default Follow-up of #1034

yanns mentioned this pull request Sep 10, 2023

use a max number of errors by default #1049

Merged

yanns mentioned this pull request Nov 6, 2024

Limit the amount of GraphQL validation errors returned in the response apollographql/router#6187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit number of errors returned (when variables are used) #1034

Limit number of errors returned (when variables are used) #1034

bc-dima-pasieka commented Jul 17, 2023

bc-dima-pasieka Jul 17, 2023 •

edited

Loading

bc-dima-pasieka Jul 17, 2023

bc-dima-pasieka Jul 17, 2023

bc-dima-pasieka Jul 17, 2023 •

edited

Loading

bc-dima-pasieka Jul 17, 2023

bc-dima-pasieka Jul 17, 2023

bc-dima-pasieka commented Jul 17, 2023

yanns commented Jul 19, 2023

bc-dima-pasieka commented Aug 9, 2023

yanns commented Aug 14, 2023

bc-dima-pasieka commented Aug 23, 2023

bc-dima-pasieka Aug 23, 2023 •

edited

Loading

yanns Aug 24, 2023

bc-dima-pasieka Aug 24, 2023

yanns left a comment

yanns left a comment

Limit number of errors returned (when variables are used) #1034

Limit number of errors returned (when variables are used) #1034

Conversation

bc-dima-pasieka commented Jul 17, 2023

bc-dima-pasieka Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023

Choose a reason for hiding this comment

bc-dima-pasieka commented Jul 17, 2023

yanns commented Jul 19, 2023

bc-dima-pasieka commented Aug 9, 2023

yanns commented Aug 14, 2023

bc-dima-pasieka commented Aug 23, 2023

bc-dima-pasieka Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

yanns Aug 24, 2023

Choose a reason for hiding this comment

bc-dima-pasieka Aug 24, 2023

Choose a reason for hiding this comment

yanns left a comment

Choose a reason for hiding this comment

yanns left a comment

Choose a reason for hiding this comment

bc-dima-pasieka Jul 17, 2023 •

edited

Loading

bc-dima-pasieka Jul 17, 2023 •

edited

Loading

bc-dima-pasieka Aug 23, 2023 •

edited

Loading