From e824fbbba570e0d6c1e76128eeeaf62c0c90c1c8 Mon Sep 17 00:00:00 2001 From: Ben Date: Thu, 3 Mar 2022 12:31:01 -0600 Subject: [PATCH 1/5] Aggregate errors with @inaccessible Previously, `removeInaccessibleElements` would throw an error the first time it encountered an inaccessible mismatch for types & fields. With this change, the code will continue to parse the schema and report an `AggregateError` that contains all the mismatches. --- .../removeInaccessibleElements.test.ts | 58 +++++++++++++++++-- internals-js/src/inaccessibleSpec.ts | 9 ++- tsconfig.base.json | 2 +- tsconfig.test.base.json | 2 +- 4 files changed, 62 insertions(+), 9 deletions(-) diff --git a/internals-js/src/__tests__/removeInaccessibleElements.test.ts b/internals-js/src/__tests__/removeInaccessibleElements.test.ts index b683c78f4..1a72e6cf5 100644 --- a/internals-js/src/__tests__/removeInaccessibleElements.test.ts +++ b/internals-js/src/__tests__/removeInaccessibleElements.test.ts @@ -178,11 +178,59 @@ describe('removeInaccessibleElements', () => { union Bar = Foo `); - expect(() => { - removeInaccessibleElements(schema); - }).toThrow( - `Field Query.fooField returns an @inaccessible type without being marked @inaccessible itself`, - ); + try { + // Assert that an AggregateError is thrown, then allow the error to be caught for further validation + expect(removeInaccessibleElements(schema)).toThrow(AggregateError); + } catch (err) { + expect(err.errors).toHaveLength(1); + expect(err.errors[0].toString()).toMatch( + `Field Query.fooField returns an @inaccessible type without being marked @inaccessible itself.` + ); + } + }); + + it(`throws an AggregateError when there are multiple problems`, () => { + const schema = buildSchema(` + directive @core(feature: String!, as: String, for: core__Purpose) repeatable on SCHEMA + + directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION + + schema + @core(feature: "https://specs.apollo.dev/core/v0.2") + @core(feature: "https://specs.apollo.dev/inaccessible/v0.1") + { + query: Query + } + + enum core__Purpose { + EXECUTION + SECURITY + } + + type Query { + fooField: Foo + fooderationField: Foo + } + + type Foo @inaccessible { + someField: String + } + + union Bar = Foo + `); + + try { + // Assert that an AggregateError is thrown, then allow the error to be caught for further validation + expect(removeInaccessibleElements(schema)).toThrow(AggregateError); + } catch (err) { + expect(err.errors).toHaveLength(2); + expect(err.errors[0].toString()).toMatch( + `Field Query.fooField returns an @inaccessible type without being marked @inaccessible itself.` + ); + expect(err.errors[1].toString()).toMatch( + `Field Query.fooderationField returns an @inaccessible type without being marked @inaccessible itself.` + ); + } }); it(`removes @inaccessible query root type`, () => { diff --git a/internals-js/src/inaccessibleSpec.ts b/internals-js/src/inaccessibleSpec.ts index 190d95a90..200f28a88 100644 --- a/internals-js/src/inaccessibleSpec.ts +++ b/internals-js/src/inaccessibleSpec.ts @@ -56,6 +56,7 @@ export function removeInaccessibleElements(schema: Schema) { ); } + const errors = []; for (const type of schema.types()) { // @inaccessible can only be on composite types. if (!isCompositeType(type)) { @@ -69,10 +70,10 @@ export function removeInaccessibleElements(schema: Schema) { // field type will be `undefined`). if (reference.kind === 'FieldDefinition') { if (!reference.hasAppliedDirective(inaccessibleDirective)) { - throw new GraphQLError( + errors.push(new GraphQLError( `Field ${reference.coordinate} returns an @inaccessible type without being marked @inaccessible itself.`, reference.sourceAST, - ); + )); } } // Other references can be: @@ -85,4 +86,8 @@ export function removeInaccessibleElements(schema: Schema) { toRemove.forEach(f => f.remove()); } } + + if (errors.length) { + throw new AggregateError(errors) + } } diff --git a/tsconfig.base.json b/tsconfig.base.json index 68e253b8b..58c2f7564 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -17,7 +17,7 @@ "noUnusedLocals": true, "useUnknownInCatchVariables": false, "forceConsistentCasingInFileNames": true, - "lib": ["es2019", "esnext.asynciterable"], + "lib": ["es2019", "esnext.asynciterable", "es2021.promise"], "types": ["node"], "baseUrl": ".", "paths": { diff --git a/tsconfig.test.base.json b/tsconfig.test.base.json index a46ef5df0..7ce32c3a8 100644 --- a/tsconfig.test.base.json +++ b/tsconfig.test.base.json @@ -2,7 +2,7 @@ "extends": "./tsconfig.base", "compilerOptions": { "noEmit": true, - "lib": ["es2019", "esnext.asynciterable", "dom"], + "lib": ["es2019", "esnext.asynciterable", "dom", "es2021.promise"], "types": ["node", "jest"], } } From 17ba72205afd11d06b71c37f9c24f42dc371ceaf Mon Sep 17 00:00:00 2001 From: Ben Date: Wed, 9 Mar 2022 16:52:58 +0100 Subject: [PATCH 2/5] Throw a ErrGraphQLValidationFailed error instead of AggregateError `ErrGraphQLValidationFailed` supports multiple errors and node12 & node14 don't support es2019 (which is when `AggregateError` was introduced). So rather than monkeying with node versions and polyfills, we'll just use what's already available. --- .../removeInaccessibleElements.test.ts | 23 +++++++++++-------- internals-js/src/inaccessibleSpec.ts | 3 ++- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/internals-js/src/__tests__/removeInaccessibleElements.test.ts b/internals-js/src/__tests__/removeInaccessibleElements.test.ts index 1a72e6cf5..fadb49087 100644 --- a/internals-js/src/__tests__/removeInaccessibleElements.test.ts +++ b/internals-js/src/__tests__/removeInaccessibleElements.test.ts @@ -1,4 +1,5 @@ -import { ObjectType } from '../definitions'; +import { GraphQLErrorExt } from "@apollo/core-schema/dist/error"; +import { errorCauses, ObjectType } from '../definitions'; import { buildSchema } from '../buildSchema'; import { removeInaccessibleElements } from '../inaccessibleSpec'; @@ -180,16 +181,18 @@ describe('removeInaccessibleElements', () => { try { // Assert that an AggregateError is thrown, then allow the error to be caught for further validation - expect(removeInaccessibleElements(schema)).toThrow(AggregateError); + expect(removeInaccessibleElements(schema)).toThrow(GraphQLErrorExt); } catch (err) { - expect(err.errors).toHaveLength(1); - expect(err.errors[0].toString()).toMatch( + const causes = errorCauses(err); + expect(causes).toBeTruthy(); + expect(err.causes).toHaveLength(1); + expect(err.causes[0].toString()).toMatch( `Field Query.fooField returns an @inaccessible type without being marked @inaccessible itself.` ); } }); - it(`throws an AggregateError when there are multiple problems`, () => { + it(`throws when there are multiple problems`, () => { const schema = buildSchema(` directive @core(feature: String!, as: String, for: core__Purpose) repeatable on SCHEMA @@ -221,13 +224,15 @@ describe('removeInaccessibleElements', () => { try { // Assert that an AggregateError is thrown, then allow the error to be caught for further validation - expect(removeInaccessibleElements(schema)).toThrow(AggregateError); + expect(removeInaccessibleElements(schema)).toThrow(GraphQLErrorExt); } catch (err) { - expect(err.errors).toHaveLength(2); - expect(err.errors[0].toString()).toMatch( + const causes = errorCauses(err); + expect(causes).toBeTruthy(); + expect(err.causes).toHaveLength(2); + expect(err.causes[0].toString()).toMatch( `Field Query.fooField returns an @inaccessible type without being marked @inaccessible itself.` ); - expect(err.errors[1].toString()).toMatch( + expect(err.causes[1].toString()).toMatch( `Field Query.fooderationField returns an @inaccessible type without being marked @inaccessible itself.` ); } diff --git a/internals-js/src/inaccessibleSpec.ts b/internals-js/src/inaccessibleSpec.ts index 200f28a88..9d655b2f7 100644 --- a/internals-js/src/inaccessibleSpec.ts +++ b/internals-js/src/inaccessibleSpec.ts @@ -1,6 +1,7 @@ import { FeatureDefinition, FeatureDefinitions, FeatureUrl, FeatureVersion } from "./coreSpec"; import { DirectiveDefinition, + ErrGraphQLValidationFailed, FieldDefinition, isCompositeType, isInterfaceType, @@ -88,6 +89,6 @@ export function removeInaccessibleElements(schema: Schema) { } if (errors.length) { - throw new AggregateError(errors) + throw ErrGraphQLValidationFailed(errors) } } From e538f7058c819b36b14cce406cc60b67e5474ce3 Mon Sep 17 00:00:00 2001 From: Ben Date: Wed, 9 Mar 2022 16:55:29 +0100 Subject: [PATCH 3/5] Don't add es2021.promise to build --- tsconfig.base.json | 2 +- tsconfig.test.base.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tsconfig.base.json b/tsconfig.base.json index 58c2f7564..68e253b8b 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -17,7 +17,7 @@ "noUnusedLocals": true, "useUnknownInCatchVariables": false, "forceConsistentCasingInFileNames": true, - "lib": ["es2019", "esnext.asynciterable", "es2021.promise"], + "lib": ["es2019", "esnext.asynciterable"], "types": ["node"], "baseUrl": ".", "paths": { diff --git a/tsconfig.test.base.json b/tsconfig.test.base.json index 7ce32c3a8..a46ef5df0 100644 --- a/tsconfig.test.base.json +++ b/tsconfig.test.base.json @@ -2,7 +2,7 @@ "extends": "./tsconfig.base", "compilerOptions": { "noEmit": true, - "lib": ["es2019", "esnext.asynciterable", "dom", "es2021.promise"], + "lib": ["es2019", "esnext.asynciterable", "dom"], "types": ["node", "jest"], } } From f533823fbd37a20c5fa9f34e39e3b1dbb0cc9372 Mon Sep 17 00:00:00 2001 From: Ben Date: Wed, 23 Mar 2022 22:47:37 -0700 Subject: [PATCH 4/5] Add better message for top-level `ErrGraphQLValidationFailed` --- internals-js/src/definitions.ts | 5 +++-- internals-js/src/inaccessibleSpec.ts | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/internals-js/src/definitions.ts b/internals-js/src/definitions.ts index 6ae46770d..64d4dac26 100644 --- a/internals-js/src/definitions.ts +++ b/internals-js/src/definitions.ts @@ -43,10 +43,11 @@ import { didYouMean, suggestionList } from "./suggestions"; import { withModifiedErrorMessage } from "./error"; const validationErrorCode = 'GraphQLValidationFailed'; +const DEFAULT_VALIDATION_ERROR_MESSAGE = 'The schema is not a valid GraphQL schema.'; -export const ErrGraphQLValidationFailed = (causes: GraphQLError[]) => +export const ErrGraphQLValidationFailed = (causes: GraphQLError[], message: string = DEFAULT_VALIDATION_ERROR_MESSAGE) => err(validationErrorCode, { - message: 'The schema is not a valid GraphQL schema', + message, causes }); diff --git a/internals-js/src/inaccessibleSpec.ts b/internals-js/src/inaccessibleSpec.ts index 38d68fb77..cadf3c005 100644 --- a/internals-js/src/inaccessibleSpec.ts +++ b/internals-js/src/inaccessibleSpec.ts @@ -95,7 +95,7 @@ export function removeInaccessibleElements(schema: Schema) { } } - if (errors.length) { - throw ErrGraphQLValidationFailed(errors) + if (errors.length > 0) { + throw ErrGraphQLValidationFailed(errors, 'Schema has invalid usages of the the @inaccessible directive.'); } } From 5cb1d6c703389425f83f513b172dc8cf3969cb4c Mon Sep 17 00:00:00 2001 From: Ben Date: Fri, 25 Mar 2022 08:40:32 -0700 Subject: [PATCH 5/5] Fix wording for invalid use of inaccessible --- internals-js/src/inaccessibleSpec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internals-js/src/inaccessibleSpec.ts b/internals-js/src/inaccessibleSpec.ts index cadf3c005..e35145ec6 100644 --- a/internals-js/src/inaccessibleSpec.ts +++ b/internals-js/src/inaccessibleSpec.ts @@ -96,6 +96,6 @@ export function removeInaccessibleElements(schema: Schema) { } if (errors.length > 0) { - throw ErrGraphQLValidationFailed(errors, 'Schema has invalid usages of the the @inaccessible directive.'); + throw ErrGraphQLValidationFailed(errors, `Schema has ${errors.length === 1 ? 'an invalid use' : 'invalid uses'} of the @inaccessible directive.`); } }