Skip to content

Commit a82f21b

Browse files
andimarekandimarek
authored
Merge pull request #4447 from graphql-java/codex/backport-4388-to-26.x
Backport 26.1: Allow empty unions to be completed by extensions
2 parents 5476801 + ea4ac10 commit a82f21b

4 files changed

Lines changed: 174 additions & 23 deletions

File tree

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

Lines changed: 55 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import graphql.language.TypeDefinition;
1717
import graphql.language.TypeName;
1818
import graphql.language.UnionTypeDefinition;
19+
import graphql.language.UnionTypeExtensionDefinition;
1920
import graphql.schema.idl.errors.MissingTypeError;
2021
import graphql.schema.idl.errors.NonUniqueArgumentError;
2122
import graphql.schema.idl.errors.NonUniqueNameError;
@@ -158,26 +159,64 @@ private void checkUnionTypeExtensions(List<GraphQLError> errors, TypeDefinitionR
158159
typeRegistry.unionTypeExtensions()
159160
.forEach((name, extensions) -> {
160161
checkTypeExtensionHasCorrespondingType(errors, typeRegistry, name, extensions, UnionTypeDefinition.class);
162+
Set<String> previousMemberTypes = unionMemberTypes(typeRegistry, name);
161163

162-
extensions.forEach(extension -> {
163-
List<TypeName> memberTypes = extension.getMemberTypes().stream()
164-
.map(t -> TypeInfo.typeInfo(t).getTypeName()).collect(Collectors.toList());
165-
166-
checkNamedUniqueness(errors, memberTypes, TypeName::getName,
167-
(namedMember, memberType) -> new NonUniqueNameError(extension, namedMember));
168-
169-
memberTypes.forEach(
170-
memberType -> {
171-
ObjectTypeDefinition unionTypeDefinition = typeRegistry.getTypeOrNull(memberType, ObjectTypeDefinition.class);
172-
if (unionTypeDefinition == null) {
173-
errors.add(new MissingTypeError("union member", extension, memberType));
174-
}
175-
}
176-
);
177-
});
164+
extensions.forEach(extension -> checkUnionTypeExtension(errors, typeRegistry, previousMemberTypes, extension));
178165
});
179166
}
180167

168+
private void checkUnionTypeExtension(List<GraphQLError> errors, TypeDefinitionRegistry typeRegistry, Set<String> previousMemberTypes, UnionTypeExtensionDefinition extension) {
169+
List<TypeName> memberTypes = extension.getMemberTypes().stream()
170+
.map(t -> TypeInfo.typeInfo(t).getTypeName()).collect(Collectors.toList());
171+
172+
checkNamedUniqueness(errors, memberTypes, TypeName::getName,
173+
(namedMember, memberType) -> new NonUniqueNameError(extension, namedMember));
174+
175+
memberTypes.forEach(memberType -> checkUnionMemberTypeExists(errors, typeRegistry, extension, memberType));
176+
checkUnionMemberTypesAreNew(errors, previousMemberTypes, extension, memberTypes);
177+
}
178+
179+
private void checkUnionMemberTypeExists(List<GraphQLError> errors, TypeDefinitionRegistry typeRegistry, UnionTypeExtensionDefinition extension, TypeName memberType) {
180+
ObjectTypeDefinition unionTypeDefinition = typeRegistry.getTypeOrNull(memberType, ObjectTypeDefinition.class);
181+
if (unionTypeDefinition != null) {
182+
return;
183+
}
184+
errors.add(new MissingTypeError("union member", extension, memberType));
185+
}
186+
187+
private void checkUnionMemberTypesAreNew(List<GraphQLError> errors, Set<String> previousMemberTypes, UnionTypeExtensionDefinition extension, List<TypeName> memberTypes) {
188+
Set<String> duplicateMemberTypes = duplicateMemberTypes(memberTypes);
189+
memberTypes.stream()
190+
.filter(memberType -> !duplicateMemberTypes.contains(memberType.getName()))
191+
.filter(memberType -> previousMemberTypes.contains(memberType.getName()))
192+
.forEach(memberType -> errors.add(new NonUniqueNameError(extension, memberType.getName())));
193+
194+
memberTypes.forEach(memberType -> previousMemberTypes.add(memberType.getName()));
195+
}
196+
197+
private Set<String> duplicateMemberTypes(List<TypeName> memberTypes) {
198+
Set<String> seen = new HashSet<>();
199+
Set<String> duplicates = new HashSet<>();
200+
memberTypes.forEach(memberType -> {
201+
if (!seen.add(memberType.getName())) {
202+
duplicates.add(memberType.getName());
203+
}
204+
});
205+
return duplicates;
206+
}
207+
208+
private Set<String> unionMemberTypes(TypeDefinitionRegistry typeRegistry, String name) {
209+
Set<String> memberTypes = new HashSet<>();
210+
UnionTypeDefinition baseTypeDef = typeRegistry.getTypeOrNull(name, UnionTypeDefinition.class);
211+
if (baseTypeDef == null) {
212+
return memberTypes;
213+
}
214+
baseTypeDef.getMemberTypes().stream()
215+
.map(t -> TypeInfo.typeInfo(t).getTypeName().getName())
216+
.forEach(memberTypes::add);
217+
return memberTypes;
218+
}
219+
181220
/*
182221
* Enum type extensions have the potential to be invalid if incorrectly defined.
183222
*

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313
import java.util.LinkedHashSet;
1414
import java.util.List;
1515
import java.util.Set;
16-
import java.util.stream.Stream;
1716

1817
import static java.lang.String.format;
18+
import static java.util.Collections.emptyList;
1919

2020
/**
2121
* UnionType check, details in https://spec.graphql.org/June2018/#sec-Type-System.
@@ -33,18 +33,15 @@ class UnionTypesChecker {
3333

3434
void checkUnionType(List<GraphQLError> errors, TypeDefinitionRegistry typeRegistry) {
3535
List<UnionTypeDefinition> unionTypes = typeRegistry.getTypes(UnionTypeDefinition.class);
36-
List<UnionTypeExtensionDefinition> unionTypeExtensions = typeRegistry.getTypes(UnionTypeExtensionDefinition.class);
3736

38-
Stream.concat(unionTypes.stream(), unionTypeExtensions.stream())
39-
.forEach(type -> checkUnionType(typeRegistry, type, errors));
37+
unionTypes.forEach(type -> checkUnionType(typeRegistry, type, errors));
4038
}
4139

4240
private void checkUnionType(TypeDefinitionRegistry typeRegistry, UnionTypeDefinition unionTypeDefinition, List<GraphQLError> errors) {
4341
assertTypeName(unionTypeDefinition, errors);
4442

45-
//noinspection rawtypes
4643
List<Type> memberTypes = unionTypeDefinition.getMemberTypes();
47-
if (memberTypes == null || memberTypes.isEmpty()) {
44+
if (!hasMemberTypes(typeRegistry, unionTypeDefinition)) {
4845
errors.add(new UnionTypeError(unionTypeDefinition, format("Union type '%s' must include one or more member types.", unionTypeDefinition.getName())));
4946
return;
5047
}
@@ -66,6 +63,16 @@ private void checkUnionType(TypeDefinitionRegistry typeRegistry, UnionTypeDefini
6663
}
6764
}
6865

66+
private boolean hasMemberTypes(TypeDefinitionRegistry typeRegistry, UnionTypeDefinition unionTypeDefinition) {
67+
if (!unionTypeDefinition.getMemberTypes().isEmpty()) {
68+
return true;
69+
}
70+
71+
List<UnionTypeExtensionDefinition> extensions = typeRegistry.unionTypeExtensions()
72+
.getOrDefault(unionTypeDefinition.getName(), emptyList());
73+
return extensions.stream().anyMatch(extension -> !extension.getMemberTypes().isEmpty());
74+
}
75+
6976
private void assertTypeName(UnionTypeDefinition unionTypeDefinition, List<GraphQLError> errors) {
7077
if (unionTypeDefinition.getName().length() >= 2 && unionTypeDefinition.getName().startsWith("__")) {
7178
errors.add((new UnionTypeError(unionTypeDefinition, String.format("'%s' must not begin with '__', which is reserved by GraphQL introspection.", unionTypeDefinition.getName()))));

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import graphql.schema.idl.errors.NotAnInputTypeError
2929
import graphql.schema.idl.errors.NotAnOutputTypeError
3030
import graphql.schema.idl.errors.SchemaProblem
3131
import graphql.schema.visibility.GraphqlFieldVisibility
32+
import spock.lang.Issue
3233
import spock.lang.Specification
3334

3435
import java.util.function.UnaryOperator
@@ -1530,6 +1531,64 @@ class SchemaGeneratorTest extends Specification {
15301531
unionType.directivesByName.containsKey("directive")
15311532
}
15321533

1534+
@Issue("https://github.com/graphql-java/graphql-java/issues/4200")
1535+
def "empty union base definition gets member types from extension"() {
1536+
def spec = """
1537+
type Cat {
1538+
meow: String
1539+
}
1540+
1541+
type Dog {
1542+
bark: String
1543+
}
1544+
1545+
union Pet
1546+
1547+
extend union Pet = Cat | Dog
1548+
1549+
type Query {
1550+
pet: Pet
1551+
}
1552+
"""
1553+
1554+
when:
1555+
def schema = schema(spec)
1556+
GraphQLUnionType unionType = schema.getType("Pet") as GraphQLUnionType
1557+
1558+
then:
1559+
unionType.types*.name == ["Cat", "Dog"]
1560+
}
1561+
1562+
@Issue("https://github.com/graphql-java/graphql-java/issues/4200")
1563+
def "empty union base definition gets member types from multiple extensions"() {
1564+
def spec = """
1565+
type Cat {
1566+
meow: String
1567+
}
1568+
1569+
type Dog {
1570+
bark: String
1571+
}
1572+
1573+
union Pet
1574+
1575+
extend union Pet = | Cat
1576+
1577+
extend union Pet = Dog
1578+
1579+
type Query {
1580+
pet: Pet
1581+
}
1582+
"""
1583+
1584+
when:
1585+
def schema = schema(spec)
1586+
GraphQLUnionType unionType = schema.getType("Pet") as GraphQLUnionType
1587+
1588+
then:
1589+
unionType.types*.name == ["Cat", "Dog"]
1590+
}
1591+
15331592
def "enum extension types are combined"() {
15341593
def spec = """
15351594
type Query {

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

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1179,7 +1179,7 @@ class SchemaTypeCheckerTest extends Specification {
11791179

11801180
expect:
11811181

1182-
result.size() == 3
1182+
result.size() == 4
11831183
errorContaining(result, "The extension 'NonExistent' type [@n:n] is missing its base underlying type")
11841184
errorContaining(result, "The union member type 'Buzz' is not present when resolving type 'FooBar' [@n:n]")
11851185
errorContaining(result, "The type 'FooBar' [@n:n] has declared an union member with a non unique name 'Foo'")
@@ -1952,6 +1952,52 @@ class SchemaTypeCheckerTest extends Specification {
19521952
errorContaining(result, "Union type 'UnionType' must include one or more member types.")
19531953
}
19541954

1955+
def "union type with directive only extension must include one or more member types"() {
1956+
given:
1957+
def sdl = """
1958+
directive @directive on UNION
1959+
1960+
type Query { hello: String }
1961+
1962+
union UnionType
1963+
1964+
extend union UnionType @directive
1965+
"""
1966+
1967+
when:
1968+
def result = check(sdl)
1969+
1970+
then:
1971+
errorContaining(result, "Union type 'UnionType' must include one or more member types.")
1972+
}
1973+
1974+
@Unroll
1975+
def "union extension must not redefine member types from previous union type: #scenario"() {
1976+
given:
1977+
def sdl = """
1978+
type Query { pet: Pet }
1979+
1980+
type Cat {
1981+
id: ID
1982+
}
1983+
1984+
union Pet $baseMembers
1985+
1986+
$extensions
1987+
"""
1988+
1989+
when:
1990+
def result = check(sdl)
1991+
1992+
then:
1993+
errorContaining(result, "The type 'Pet' [@n:n] has declared an union member with a non unique name 'Cat'")
1994+
1995+
where:
1996+
scenario | baseMembers | extensions
1997+
"base definition" | "= Cat" | "extend union Pet = Cat"
1998+
"earlier extension" | "" | "extend union Pet = Cat\nextend union Pet = Cat"
1999+
}
2000+
19552001
def "The member types of a Union type must all be object base types"() {
19562002
given:
19572003
def sdl = """

0 commit comments

Comments
 (0)