Skip to content

Commit

Permalink
SPEC/BUG: Ambiguity with null variable values and default values (#1274)
Browse files Browse the repository at this point in the history 
This change corresponds to a spec proposal (graphql/graphql-spec#418) which solves an ambiguity in how variable values and default values behave with explicit null values, and changes validation rules to better define the behavior of default values. Otherwise, this ambiguity can allows for null values to appear in non-null argument values, which may result in unforeseen null-pointer-errors.

In summary this change includes:

* **BREAKING:** Removal of `VariablesDefaultValueAllowed` validation rule. All variables may now specify a default value.

* Change to `VariablesInAllowedPosition` rule to explicitly not allow a `null` default value when flowing into a non-null argument, and now allows optional (nullable) variables in non-null arguments that provide default values.

* Changes to `ProvidedRequiredArguments` rule (**BREAKING:** renamed from `ProvidedNonNullArguments`) to no longer require values to be provided to non-null arguments which provide a default value.

* Changes to `getVariableValues()` and `getArgumentValues()` to ensure a `null` value never flows into a non-null argument.

* Changes to `valueFromAST()` to ensure `null` variable values do not flow into non-null types.

* Adds to the `TypeInfo` API to allow referencing the expected default value at a given AST position.
  • Loading branch information
@leebyron
leebyron authored Apr 18, 2018
1 parent 54f631f commit 4d2438a
Show file tree
Hide file tree
Showing 19 changed files with 614 additions and 317 deletions.
195 changes: 195 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,199 @@ 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),
},
},
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('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'],
},
],
});
});

it('field error when non-null arg provided variable with explicit null 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: null,
},
errors: [
{
message:
'Argument "cannotBeNull" of non-null type "String!" must not be null.',
locations: [{ line: 3, column: 43 }],
path: ['withNonNullArg'],
},
],
});
});
});
});
111 changes: 105 additions & 6 deletions src/execution/__tests__/variables-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ const TestType = new GraphQLObjectType({
type: GraphQLString,
defaultValue: 'Hello World',
}),
fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({
type: GraphQLNonNull(GraphQLString),
defaultValue: 'Hello World',
}),
fieldWithNestedInputObject: fieldWithInputArg({
type: TestNestedInputObject,
defaultValue: 'Hello World',
Expand Down Expand Up @@ -222,6 +226,40 @@ describe('Execute: Handles inputs', () => {
});
});

it('uses undefined when variable not provided', () => {
const result = executeQuery(
`
query q($input: String) {
fieldWithNullableStringInput(input: $input)
}`,
{
// Intentionally missing variable values.
},
);

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

it('uses null when variable provided explicit null value', () => {
const result = executeQuery(
`
query q($input: String) {
fieldWithNullableStringInput(input: $input)
}`,
{ input: null },
);

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

it('uses default value when not provided', () => {
const result = executeQuery(`
query ($input: TestInputObject = {a: "foo", b: ["bar"], c: "baz"}) {
Expand All @@ -236,6 +274,55 @@ describe('Execute: Handles inputs', () => {
});
});

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

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

it('uses explicit null value instead of default value', () => {
const result = executeQuery(
`
query q($input: String = "Default value") {
fieldWithNullableStringInput(input: $input)
}`,
{ input: null },
);

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

it('uses null default value when not provided', () => {
const result = executeQuery(
`
query q($input: String = null) {
fieldWithNullableStringInput(input: $input)
}`,
{
// Intentionally missing variable values.
},
);

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

it('properly parses single value to list', () => {
const params = { input: { a: 'foo', b: 'bar', c: 'baz' } };
const result = executeQuery(doc, params);
Expand Down Expand Up @@ -485,8 +572,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 non-null type "String!" must not be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -644,8 +730,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 non-null type "[String]!" must not be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -728,8 +813,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 non-null type "[String!]!" must not be null.',
locations: [{ line: 2, column: 16 }],
},
],
Expand Down Expand Up @@ -853,5 +937,20 @@ describe('Execute: Handles inputs', () => {
],
});
});

it('when no runtime value is provided to a non-null argument', () => {
const result = executeQuery(`
query optionalVariable($optional: String) {
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional)
}
`);

expect(result).to.deep.equal({
data: {
fieldWithNonNullableStringInputAndDefaultArgumentValue:
"'Hello World'",
},
});
});
});
});
Loading

0 comments on commit 4d2438a

Please sign in to comment.