Add GraphQLValidationError as a subclass of GraphQLError

[IvanGoncharov]

Also deprecated ValidationContext.reportError in favor of new report
function with named argument.

Motivation: subclassing allows you to distinguish between errors produced by graphql-js.
Idea is to subclass all errors into different types e.g. GraphQLValidationError, GraphQLSyntaxError, etc.
That way clients can be sure what type of error it is.

const { data, errors, extensions } = graphql({ /* ... */ });

const response = {
  data,
  errors.map((error) => {
    if (error instanceof GraphQLValidationError) {
      // do a special handling of validation errors
    } else if (error instanceof GraphQLSyntaxError) {
      // do a special handling of syntax errors
    }
  }),
  extensions,
};

Without this change, you can't distinguish one type of error from other types of errors.
Subclassing errors for that purpose is a standard approach in JS (e.g. TypeError is a subclass of Error) and other OOP languages.

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	306fae9
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6282a252d22711000874c196
😎 Deploy Preview	https://deploy-preview-3593--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

Hi @IvanGoncharov, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[n1ru4l]

Why is this subclass necessary? What value does it provide over the normal GraphQLError class? Can you please update the PR description so people can understand why this change is necessary and what kind of issue it solves? I see a lot of PRs by you without a proper description merged quickly (no reviews from additional people) and I really struggle being able to follow the progress and motivation behind a lot of these changes. I don't think that is healthy for an open-source project that serves as a reference implementation..

[IvanGoncharov]

I see a lot of PRs by you without a proper description merged quickly (no reviews from additional people) and I really struggle being able to follow the progress and motivation behind a lot of these changes.

Most of them related to internal scripts, removing deprecated stuff, dropping old dependencies, or "polishing changes" without API change.
For example, I don't know what additional description should PR have if it drops an already deprecated API, switches some internal scripts to TS, or add support for "mutation testing".

But I agree that providing more context for changes related to public API is necessary.
Thanks for reminding me that I updated the PR description.

[yaacovCR]

@IvanGoncharov thanks for additional context. I think more information is necessary to determine whether a type hierarchy and its maintenance burden for caught errors is necessary.

I understand from abbreviated review that all errors returned by validate are essentially validation errors. Inasmuch as we should be encouraging sophisticated clients to use the individual life cycle methods including parse, validate, execute, isn't the error type self evident from the stage?

What specific use case is desired here? A local subgraph for Apollo Federation is one possible use case where the subgraph does not make clear the stage at which the pipeline failed. But even that seems far fetched as a gateway could ensure proper syntax and the presence of data signifies that execution has begun.

I would reject this change without a compelling use case. I bet there could be one I'm missing, and I'd love to hear more.

[IvanGoncharov]

All implementation of GraphQL server I know provide formatError, and client code doesn't have any means to distinguish errors of different types.
For example, envelope see here: https://github.com/dotansimha/envelop/blob/9449ef36f61d4d9561f88a87204b3a16f3c6f98e/packages/core/src/plugins/use-masked-errors.ts#L13

Apollo Server has custom code to handle that by injecting extensions.code but this requires recreating errors with a bunch of hacks: https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-errors/src/index.ts
The entire file is dedicated to that purpose and is full of hacks.

I don't think it's worth having this hack in every package implementing pipeline, e.g. expess-graphql that also has formatError callback.

Also as a bonus now you will have a more meaningful error name (GraphQLError => GraphQLValidationError) in console/log without changing anything in your code.

P.S. Currently, I'm working on a spec proposal to standardize "error category" field on errors but even if it is accepted we still need to have a way to set it up without passing additional arg to every constructor call.

[IvanGoncharov]

@yaacovCR Also why do we need to force API users to wrap everything step in the pipeline into try/catch if we can have a single try/catch for the entire pipeline.
Making code that interacts with graphql-js easier is the good goal by itself.

[yaacovCR]

All implementation of GraphQL server I know provide formatError, and client code doesn't have any means to distinguish errors of different types.

For example, envelope see here: https://github.com/dotansimha/envelop/blob/9449ef36f61d4d9561f88a87204b3a16f3c6f98e/packages/core/src/plugins/use-masked-errors.ts#L13

I am not familiar with envelop internals but looking further down in that file, it looks like the plug-in determines the error type as expected from the lifecycle stage. It looks like formatError can add some additional pan-error formatting but that the plug-in itself is handling the errors as appropriate based on the stage, which is what I would expect.

Maybe @n1ru4l can add some context there if interested.

Apollo Server has custom code to handle that by injecting extensions.code but this requires recreating errors with a bunch of hacks: https://github.com/apollographql/apollo-server/blob/main/packages/apollo-server-errors/src/index.ts

The entire file is dedicated to that purpose and is full of hacks.

I am not familiar with their error hierarchy, but just from looking at some of the custom error types, it seems more involved than just the lifecycle stages, for better or for worse.

You do not link to Apollo code where parse, validate, execute is called, so I cannot say much more about where Apollo Server loses (or do not lose!) the error context.

I don't think it's worth having this hack in every package implementing pipeline, e.g. expess-graphql that also has formatError callback.

I think envelop and express-graphql use the formatError callback very differently. I don't think either is a "hack" although I am not sure what you mean by that.

Also as a bonus now you will have a more meaningful error name (GraphQLError => GraphQLValidationError) in console/log without changing anything in your code.

I don't understand why that is necessary. Are errors typically logged without the message? These are not thrown errors...

P.S. Currently, I'm working on a spec proposal to standardize "error category" field on errors but even if it is accepted we still need to have a way to set it up without passing additional arg to every constructor call.

Certainly if this becomes part of the spec, it makes sense to implement it within the reference implementation as part of the spec rfc process. In general, errors are probably underspecified and deserves attention. But that makes it all the more puzzling why you would choose to implement it first within the reference implementation and only later bring a spec proposal, especially if the two may clash. Or perhaps this simply is the spec rfc PR and I misunderstood the timeline of when you plan to merge?

[yaacovCR]

@yaacovCR Also why do we need to force API users to wrap everything step in the pipeline into try/catch if we can have a single try/catch for the entire pipeline.

I don’t understand, probably because I never set up the pipeline. Here is some of my confusion, numbered:

1. These errors are mostly returned or caught, I think — only parse throws, right, so only parse needs to be wrapped in try/catch.
2. Sophisticated servers presumably call the individual life cycle methods already, some of which don’t throw, so a global try/catch would not seem to work. [Parenthetically, should parse be changed so it also doesn’t throw? I think status quo is fine.]
3. There is not yet complete consensus on whether some errors are validation errors or execution errors! See discussion at Clarify 'before execution begins' in response graphql-spec#894 (comment).
4. Why is a global try/catch (if it would work) with a switch statement on error type better than multiple points of error handling depending on stage? Is there a reason or feature that makes it better? The latter would be the teeniest bit faster as the error context/stage would not be discarded forcing it to be rechecked.
5. The only benefit I see is that it would allow server users to specify a single formatError function instead of formatValidationError functions, etc, reducing the number of options you would need to pass to server. But it seems prudent to allow this to be solved in user land by having the server framework call a single formatError method with a second option of the stage that triggered the error.

Making code that interacts with graphql-js easier is the good goal by itself.

Agreed! But I think in practice this is more trouble than it is worth. If a spec PR is raised, certainly makes sense to revisit. Or maybe you want to add to discussion at next js-wg as well?

Also, please feel free to get info the weeds and point out the exact code change in Apollo Server or whatever other library that would be unlocked by this PR. So far, I just don’t see a convincing use case that justifies the added complexity.

[n1ru4l]

Regarding envelops useMaskedErrors plugin, @yaacovCR is correct here. We only specifically wrap the execute and subscribe functions (and the buildContext function, which is not a core part of graphql-js), and only mask those types of errors. execute would never raise a GraphQLValidationError, so I think this subclassing is not needed at all. You can just rely on the fact that any GraphQLError returned from validate can be treated as if it was a GraphQLValidationError. In general, you would not want to mask validation or parse errors as the GraphQL API consumers would have a real hard time figuring out what they are doing wrong.

I don't see any strong arguments or use-cases that justify introducing this sub-classing.

A bit off-topic, but I would also highly advise against using the graphql function over parse, validate, and execute/subscribe as it does not allow doing any optimizations such as parse and validation caching, which is a no-go for any production GraphQL server. 🤔

Another off-topic, since the validate and execute/subscribe function return errors as { errors: [GraphQLError] }, would it make sense to alter the parse function to return the error in the same shape? 🤔 Happy to discuss the pros and cons of this in another issue.

[IvanGoncharov]

I propose a simpler solution to this discussion. I plan to present "error categories" (in response) on the next WG. If accepted subclassing errors is a way forward since it's the most ergonomic way to set the error categories.

So after finishing spec PR I will work on a matching graphql-js PR and hopefully will finish it in time before the next WG.

If rejected, I agree that having "graphql-js" specific "error categories" via subclassing doesn't make sense.

A bit off-topic, but I would also highly advise against using the graphql function over parse, validate, and execute/subscribe as it does not allow doing any optimizations such as parse and validation caching, which is a no-go for any production GraphQL server.

I want to change that, but it's a long-term plan and nothing concrete yet.
Having a pluggable pipeline as part of the reference implementation can be very valuable to the entire GraphQL ecosystem even beyond JS.

[yaacovCR]

I propose a simpler solution to this discussion. I plan to present "error categories" (in response) on the next WG. If accepted subclassing errors is a way forward since it's the most ergonomic way to set the error categories.

Any additional specification on errors is long overdue and kudos on tackling it. Looking forward.

[yaacovCR]

Most of them related to internal scripts, removing deprecated stuff, dropping old dependencies, or "polishing changes" without API change. For example, I don't know what additional description should PR have if it drops an already deprecated API, switches some internal scripts to TS, or add support for "mutation testing".

But I agree that providing more context for changes related to public API is necessary. Thanks for reminding me that I updated the PR description.

@IvanGoncharov even non-public API changes should have descriptions that are as detailed as practical. graphql-js is a community-wide open-source project relied on by a large community with many interested contributors, many of whom would happily take larger roles as maintainers.

Changes that seem obvious to you because of your many years of familiarity with the code base are not necessarily straightforward to all of the interested parties following along and hoping to contribute.

Some more numbered thoughts:

1. Even when simply completing pre-planned deprecation, a PR would be more informative to the community with a sentence or two explaining why the bits were deprecated to begin with.
2. Internal tooling in some ways requires even more description than the public API; the public API usually has a counterpart within the spec, and so is sometimes much better documented. The internal tooling is completely sans documentation. Adding descriptions to PRs helps understand the specific code changes, but more often helps interested parties understand the medium if not long-term overall direction of the code base.
3. Just as an example, take the PR adding support for "mutation testing" with Stryker. This change to me was very puzzling. Although experimenting with mutation testing is very interesting, the PR lacks context for understanding how "mutations testing" will be used. Is there a particular coverage target we are hoping for? Will we be rejecting PRs that change this target? Do everyday contributors need to be aware of this new type of testing? What alternatives to Stryker exist? Is the maintenance burden of the additional dev dependency worth it? A few sentences could have been very explanatory, something like "Adding support for mutations testing; Stryker is a popular choice that is easily to configure, open to alternatives, no specific target in mind at this time, but important to understand how sound our code base is, will work when possible on decreasing the number of surviving mutants, no plans at this time to integrate into PR checks."
4. I would suggest that every PR should be required to undergo review. This will help you in the short run, as it will lead to fewer instances when PRs must be reverted, as occurred recently. It will also provide a forcing function to make sure that the quick couple of explanatory sentences for each PR always make it in. More importantly, it will help you in the long-run by fostering a healthier multi-maintainer setup for graphql-js that decreases your workload, so you can work on the open-source parts that you most enjoy.

[IvanGoncharov]

Agree with adding more text to each PR.

But I'm not sure about the review part.
For example what the status of #3596 we commented there back and forth.
Its status is unclear.

This will help you in the short run, as it will lead to fewer instances when PRs must be reverted, as occurred recently.

In the whole multi-year history of this repo, we had only couple of reverts.
Moreover the whole idea of "unstable main" means it is safe to revert stuff until it branched out into release.

It will also provide a forcing function to make sure that the quick couple of explanatory sentences for each PR always make it in.

I agree with that point and promise to specify details on every PR and also answered questions on those PRs (that I already do).

I would suggest that every PR should be required to undergo review.

I proposed a middle ground that aligned with what other projects under https://github.com/graphql/graphiql do, e.g. graphql-spec, graphiql, etc.

Gave a reasonable amount of time for the community to comment on PRs.
Meaning if the change has a significant impact on the client feedback is gathered for longer (especially in the RC phase) if it smaller change then it's merged faster.

I ok to go even further than other projects under "graphql" do, and run an experiment of a minimal 24h before merging for absolutely all PRs to main (it doesn't apply to other branches since it either backport or something experimental). For changes related to the API, I will definitely wait for a longer time to gather more feedback.

It will solve all issues you mentioned above and also address @n1ru4l original comment:

I see a lot of PRs by you without a proper description merged quickly (no reviews from additional people) and I really struggle being able to follow the progress and motivation behind a lot of these changes. I don't think that is healthy for an open-source project that serves as a reference implementation..

Let's see how this experiment will go and if it will not address your concerns we can always discuss it on a graphql-js-wg call.

Also, please note in 'unstable master' model nothing gets released without a long RC phase, the same as we had in during v16.0.0 release.

More importantly, it will help you in the long-run by fostering a healthier multi-maintainer setup for graphql-js that decreases your workload, so you can work on the open-source parts that you most enjoy.

I have to balance my work/life (especially now) and my responsibility to Apollo as my employer. I want to onboard new maintainers but I need to balance everything else I need to do.

I also need to onboard someone from Apollo because otherwise every time something is happing I'm blocking my team and ATM there are progressively increasing probability I will disappear for a very long period of time (compulsory military service).
For him/her I plan to do it in public and in the same manner, I did for @saihaj and Daniel (sadly, not active anymore) that worked for "The Guild" before him so I think it is fair to the community.

Also, please use people from the review team. If you see some PR stuck or someone need help please ping all of them. The entire premise of the "review team" was to distribute onboarding.

P.S. If we doing this for all the PRs, please be clear with your review, if asking a question or have a proposal please comment but If you want to block merge please reject PR otherwise it's not clear to me if you block this PR or not, e.g. #3596 it's not clear if you suggesting another benchmark in addition to one I proposed (which can be done separately) or you thing proposed benchmark should be replaced with lexicographicallySortSchema.

I will answer all the questions before merging but don't want to wait for another round of communication to clarify if it is ready to be merged or not.

Also if you agree with the change please mark with approving otherwise it is also non-clear if the community supports something or not.

[yaacovCR]

There is no review on #3596 because reviews of your PRs are not required. In fact, you explicitly reject that above. I as well am busy and I will not signal explicit approval if you are not blocked by it. That’s why forcing functions are important.

I don’t think you should block your team, but I don’t think that means you should onboard another maintainer from Apollo. You have many interested individuals from all sorts of companies and Apollo is already represented.

I read from the above that you want to move fast. In fact, I sympathize! But others want you to move slower so they have time to process and understand your changes. Others want to move fast when it comes to their changes, after all. You want to carefully review them. I suggest a balance where the same process is applied for all maintainers.

[yaacovCR]

Explicitly: I do not think we should adopt an experimental period with your suggestions above. I think we should equalize how all PRs are treated, from all members of the community and they should all be allowed to merge to unstable main if they have an approving review and no registered dissent.

Other forcing functions that I would suggest: an issue should be started for every PR: could be even multiple PRs for one “polish” issue so that the intent of PRs over the medium term can be tracked.

a roadmap file should be introduced and issues/PRs should be required to correspond to items in the roadmap so long term planning could be clear.

i think you may find it frustrating considering your expertise to introduce these checks because it will slow your ability to make changes. But consider the broader needs of the graphql community, which would benefit from a broader cross section of institutional representation. Also consider the fact that actual public api and hot fixes account for a minority of your changes, such that the community will in my opinion scarcely be slowed, and the fact that adopting balanced consistent standards for all contributors will make this GitHub repository a more welcoming place for open source development which will ensure the health of the community over the long term and its resilience to scary shocks (!!!!!!!!!) to individual personnel.

graphql is a very special open source technology and much of that is because of your own diligent and excellent work! I thank you personally for it. I make the above suggestions because despite the limitation on your personal velocity, I think it will be in the community’s and your own long term interests. It is worth sacrificing some of your own productivity to make the overall project environment healthier.

[IvanGoncharov]

Explicitly: I do not think we should adopt an experimental period with your suggestions above. I think we should equalize how all PRs are treated, from all members of the community and they should all be allowed to merge to unstable main if they have an approving review and no registered dissent.

Ok, it was a measure I was willing to do as a compromise.
So if you disagree I'm not putting "24hr limit" as an official policy for a repo.
That said I will implement your and @n1ru4l suggestions into my personal workflows.
Namely, I will do a mandatory and detailed description of PRs and give the community a reasonable time to feedback on them before merging them.

graphql is a very special open source technology and much of that is because of your own diligent and excellent work! I thank you personally for it. I make the above suggestions because despite the limitation on your personal velocity, I think it will be in the community’s and your own long term interests. It is worth sacrificing some of your own productivity to make the overall project environment healthier.

I don't understand why "improving community/onboarding" should first be done on the most technically challenging project under the GraphQL Foundation.
The guild has full control over https://github.com/graphql/graphql.github.io (nothing get done except for merging update to yoga docs owned by the guild) and https://github.com/graphql/dataloader (no review procedure on every commit being merged and no updates in a month).
There is a bunch of work the long overdue on both projects.
And those projects can be used as a good testing ground for every proposal you made.

In a previous discussion we had, I was named the "bottleneck" but the guild and was pressured to onboard guild maintainers to review the group (with the full merge rights) which I did: https://youtu.be/S-VoLuwW1sM?t=3003

Now it looks like it situation is repeating and I don't want to go through another hour of pressure.

Suggesting stuff is easy but implementing them is hard.
So if you truly want to help the entire GraphQL community I suggest you implement those suggestions that you made on https://github.com/graphql/graphql.github.io to solve the "bottleneck" there since Guild has full control and all technical knowledge there (the whole site was rewritten by the guild).

I think that leading by example, is an instrument here.
So before spending my time on a proposal that looked good on the surface I want to see it working on a similar project but on a smaller scale. And I see https://github.com/graphql/graphql.github.io as an excellent opportunity to do that with similar onboarding and a way lower price of doing mistakes.

That said I listen to your @n1ru4l feedback about the process and suggest real steps based on them. I just don't want "change everything" in one go and evolve our processes over time.

[yaacovCR]

I don’t work for the Guild, although in the spirit of full disclosure, they do support me via GitHub Sponsors. Although their support is appreciated, it does not “pay the bills.” I am very thankful for my professional life outside of tech/GraphQL. I believe the majority of what you wrote above has nothing to do with me. I never volunteered to work on DataLoader or the docs site, for example.

I was quoted by @Urigo at one working group reporting frustration with governance within this repo. That was with regards to the PR stack interest introducing an Executor class that hit its own velocity issues with integrating subscribe fully into execute. I was quoted correctly! I was frustrated, but rather than relitigating why, especially considering that I only recall the vaguest contours of why, maybe it makes sense to try again and see how that goes.

[yaacovCR]

@IvanGoncharov should we make this one a draft as well?

[yaacovCR]

Closing this old PR. Feel free to reopen!