graphql-java · bbakerman · Oct 9, 2022 · Sep 21, 2022 · Sep 23, 2022 · Sep 23, 2022
diff --git a/src/main/java/graphql/schema/diff/SchemaDiff.java b/src/main/java/graphql/schema/diff/SchemaDiff.java
@@ -397,7 +397,7 @@ private void checkInputFields(DiffCtx ctx, TypeDefinition old, List<InputValueDe
                         .reasonMsg(message, mkDotName(old.getName(), oldField.getName()))
                         .build());
             } else {
-                DiffCategory category = checkTypeWithNonNullAndList(oldField.getType(), newField.get().getType());
+                DiffCategory category = checkTypeWithNonNullAndListOnInputOrArg(oldField.getType(), newField.get().getType());
                 if (category != null) {
                     ctx.report(DiffEvent.apiBreakage()
                             .category(category)
@@ -612,7 +612,7 @@ private void checkField(DiffCtx ctx, TypeDefinition old, FieldDefinition oldFiel
         Type oldFieldType = oldField.getType();
         Type newFieldType = newField.getType();
 
-        DiffCategory category = checkTypeWithNonNullAndList(oldFieldType, newFieldType);
+        DiffCategory category = checkTypeWithNonNullAndListOnObjectOrInterface(oldFieldType, newFieldType);
         if (category != null) {
             ctx.report(DiffEvent.apiBreakage()
                     .category(category)
@@ -701,7 +701,7 @@ private void checkFieldArg(DiffCtx ctx, TypeDefinition oldDef, FieldDefinition o
         Type oldArgType = oldArg.getType();
         Type newArgType = newArg.getType();
 
-        DiffCategory category = checkTypeWithNonNullAndList(oldArgType, newArgType);
+        DiffCategory category = checkTypeWithNonNullAndListOnInputOrArg(oldArgType, newArgType);
         if (category != null) {
             ctx.report(DiffEvent.apiBreakage()
                     .category(category)
@@ -831,7 +831,7 @@ void checkDirectives(DiffCtx ctx, TypeDefinition old, List<Directive> oldDirecti
         }
     }
 
-    DiffCategory checkTypeWithNonNullAndList(Type oldType, Type newType) {
+    DiffCategory checkTypeWithNonNullAndListOnInputOrArg(Type oldType, Type newType) {
         TypeInfo oldTypeInfo = typeInfo(oldType);
         TypeInfo newTypeInfo = typeInfo(newType);
 
@@ -840,31 +840,83 @@ DiffCategory checkTypeWithNonNullAndList(Type oldType, Type newType) {
         }
 
         while (true) {
-            //
-            // its allowed to get more less strict in the new but not more strict
-            if (oldTypeInfo.isNonNull() && newTypeInfo.isNonNull()) {
-                oldTypeInfo = oldTypeInfo.unwrapOne();
-                newTypeInfo = newTypeInfo.unwrapOne();
-            } else if (oldTypeInfo.isNonNull() && !newTypeInfo.isNonNull()) {
-                oldTypeInfo = oldTypeInfo.unwrapOne();
-            } else if (!oldTypeInfo.isNonNull() && newTypeInfo.isNonNull()) {
-                return DiffCategory.STRICTER;
-            }
-            // lists
-            if (oldTypeInfo.isList() && !newTypeInfo.isList()) {
-                return DiffCategory.INVALID;
+            if (oldTypeInfo.isNonNull()) {
+                if (newTypeInfo.isNonNull()) {
+                    // if they're both non-null, compare the unwrapped types
+                    oldTypeInfo = oldTypeInfo.unwrapOne();
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else {
+                    // non-null to nullable is valid, as long as the underlying types are also valid
+                    oldTypeInfo = oldTypeInfo.unwrapOne();
+                }
+            } else if (oldTypeInfo.isList()) {
+                if (newTypeInfo.isList()) {
+                    // if they're both lists, compare the unwrapped types
+                    oldTypeInfo = oldTypeInfo.unwrapOne();
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else if (newTypeInfo.isNonNull()) {
+                    // nullable to non-null creates a stricter input requirement for clients to specify
+                    return DiffCategory.STRICTER;
+                } else {
+                    // list to non-list is not valid
+                    return DiffCategory.INVALID;
+                }
+            } else {
+                if (newTypeInfo.isNonNull()) {
+                    // nullable to non-null creates a stricter input requirement for clients to specify
+                    return DiffCategory.STRICTER;
+                } else if (newTypeInfo.isList()) {
+                    // non-list to list is not valid
+                    return DiffCategory.INVALID;
+                } else {
+                    return null;
+                }
             }
-            // plain
-            if (oldTypeInfo.isPlain()) {
-                if (!newTypeInfo.isPlain()) {
+        }
+    }
+
+    DiffCategory checkTypeWithNonNullAndListOnObjectOrInterface(Type oldType, Type newType) {
+        TypeInfo oldTypeInfo = typeInfo(oldType);
+        TypeInfo newTypeInfo = typeInfo(newType);
+
+        if (!oldTypeInfo.getName().equals(newTypeInfo.getName())) {
+            return DiffCategory.INVALID;
+        }
+
+        while (true) {
+            if (oldTypeInfo.isNonNull()) {
+                if (newTypeInfo.isNonNull()) {
+                    // if they're both non-null, compare the unwrapped types
+                    oldTypeInfo = oldTypeInfo.unwrapOne();
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else {
+                    // non-null to nullable requires a stricter check from clients since it removes the guarantee of presence
+                    return DiffCategory.STRICTER;
+                }
+            } else if (oldTypeInfo.isList()) {
+                if (newTypeInfo.isList()) {
+                    // if they're both lists, compare the unwrapped types
+                    oldTypeInfo = oldTypeInfo.unwrapOne();
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else if (newTypeInfo.isNonNull()) {
+                    // nullable to non-null is valid, as long as the underlying types are also valid
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else {
+                    // list to non-list is not valid
                     return DiffCategory.INVALID;
                 }
-                break;
+            } else {
+                if (newTypeInfo.isNonNull()) {
+                    // nullable to non-null is valid, as long as the underlying types are also valid
+                    newTypeInfo = newTypeInfo.unwrapOne();
+                } else if (newTypeInfo.isList()) {
+                    // non-list to list is not valid
+                    return DiffCategory.INVALID;
+                } else {
+                    return null;
+                }
             }
-            oldTypeInfo = oldTypeInfo.unwrapOne();
-            newTypeInfo = newTypeInfo.unwrapOne();
         }
-        return null;
     }
 
 

diff --git a/src/test/groovy/graphql/schema/diff/SchemaDiffTest.groovy b/src/test/groovy/graphql/schema/diff/SchemaDiffTest.groovy
@@ -105,7 +105,7 @@ class SchemaDiffTest extends Specification {
     }
 
 
-    def "change_in_null_ness"() {
+    def "change_in_null_ness_input_or_arg"() {
 
         given:
         Type baseLine = new NonNullType(new ListType(new TypeName("foo")))
@@ -116,12 +116,12 @@ class SchemaDiffTest extends Specification {
 
         def diff = new SchemaDiff()
 
-        def sameType = diff.checkTypeWithNonNullAndList(baseLine, same)
+        def sameType = diff.checkTypeWithNonNullAndListOnInputOrArg(baseLine, same)
 
-        def lessStrict = diff.checkTypeWithNonNullAndList(baseLine, less)
+        def lessStrict = diff.checkTypeWithNonNullAndListOnInputOrArg(baseLine, less)
 
         // not allowed as old clients wont work
-        def moreStrict = diff.checkTypeWithNonNullAndList(less, baseLine)
+        def moreStrict = diff.checkTypeWithNonNullAndListOnInputOrArg(less, baseLine)
 
 
         expect:
@@ -130,18 +130,60 @@ class SchemaDiffTest extends Specification {
         moreStrict == DiffCategory.STRICTER
     }
 
-    def "change_in_list_ness"() {
+    def "change_in_null_ness_object_or_interface"() {
 
         given:
-        Type baseLine = new NonNullType(new ListType(new TypeName("foo")))
+        Type nonNull = new NonNullType(new ListType(new TypeName("foo")))
+        Type nonNullDuplicate = new NonNullType(new ListType(new TypeName("foo")))
+
+        Type nullable = new ListType(new TypeName("foo"))
+
+
+        def diff = new SchemaDiff()
+
+        def sameType = diff.checkTypeWithNonNullAndListOnObjectOrInterface(nonNull, nonNullDuplicate)
+
+        def removeGuarantee = diff.checkTypeWithNonNullAndListOnObjectOrInterface(nonNull, nullable)
+
+        def addGuarantee = diff.checkTypeWithNonNullAndListOnObjectOrInterface(nullable, nonNull)
+
+
+        expect:
+        sameType == null
+        removeGuarantee == DiffCategory.STRICTER
+        addGuarantee == null
+    }
+
+    def "change_in_list_ness_input_or_arg"() {
+
+        given:
+        Type list = new NonNullType(new ListType(new TypeName("foo")))
         Type notList = new NonNullType(new TypeName("foo"))
 
         def diff = new SchemaDiff()
 
-        def noLongerList = diff.checkTypeWithNonNullAndList(baseLine, notList)
+        def noLongerList = diff.checkTypeWithNonNullAndListOnInputOrArg(list, notList)
+        def nowList = diff.checkTypeWithNonNullAndListOnInputOrArg(notList, list)
 
         expect:
         noLongerList == DiffCategory.INVALID
+        nowList == DiffCategory.INVALID
+    }
+
+    def "change_in_list_ness_object_or_interface"() {
+
+        given:
+        Type list = new NonNullType(new ListType(new TypeName("foo")))
+        Type notList = new NonNullType(new TypeName("foo"))
+
+        def diff = new SchemaDiff()
+
+        def noLongerList = diff.checkTypeWithNonNullAndListOnObjectOrInterface(list, notList)
+        def nowList = diff.checkTypeWithNonNullAndListOnObjectOrInterface(list, notList)
+
+        expect:
+        noLongerList == DiffCategory.INVALID
+        nowList == DiffCategory.INVALID
     }
 
     DiffEvent lastBreakage(CapturingReporter capturingReporter) {
@@ -344,16 +386,26 @@ class SchemaDiffTest extends Specification {
         diff.diffSchema(diffSet, chainedReporter)
 
         expect:
-        reporter.breakageCount == 2
-        reporter.breakages[0].category == DiffCategory.INVALID
-        reporter.breakages[0].typeName == 'Questor'
-        reporter.breakages[0].typeKind == TypeKind.InputObject
-        reporter.breakages[0].fieldName == 'queryTarget'
+        reporter.breakageCount == 4
+        reporter.breakages[0].category == DiffCategory.STRICTER
+        reporter.breakages[0].typeName == 'Query'
+        reporter.breakages[0].typeKind == TypeKind.Object
+        reporter.breakages[0].fieldName == 'being'
 
         reporter.breakages[1].category == DiffCategory.STRICTER
         reporter.breakages[1].typeName == 'Questor'
         reporter.breakages[1].typeKind == TypeKind.InputObject
-        reporter.breakages[1].fieldName == 'newMandatoryField'
+        reporter.breakages[1].fieldName == 'nestedInput'
+
+        reporter.breakages[2].category == DiffCategory.INVALID
+        reporter.breakages[2].typeName == 'Questor'
+        reporter.breakages[2].typeKind == TypeKind.InputObject
+        reporter.breakages[2].fieldName == 'queryTarget'
+
+        reporter.breakages[3].category == DiffCategory.STRICTER
+        reporter.breakages[3].typeName == 'Questor'
+        reporter.breakages[3].typeKind == TypeKind.InputObject
+        reporter.breakages[3].fieldName == 'newMandatoryField'
 
     }
 
@@ -433,27 +485,21 @@ class SchemaDiffTest extends Specification {
         diff.diffSchema(diffSet, chainedReporter)
 
         expect:
-        reporter.breakageCount == 4
+        reporter.breakageCount == 3
         reporter.breakages[0].category == DiffCategory.STRICTER
-        reporter.breakages[0].typeName == 'Query'
+        reporter.breakages[0].typeName == 'Istari'
         reporter.breakages[0].typeKind == TypeKind.Object
-        reporter.breakages[0].fieldName == 'being'
+        reporter.breakages[0].fieldName == 'temperament'
 
         reporter.breakages[1].category == DiffCategory.INVALID
         reporter.breakages[1].typeName == 'Query'
         reporter.breakages[1].typeKind == TypeKind.Object
         reporter.breakages[1].fieldName == 'beings'
 
-        reporter.breakages[2].category == DiffCategory.STRICTER
+        reporter.breakages[2].category == DiffCategory.INVALID
         reporter.breakages[2].typeName == 'Query'
         reporter.breakages[2].typeKind == TypeKind.Object
         reporter.breakages[2].fieldName == 'customScalar'
-
-        reporter.breakages[3].category == DiffCategory.STRICTER
-        reporter.breakages[3].typeName == 'Query'
-        reporter.breakages[3].typeKind == TypeKind.Object
-        reporter.breakages[3].fieldName == 'wizards'
-
     }
 
     def "dangerous changes"() {
@@ -504,7 +550,7 @@ class SchemaDiffTest extends Specification {
         diff.diffSchema(diffSet, chainedReporter)
 
         expect:
-        reporter.dangerCount == 13
+        reporter.dangerCount == 14
         reporter.breakageCount == 0
         reporter.dangers.every {
             it.getCategory() == DiffCategory.DEPRECATION_ADDED
@@ -523,7 +569,7 @@ class SchemaDiffTest extends Specification {
 
         expect:
         reporter.dangerCount == 0
-        reporter.breakageCount == 11
+        reporter.breakageCount == 12
         reporter.breakages.every {
             it.getCategory() == DiffCategory.DEPRECATION_REMOVED
         }

diff --git a/src/test/resources/diff/schema_ABaseLine.graphqls b/src/test/resources/diff/schema_ABaseLine.graphqls
@@ -12,6 +12,7 @@ type Query {
     deities : [Deity]
 
     allCharacters : [Character!] @deprecated(reason: "no longer supported")
+    allCharactersByTemperament : [[Character]]
 
     customScalar : CustomScalar
 }
@@ -22,7 +23,7 @@ type Mutation {
 }
 
 input Questor {
-    beingID : ID
+    beingID : ID!
     queryTarget : String
     nestedInput : NestedInput
 }

diff --git a/src/test/resources/diff/schema_changed_field_arguments.graphqls b/src/test/resources/diff/schema_changed_field_arguments.graphqls
@@ -12,6 +12,7 @@ type Query {
     deities : [Deity]
 
     allCharacters : [Character!] @deprecated(reason: "no longer supported")
+    allCharactersByTemperament : [[Character]]
 
     customScalar : CustomScalar
 }
@@ -23,7 +24,7 @@ type Mutation {
 }
 
 input Questor {
-    beingID : ID
+    beingID : ID!
     queryTarget : String
     nestedInput : NestedInput
 }

diff --git a/src/test/resources/diff/schema_changed_input_object_fields.graphqls b/src/test/resources/diff/schema_changed_input_object_fields.graphqls
@@ -4,14 +4,16 @@ schema {
 }
 
 type Query {
-    being(id : ID, type : String = "wizard") : Being
+    #being(id : ID, type : String = "wizard") : Being
+    being(id : ID, type : String, newArg : String!) : Being
     beings(type : String) : [Being]
 
     wizards : [Istari]
     gods : [Ainur]
     deities : [Deity]
 
     allCharacters : [Character!] @deprecated(reason: "no longer supported")
+    allCharactersByTemperament : [[Character]]
 
     customScalar : CustomScalar
 }
@@ -22,9 +24,14 @@ type Mutation {
 }
 
 input Questor {
+    #beingID : ID!
     beingID : ID
+
+    #queryTarget : String
     queryTarget : Int
-    nestedInput : NestedInput
+
+    #nestedInput : NestedInput
+    nestedInput : NestedInput!
     newMandatoryField : String!
 }
 

diff --git a/src/test/resources/diff/schema_changed_nested_input_object_fields.graphqls b/src/test/resources/diff/schema_changed_nested_input_object_fields.graphqls
@@ -12,6 +12,7 @@ type Query {
     deities : [Deity]
 
     allCharacters : [Character!] @deprecated(reason: "no longer supported")
+    allCharactersByTemperament : [[Character]]
 
     customScalar : CustomScalar
 }
@@ -22,7 +23,7 @@ type Mutation {
 }
 
 input Questor {
-    beingID : ID
+    beingID : ID!
     queryTarget : String
     nestedInput : NestedInput
 }