findBreakingChanges: use string representation to compare default val…

…ues (#1916) Fixes #1567 Closes #1593
graphql · May 27, 2019 · fc0c06c · fc0c06c
1 parent e2fe67e
commit fc0c06c
Show file tree

Hide file tree

Showing 2 changed files with 128 additions and 17 deletions.
diff --git 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
@@ -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<T: { name: string }>(
   oldArray: $ReadOnlyArray<T>,
   newArray: $ReadOnlyArray<T>,