Skip to content

Commit 5e57f9a

Browse files
ashpak-shaikhashpak-shaikh
authored
Merge pull request #157 from graph-quilt/reusing-types-conflict
Preventing service providers from registering if they introduce types…
2 parents 9143234 + 0127c6d commit 5e57f9a

File tree

6 files changed

+138
-32
lines changed

6 files changed

+138
-32
lines changed

‎src/main/java/com/intuit/graphql/orchestrator/schema/transform/FederationTransformerPostMerge.java

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,5 @@
11
package com.intuit.graphql.orchestrator.schema.transform;
22

3-
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_DIRECTIVE_NAME;
4-
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_EXTENDS_DIRECTIVE;
5-
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_EXTERNAL_DIRECTIVE;
6-
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_INACCESSIBLE_DIRECTIVE;
7-
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_KEY_DIRECTIVE;
8-
import static com.intuit.graphql.orchestrator.utils.FederationUtils.isTypeSystemForBaseType;
9-
import static com.intuit.graphql.orchestrator.utils.XtextGraphUtils.addToCodeRegistry;
10-
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.getFieldDefinitions;
11-
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isInterfaceTypeDefinition;
12-
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isInterfaceTypeExtensionDefinition;
13-
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isObjectTypeDefinition;
14-
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isObjectTypeExtensionDefinition;
15-
import static com.intuit.graphql.orchestrator.utils.XtextUtils.definitionContainsDirective;
16-
import static java.lang.String.format;
17-
183
import com.intuit.graphql.graphQL.Directive;
194
import com.intuit.graphql.graphQL.FieldDefinition;
205
import com.intuit.graphql.graphQL.TypeDefinition;
@@ -32,14 +17,30 @@
3217
import com.intuit.graphql.orchestrator.xtext.DataFetcherContext;
3318
import com.intuit.graphql.orchestrator.xtext.FieldContext;
3419
import com.intuit.graphql.orchestrator.xtext.UnifiedXtextGraph;
20+
import org.apache.commons.lang3.StringUtils;
21+
3522
import java.util.Collection;
3623
import java.util.List;
3724
import java.util.Objects;
3825
import java.util.Optional;
3926
import java.util.concurrent.atomic.AtomicBoolean;
4027
import java.util.stream.Collectors;
4128
import java.util.stream.Stream;
42-
import org.apache.commons.lang3.StringUtils;
29+
30+
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_DIRECTIVE_NAME;
31+
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_EXTENDS_DIRECTIVE;
32+
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_EXTERNAL_DIRECTIVE;
33+
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_INACCESSIBLE_DIRECTIVE;
34+
import static com.intuit.graphql.orchestrator.utils.FederationConstants.FEDERATION_KEY_DIRECTIVE;
35+
import static com.intuit.graphql.orchestrator.utils.FederationUtils.isTypeSystemForBaseType;
36+
import static com.intuit.graphql.orchestrator.utils.XtextGraphUtils.addToCodeRegistry;
37+
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.getFieldDefinitions;
38+
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isInterfaceTypeDefinition;
39+
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isInterfaceTypeExtensionDefinition;
40+
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isObjectTypeDefinition;
41+
import static com.intuit.graphql.orchestrator.utils.XtextTypeUtils.isObjectTypeExtensionDefinition;
42+
import static com.intuit.graphql.orchestrator.utils.XtextUtils.definitionContainsDirective;
43+
import static java.lang.String.format;
4344

4445
public class FederationTransformerPostMerge implements Transformer<UnifiedXtextGraph, UnifiedXtextGraph> {
4546

@@ -124,8 +125,11 @@ private void pruneInaccessibleInfo(UnifiedXtextGraph xtextGraph, Collection<Type
124125
xtextGraph.getTypes().remove(typeDefinition.getName());
125126
xtextGraph.getBlacklistedTypes().add(typeDefinition.getName());
126127
} else {
127-
getFieldDefinitions(typeDefinition, true)
128+
boolean containsInaccessibleFields = getFieldDefinitions(typeDefinition, true)
128129
.removeIf(fieldDefinition -> definitionContainsDirective(fieldDefinition, FEDERATION_INACCESSIBLE_DIRECTIVE));
130+
if(containsInaccessibleFields) {
131+
xtextGraph.getTypesWithInaccessibleFields().add(typeDefinition.getName());
132+
}
129133
}
130134
});
131135
}

‎src/main/java/com/intuit/graphql/orchestrator/stitching/XtextStitcher.java

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,5 @@
11
package com.intuit.graphql.orchestrator.stitching;
22

3-
import static com.intuit.graphql.orchestrator.batch.DataLoaderKeyUtil.createDataLoaderKey;
4-
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_ARGUMENT_INPUT_NAME;
5-
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_DIRECTIVE_NAME;
6-
import static com.intuit.graphql.orchestrator.utils.XtextUtils.getAllTypes;
7-
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.ENTITY_FETCHER;
8-
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.RESOLVER_ARGUMENT;
9-
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.RESOLVER_ON_FIELD_DEFINITION;
10-
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.SERVICE;
11-
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.STATIC;
12-
import static graphql.schema.FieldCoordinates.coordinates;
13-
import static java.util.Objects.requireNonNull;
14-
153
import com.intuit.graphql.graphQL.ObjectTypeDefinition;
164
import com.intuit.graphql.graphQL.TypeDefinition;
175
import com.intuit.graphql.orchestrator.ServiceProvider;
@@ -61,6 +49,8 @@
6149
import graphql.schema.GraphQLObjectType;
6250
import graphql.schema.GraphQLType;
6351
import graphql.schema.StaticDataFetcher;
52+
import org.dataloader.BatchLoader;
53+
6454
import java.util.Arrays;
6555
import java.util.Collections;
6656
import java.util.EnumMap;
@@ -70,7 +60,18 @@
7060
import java.util.Set;
7161
import java.util.function.Function;
7262
import java.util.stream.Collectors;
73-
import org.dataloader.BatchLoader;
63+
64+
import static com.intuit.graphql.orchestrator.batch.DataLoaderKeyUtil.createDataLoaderKey;
65+
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_ARGUMENT_INPUT_NAME;
66+
import static com.intuit.graphql.orchestrator.resolverdirective.FieldResolverDirectiveUtil.RESOLVER_DIRECTIVE_NAME;
67+
import static com.intuit.graphql.orchestrator.utils.XtextUtils.getAllTypes;
68+
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.ENTITY_FETCHER;
69+
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.RESOLVER_ARGUMENT;
70+
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.RESOLVER_ON_FIELD_DEFINITION;
71+
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.SERVICE;
72+
import static com.intuit.graphql.orchestrator.xtext.DataFetcherContext.DataFetcherType.STATIC;
73+
import static graphql.schema.FieldCoordinates.coordinates;
74+
import static java.util.Objects.requireNonNull;
7475

7576
/**
7677
* The type Xtext stitcher.
@@ -287,6 +288,7 @@ private RuntimeGraph.Builder createRuntimeGraph(UnifiedXtextGraph unifiedXtextGr
287288

288289
XtextToGraphQLJavaVisitor visitor = XtextToGraphQLJavaVisitor.newBuilder()
289290
.graphqlBlackList(unifiedXtextGraph.getBlacklistedTypes())
291+
.typesWithInaccessibleFields(unifiedXtextGraph.getTypesWithInaccessibleFields())
290292
.build();
291293
//fill operations
292294
final Map<Operation, GraphQLObjectType> operationMap = new EnumMap<>(Operation.class);

‎src/main/java/com/intuit/graphql/orchestrator/utils/XtextToGraphQLJavaVisitor.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import com.intuit.graphql.orchestrator.datafetcher.AliasablePropertyDataFetcher;
2828
import com.intuit.graphql.orchestrator.schema.SchemaParseException;
2929
import com.intuit.graphql.orchestrator.schema.transform.ExplicitTypeResolver;
30+
import com.intuit.graphql.orchestrator.stitching.StitchingException;
3031
import graphql.Scalars;
3132
import graphql.introspection.Introspection.DirectiveLocation;
3233
import graphql.language.StringValue;
@@ -83,8 +84,10 @@ public class XtextToGraphQLJavaVisitor extends GraphQLSwitch<GraphQLSchemaElemen
8384
private final Map<String, GraphQLType> graphQLObjectTypes;
8485
public final Map<String, GraphQLDirective> directiveDefinitions;
8586
public final HashSet<String> blacklistedTypes;
87+
public final Set<String> typesWithInaccessibleFields;
8688

8789
private static final GraphQLScalarType FIELD_SET_SCALAR;
90+
private static final String ERRMSG_REUSED_CONFLICTING_TYPE = "FORBIDDEN: Subgraphs %s are reusing type %s with different field definitions.";
8891

8992
static {
9093
STANDARD_SCALAR_TYPES = ScalarInfo.GRAPHQL_SPECIFICATION_SCALARS.stream()
@@ -122,6 +125,7 @@ private XtextToGraphQLJavaVisitor(Builder builder) {
122125
graphQLObjectTypes = builder.graphQLObjectTypes;
123126
directiveDefinitions = builder.directiveDefinitions;
124127
blacklistedTypes = Sets.newHashSet(builder.blacklistedTypes);
128+
typesWithInaccessibleFields = Sets.newHashSet(builder.typesWithInaccessibleFields);
125129

126130
createBuiltInDirectives();
127131
}
@@ -193,6 +197,13 @@ public GraphQLSchemaElement caseObjectTypeDefinition(final ObjectTypeDefinition
193197

194198
GraphQLType graphQLType = graphQLObjectTypes.get(me);
195199
if (Objects.nonNull(graphQLType)) {
200+
int cachedFieldSize = ((GraphQLObjectType) graphQLType).getFieldDefinitions().size();
201+
int incomingFieldSize = object.getFieldDefinition().size();
202+
//No need to check types with same field size and with different fields b/c it would conflict earlier on and would fail to stitch
203+
if(cachedFieldSize != incomingFieldSize && !typesWithInaccessibleFields.contains(me)) {
204+
String conflictingNamespace = (cachedFieldSize > incomingFieldSize) ? ((GraphQLObjectType) graphQLType).getDescription() : object.getDesc();
205+
throw new StitchingException(String.format(ERRMSG_REUSED_CONFLICTING_TYPE, conflictingNamespace, me));
206+
}
196207
return graphQLType;
197208
}
198209

@@ -604,6 +615,7 @@ public static final class Builder {
604615
private Map<String, GraphQLType> graphQLObjectTypes = new HashMap<>();
605616
private Map<String, GraphQLDirective> directiveDefinitions = new HashMap<>();
606617
private HashSet<String> blacklistedTypes = new HashSet<>();
618+
private Set<String> typesWithInaccessibleFields = new HashSet<>();
607619

608620
private Builder() {
609621
}
@@ -617,6 +629,10 @@ public Builder graphqlBlackList(Set<String> blacklistedTypes) {
617629
this.blacklistedTypes.addAll(blacklistedTypes);
618630
return this;
619631
}
632+
public Builder typesWithInaccessibleFields(Set<String> typesWithInaccessibleFields) {
633+
this.typesWithInaccessibleFields.addAll(typesWithInaccessibleFields);
634+
return this;
635+
}
620636

621637
public Builder directiveDefinitions(Map<String, GraphQLDirective> val) {
622638
directiveDefinitions.putAll(requireNonNull(val));

‎src/main/java/com/intuit/graphql/orchestrator/xtext/UnifiedXtextGraph.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ public class UnifiedXtextGraph {
4949
private final Map<String, FederationMetadata> federationMetadataByNamespace;
5050
private final Map<String, RenamedMetadata> renamedMetadataByNamespace;
5151
private final Set<String> blacklistedTypes = new HashSet<>();
52+
private final Set<String> typesWithInaccessibleFields = new HashSet<>();
5253

5354
UnifiedXtextGraph(Builder builder) {
5455
//TODO: Research on all Providers having an XtextResource instead of a ResourceSet

‎src/test/groovy/com/intuit/graphql/orchestrator/integration/NestedLevelStitchingSpec.groovy

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package com.intuit.graphql.orchestrator.integration
22

3+
import com.intuit.graphql.orchestrator.ServiceProvider
34
import com.intuit.graphql.orchestrator.datafetcher.ServiceDataFetcher
45
import com.intuit.graphql.orchestrator.schema.Operation
6+
import com.intuit.graphql.orchestrator.stitching.StitchingException
57
import graphql.schema.FieldCoordinates
68
import graphql.schema.GraphQLCodeRegistry
79
import graphql.schema.GraphQLSchema
@@ -18,6 +20,40 @@ class NestedLevelStitchingSpec extends BaseIntegrationTestSpecification {
1820

1921
def mockServiceResponse = new HashMap()
2022

23+
def conflictingSchema = """
24+
type Query {
25+
root: RootType
26+
topLevelField: NestedType
27+
}
28+
29+
type RootType {
30+
nestedField: NestedType
31+
}
32+
33+
type NestedType {
34+
fieldA: String
35+
fieldB: String
36+
}
37+
"""
38+
39+
def conflictingSchema2 = """
40+
type Query {
41+
root: RootType
42+
}
43+
44+
type RootType {
45+
nestedField: NestedType
46+
}
47+
48+
type NestedType {
49+
fieldC: String
50+
fieldD: String
51+
}
52+
"""
53+
ServiceProvider topLevelDeprecatedService1
54+
ServiceProvider topLevelDeprecatedService2
55+
56+
2157
@Subject
2258
def specUnderTest
2359

@@ -62,7 +98,6 @@ class NestedLevelStitchingSpec extends BaseIntegrationTestSpecification {
6298
codeRegistry?.getDataFetcher(FieldCoordinates.coordinates("ConsumerType", "turboExperiences"), turboExperiences) instanceof ServiceDataFetcher
6399
}
64100

65-
66101
def "Nested Type Description With Namespace And Empty Description"() {
67102
given:
68103
def bSchema = "schema { query: Query } type Query { a: A } \"\n" +\
@@ -142,5 +177,16 @@ class NestedLevelStitchingSpec extends BaseIntegrationTestSpecification {
142177
aType.description.contains("description for schema2")
143178
}
144179

180+
def "deprecated fields can not be referenced again"() {
181+
given:
182+
topLevelDeprecatedService1 = createSimpleMockService("test1", conflictingSchema, new HashMap<String, Object>())
183+
topLevelDeprecatedService2 = createSimpleMockService("test2", conflictingSchema2, new HashMap<String, Object>())
184+
185+
when:
186+
specUnderTest = createGraphQLOrchestrator([topLevelDeprecatedService1, topLevelDeprecatedService2])
145187

188+
then:
189+
def exception = thrown(StitchingException)
190+
exception.message == "FORBIDDEN: Subgraphs [test2,test1] are reusing type NestedType with different field definitions."
191+
}
146192
}

‎src/test/groovy/com/intuit/graphql/orchestrator/schema/transform/FederationTransformerPostMergeSpec.groovy

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ class FederationTransformerPostMergeSpec extends Specification {
4141
return entitiesByTypeName
4242
}
4343

44-
Map<String, Map<String, TypeSystemDefinition>> getEntityExtensions() {
44+
Map<String, Map<String, TypeSystemDefinition>> getEntityExtensions() {
4545
ObjectTypeDefinition objectTypeExtension =
4646
buildObjectTypeDefinition("EntityType", singletonList(EXTENSION_FIELD_DEFINITION))
4747

@@ -148,6 +148,43 @@ class FederationTransformerPostMergeSpec extends Specification {
148148
actual.is(unifiedXtextGraphMock)
149149
}
150150

151+
def "transform adds inaccessible info"() {
152+
given:
153+
def unifiedXtextGraphMock = Mock(UnifiedXtextGraph)
154+
155+
Directive inaccessibleDirective = buildDirective(buildDirectiveDefinition("inaccessible"), new ArrayList<Argument>())
156+
FieldDefinition inaccessibleField = buildFieldDefinition("inactiveField", singletonList(inaccessibleDirective))
157+
FieldDefinition accessibleField = buildFieldDefinition("activeField")
158+
159+
List<FieldDefinition> inaccessibleFieldList = new ArrayList<>()
160+
inaccessibleFieldList.add(accessibleField)
161+
inaccessibleFieldList.add(inaccessibleField)
162+
163+
List<FieldDefinition> fieldList = new ArrayList<>()
164+
fieldList.add(accessibleField)
165+
166+
ObjectTypeDefinition objectTypeDefinition = buildObjectTypeDefinition("TestObject", fieldList)
167+
ObjectTypeDefinition InaccessibleObjectDefinition = buildObjectTypeDefinition("InaccessibleTestObject", inaccessibleFieldList)
168+
169+
def valueTypesByName = new HashMap()
170+
valueTypesByName.put("TestObject", objectTypeDefinition)
171+
valueTypesByName.put("InaccessibleTestObject", InaccessibleObjectDefinition)
172+
173+
Set<String> inaccessibleTypes = new HashSet()
174+
175+
unifiedXtextGraphMock.getTypesWithInaccessibleFields() >> inaccessibleTypes
176+
unifiedXtextGraphMock.getEntityExtensionsByNamespace() >> new HashMap<>()
177+
unifiedXtextGraphMock.getEntityExtensionMetadatas() >> []
178+
unifiedXtextGraphMock.getValueTypesByName() >> valueTypesByName
179+
180+
when:
181+
UnifiedXtextGraph actual = subjectUnderTest.transform(unifiedXtextGraphMock)
182+
183+
then:
184+
actual.getTypesWithInaccessibleFields().size() == 1
185+
actual.getTypesWithInaccessibleFields().getAt(0) == "InaccessibleTestObject"
186+
}
187+
151188
def "transform fails extension key not subset"() {
152189
given:
153190
def unifiedXtextGraphMock = Mock(UnifiedXtextGraph)

0 commit comments

Comments
 (0)