-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add INTROSPECTION_DISABLED error enum, set it on validation errors #7035
Add INTROSPECTION_DISABLED error enum, set it on validation errors #7035
Conversation
@barryhagan: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
✅ Deploy Preview for apollo-server-docs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit be1bb93:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This otherwise looks pretty good. I want to check in with @IvanGoncharov to hear what he thinks about the likelihood that this feature will be supported natively in graphql-js though.
@@ -35,7 +35,8 @@ export class ValidationError extends GraphQLErrorWithCode { | |||
constructor(graphqlError: GraphQLError) { | |||
super( | |||
graphqlError.message, | |||
ApolloServerErrorCode.GRAPHQL_VALIDATION_FAILED, | |||
(graphqlError.extensions?.code as ApolloServerErrorCode) ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be simpler to just not use GraphQLErrorWithCode here and put this in the extensions
below. (You would need to add the this.name = this.constructor.name
or this.name = 'ValidationError'
though; perhaps we should have a NamedGraphQLError
superclass that just does this? This is really only for convenience when the errors are viewed in JS rather than serialized as GraphQL.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made that change, it reads better to not extend GraphQLErrorWithCode.
Updated the test to validate the plugin-observed error matches what is serialized to the client.
Yeah, unfortunately if the validation happens natively in graphql-js, then I'm back to string matching on the message. But I guess this change still allows a custom validation rule to set the error.extensions.code as they see fit (assuming that is desirable). |
Upon further thought and chatting with @IvanGoncharov I think it makes sense to keep all errors that occur during the validation phase as having the same What do you think about just adding an |
(the NodeJS 14 failure is a flaky test that we should fix, sorry) |
d315a1a
to
3b7acb1
Compare
I updated the PR to go the I'm ok either way on this merging or being closed. I can always make a duplicate custom rule that sets the extension value, or go back to string matching the GraphQLError message. |
Thanks @barryhagan, this looks great! I tweaked the changeset description a bit. |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-integration-testsuite@4.0.2 ### Patch Changes - [#7035](#7035) [`b3f400063`](b3f4000) Thanks [@barryhagan](https://github.com/barryhagan)! - Errors resulting from an attempt to use introspection when it is not enabled now have an additional `validationErrorCode: 'INTROSPECTION_DISABLED'` extension; this value is part of a new enum `ApolloServerValidationErrorCode` exported from `@apollo/server/errors`. - [#7066](#7066) [`f11d55a83`](f11d55a) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add a test to validate error message and code for invalid operation names via GET - [#7055](#7055) [`d0d8f4be7`](d0d8f4b) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Fix build configuration issue and align on CJS correctly - Updated dependencies \[[`b3f400063`](b3f4000)]: - @apollo/server@4.0.2 ## @apollo/server@4.0.2 ### Patch Changes - [#7035](#7035) [`b3f400063`](b3f4000) Thanks [@barryhagan](https://github.com/barryhagan)! - Errors resulting from an attempt to use introspection when it is not enabled now have an additional `validationErrorCode: 'INTROSPECTION_DISABLED'` extension; this value is part of a new enum `ApolloServerValidationErrorCode` exported from `@apollo/server/errors`. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This PR implements the feature discussed in #7033.
A new error enum
ApolloServerErrorCode.INTROSPECTION_DISABLED
was added and used when creating an introspection validation error. This will allow better handling of this specific error condition for observers ofdidEncounterErrors
events.I intend to use this to suppress logging of these errors when someone attempts to point a studio sandbox at my production servers and leaves schema auto-update on. :|
To make this work with the existing code, it is now possible to set any custom GraphQLError.extensions.code when calling
ValidationContext.reportError()
. Someone can now wire through their own validation error codes and see them on thedidEncounterErrors
events instead of them all being masked as GRAPHQL_VALIDATION_FAILED (which is really the root reason for this PR).Looking for feedback if you want to use the suspect typescript cast in the
ValidationError
constructor:or change the
GraphQLErrorWithCode
constructor signature to use: