Skip to content

Commit 93f7586

Browse files
bbakermanbbakerman
committed
PR feedback for co-variant detection
1 parent 7dbd073 commit 93f7586

File tree

3 files changed

+110
-56
lines changed

3 files changed

+110
-56
lines changed

‎src/main/java/graphql/schema/idl/SchemaTypeChecker.java‎

Lines changed: 1 addition & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ private Consumer<? super Type> checkInterfaceIsImplemented(String typeOfType, Ty
441441
if (objectFieldDef == null) {
442442
errors.add(new MissingInterfaceFieldError(typeOfType, objectTypeDef, interfaceTypeDef, interfaceFieldDef));
443443
} else {
444-
if (!isTypeSubTypeOf(typeRegistry, objectFieldDef.getType(), interfaceFieldDef.getType())) {
444+
if (! typeRegistry.isSubTypeOf(objectFieldDef.getType(), interfaceFieldDef.getType())) {
445445
String interfaceFieldType = AstPrinter.printAst(interfaceFieldDef.getType());
446446
String objectFieldType = AstPrinter.printAst(objectFieldDef.getType());
447447
errors.add(new InterfaceFieldRedefinitionError(typeOfType, objectTypeDef, interfaceTypeDef, objectFieldDef, objectFieldType, interfaceFieldType));
@@ -461,51 +461,6 @@ private Consumer<? super Type> checkInterfaceIsImplemented(String typeOfType, Ty
461461
};
462462
}
463463

464-
@SuppressWarnings("SimplifiableIfStatement")
465-
private boolean isTypeSubTypeOf(TypeDefinitionRegistry registry, Type maybeSubType, Type superType) {
466-
TypeInfo maybeSubTypeInfo = TypeInfo.typeInfo(maybeSubType);
467-
TypeInfo superTypeInfo = TypeInfo.typeInfo(superType);
468-
// Equivalent type is a valid subtype
469-
if (maybeSubTypeInfo.equals(superTypeInfo)) {
470-
return true;
471-
}
472-
473-
474-
// If superType is non-null, maybeSubType must also be non-null.
475-
if (superTypeInfo.isNonNull()) {
476-
if (maybeSubTypeInfo.isNonNull()) {
477-
return isTypeSubTypeOf(registry, maybeSubTypeInfo.unwrapOneType(), superTypeInfo.unwrapOneType());
478-
}
479-
return false;
480-
}
481-
if (maybeSubTypeInfo.isNonNull()) {
482-
// If superType is nullable, maybeSubType may be non-null or nullable.
483-
return isTypeSubTypeOf(registry, maybeSubTypeInfo.unwrapOneType(), superType);
484-
}
485-
486-
// If superType type is a list, maybeSubType type must also be a list.
487-
if (superTypeInfo.isList()) {
488-
if (maybeSubTypeInfo.isList()) {
489-
return isTypeSubTypeOf(registry, maybeSubTypeInfo.unwrapOneType(), superTypeInfo.unwrapOneType());
490-
}
491-
return false;
492-
}
493-
if (maybeSubTypeInfo.isList()) {
494-
// If superType is not a list, maybeSubType must also be not a list.
495-
return false;
496-
}
497-
498-
// If superType type is an abstract type, maybeSubType type may be a currently
499-
// possible object type.
500-
if (registry.isAbstractType(superType) &&
501-
registry.isObjectType(maybeSubType) &&
502-
registry.isPossibleType(superType, maybeSubType)) {
503-
return true;
504-
}
505-
506-
// Otherwise, the child type is not a valid subtype of the parent type.
507-
return false;
508-
}
509464

510465
private void checkArgumentConsistency(String typeOfType, ObjectTypeDefinition objectTypeDef, InterfaceTypeDefinition interfaceTypeDef, FieldDefinition objectFieldDef, FieldDefinition interfaceFieldDef, List<GraphQLError> errors) {
511466
List<InputValueDefinition> objectArgs = objectFieldDef.getInputValueDefinitions();

‎src/main/java/graphql/schema/idl/TypeDefinitionRegistry.java‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ public <T extends TypeDefinition> Optional<T> getType(String typeName, Class<T>
274274
*
275275
* @return true if its abstract
276276
*/
277-
public boolean isAbstractType(Type type) {
277+
public boolean isInterfaceOrUnion(Type type) {
278278
Optional<TypeDefinition> typeDefinition = getType(type);
279279
if (typeDefinition.isPresent()) {
280280
TypeDefinition definition = typeDefinition.get();
@@ -356,7 +356,7 @@ public List<ObjectTypeDefinition> getImplementationsOf(InterfaceTypeDefinition t
356356
*/
357357
@SuppressWarnings("ConstantConditions")
358358
public boolean isPossibleType(Type abstractType, Type possibleObjectType) {
359-
if (!isAbstractType(abstractType)) {
359+
if (!isInterfaceOrUnion(abstractType)) {
360360
return false;
361361
}
362362
if (!isObjectType(possibleObjectType)) {
@@ -382,4 +382,59 @@ public boolean isPossibleType(Type abstractType, Type possibleObjectType) {
382382
.anyMatch(od -> od.getName().equals(targetObjectTypeDef.getName()));
383383
}
384384
}
385+
386+
/**
387+
* Returns true if the maybe type is either equal or a subset of the second super type (covariant).
388+
*
389+
* @param maybeSubType the type to check
390+
* @param superType the equality checked type
391+
*
392+
* @return true if maybeSubType is covariant or equal to superType
393+
*/
394+
@SuppressWarnings("SimplifiableIfStatement")
395+
public boolean isSubTypeOf(Type maybeSubType, Type superType) {
396+
TypeInfo maybeSubTypeInfo = TypeInfo.typeInfo(maybeSubType);
397+
TypeInfo superTypeInfo = TypeInfo.typeInfo(superType);
398+
// Equivalent type is a valid subtype
399+
if (maybeSubTypeInfo.equals(superTypeInfo)) {
400+
return true;
401+
}
402+
403+
404+
// If superType is non-null, maybeSubType must also be non-null.
405+
if (superTypeInfo.isNonNull()) {
406+
if (maybeSubTypeInfo.isNonNull()) {
407+
return isSubTypeOf(maybeSubTypeInfo.unwrapOneType(), superTypeInfo.unwrapOneType());
408+
}
409+
return false;
410+
}
411+
if (maybeSubTypeInfo.isNonNull()) {
412+
// If superType is nullable, maybeSubType may be non-null or nullable.
413+
return isSubTypeOf(maybeSubTypeInfo.unwrapOneType(), superType);
414+
}
415+
416+
// If superType type is a list, maybeSubType type must also be a list.
417+
if (superTypeInfo.isList()) {
418+
if (maybeSubTypeInfo.isList()) {
419+
return isSubTypeOf(maybeSubTypeInfo.unwrapOneType(), superTypeInfo.unwrapOneType());
420+
}
421+
return false;
422+
}
423+
if (maybeSubTypeInfo.isList()) {
424+
// If superType is not a list, maybeSubType must also be not a list.
425+
return false;
426+
}
427+
428+
// If superType type is an abstract type, maybeSubType type may be a currently
429+
// possible object type.
430+
if (isInterfaceOrUnion(superType) &&
431+
isObjectType(maybeSubType) &&
432+
isPossibleType(superType, maybeSubType)) {
433+
return true;
434+
}
435+
436+
// Otherwise, the child type is not a valid subtype of the parent type.
437+
return false;
438+
}
439+
385440
}

‎src/test/groovy/graphql/schema/idl/TypeDefinitionRegistryTest.groovy‎

Lines changed: 52 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
package graphql.schema.idl
22

33
import graphql.language.InterfaceTypeDefinition
4+
import graphql.language.ListType
5+
import graphql.language.NonNullType
46
import graphql.language.ObjectTypeDefinition
57
import graphql.language.SchemaDefinition
8+
import graphql.language.Type
69
import graphql.language.TypeName
710
import graphql.schema.idl.errors.SchemaProblem
811
import graphql.schema.idl.errors.SchemaRedefinitionError
@@ -241,20 +244,36 @@ class TypeDefinitionRegistryTest extends Specification {
241244
242245
'''
243246

244-
private TypeName type(String name) {
247+
private static TypeName type(String name) {
245248
new TypeName(name)
246249
}
247250

251+
private static Type nonNullType(Type type) {
252+
new NonNullType(type)
253+
}
254+
255+
private static Type nonNullType(String name) {
256+
new NonNullType(new TypeName(name))
257+
}
258+
259+
private static Type listType(String name) {
260+
new ListType(new TypeName(name))
261+
}
262+
263+
private static Type listType(Type type) {
264+
new ListType(type)
265+
}
266+
248267
def "test abstract type detection"() {
249268

250269
when:
251270
def registry = parse(commonSpec)
252271

253272
then:
254-
registry.isAbstractType(type("Interface"))
255-
registry.isAbstractType(type("Union"))
256-
!registry.isAbstractType(type("Type"))
257-
!registry.isAbstractType(type("Scalar"))
273+
registry.isInterfaceOrUnion(type("Interface"))
274+
registry.isInterfaceOrUnion(type("Union"))
275+
!registry.isInterfaceOrUnion(type("Type"))
276+
!registry.isInterfaceOrUnion(type("Scalar"))
258277
}
259278

260279
def "test object type detection"() {
@@ -321,8 +340,7 @@ class TypeDefinitionRegistryTest extends Specification {
321340
names == ["Type1", "Type2", "Type3"]
322341
}
323342

324-
def "test possible type detection"() {
325-
def spec = '''
343+
def animalia = '''
326344
327345
interface Animal {
328346
id: String!
@@ -356,8 +374,10 @@ class TypeDefinitionRegistryTest extends Specification {
356374
357375
358376
'''
377+
378+
def "test possible type detection"() {
359379
when:
360-
def registry = parse(spec)
380+
def registry = parse(animalia)
361381

362382
then:
363383
registry.isPossibleType(type("Mammal"), type("Dog"))
@@ -378,4 +398,28 @@ class TypeDefinitionRegistryTest extends Specification {
378398
!registry.isPossibleType(type("Platypus"), type("Cat"))
379399

380400
}
401+
402+
403+
def "isSubTypeOf detection"() {
404+
when:
405+
def registry = parse(animalia)
406+
407+
then:
408+
registry.isSubTypeOf(type("Mammal"), type("Mammal"))
409+
registry.isSubTypeOf(type("Dog"), type("Mammal"))
410+
411+
registry.isSubTypeOf(type("Turtle"), type("Animal"))
412+
!registry.isSubTypeOf(type("Turtle"), type("Mammal"))
413+
414+
registry.isSubTypeOf(nonNullType("Dog"), type("Mammal"))
415+
!registry.isSubTypeOf(type("Dog"), nonNullType("Mammal")) // but not the other way around
416+
417+
registry.isSubTypeOf(listType("Mammal"), listType("Mammal"))
418+
!registry.isSubTypeOf(listType("Mammal"), type("Mammal")) // but not if they aren't both lists
419+
420+
// unwraps all the way down
421+
registry.isSubTypeOf(listType(nonNullType(listType(type("Dog")))), listType(nonNullType(listType(type("Mammal")))))
422+
! registry.isSubTypeOf(listType(nonNullType(listType(type("Turtle")))), listType(nonNullType(listType(type("Mammal")))))
423+
424+
}
381425
}

0 commit comments

Comments
 (0)