Skip to content
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

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

Conversation

bc-dima-pasieka
Copy link
Contributor

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":[{},{},{},{},{}, ...]
    }
  }

@yanns I'd appreciate your guidance here on the best way to design it 🙂

@@ -43,7 +44,9 @@ case class Executor[Ctx, Root](
userContext,
exceptionHandler,
scalarMiddleware,
false)(um)
false,
errorsLimit = queryValidator.errorsLimit
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to define errorsLimit only once (e.g. on the queryValidator) and re-use in other places

.flatMap(v =>
def isValidValue[In](tpe: InputType[_], input: Option[In], errors: ListBuffer[Violation])(implicit
um: InputUnmarshaller[In]): Vector[Violation] = {
// TODO: NEED TO ADD VALUES TO `errors` to make it work properly
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code doesn't work correctly because I don't mutate errors buffer, but this is one of the options how it can be implemented

val errors = errorsLimit match {
case Some(limit) => allErrors.take(limit)
case _ => allErrors
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like there are other places where violations could be returned, but I'm not sure if they should be updated as well.

@@ -12,6 +12,7 @@ import scala.reflect.{ClassTag, classTag}

trait QueryValidator {
def validateQuery(schema: Schema[_, _], queryAst: ast.Document): Vector[Violation]
def errorsLimit: Option[Int]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it public so it can be re-used?

@yanns
Copy link
Contributor

yanns commented Jun 30, 2023

Thanks for PR.
I'm not very familiar with this code. What kind of help do you need?

@bc-dima-pasieka
Copy link
Contributor Author

bc-dima-pasieka commented Jun 30, 2023

I'm not very familiar with this code

Got it! I was looking for some guidance on the best way/place to fix it. I guess I will need to dig deeper into the logic then 😃

@bc-dima-pasieka
Copy link
Contributor Author

Closing in favor of #1034

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.

2 participants