Skip to content

Commit

Permalink
fix(generate): harmonize validators
Browse files Browse the repository at this point in the history
see #2103
  • Loading branch information
yaacovCR committed Oct 10, 2020
1 parent 71df0ae commit 10db869
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 90 deletions.
7 changes: 7 additions & 0 deletions .changeset/weak-peaches-count.md
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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`.
Expand Down
10 changes: 5 additions & 5 deletions packages/merge/src/merge-schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ export interface MergeSchemasConfig<Resolvers extends IResolvers = IResolvers> e
}

const defaultResolverValidationOptions: Partial<IResolverValidationOptions> = {
requireResolversForArgs: false,
requireResolversForNonScalar: false,
requireResolversForAllFields: false,
requireResolversForResolveType: false,
allowResolversNotInSchema: true,
requireResolversForArgs: 'ignore',
requireResolversForNonScalar: 'ignore',
requireResolversForAllFields: 'ignore',
requireResolversForResolveType: 'ignore',
requireResolversToMatchSchema: 'ignore',
};

/**
Expand Down
5 changes: 0 additions & 5 deletions packages/mock/tests/mocking.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -983,11 +983,6 @@ describe('Mock', () => {
let jsSchema = makeExecutableSchema({
typeDefs: [shorthand],
resolvers,
resolverValidationOptions: {
requireResolversForArgs: false,
requireResolversForNonScalar: false,
requireResolversForAllFields: false,
},
logger: console,
});
const mockMap = {
Expand Down
15 changes: 10 additions & 5 deletions packages/schema/src/addResolversToSchema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -85,7 +85,7 @@ export function addResolversToSchema(
const type = schema.getType(typeName);

if (type == null) {
if (allowResolversNotInSchema) {
if (requireResolversToMatchSchema === 'ignore') {
return;
}

Expand All @@ -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`
);
Expand All @@ -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`);
}

Expand Down
48 changes: 30 additions & 18 deletions packages/schema/src/assertResolversPresent.ts
Original file line number Diff line number Diff line change
@@ -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)) {
Expand All @@ -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<any, any>, typeName: string, fieldName: string) {
function expectResolver(
validator: string,
behavior: ValidatorBehavior,
field: GraphQLField<any, any>,
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') {
Expand Down
19 changes: 13 additions & 6 deletions packages/schema/src/checkForResolveTypeResolver.ts
Original file line number Diff line number Diff line change
@@ -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;
},
Expand Down

0 comments on commit 10db869

Please sign in to comment.