From fc0c06c31505b9efb6cdf2690947155401b46071 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Mon, 27 May 2019 13:42:12 +0300 Subject: [PATCH] findBreakingChanges: use string representation to compare default values (#1916) Fixes #1567 Closes #1593 --- .../__tests__/findBreakingChanges-test.js | 103 ++++++++++++++++-- src/utilities/findBreakingChanges.js | 42 +++++-- 2 files changed, 128 insertions(+), 17 deletions(-) diff --git a/src/utilities/__tests__/findBreakingChanges-test.js b/src/utilities/__tests__/findBreakingChanges-test.js index 94060c6a7b..f9dad88584 100644 --- a/src/utilities/__tests__/findBreakingChanges-test.js +++ b/src/utilities/__tests__/findBreakingChanges-test.js @@ -820,27 +820,116 @@ describe('findBreakingChanges', () => { }); describe('findDangerousChanges', () => { - it("should detect if an argument's defaultValue has changed", () => { - const oldSchema = buildSchema(` + it('should detect if a defaultValue changed on an argument', () => { + const oldSDL = ` + input Input1 { + innerInputArray: [Input2] + } + + input Input2 { + arrayField: [Int] + } + type Type1 { - field1(name: String = "test"): String + field1( + withDefaultValue: String = "TO BE DELETED" + stringArg: String = "test" + emptyArray: [Int!] = [] + valueArray: [[String]] = [["a", "b"], ["c"]] + complexObject: Input1 = { + innerInputArray: [{ arrayField: [1, 2, 3] }] + } + ): String } - `); + `; + + const oldSchema = buildSchema(oldSDL); + const copyOfOldSchema = buildSchema(oldSDL); + expect(findDangerousChanges(oldSchema, copyOfOldSchema)).to.deep.equal([]); const newSchema = buildSchema(` + input Input1 { + innerInputArray: [Input2] + } + + input Input2 { + arrayField: [Int] + } + type Type1 { - field1(name: String = "Test"): String + field1( + withDefaultValue: String + stringArg: String = "Test" + emptyArray: [Int!] = [7] + valueArray: [[String]] = [["b", "a"], ["d"]] + complexObject: Input1 = { + innerInputArray: [{ arrayField: [3, 2, 1] }] + } + ): String } `); expect(findDangerousChanges(oldSchema, newSchema)).to.deep.equal([ { type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, - description: 'Type1.field1 arg name has changed defaultValue.', + description: + 'Type1.field1 arg withDefaultValue defaultValue was removed.', + }, + { + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + 'Type1.field1 arg stringArg has changed defaultValue from "test" to "Test".', + }, + { + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + 'Type1.field1 arg emptyArray has changed defaultValue from [] to [7].', + }, + { + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + 'Type1.field1 arg valueArray has changed defaultValue from [["a", "b"], ["c"]] to [["b", "a"], ["d"]].', + }, + { + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + 'Type1.field1 arg complexObject has changed defaultValue from {innerInputArray: [{arrayField: [1, 2, 3]}]} to {innerInputArray: [{arrayField: [3, 2, 1]}]}.', }, ]); }); + it('should ignore changes in field order of defaultValue', () => { + const oldSchema = buildSchema(` + input Input1 { + a: String + b: String + c: String + } + + type Type1 { + field1( + arg1: Input1 = { a: "a", b: "b", c: "c" } + ): String + } + `); + + const newSchema = buildSchema(` + input Input1 { + a: String + b: String + c: String + } + + type Type1 { + field1( + arg1: Input1 = { c: "c", b: "b", a: "a" } + ): String + } + `); + + expect(findDangerousChanges(oldSchema, newSchema)).to.deep.equal([]); + }); + it('should detect if a value was added to an enum type', () => { const oldSchema = buildSchema(` enum EnumType1 { @@ -979,7 +1068,7 @@ describe('findDangerousChanges', () => { { type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, description: - 'Type1.field1 arg argThatChangesDefaultValue has changed defaultValue.', + 'Type1.field1 arg argThatChangesDefaultValue has changed defaultValue from "test" to "Test".', }, { type: DangerousChangeType.INTERFACE_ADDED_TO_OBJECT, diff --git a/src/utilities/findBreakingChanges.js b/src/utilities/findBreakingChanges.js index 91ac70f05e..42e705b1b4 100644 --- a/src/utilities/findBreakingChanges.js +++ b/src/utilities/findBreakingChanges.js @@ -10,9 +10,12 @@ import objectValues from '../polyfills/objectValues'; import keyMap from '../jsutils/keyMap'; import inspect from '../jsutils/inspect'; +import invariant from '../jsutils/invariant'; +import { print } from '../language/printer'; import { type GraphQLField, type GraphQLType, + type GraphQLInputType, type GraphQLNamedType, type GraphQLEnumType, type GraphQLUnionType, @@ -32,6 +35,7 @@ import { isRequiredInputField, } from '../type/definition'; import { type GraphQLSchema } from '../type/schema'; +import { astFromValue } from './astFromValue'; export const BreakingChangeType = Object.freeze({ TYPE_REMOVED: 'TYPE_REMOVED', @@ -404,16 +408,28 @@ function findArgChanges( `${oldArg.name} has changed type from ` + `${String(oldArg.type)} to ${String(newArg.type)}.`, }); - } else if ( - oldArg.defaultValue !== undefined && - oldArg.defaultValue !== newArg.defaultValue - ) { - schemaChanges.push({ - type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, - description: - `${oldType.name}.${oldField.name} arg ` + - `${oldArg.name} has changed defaultValue.`, - }); + } else if (oldArg.defaultValue !== undefined) { + if (newArg.defaultValue === undefined) { + schemaChanges.push({ + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + `${oldType.name}.${oldField.name} arg ` + + `${oldArg.name} defaultValue was removed.`, + }); + } else { + const oldValueStr = stringifyValue(oldArg.defaultValue, oldArg.type); + const newValueStr = stringifyValue(newArg.defaultValue, newArg.type); + + if (oldValueStr !== newValueStr) { + schemaChanges.push({ + type: DangerousChangeType.ARG_DEFAULT_VALUE_CHANGE, + description: + `${oldType.name}.${oldField.name} arg ` + + `${oldArg.name} has changed defaultValue ` + + `from ${oldValueStr} to ${newValueStr}.`, + }); + } + } } } @@ -529,6 +545,12 @@ function typeKindName(type: GraphQLNamedType): string { throw new TypeError(`Unexpected type: ${inspect((type: empty))}.`); } +function stringifyValue(value: mixed, type: GraphQLInputType): string { + const ast = astFromValue(value, type); + invariant(ast != null); + return print(ast); +} + function diff( oldArray: $ReadOnlyArray, newArray: $ReadOnlyArray,