From 2c18604208abbb475039076f3dfdc32736e5443c Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Sat, 10 Oct 2020 20:19:23 +0300 Subject: [PATCH] fix(generate): harmonize validators see #2103 --- .changeset/weak-peaches-count.md | 7 ++ packages/merge/src/merge-schemas.ts | 10 +-- packages/mock/tests/mocking.spec.ts | 5 -- packages/schema/src/addResolversToSchema.ts | 15 +++-- packages/schema/src/assertResolversPresent.ts | 48 ++++++++------ .../schema/src/checkForResolveTypeResolver.ts | 19 ++++-- packages/schema/tests/schemaGenerator.test.ts | 64 ++++++++++--------- packages/utils/src/Interfaces.ts | 32 +++++----- website/docs/generate-schema.md | 12 ++-- website/docs/migration-from-tools.md | 6 ++ 10 files changed, 128 insertions(+), 90 deletions(-) diff --git a/.changeset/weak-peaches-count.md b/.changeset/weak-peaches-count.md index 50a85d1ddf7..33ea0af8877 100644 --- a/.changeset/weak-peaches-count.md +++ b/.changeset/weak-peaches-count.md @@ -1,6 +1,7 @@ --- '@graphql-tools/batch-execute': major '@graphql-tools/delegate': major +'@graphql-tools/generate': major '@graphql-tools/mock': major '@graphql-tools/stitch': major '@graphql-tools/utils': major @@ -12,6 +13,12 @@ ## Breaking Changes: +#### Schema Generation and Decoration API (`@graphql-tools/schema`) + +- Resolver validation options should now be set to `error`, `warn` or `ignore` rather than `true` or `false`. In previous versions, some of the validators caused errors to be thrown, while some issued warnings. This changes brings consistency to validator behavior. + +- The `allowResolversNotInSchema` has been renamed to `requireResolversToMatchSchema`, to harmonize the naming convention of all the validators. The default setting of `requireResolversToMatchSchema` is `error`, matching the previous behavior. + #### Schema Delegation (`delegateToSchema` & `@graphql-tools/delegate`) - The `delegateToSchema` return value has matured and been formalized as an `ExternalObject`, in which all errors are integrated into the GraphQL response, preserving their initial path. Those advanced users accessing the result directly will note the change in error handling. This also allows for the deprecation of unnecessary helper functions including `slicedError`, `getErrors`, `getErrorsByPathSegment` functions. Only external errors with missing or invalid paths must still be preserved by annotating the remote object with special properties. The new `getUnpathedErrors` function is therefore necessary for retrieving only these errors. Note also the new `annotateExternalObject` and `mergeExternalObjects` functions, as well as the renaming of `handleResult` to `resolveExternalValue`. diff --git a/packages/merge/src/merge-schemas.ts b/packages/merge/src/merge-schemas.ts index c3df20b8c30..27bf24200fe 100644 --- a/packages/merge/src/merge-schemas.ts +++ b/packages/merge/src/merge-schemas.ts @@ -42,11 +42,11 @@ export interface MergeSchemasConfig e } const defaultResolverValidationOptions: Partial = { - requireResolversForArgs: false, - requireResolversForNonScalar: false, - requireResolversForAllFields: false, - requireResolversForResolveType: false, - allowResolversNotInSchema: true, + requireResolversForArgs: 'ignore', + requireResolversForNonScalar: 'ignore', + requireResolversForAllFields: 'ignore', + requireResolversForResolveType: 'ignore', + requireResolversToMatchSchema: 'ignore', }; /** diff --git a/packages/mock/tests/mocking.spec.ts b/packages/mock/tests/mocking.spec.ts index 126eaa4950c..4081db790a6 100644 --- a/packages/mock/tests/mocking.spec.ts +++ b/packages/mock/tests/mocking.spec.ts @@ -983,11 +983,6 @@ describe('Mock', () => { let jsSchema = makeExecutableSchema({ typeDefs: [shorthand], resolvers, - resolverValidationOptions: { - requireResolversForArgs: false, - requireResolversForNonScalar: false, - requireResolversForAllFields: false, - }, logger: console, }); const mockMap = { diff --git a/packages/schema/src/addResolversToSchema.ts b/packages/schema/src/addResolversToSchema.ts index 4225ac03d22..19d564db3a2 100644 --- a/packages/schema/src/addResolversToSchema.ts +++ b/packages/schema/src/addResolversToSchema.ts @@ -55,7 +55,7 @@ export function addResolversToSchema( updateResolversInPlace = false, } = options; - const { allowResolversNotInSchema = false, requireResolversForResolveType } = resolverValidationOptions; + const { requireResolversToMatchSchema = 'error', requireResolversForResolveType } = resolverValidationOptions; const resolvers = inheritResolversFromInterfaces ? extendResolversFromInterfaces(schema, inputResolvers) @@ -85,7 +85,7 @@ export function addResolversToSchema( const type = schema.getType(typeName); if (type == null) { - if (allowResolversNotInSchema) { + if (requireResolversToMatchSchema === 'ignore') { return; } @@ -106,14 +106,19 @@ export function addResolversToSchema( if ( !fieldName.startsWith('__') && !values.some(value => value.name === fieldName) && - !allowResolversNotInSchema + requireResolversToMatchSchema && + requireResolversToMatchSchema !== 'ignore' ) { throw new Error(`${type.name}.${fieldName} was defined in resolvers, but not present within ${type.name}`); } }); } else if (isUnionType(type)) { Object.keys(resolverValue).forEach(fieldName => { - if (!fieldName.startsWith('__') && !allowResolversNotInSchema) { + if ( + !fieldName.startsWith('__') && + requireResolversToMatchSchema && + requireResolversToMatchSchema !== 'ignore' + ) { throw new Error( `${type.name}.${fieldName} was defined in resolvers, but ${type.name} is not an object or interface type` ); @@ -125,7 +130,7 @@ export function addResolversToSchema( const fields = type.getFields(); const field = fields[fieldName]; - if (field == null && !allowResolversNotInSchema) { + if (field == null && requireResolversToMatchSchema && requireResolversToMatchSchema !== 'ignore') { throw new Error(`${typeName}.${fieldName} defined in resolvers, but not in schema`); } diff --git a/packages/schema/src/assertResolversPresent.ts b/packages/schema/src/assertResolversPresent.ts index a935c143975..9026b858a27 100644 --- a/packages/schema/src/assertResolversPresent.ts +++ b/packages/schema/src/assertResolversPresent.ts @@ -1,15 +1,15 @@ import { GraphQLSchema, GraphQLField, getNamedType, isScalarType } from 'graphql'; -import { IResolverValidationOptions, forEachField } from '@graphql-tools/utils'; +import { IResolverValidationOptions, forEachField, ValidatorBehavior } from '@graphql-tools/utils'; export function assertResolversPresent( schema: GraphQLSchema, resolverValidationOptions: IResolverValidationOptions = {} ): void { const { - requireResolversForArgs = false, - requireResolversForNonScalar = false, - requireResolversForAllFields = false, + requireResolversForArgs, + requireResolversForNonScalar, + requireResolversForAllFields, } = resolverValidationOptions; if (requireResolversForAllFields && (requireResolversForArgs || requireResolversForNonScalar)) { @@ -23,32 +23,44 @@ export function assertResolversPresent( forEachField(schema, (field, typeName, fieldName) => { // requires a resolver for *every* field. if (requireResolversForAllFields) { - expectResolver(field, typeName, fieldName); + expectResolver('requireResolversForAllFields', requireResolversForAllFields, field, typeName, fieldName); } // requires a resolver on every field that has arguments if (requireResolversForArgs && field.args.length > 0) { - expectResolver(field, typeName, fieldName); + expectResolver('requireResolversForArgs', requireResolversForArgs, field, typeName, fieldName); } // requires a resolver on every field that returns a non-scalar type - if (requireResolversForNonScalar && !isScalarType(getNamedType(field.type))) { - expectResolver(field, typeName, fieldName); + if (requireResolversForNonScalar !== 'ignore' && !isScalarType(getNamedType(field.type))) { + expectResolver('requireResolversForNonScalar', requireResolversForNonScalar, field, typeName, fieldName); } }); } -function expectResolver(field: GraphQLField, typeName: string, fieldName: string) { +function expectResolver( + validator: string, + behavior: ValidatorBehavior, + field: GraphQLField, + typeName: string, + fieldName: string +) { if (!field.resolve) { - // eslint-disable-next-line no-console - console.warn( - `Resolver missing for "${typeName}.${fieldName}". -To disable this warning check pass; -resolverValidationOptions: { - requireResolversForNonScalar: false -} - ` - ); + const message = `Resolver missing for "${typeName}.${fieldName}". +To disable this validator, use: + resolverValidationOptions: { + ${validator}: 'ignore' + }`; + + if (behavior === 'error') { + throw new Error(message); + } + + if (behavior === 'warn') { + // eslint-disable-next-line no-console + console.warn(message); + } + return; } if (typeof field.resolve !== 'function') { diff --git a/packages/schema/src/checkForResolveTypeResolver.ts b/packages/schema/src/checkForResolveTypeResolver.ts index 327f5269ebd..1a2a8cd0408 100644 --- a/packages/schema/src/checkForResolveTypeResolver.ts +++ b/packages/schema/src/checkForResolveTypeResolver.ts @@ -1,16 +1,23 @@ import { GraphQLSchema } from 'graphql'; -import { MapperKind, mapSchema } from '@graphql-tools/utils'; +import { MapperKind, mapSchema, ValidatorBehavior } from '@graphql-tools/utils'; // If we have any union or interface types throw if no there is no resolveType resolver -export function checkForResolveTypeResolver(schema: GraphQLSchema, requireResolversForResolveType?: boolean) { +export function checkForResolveTypeResolver(schema: GraphQLSchema, requireResolversForResolveType?: ValidatorBehavior) { mapSchema(schema, { [MapperKind.ABSTRACT_TYPE]: type => { if (!type.resolveType && requireResolversForResolveType) { - throw new Error( - `Type "${type.name}" is missing a "__resolveType" resolver. Pass false into ` + - '"resolverValidationOptions.requireResolversForResolveType" to disable this error.' - ); + const message = + `Type "${type.name}" is missing a "__resolveType" resolver. Pass 'ignore' into ` + + '"resolverValidationOptions.requireResolversForResolveType" to disable this error.'; + if (requireResolversForResolveType === 'error') { + throw new Error(message); + } + + if (requireResolversForResolveType === 'warn') { + // eslint-disable-next-line no-console + console.warn(message); + } } return undefined; }, diff --git a/packages/schema/tests/schemaGenerator.test.ts b/packages/schema/tests/schemaGenerator.test.ts index 6c4d10ba5d1..38738dda2fe 100644 --- a/packages/schema/tests/schemaGenerator.test.ts +++ b/packages/schema/tests/schemaGenerator.test.ts @@ -1517,7 +1517,7 @@ describe('generating schema from shorthand', () => { typeDefs: short, resolvers: rf, resolverValidationOptions: { - requireResolversForArgs: true, + requireResolversForArgs: 'warn', }, }); }, 'Resolver missing for "Query.bird"'); @@ -1573,7 +1573,7 @@ describe('generating schema from shorthand', () => { const rf = {}; const resolverValidationOptions: IResolverValidationOptions = { - requireResolversForNonScalar: true, + requireResolversForNonScalar: 'warn', }; expectWarning(() => { @@ -1582,7 +1582,11 @@ describe('generating schema from shorthand', () => { resolvers: rf, resolverValidationOptions, }); - }, 'Resolver missing for "Query.bird"'); + }, `Resolver missing for "Query.bird". +To disable this validator, use: + resolverValidationOptions: { + requireResolversForNonScalar: 'ignore' + }`); }); test('allows non-scalar field to use default resolver if `resolverValidationOptions.requireResolversForNonScalar` = false', () => { @@ -1644,7 +1648,7 @@ describe('generating schema from shorthand', () => { typeDefs: short, resolvers: rf, resolverValidationOptions: { - allowResolversNotInSchema: true, + requireResolversToMatchSchema: 'ignore', }, }), ).not.toThrowError(); @@ -1679,7 +1683,7 @@ describe('generating schema from shorthand', () => { typeDefs: short, resolvers: rf, resolverValidationOptions: { - allowResolversNotInSchema: true, + requireResolversToMatchSchema: 'ignore', }, }), ).not.toThrowError(); @@ -1740,7 +1744,7 @@ describe('generating schema from shorthand', () => { typeDefs: short, resolvers: rf, resolverValidationOptions: { - allowResolversNotInSchema: true, + requireResolversToMatchSchema: 'ignore', }, }), ).not.toThrowError(); @@ -1787,7 +1791,7 @@ describe('generating schema from shorthand', () => { typeDefs: short, resolvers: rf, resolverValidationOptions: { - allowResolversNotInSchema: true, + requireResolversToMatchSchema: 'ignore', }, }), ).not.toThrowError(); @@ -1819,21 +1823,21 @@ describe('generating schema from shorthand', () => { } assertOptionsError({ - requireResolversForAllFields: true, - requireResolversForNonScalar: true, - requireResolversForArgs: true, + requireResolversForAllFields: 'error', + requireResolversForNonScalar: 'error', + requireResolversForArgs: 'error', }); assertOptionsError({ - requireResolversForAllFields: true, - requireResolversForNonScalar: true, + requireResolversForAllFields: 'error', + requireResolversForNonScalar: 'error', }); assertOptionsError({ - requireResolversForAllFields: true, - requireResolversForArgs: true, + requireResolversForAllFields: 'error', + requireResolversForArgs: 'error', }); }); - test('throws for any missing field if `resolverValidationOptions.requireResolversForAllFields` = true', () => { + test('warns for any missing field if `resolverValidationOptions.requireResolversForAllFields` = warn', () => { const typeDefs = ` type Bird { id: ID @@ -1851,7 +1855,7 @@ describe('generating schema from shorthand', () => { typeDefs, resolvers, resolverValidationOptions: { - requireResolversForAllFields: true, + requireResolversForAllFields: 'warn', }, }); }, errorMatcher); @@ -1869,7 +1873,7 @@ describe('generating schema from shorthand', () => { }); }); - test('does not throw if all fields are satisfied when `resolverValidationOptions.requireResolversForAllFields` = true', () => { + test('does not throw if all fields are satisfied when `resolverValidationOptions.requireResolversForAllFields` = error', () => { const typeDefs = ` type Bird { id: ID @@ -1894,7 +1898,7 @@ describe('generating schema from shorthand', () => { makeExecutableSchema({ typeDefs, resolvers, - resolverValidationOptions: { requireResolversForAllFields: true }, + resolverValidationOptions: { requireResolversForAllFields: 'error' }, }), ).not.toThrow(); }); @@ -2543,11 +2547,11 @@ describe('interfaces', () => { makeExecutableSchema({ typeDefs: testSchemaWithInterfaces, resolvers, - resolverValidationOptions: { requireResolversForResolveType: true }, + resolverValidationOptions: { requireResolversForResolveType: 'error' }, }); } catch (error) { expect(error.message).toEqual( - 'Type "Node" is missing a "__resolveType" resolver. Pass false into "resolverValidationOptions.requireResolversForResolveType" to disable this error.', + `Type "Node" is missing a "__resolveType" resolver. Pass 'ignore' into "resolverValidationOptions.requireResolversForResolveType" to disable this error.`, ); return; } @@ -2563,7 +2567,7 @@ describe('interfaces', () => { const schema = makeExecutableSchema({ typeDefs: testSchemaWithInterfaces, resolvers, - resolverValidationOptions: { requireResolversForResolveType: true }, + resolverValidationOptions: { requireResolversForResolveType: 'error' }, }); const response = await graphql(schema, query); expect(response.errors).not.toBeDefined(); @@ -2575,7 +2579,7 @@ describe('interfaces', () => { makeExecutableSchema({ typeDefs: testSchemaWithInterfaces, resolvers, - resolverValidationOptions: { requireResolversForResolveType: false }, + resolverValidationOptions: { requireResolversForResolveType: 'ignore' }, }); }); }); @@ -2615,8 +2619,8 @@ describe('interface resolver inheritance', () => { resolvers, inheritResolversFromInterfaces: true, resolverValidationOptions: { - requireResolversForAllFields: true, - requireResolversForResolveType: true, + requireResolversForAllFields: 'error', + requireResolversForResolveType: 'error', }, }); const query = '{ user { id name } }'; @@ -2679,8 +2683,8 @@ describe('interface resolver inheritance', () => { resolvers, inheritResolversFromInterfaces: true, resolverValidationOptions: { - requireResolversForAllFields: true, - requireResolversForResolveType: true, + requireResolversForAllFields: 'error', + requireResolversForResolveType: 'error', }, }); const query = '{ cyborg { id name } replicant { id name }}'; @@ -2742,11 +2746,11 @@ describe('unions', () => { makeExecutableSchema({ typeDefs: testSchemaWithUnions, resolvers, - resolverValidationOptions: { requireResolversForResolveType: true }, + resolverValidationOptions: { requireResolversForResolveType: 'error' }, }); } catch (error) { expect(error.message).toEqual( - 'Type "Displayable" is missing a "__resolveType" resolver. Pass false into "resolverValidationOptions.requireResolversForResolveType" to disable this error.', + `Type "Displayable" is missing a "__resolveType" resolver. Pass 'ignore' into "resolverValidationOptions.requireResolversForResolveType" to disable this error.`, ); return; } @@ -2762,7 +2766,7 @@ describe('unions', () => { const schema = makeExecutableSchema({ typeDefs: testSchemaWithUnions, resolvers, - resolverValidationOptions: { requireResolversForResolveType: true }, + resolverValidationOptions: { requireResolversForResolveType: 'error' }, }); const response = await graphql(schema, query); expect(response.errors).not.toBeDefined(); @@ -2774,7 +2778,7 @@ describe('unions', () => { makeExecutableSchema({ typeDefs: testSchemaWithUnions, resolvers, - resolverValidationOptions: { requireResolversForResolveType: false }, + resolverValidationOptions: { requireResolversForResolveType: 'ignore' }, }); }); }); diff --git a/packages/utils/src/Interfaces.ts b/packages/utils/src/Interfaces.ts index 0fb10b96c6e..2b89b7908c0 100644 --- a/packages/utils/src/Interfaces.ts +++ b/packages/utils/src/Interfaces.ts @@ -82,35 +82,37 @@ export interface GraphQLParseOptions { // graphql-tools typings +export type ValidatorBehavior = 'error' | 'warn' | 'ignore'; + /** * Options for validating resolvers */ export interface IResolverValidationOptions { /** - * Set to `true` to require a resolver to be defined for any field that has - * arguments. Defaults to `false`. + * Enable to require a resolver to be defined for any field that has + * arguments. Defaults to `ignore`. */ - requireResolversForArgs?: boolean; + requireResolversForArgs?: ValidatorBehavior; /** - * Set to `true` to require a resolver to be defined for any field which has - * a return type that isn't a scalar. Defaults to `false`. + * Enable to require a resolver to be defined for any field which has + * a return type that isn't a scalar. Defaults to `ignore`. */ - requireResolversForNonScalar?: boolean; + requireResolversForNonScalar?: ValidatorBehavior; /** - * Set to `true` to require a resolver for be defined for all fields defined - * in the schema. Defaults to `false`. + * Enable to require a resolver for be defined for all fields defined + * in the schema. Defaults to `ignore`. */ - requireResolversForAllFields?: boolean; + requireResolversForAllFields?: ValidatorBehavior; /** - * Set to `true` to require a `resolveType()` for Interface and Union types. - * Defaults to `false`. + * Enable to require a `resolveType()` for Interface and Union types. + * Defaults to `ignore`. */ - requireResolversForResolveType?: boolean; + requireResolversForResolveType?: ValidatorBehavior; /** - * Set to `false` to require all defined resolvers to match fields that - * actually exist in the schema. Defaults to `true`. + * Enable to require all defined resolvers to match fields that + * actually exist in the schema. Defaults to `error` to catch common errors. */ - allowResolversNotInSchema?: boolean; + requireResolversToMatchSchema?: ValidatorBehavior; } /** diff --git a/website/docs/generate-schema.md b/website/docs/generate-schema.md index 09441db8610..06d8746115e 100644 --- a/website/docs/generate-schema.md +++ b/website/docs/generate-schema.md @@ -239,16 +239,16 @@ const jsSchema = makeExecutableSchema({ - `allowUndefinedInResolve` is an optional argument, which is `true` by default. When set to `false`, causes your resolver to throw errors if they return undefined, which can help make debugging easier. -- `resolverValidationOptions` is an optional argument which accepts an `ResolverValidationOptions` object which has the following boolean properties: - - `requireResolversForArgs` will cause `makeExecutableSchema` to throw an error if no resolver is defined for a field that has arguments. +- `resolverValidationOptions` is an optional argument with the following properties, each of which can be set to `error`, `warn`, or `ignore`: + - `requireResolversForArgs` will cause `makeExecutableSchema` to throw an error (`error`) or issue a warning (`warn`)unless a resolver is defined for every field with arguments. The default is `ignore`, causing this validator to be skipped. - - `requireResolversForNonScalar` will cause `makeExecutableSchema` to throw an error if a non-scalar field has no resolver defined. Setting this to `true` can be helpful in catching errors, but defaults to `false` to avoid confusing behavior for those coming from other GraphQL libraries. + - `requireResolversForNonScalar` require a resolver for every non-scalar field. Default is `ignore`. - - `requireResolversForAllFields` asserts that *all* fields have valid resolvers. + - `requireResolversForAllFields` asserts that *all* fields have valid resolvers. This option cannot be set in combination with the previous two validators. Default is `ignore`. - - `requireResolversForResolveType` will require a `resolveType()` method for Interface and Union types. This can be passed in with the field resolvers as `__resolveType()`. False to disable the warning. + - `requireResolversForResolveType` will require a `resolveType()` method for Interface and Union types. This can be passed in with the field resolvers as `__resolveType()`. Default is `ignore`. - - `allowResolversNotInSchema` turns off the functionality which throws errors when resolvers are found which are not present in the schema. Defaults to `false`, to help catch common errors. + - `requireResolversToMatchSchema` requires every resolver within the resolver map to correspond to a GraphQL entity within the schema. Defaults to `error`, to help catch common errors. - `inheritResolversFromInterfaces` GraphQL Objects that implement interfaces will inherit missing resolvers from their interface types defined in the `resolvers` object. diff --git a/website/docs/migration-from-tools.md b/website/docs/migration-from-tools.md index d3217499f2e..2e64296f23e 100644 --- a/website/docs/migration-from-tools.md +++ b/website/docs/migration-from-tools.md @@ -9,6 +9,12 @@ description: Migration from GraphQL Tools v4 - v6 If you are using GraphQL Tools v6, there are several breaking changes to be aware of. +#### Schema Generation and Decoration API (`@graphql-tools/schema`) + +- Resolver validation options should now be set to `error`, `warn` or `ignore` rather than `true` or `false`. In previous versions, some of the validators caused errors to be thrown, while some issued warnings. This changes brings consistency to validator behavior. + +- The `allowResolversNotInSchema` has been renamed to `requireResolversToMatchSchema`, to harmonize the naming convention of all the validators. The default setting of `requireResolversToMatchSchema` is `error`, matching the previous behavior. + #### Schema Delegation (`delegateToSchema` & `@graphql-tools/delegate`) - The `delegateToSchema` return value has matured and been formalized as an `ExternalObject`, in which all errors are integrated into the GraphQL response, preserving their initial path. Those advanced users accessing the result directly will note the change in error handling. This also allows for the deprecation of unnecessary helper functions including `slicedError`, `getErrors`, `getErrorsByPathSegment` functions. Only external errors with missing or invalid paths must still be preserved by annotating the remote object with special properties. The new `getUnpathedErrors` function is therefore necessary for retrieving only these errors. Note also the new `annotateExternalObject` and `mergeExternalObjects` functions, as well as the renaming of `handleResult` to `resolveExternalValue`.