Skip to content
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

Semantic nullability rfc implementation #4337

Open
wants to merge 10 commits into
base: 16.x.x
Choose a base branch
from
1 change: 1 addition & 0 deletions src/__tests__/starWarsIntrospection-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe('Star Wars Introspection Tests', () => {
{ name: '__TypeKind' },
{ name: '__Field' },
{ name: '__InputValue' },
{ name: '__TypeNullability' },
{ name: '__EnumValue' },
{ name: '__Directive' },
{ name: '__DirectiveLocation' },
Expand Down
2 changes: 2 additions & 0 deletions src/execution/__tests__/executor-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ describe('Execute: Handles basic execution tasks', () => {
'rootValue',
'operation',
'variableValues',
'errorPropagation',
);

const operation = document.definitions[0];
Expand All @@ -275,6 +276,7 @@ describe('Execute: Handles basic execution tasks', () => {
schema,
rootValue,
operation,
errorPropagation: true,
});

const field = operation.selectionSet.selections[0];
Expand Down
219 changes: 219 additions & 0 deletions src/execution/__tests__/semantic-nullability-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
import { expect } from 'chai';
import { describe, it } from 'mocha';

import { GraphQLError } from '../../error/GraphQLError';

import type { ExecutableDefinitionNode, FieldNode } from '../../language/ast';
import { parse } from '../../language/parser';

import {
GraphQLNonNull,
GraphQLObjectType,
GraphQLSemanticNonNull,
} from '../../type/definition';
import { GraphQLString } from '../../type/scalars';
import { GraphQLSchema } from '../../type/schema';

import { execute } from '../execute';

describe('Execute: Handles Semantic Nullability', () => {
const DeepDataType = new GraphQLObjectType({
name: 'DeepDataType',
fields: {
f: { type: new GraphQLNonNull(GraphQLString) },
},
});

const DataType: GraphQLObjectType = new GraphQLObjectType({
name: 'DataType',
fields: () => ({
a: { type: GraphQLString },
b: { type: new GraphQLSemanticNonNull(GraphQLString) },
c: { type: new GraphQLNonNull(GraphQLString) },
d: { type: new GraphQLSemanticNonNull(DeepDataType) },
}),
});

it('SemanticNonNull throws error on null without error', async () => {
const data = {
a: () => 'Apple',
b: () => null,
c: () => 'Cookie',
};

const document = parse(`
query {
b
}
`);

const result = await execute({
schema: new GraphQLSchema({ query: DataType }),
document,
rootValue: data,
});

const executable = document.definitions?.values().next()
.value as ExecutableDefinitionNode;
const selectionSet = executable.selectionSet.selections
.values()
.next().value;

expect(result).to.deep.equal({
data: {
b: null,
},
errors: [
new GraphQLError(
'Cannot return null for semantic-non-nullable field DataType.b.',
{
nodes: selectionSet,
path: ['b'],
},
),
],
});
});

it('SemanticNonNull succeeds on null with error', async () => {
const data = {
a: () => 'Apple',
b: () => {
throw new Error('Something went wrong');
},
c: () => 'Cookie',
};

const document = parse(`
query {
b
}
`);

const executable = document.definitions?.values().next()
.value as ExecutableDefinitionNode;
const selectionSet = executable.selectionSet.selections
.values()
.next().value;

const result = await execute({
schema: new GraphQLSchema({ query: DataType }),
document,
rootValue: data,
});

expect(result).to.deep.equal({
data: {
b: null,
},
errors: [
new GraphQLError('Something went wrong', {
nodes: selectionSet,
path: ['b'],
}),
],
});
});

it('SemanticNonNull halts null propagation', async () => {
const deepData = {
f: () => null,
};

const data = {
a: () => 'Apple',
b: () => null,
c: () => 'Cookie',
d: () => deepData,
};

const document = parse(`
query {
d {
f
}
}
`);

const result = await execute({
schema: new GraphQLSchema({ query: DataType }),
document,
rootValue: data,
});

const executable = document.definitions?.values().next()
.value as ExecutableDefinitionNode;
const dSelectionSet = executable.selectionSet.selections.values().next()
.value as FieldNode;
const fSelectionSet = dSelectionSet.selectionSet?.selections
.values()
.next().value;

expect(result).to.deep.equal({
data: {
d: null,
},
errors: [
new GraphQLError(
'Cannot return null for non-nullable field DeepDataType.f.',
{
nodes: fSelectionSet,
path: ['d', 'f'],
},
),
],
});
});

it('SemanticNullable allows null values', async () => {
const data = {
a: () => null,
b: () => null,
c: () => 'Cookie',
};

const document = parse(`
query {
a
}
`);

const result = await execute({
schema: new GraphQLSchema({ query: DataType }),
document,
rootValue: data,
});

expect(result).to.deep.equal({
data: {
a: null,
},
});
});

it('SemanticNullable allows non-null values', async () => {
const data = {
a: () => 'Apple',
b: () => null,
c: () => 'Cookie',
};

const document = parse(`
query {
a
}
`);

const result = await execute({
schema: new GraphQLSchema({ query: DataType }),
document,
rootValue: data,
});

expect(result).to.deep.equal({
data: {
a: 'Apple',
},
});
});
});
31 changes: 31 additions & 0 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import {
isListType,
isNonNullType,
isObjectType,
isSemanticNonNullType,
} from '../type/definition';
import {
SchemaMetaFieldDef,
Expand Down Expand Up @@ -115,6 +116,7 @@ export interface ExecutionContext {
typeResolver: GraphQLTypeResolver<any, any>;
subscribeFieldResolver: GraphQLFieldResolver<any, any>;
errors: Array<GraphQLError>;
errorPropagation: boolean;
}

/**
Expand Down Expand Up @@ -152,6 +154,13 @@ export interface ExecutionArgs {
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>;
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
/**
* Set to `false` to disable error propagation. Experimental.
* TODO: describe what this does
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outstanding TODO. We might want to say something here about how this is being used and include a link to the RFC or spec edits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot find where this setting is used.

i.e. I would expect something similar for this PR as in #4271 where there is a change within handleFieldError() https://github.com/twof/graphql-js/blob/163785d2957d0098e11092482eac8bed765689f3/src/execution/execute.ts#L610

function handleFieldError(...):  null {
  // If the field type is non-nullable, then it is resolved without any
  // protection from errors, however it still properly locates the error.
  if (exeContext.errorPropagation && isNonNullType(returnType)) {  //   <=== used here
    throw error;
  }
  ...
}

If I am correct, I assume there are also missing tests around this behavior, which I think is also missing from #4271 and #4338

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original PR it's used on line 609 https://github.com/graphql/graphql-js/pull/4192/files#diff-1d705c6a4c73cd3ce46190029e75abd3015b49de3ec250357dcf9b44a35cb7d0R609 exactly as you describe:

// If the field type is non-nullable, then it is resolved without any
// protection from errors, however it still properly locates the error.
if (exeContext.errorPropagation && isNonNullType(returnType)) {
throw error;
}

*
* @experimental
*/
errorPropagation?: boolean;
}

/**
Expand Down Expand Up @@ -286,6 +295,7 @@ export function buildExecutionContext(
fieldResolver,
typeResolver,
subscribeFieldResolver,
errorPropagation,
} = args;

let operation: OperationDefinitionNode | undefined;
Expand Down Expand Up @@ -347,6 +357,7 @@ export function buildExecutionContext(
typeResolver: typeResolver ?? defaultTypeResolver,
subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver,
errors: [],
errorPropagation: errorPropagation ?? true,
};
}

Expand Down Expand Up @@ -585,6 +596,7 @@ export function buildResolveInfo(
rootValue: exeContext.rootValue,
operation: exeContext.operation,
variableValues: exeContext.variableValues,
errorPropagation: exeContext.errorPropagation,
};
}

Expand Down Expand Up @@ -658,6 +670,25 @@ function completeValue(
return completed;
}

// If field type is SemanticNonNull, complete for inner type, and throw field error
// if result is null and an error doesn't exist.
if (isSemanticNonNullType(returnType)) {
const completed = completeValue(
exeContext,
returnType.ofType,
fieldNodes,
info,
path,
result,
);
if (completed === null) {
throw new Error(
`Cannot return null for semantic-non-nullable field ${info.parentType.name}.${info.fieldName}.`,
);
}
return completed;
}

// If result value is null or undefined then return null.
if (result == null) {
return null;
Expand Down
8 changes: 8 additions & 0 deletions src/graphql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ export interface GraphQLArgs {
operationName?: Maybe<string>;
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>;
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>;
/**
* Set to `false` to disable error propagation. Experimental.
*
* @experimental
*/
errorPropagation?: boolean;
}

export function graphql(args: GraphQLArgs): Promise<ExecutionResult> {
Expand Down Expand Up @@ -106,6 +112,7 @@ function graphqlImpl(args: GraphQLArgs): PromiseOrValue<ExecutionResult> {
operationName,
fieldResolver,
typeResolver,
errorPropagation,
} = args;

// Validate Schema
Expand Down Expand Up @@ -138,5 +145,6 @@ function graphqlImpl(args: GraphQLArgs): PromiseOrValue<ExecutionResult> {
operationName,
fieldResolver,
typeResolver,
errorPropagation,
});
}
6 changes: 6 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export {
GraphQLInputObjectType,
GraphQLList,
GraphQLNonNull,
GraphQLSemanticNonNull,
// Standard GraphQL Scalars
specifiedScalarTypes,
GraphQLInt,
Expand All @@ -74,6 +75,7 @@ export {
__Schema,
__Directive,
__DirectiveLocation,
__TypeNullability,
__Type,
__Field,
__InputValue,
Expand All @@ -95,6 +97,7 @@ export {
isInputObjectType,
isListType,
isNonNullType,
isSemanticNonNullType,
isInputType,
isOutputType,
isLeafType,
Expand All @@ -120,6 +123,7 @@ export {
assertInputObjectType,
assertListType,
assertNonNullType,
assertSemanticNonNullType,
assertInputType,
assertOutputType,
assertLeafType,
Expand Down Expand Up @@ -286,6 +290,7 @@ export type {
TypeNode,
NamedTypeNode,
ListTypeNode,
SemanticNonNullTypeNode,
NonNullTypeNode,
TypeSystemDefinitionNode,
SchemaDefinitionNode,
Expand Down Expand Up @@ -481,6 +486,7 @@ export type {
IntrospectionNamedTypeRef,
IntrospectionListTypeRef,
IntrospectionNonNullTypeRef,
IntrospectionSemanticNonNullTypeRef,
IntrospectionField,
IntrospectionInputValue,
IntrospectionEnumValue,
Expand Down
Loading
Loading