Improve default ValidationRule failure behavior

[benasher44]

We were caught by surprise that when using the rules as GraphQL validation rules, the default behavior is to throw the error. At least for Apollo Server, this results in a 500-style crash. I would have expected the error to propagate through the passed graphql.ValidationContext. It's easy enough to write default configuration that does this:

  propagateOnRejection: false,
  onReject: [
    (context, error) => {
      context?.reportError(error);
    },
  ],

As a future breaking change, it might be preferred to replace propagateOnRejection be with something like rejectionPropagationMode: "context" | "throw" | "none", where "context" is the default (results in the appropriate 4xx error, when propagated through the graphql.ValidationContext)

[nullswan]

If you push the error to the context, the query won't be canceled. Instead, it will still be processed and go through the entire flow until it reaches the next step in the GraphQL parser, @benasher44. If that's the expected behavior, then this approach is fine, but it won't prevent the query from consuming resources.

Let me know how I can help further.

[benasher44]

Are you sure? I would imagine this is where server implementations could break from the spec, but looking at this spec (hopefully looking at the right version) (section 6.1.1), only operations that pass all validation rules should be executed.

[benasher44]

Specifically this part makes me hopeful that servers really shouldn't do this:

If validation errors are known, they should be reported in the list of “errors” in the response and the request must fail without execution.

[benasher44]

FWIW, graphql-js's reference implementation seems to honor this: https://github.com/graphql/graphql-js/blob/e15c3ec4dc21d9fd1df34fe9798cadf3bf02c6ea/src/graphql.ts#L122

[nullswan]

FWIW, graphql-js's reference implementation seems to honor this: https://github.com/graphql/graphql-js/blob/e15c3ec4dc21d9fd1df34fe9798cadf3bf02c6ea/src/graphql.ts#L122

Indeed, request cancellation is not supported.

Parse, return errors, Validate, return errors.

But that is too late.
Let's take maxTokens as an example, once you reached n tokens, you want to stop the processing, otherwise, you are vulnerable to CPU overloading.

To be more precise, the problem is the parser in most of overloading cases.

[benasher44]

I see! Yeah in that case, I understand. I think my comment still stands. What you're getting from this package is a ValidationRule, which implies hook-ability into the graphql validation phase. I think it's reasonable to assume that if you have a ValidationRule, it's going to report errors to graphql-js the expected way, when used as one.

In the case of maxTokens (and possibly others), afaik you'll need to take extra care to use the rule before graphql gets to parse the document. In that case, I think throwing makes sense (no longer trying to fit into graphql's validation step), since the implementation will be highly server-specific (i.e. depends on how/where you hook into parsing) (e.g. might attempt to validate max tokens in express before the express json handler decodes the string).

[benasher44]

In any case, improvements to the API here might provide a good way to educate users on the tradeoffs

[nullswan]

I'll be happy to review a merge request about this topic ! 🥳

[github-actions]

This issue is stale because it has been open for 30 days with no activity.

[github-actions]

This issue was closed because it has been inactive for 14 days since being marked as stale.