Skip to content

Commit

Permalink
SPEC/BUG: Ambiguity with null variable values and default values
Browse files Browse the repository at this point in the history 
This change corresponds to a spec proposal which solves an ambiguity in how variable values and default values behave with explicit null values. Otherwise, this ambiguity allows for null values to appear in non-null argument values, which may result in unforseen null-pointer-errors.

This appears in three distinct but related issues:

**VariablesInAllowedPosition validation rule**

The explicit value `null` may be used as a default value for a variable, however `VariablesInAllowedPositions` allowed a nullable type with a default value to flow into an argument expecting a non-null type. This validation rule must explicitly not allow `null` default values to flow in this manner.

**Coercing Variable Values**

coerceVariableValues allows the explicit `null` value to be used over a default value, which can result in flowing a null value to a non-null argument when paired with the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided.

**Coercing Argument Values**

coerceArgumentValues allows the explicit `null` default value to be used before checking for a non-null type. This could inadvertently allow a null value into a non-null typed argument.
  • Loading branch information
@leebyron
leebyron committed Mar 6, 2018
1 parent 8913bf0 commit b105b84
Show file tree
Hide file tree
Showing 5 changed files with 362 additions and 27 deletions.
188 changes: 188 additions & 0 deletions src/execution/__tests__/nonnull-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -486,4 +486,192 @@ describe('Execute: handles non-nullable types', () => {
],
},
);

describe('Handles non-null argument', () => {
const schemaWithNonNullArg = new GraphQLSchema({
query: new GraphQLObjectType({
name: 'Query',
fields: {
withNonNullArg: {
type: GraphQLString,
args: {
cannotBeNull: {
type: GraphQLNonNull(GraphQLString),
defaultValue: null,
},
},
resolve: async (_, args) => {
if (typeof args.cannotBeNull === 'string') {
return 'Passed: ' + args.cannotBeNull;
}
},
},
},
}),
});

it('succeeds when passed non-null literal value', async () => {
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query {
withNonNullArg (cannotBeNull: "literal value")
}
`),
});

expect(result).to.deep.equal({
data: {
withNonNullArg: 'Passed: literal value',
},
});
});

it('succeeds when passed non-null variable value', async () => {
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query ($testVar: String!) {
withNonNullArg (cannotBeNull: $testVar)
}
`),
variableValues: {
testVar: 'variable value',
},
});

expect(result).to.deep.equal({
data: {
withNonNullArg: 'Passed: variable value',
},
});
});

it('succeeds when missing variable has default value', async () => {
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query ($testVar: String = "default value") {
withNonNullArg (cannotBeNull: $testVar)
}
`),
variableValues: {
// Intentionally missing variable
},
});

expect(result).to.deep.equal({
data: {
withNonNullArg: 'Passed: default value',
},
});
});

it('succeeds when null variable has default value', async () => {
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query ($testVar: String = "default value") {
withNonNullArg (cannotBeNull: $testVar)
}
`),
variableValues: {
testVar: null,
},
});

expect(result).to.deep.equal({
data: {
withNonNullArg: 'Passed: default value',
},
});
});

it('field error when missing non-null arg', async () => {
// Note: validation should identify this issue first (missing args rule)
// however execution should still protect against this.
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query {
withNonNullArg
}
`),
});

expect(result).to.deep.equal({
data: {
withNonNullArg: null,
},
errors: [
{
message:
'Argument "cannotBeNull" of required type "String!" was not provided.',
locations: [{ line: 3, column: 13 }],
path: ['withNonNullArg'],
},
],
});
});

it('field error when non-null arg provided null', async () => {
// Note: validation should identify this issue first (values of correct
// type rule) however execution should still protect against this.
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query {
withNonNullArg(cannotBeNull: null)
}
`),
});

expect(result).to.deep.equal({
data: {
withNonNullArg: null,
},
errors: [
{
message:
'Argument "cannotBeNull" of non-null type "String!" must ' +
'not be null.',
locations: [{ line: 3, column: 42 }],
path: ['withNonNullArg'],
},
],
});
});

it('field error when non-null arg not provided variable value', async () => {
// Note: validation should identify this issue first (variables in allowed
// position rule) however execution should still protect against this.
const result = await execute({
schema: schemaWithNonNullArg,
document: parse(`
query ($testVar: String) {
withNonNullArg(cannotBeNull: $testVar)
}
`),
variableValues: {
// Intentionally missing variable
},
});

expect(result).to.deep.equal({
data: {
withNonNullArg: null,
},
errors: [
{
message:
'Argument "cannotBeNull" of required type "String!" was ' +
'provided the variable "$testVar" which was not provided a ' +
'runtime value.',
locations: [{ line: 3, column: 42 }],
path: ['withNonNullArg'],
},
],
});
});
});
});
106 changes: 100 additions & 6 deletions src/execution/__tests__/variables-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ const TestType = new GraphQLObjectType({
args: { input: { type: GraphQLString, defaultValue: 'Hello World' } },
resolve: (_, { input }) => input && JSON.stringify(input),
},
fieldWithNonNullableStringInputAndDefaultArgumentValue: {
type: GraphQLString,
args: {
input: {
type: GraphQLNonNull(GraphQLString),
defaultValue: 'Unreachable',
},
},
resolve: (_, { input }) => input && JSON.stringify(input),
},
fieldWithNestedInputObject: {
type: GraphQLString,
args: {
Expand Down Expand Up @@ -264,6 +274,72 @@ describe('Execute: Handles inputs', () => {
});
});

it('does not use default value when provided', async () => {
const withDefaultsAST = parse(`
query q($input: String = "Default value") {
fieldWithNullableStringInput(input: $input)
}
`);

const result = await execute({
schema,
document: withDefaultsAST,
variableValues: {
input: 'Variable value',
},
});

expect(result).to.deep.equal({
data: {
fieldWithNullableStringInput: '"Variable value"',
},
});
});

it('uses default value when explicit null value provided', async () => {
const withDefaultsAST = parse(`
query q($input: String = "Default value") {
fieldWithNullableStringInput(input: $input)
}
`);

const result = await execute({
schema,
document: withDefaultsAST,
variableValues: {
input: null,
},
});

expect(result).to.deep.equal({
data: {
fieldWithNullableStringInput: '"Default value"',
},
});
});

it('uses null default value when not provided', async () => {
const withDefaultsAST = parse(`
query q($input: String = null) {
fieldWithNullableStringInput(input: $input)
}
`);

const result = await execute({
schema,
document: withDefaultsAST,
variableValues: {
// Intentionally missing value.
},
});

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

it('properly parses single value to list', async () => {
const params = { input: { a: 'foo', b: 'bar', c: 'baz' } };
const result = await execute(schema, ast, null, null, params);
Expand Down Expand Up @@ -534,8 +610,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$value" got invalid value null; ' +
'Expected non-nullable type String! not to be null.',
'Variable "$value" of required type "String!" was not provided.',
locations: [{ line: 2, column: 31 }],
path: undefined,
},
Expand Down Expand Up @@ -733,8 +808,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String]! not to be null.',
'Variable "$input" of required type "[String]!" was not provided.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -846,8 +920,7 @@ describe('Execute: Handles inputs', () => {
errors: [
{
message:
'Variable "$input" got invalid value null; ' +
'Expected non-nullable type [String!]! not to be null.',
'Variable "$input" of required type "[String!]!" was not provided.',
locations: [{ line: 2, column: 17 }],
path: undefined,
},
Expand Down Expand Up @@ -985,5 +1058,26 @@ describe('Execute: Handles inputs', () => {
],
});
});

it('not when argument type is non-null', async () => {
const ast = parse(`query optionalVariable($optional: String) {
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional)
}`);

expect(await execute(schema, ast)).to.deep.equal({
data: {
fieldWithNonNullableStringInputAndDefaultArgumentValue: null,
},
errors: [
{
message:
'Argument "input" of required type "String!" was provided the ' +
'variable "$optional" which was not provided a runtime value.',
locations: [{ line: 2, column: 71 }],
path: ['fieldWithNonNullableStringInputAndDefaultArgumentValue'],
},
],
});
});
});
});
Loading

0 comments on commit b105b84

Please sign in to comment.