From da7ef46d1032e40a1de224d197a23ea568feb023 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Wed, 29 Aug 2018 18:34:12 +0300 Subject: [PATCH] Correctly detect required/optional args/fields Disscussed here: https://github.com/graphql/graphql-js/pull/1465#issuecomment-413699023 --- .../__tests__/findBreakingChanges-test.js | 52 +++++++++------- src/utilities/findBreakingChanges.js | 59 ++++++++++--------- 2 files changed, 61 insertions(+), 50 deletions(-) diff --git a/src/utilities/__tests__/findBreakingChanges-test.js b/src/utilities/__tests__/findBreakingChanges-test.js index ae1907642f..4cfc15baec 100644 --- a/src/utilities/__tests__/findBreakingChanges-test.js +++ b/src/utilities/__tests__/findBreakingChanges-test.js @@ -333,7 +333,7 @@ describe('findBreakingChanges', () => { ).to.eql(expectedFieldChanges); }); - it('should detect if a non-null field is added to an input type', () => { + it('should detect if a required field is added to an input type', () => { const oldSchema = buildSchema(` input InputType1 { field1: String @@ -348,7 +348,8 @@ describe('findBreakingChanges', () => { input InputType1 { field1: String requiredField: Int! - optionalField: Boolean + optionalField1: Boolean + optionalField2: Boolean! = false } type Query { @@ -358,10 +359,9 @@ describe('findBreakingChanges', () => { const expectedFieldChanges = [ { - type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED, + type: BreakingChangeType.REQUIRED_INPUT_FIELD_ADDED, description: - 'A non-null field requiredField on input type ' + - 'InputType1 was added.', + 'A required field requiredField on input type InputType1 was added.', }, ]; expect( @@ -609,7 +609,7 @@ describe('findBreakingChanges', () => { ]); }); - it('should detect if a non-null field argument was added', () => { + it('should detect if a required field argument was added', () => { const oldSchema = buildSchema(` type Type1 { field1(arg1: String): String @@ -622,7 +622,12 @@ describe('findBreakingChanges', () => { const newSchema = buildSchema(` type Type1 { - field1(arg1: String, newRequiredArg: String!, newOptionalArg: Int): String + field1( + arg1: String, + newRequiredArg: String! + newOptionalArg1: Int + newOptionalArg2: Int! = 0 + ): String } type Query { @@ -632,8 +637,8 @@ describe('findBreakingChanges', () => { expect(findArgChanges(oldSchema, newSchema).breakingChanges).to.eql([ { - type: BreakingChangeType.NON_NULL_ARG_ADDED, - description: 'A non-null arg newRequiredArg on Type1.field1 was added', + type: BreakingChangeType.REQUIRED_ARG_ADDED, + description: 'A required arg newRequiredArg on Type1.field1 was added', }, ]); }); @@ -882,9 +887,9 @@ describe('findBreakingChanges', () => { description: 'arg1 was removed from DirectiveThatRemovesArg', }, { - type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED, + type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED, description: - 'A non-null arg arg1 on directive NonNullDirectiveAdded was added', + 'A required arg arg1 on directive NonNullDirectiveAdded was added', }, { type: BreakingChangeType.DIRECTIVE_LOCATION_REMOVED, @@ -946,19 +951,24 @@ describe('findBreakingChanges', () => { ]); }); - it('should detect if a non-nullable directive argument was added', () => { + it('should detect if an optional directive argument was added', () => { const oldSchema = buildSchema(` directive @DirectiveName on FIELD_DEFINITION `); const newSchema = buildSchema(` - directive @DirectiveName(arg1: Boolean!) on FIELD_DEFINITION + directive @DirectiveName( + newRequiredArg: String! + newOptionalArg1: Int + newOptionalArg2: Int! = 0 + ) on FIELD_DEFINITION `); expect(findAddedNonNullDirectiveArgs(oldSchema, newSchema)).to.eql([ { - type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED, - description: 'A non-null arg arg1 on directive DirectiveName was added', + type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED, + description: + 'A required arg newRequiredArg on directive DirectiveName was added', }, ]); }); @@ -1131,7 +1141,7 @@ describe('findDangerousChanges', () => { ]); }); - it('should detect if a nullable field was added to an input', () => { + it('should detect if an optional field was added to an input', () => { const oldSchema = buildSchema(` input InputType1 { field1: String @@ -1155,9 +1165,9 @@ describe('findDangerousChanges', () => { const expectedFieldChanges = [ { - type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED, + type: DangerousChangeType.OPTIONAL_INPUT_FIELD_ADDED, description: - 'A nullable field field2 on input type InputType1 was added.', + 'An optional field field2 on input type InputType1 was added.', }, ]; @@ -1253,7 +1263,7 @@ describe('findDangerousChanges', () => { ); }); - it('should detect if a nullable field argument was added', () => { + it('should detect if an optional field argument was added', () => { const oldSchema = buildSchema(` type Type1 { field1(arg1: String): String @@ -1276,8 +1286,8 @@ describe('findDangerousChanges', () => { expect(findArgChanges(oldSchema, newSchema).dangerousChanges).to.eql([ { - type: DangerousChangeType.NULLABLE_ARG_ADDED, - description: 'A nullable arg arg2 on Type1.field1 was added', + type: DangerousChangeType.OPTIONAL_ARG_ADDED, + description: 'An optional arg arg2 on Type1.field1 was added', }, ]); }); diff --git a/src/utilities/findBreakingChanges.js b/src/utilities/findBreakingChanges.js index e2d444d892..9f7962138f 100644 --- a/src/utilities/findBreakingChanges.js +++ b/src/utilities/findBreakingChanges.js @@ -17,6 +17,8 @@ import { isNonNullType, isListType, isNamedType, + isRequiredArgument, + isRequiredInputField, } from '../type/definition'; import type { @@ -42,13 +44,13 @@ export const BreakingChangeType = { VALUE_REMOVED_FROM_ENUM: 'VALUE_REMOVED_FROM_ENUM', ARG_REMOVED: 'ARG_REMOVED', ARG_CHANGED_KIND: 'ARG_CHANGED_KIND', - NON_NULL_ARG_ADDED: 'NON_NULL_ARG_ADDED', - NON_NULL_INPUT_FIELD_ADDED: 'NON_NULL_INPUT_FIELD_ADDED', + REQUIRED_ARG_ADDED: 'REQUIRED_ARG_ADDED', + REQUIRED_INPUT_FIELD_ADDED: 'REQUIRED_INPUT_FIELD_ADDED', INTERFACE_REMOVED_FROM_OBJECT: 'INTERFACE_REMOVED_FROM_OBJECT', DIRECTIVE_REMOVED: 'DIRECTIVE_REMOVED', DIRECTIVE_ARG_REMOVED: 'DIRECTIVE_ARG_REMOVED', DIRECTIVE_LOCATION_REMOVED: 'DIRECTIVE_LOCATION_REMOVED', - NON_NULL_DIRECTIVE_ARG_ADDED: 'NON_NULL_DIRECTIVE_ARG_ADDED', + REQUIRED_DIRECTIVE_ARG_ADDED: 'REQUIRED_DIRECTIVE_ARG_ADDED', }; export const DangerousChangeType = { @@ -56,8 +58,8 @@ export const DangerousChangeType = { VALUE_ADDED_TO_ENUM: 'VALUE_ADDED_TO_ENUM', INTERFACE_ADDED_TO_OBJECT: 'INTERFACE_ADDED_TO_OBJECT', TYPE_ADDED_TO_UNION: 'TYPE_ADDED_TO_UNION', - NULLABLE_INPUT_FIELD_ADDED: 'NULLABLE_INPUT_FIELD_ADDED', - NULLABLE_ARG_ADDED: 'NULLABLE_ARG_ADDED', + OPTIONAL_INPUT_FIELD_ADDED: 'OPTIONAL_INPUT_FIELD_ADDED', + OPTIONAL_ARG_ADDED: 'OPTIONAL_ARG_ADDED', }; export type BreakingChange = { @@ -242,24 +244,25 @@ export function findArgChanges( } } } - // Check if a non-null arg was added to the field + // Check if arg was added to the field for (const newArgDef of newTypeFields[fieldName].args) { const oldArgs = oldTypeFields[fieldName].args; const oldArgDef = oldArgs.find(arg => arg.name === newArgDef.name); if (!oldArgDef) { - if (isNonNullType(newArgDef.type)) { + const argName = newArgDef.name; + if (isRequiredArgument(newArgDef)) { breakingChanges.push({ - type: BreakingChangeType.NON_NULL_ARG_ADDED, + type: BreakingChangeType.REQUIRED_ARG_ADDED, description: - `A non-null arg ${newArgDef.name} on ` + - `${newType.name}.${fieldName} was added`, + `A required arg ${argName} on ` + + `${typeName}.${fieldName} was added`, }); } else { dangerousChanges.push({ - type: DangerousChangeType.NULLABLE_ARG_ADDED, + type: DangerousChangeType.OPTIONAL_ARG_ADDED, description: - `A nullable arg ${newArgDef.name} on ` + - `${newType.name}.${fieldName} was added`, + `An optional arg ${argName} on ` + + `${typeName}.${fieldName} was added`, }); } } @@ -405,19 +408,19 @@ export function findFieldsThatChangedTypeOnInputObjectTypes( // Check if a field was added to the input object type for (const fieldName of Object.keys(newTypeFieldsDef)) { if (!(fieldName in oldTypeFieldsDef)) { - if (isNonNullType(newTypeFieldsDef[fieldName].type)) { + if (isRequiredInputField(newTypeFieldsDef[fieldName])) { breakingChanges.push({ - type: BreakingChangeType.NON_NULL_INPUT_FIELD_ADDED, + type: BreakingChangeType.REQUIRED_INPUT_FIELD_ADDED, description: - `A non-null field ${fieldName} on ` + - `input type ${newType.name} was added.`, + `A required field ${fieldName} on ` + + `input type ${typeName} was added.`, }); } else { dangerousChanges.push({ - type: DangerousChangeType.NULLABLE_INPUT_FIELD_ADDED, + type: DangerousChangeType.OPTIONAL_INPUT_FIELD_ADDED, description: - `A nullable field ${fieldName} on ` + - `input type ${newType.name} was added.`, + `An optional field ${fieldName} on ` + + `input type ${typeName} was added.`, }); } } @@ -780,16 +783,14 @@ export function findAddedNonNullDirectiveArgs( } for (const arg of findAddedArgsForDirective(oldDirective, newDirective)) { - if (!isNonNullType(arg.type)) { - continue; + if (isRequiredArgument(arg)) { + addedNonNullableArgs.push({ + type: BreakingChangeType.REQUIRED_DIRECTIVE_ARG_ADDED, + description: + `A required arg ${arg.name} on directive ` + + `${newDirective.name} was added`, + }); } - - addedNonNullableArgs.push({ - type: BreakingChangeType.NON_NULL_DIRECTIVE_ARG_ADDED, - description: - `A non-null arg ${arg.name} on directive ` + - `${newDirective.name} was added`, - }); } }