From a7af3753a4c9345a04c9a4da9c4646bf1739e8ef Mon Sep 17 00:00:00 2001 From: Ruediger zu Dohna Date: Sun, 8 Jan 2023 15:35:46 +0100 Subject: [PATCH] fix #1689: properly add/remove federation directives --- .../graphql/schema/SchemaBuilder.java | 10 +- .../graphql/schema/model/DirectiveType.java | 4 + .../smallrye/graphql/bootstrap/Bootstrap.java | 48 ++++--- .../smallrye/graphql/schema/SchemaTest.java | 123 +++++++++--------- 4 files changed, 93 insertions(+), 92 deletions(-) diff --git a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/SchemaBuilder.java b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/SchemaBuilder.java index 4cc790c13..f9b382c24 100644 --- a/common/schema-builder/src/main/java/io/smallrye/graphql/schema/SchemaBuilder.java +++ b/common/schema-builder/src/main/java/io/smallrye/graphql/schema/SchemaBuilder.java @@ -69,8 +69,6 @@ public class SchemaBuilder { private final DirectiveTypeCreator directiveTypeCreator; private final UnionCreator unionCreator; - private final DotName FEDERATION_ANNOTATIONS_PACKAGE = DotName.createSimple("io.smallrye.graphql.api.federation"); - /** * This builds the Schema from Jandex * @@ -162,13 +160,7 @@ private void addDirectiveTypes(Schema schema) { // custom directives from annotations for (AnnotationInstance annotationInstance : ScanningContext.getIndex().getAnnotations(DIRECTIVE)) { ClassInfo classInfo = annotationInstance.target().asClass(); - boolean federationEnabled = Boolean.getBoolean("smallrye.graphql.federation.enabled"); - // only add federation-related directive types to the schema if federation is enabled - DotName packageName = classInfo.name().packagePrefixName(); - if (packageName == null || !packageName.equals(FEDERATION_ANNOTATIONS_PACKAGE) || federationEnabled) { - schema.addDirectiveType(directiveTypeCreator.create(classInfo)); - } - + schema.addDirectiveType(directiveTypeCreator.create(classInfo)); } // bean validation directives schema.addDirectiveType(BeanValidationDirectivesHelper.CONSTRAINT_DIRECTIVE_TYPE); diff --git a/common/schema-model/src/main/java/io/smallrye/graphql/schema/model/DirectiveType.java b/common/schema-model/src/main/java/io/smallrye/graphql/schema/model/DirectiveType.java index 0127e92c7..4b519b3a4 100644 --- a/common/schema-model/src/main/java/io/smallrye/graphql/schema/model/DirectiveType.java +++ b/common/schema-model/src/main/java/io/smallrye/graphql/schema/model/DirectiveType.java @@ -69,6 +69,10 @@ public void setRepeatable(boolean repeatable) { this.repeatable = repeatable; } + public boolean isFederation() { + return className != null && className.startsWith("io.smallrye.graphql.api.federation"); + } + /** * Helper 'getter' methods, but DON'T add 'get' into their names, otherwise it breaks Quarkus bytecode recording, * because they would be detected as actual property getters while they are actually not diff --git a/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java b/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java index 912225a0d..d9335c9b3 100644 --- a/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java +++ b/server/implementation/src/main/java/io/smallrye/graphql/bootstrap/Bootstrap.java @@ -220,11 +220,17 @@ private TypeResolver fetchEntityType() { private void createGraphQLDirectiveTypes() { if (schema.hasDirectiveTypes()) { for (DirectiveType directiveType : schema.getDirectiveTypes()) { - createGraphQLDirectiveType(directiveType); + if (enabled(directiveType)) { + createGraphQLDirectiveType(directiveType); + } } } } + private static boolean enabled(DirectiveType directiveType) { + return Config.get().isFederationEnabled() || !directiveType.isFederation(); + } + private void createGraphQLDirectiveType(DirectiveType directiveType) { GraphQLDirective.Builder directiveBuilder = GraphQLDirective.newDirective() .name(directiveType.getName()) @@ -372,7 +378,8 @@ private void createGraphQLEnumType(EnumType enumType) { .description(enumType.getDescription()); // Directives if (enumType.hasDirectiveInstances()) { - enumBuilder = enumBuilder.withDirectives(createGraphQLDirectives(enumType.getDirectiveInstances())); + enumBuilder = enumBuilder + .withDirectives(createGraphQLDirectives(enumType.getDirectiveInstances())); } // Values for (EnumValue value : enumType.getValues()) { @@ -381,7 +388,8 @@ private void createGraphQLEnumType(EnumType enumType) { .value(value.getValue()) .description(value.getDescription()); if (value.hasDirectiveInstances()) { - definitionBuilder = definitionBuilder.withDirectives(createGraphQLDirectives(value.getDirectiveInstances())); + definitionBuilder = definitionBuilder + .withDirectives(createGraphQLDirectives(value.getDirectiveInstances())); } enumBuilder = enumBuilder.value(definitionBuilder.build()); } @@ -411,9 +419,8 @@ private void createGraphQLInterfaceType(Type interfaceType) { // Directives if (interfaceType.hasDirectiveInstances()) { - for (DirectiveInstance directiveInstance : interfaceType.getDirectiveInstances()) { - interfaceTypeBuilder.withDirective(createGraphQLDirectiveFrom(directiveInstance)); - } + interfaceTypeBuilder = interfaceTypeBuilder + .withDirectives(createGraphQLDirectives(interfaceType.getDirectiveInstances())); } // Interfaces @@ -502,7 +509,7 @@ private GraphQLInputObjectType createGraphQLInputObjectType(InputType inputType) // Directives if (inputType.hasDirectiveInstances()) { inputObjectTypeBuilder = inputObjectTypeBuilder - .withDirectives(createGraphQLDirectives(inputType.getDirectiveInstances())); + .withDirectives(createGraphQLDirectives(inputType.getDirectiveInstances())); } // Fields @@ -535,9 +542,8 @@ private void createGraphQLObjectType(Type type) { // Directives if (type.hasDirectiveInstances()) { - for (DirectiveInstance directiveInstance : type.getDirectiveInstances()) { - objectTypeBuilder.withDirective(createGraphQLDirectiveFrom(directiveInstance)); - } + objectTypeBuilder = objectTypeBuilder + .withDirectives(createGraphQLDirectives(type.getDirectiveInstances())); } // Fields @@ -657,7 +663,8 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromOperation(String // Directives if (operation.hasDirectiveInstances()) { - fieldBuilder = fieldBuilder.withDirectives(createGraphQLDirectives(operation.getDirectiveInstances())); + fieldBuilder = fieldBuilder + .withDirectives(createGraphQLDirectives(operation.getDirectiveInstances())); } GraphQLFieldDefinition graphQLFieldDefinition = fieldBuilder.build(); @@ -673,6 +680,7 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromOperation(String private GraphQLDirective[] createGraphQLDirectives(Collection directiveInstances) { return directiveInstances.stream() + .filter(directiveInstance -> enabled(directiveInstance.getType())) .map(this::createGraphQLDirectiveFrom) .toArray(GraphQLDirective[]::new); } @@ -695,9 +703,8 @@ private GraphQLFieldDefinition createGraphQLFieldDefinitionFromField(Reference o // Directives if (field.hasDirectiveInstances()) { - for (DirectiveInstance directiveInstance : field.getDirectiveInstances()) { - fieldBuilder.withDirective(createGraphQLDirectiveFrom(directiveInstance)); - } + fieldBuilder = fieldBuilder + .withDirectives(createGraphQLDirectives(field.getDirectiveInstances())); } // Auto Map argument @@ -770,10 +777,10 @@ private GraphQLInputObjectField createGraphQLInputObjectFieldFromField(Field fie // Type inputFieldBuilder = inputFieldBuilder.type(createGraphQLInputType(field)); + // Directives if (field.hasDirectiveInstances()) { - for (DirectiveInstance directiveInstance : field.getDirectiveInstances()) { - inputFieldBuilder.withDirective(createGraphQLDirectiveFrom(directiveInstance)); - } + inputFieldBuilder = inputFieldBuilder + .withDirectives(createGraphQLDirectives(field.getDirectiveInstances())); } // Default value (on method) @@ -942,12 +949,13 @@ private GraphQLArgument createGraphQLArgument(Argument argument) { graphQLInputType = GraphQLNonNull.nonNull(graphQLInputType); } + // Type argumentBuilder = argumentBuilder.type(graphQLInputType); + // Directives if (argument.hasDirectiveInstances()) { - for (DirectiveInstance directiveInstance : argument.getDirectiveInstances()) { - argumentBuilder.withDirective(createGraphQLDirectiveFrom(directiveInstance)); - } + argumentBuilder = argumentBuilder + .withDirectives(createGraphQLDirectives(argument.getDirectiveInstances())); } return argumentBuilder.build(); diff --git a/server/implementation/src/test/java/io/smallrye/graphql/schema/SchemaTest.java b/server/implementation/src/test/java/io/smallrye/graphql/schema/SchemaTest.java index 1eb3dea3b..92611e17b 100644 --- a/server/implementation/src/test/java/io/smallrye/graphql/schema/SchemaTest.java +++ b/server/implementation/src/test/java/io/smallrye/graphql/schema/SchemaTest.java @@ -4,6 +4,7 @@ import static graphql.introspection.Introspection.DirectiveLocation.INTERFACE; import static graphql.introspection.Introspection.DirectiveLocation.OBJECT; import static java.util.Objects.requireNonNull; +import static java.util.stream.Collectors.toSet; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -17,6 +18,7 @@ import java.net.URISyntaxException; import java.nio.file.Files; import java.util.EnumSet; +import java.util.Set; import java.util.stream.Stream; import org.jboss.jandex.IndexView; @@ -121,14 +123,15 @@ void schemaWithInputDirectives() { @Test void testSchemaWithFederationDisabled() { config.federationEnabled = false; - // need to set it as system property because the SchemaBuilder doesn't have access to the Config object - System.setProperty("smallrye.graphql.federation.enabled", "false"); GraphQLSchema graphQLSchema = createGraphQLSchema(Directive.class, Key.class, Keys.class, TestTypeWithFederation.class, FederationTestApi.class); - assertNull(graphQLSchema.getDirective("key")); + assertEquals(Set.of("include", "specifiedBy", "deprecated", "skip", "constraint"), + graphQLSchema.getDirectives().stream().map(GraphQLDirective::getName).collect(toSet())); + assertNull(graphQLSchema.getDirective("key")); // esp. NOT this one assertNull(graphQLSchema.getType("_Entity")); + assertNull(graphQLSchema.getType("_Service")); GraphQLObjectType queryRoot = graphQLSchema.getQueryType(); assertEquals(1, queryRoot.getFields().size()); @@ -156,66 +159,60 @@ void testSchemaWithFederationDisabled() { @Test void testSchemaWithFederationEnabled() { config.federationEnabled = true; - // need to set it as system property because the SchemaBuilder doesn't have access to the Config object - System.setProperty("smallrye.graphql.federation.enabled", "true"); - try { - GraphQLSchema graphQLSchema = createGraphQLSchema(Repeatable.class, Directive.class, Key.class, Keys.class, - TestTypeWithFederation.class, FederationTestApi.class); - - GraphQLDirective keyDirective = graphQLSchema.getDirective("key"); - assertEquals("key", keyDirective.getName()); - assertTrue(keyDirective.isRepeatable()); - assertEquals( - "Designates an object type as an entity and specifies its key fields " + - "(a set of fields that the subgraph can use to uniquely identify any instance " + - "of the entity). You can apply multiple @key directives to a single entity " + - "(to specify multiple valid sets of key fields).", - keyDirective.getDescription()); - assertEquals(EnumSet.of(OBJECT, INTERFACE), keyDirective.validLocations()); - assertEquals(1, keyDirective.getArguments().size()); - assertEquals("String", ((GraphQLScalarType) keyDirective.getArgument("fields").getType()).getName()); - - GraphQLUnionType entityType = (GraphQLUnionType) graphQLSchema.getType("_Entity"); - assertNotNull(entityType); - assertEquals(1, entityType.getTypes().size()); - assertEquals(TestTypeWithFederation.class.getSimpleName(), entityType.getTypes().get(0).getName()); - - GraphQLObjectType queryRoot = graphQLSchema.getQueryType(); - assertEquals(3, queryRoot.getFields().size()); - - GraphQLFieldDefinition entities = queryRoot.getField("_entities"); - assertEquals(1, entities.getArguments().size()); - assertEquals("[_Any!]!", entities.getArgument("representations").getType().toString()); - assertEquals("[_Entity]!", entities.getType().toString()); - - GraphQLFieldDefinition service = queryRoot.getField("_service"); - assertEquals(0, service.getArguments().size()); - assertEquals("_Service!", service.getType().toString()); - - GraphQLFieldDefinition query = queryRoot.getField("testTypeWithFederation"); - assertEquals(1, query.getArguments().size()); - assertEquals(GraphQLString, query.getArgument("arg").getType()); - assertEquals("TestTypeWithFederation", ((GraphQLObjectType) query.getType()).getName()); - - GraphQLObjectType type = graphQLSchema.getObjectType("TestTypeWithFederation"); - assertEquals(2, type.getDirectives().size()); - assertKeyDirective(type.getDirectives().get(0), "id"); - assertKeyDirective(type.getDirectives().get(1), "type id"); - assertEquals(3, type.getFields().size()); - assertEquals("id", type.getFields().get(0).getName()); - assertEquals(GraphQLString, type.getFields().get(0).getType()); - assertEquals("type", type.getFields().get(1).getName()); - assertEquals(GraphQLString, type.getFields().get(1).getType()); - assertEquals("value", type.getFields().get(2).getName()); - assertEquals(GraphQLString, type.getFields().get(2).getType()); - - GraphQLObjectType serviceType = graphQLSchema.getObjectType("_Service"); - assertEquals(1, serviceType.getFields().size()); - assertEquals("sdl", serviceType.getFields().get(0).getName()); - assertEquals("String!", serviceType.getFields().get(0).getType().toString()); - } finally { - System.clearProperty("smallrye.graphql.federation.enabled"); - } + GraphQLSchema graphQLSchema = createGraphQLSchema(Repeatable.class, Directive.class, Key.class, Keys.class, + TestTypeWithFederation.class, FederationTestApi.class); + + GraphQLDirective keyDirective = graphQLSchema.getDirective("key"); + assertEquals("key", keyDirective.getName()); + assertTrue(keyDirective.isRepeatable()); + assertEquals( + "Designates an object type as an entity and specifies its key fields " + + "(a set of fields that the subgraph can use to uniquely identify any instance " + + "of the entity). You can apply multiple @key directives to a single entity " + + "(to specify multiple valid sets of key fields).", + keyDirective.getDescription()); + assertEquals(EnumSet.of(OBJECT, INTERFACE), keyDirective.validLocations()); + assertEquals(1, keyDirective.getArguments().size()); + assertEquals("String", ((GraphQLScalarType) keyDirective.getArgument("fields").getType()).getName()); + + GraphQLUnionType entityType = (GraphQLUnionType) graphQLSchema.getType("_Entity"); + assertNotNull(entityType); + assertEquals(1, entityType.getTypes().size()); + assertEquals(TestTypeWithFederation.class.getSimpleName(), entityType.getTypes().get(0).getName()); + + GraphQLObjectType queryRoot = graphQLSchema.getQueryType(); + assertEquals(3, queryRoot.getFields().size()); + + GraphQLFieldDefinition entities = queryRoot.getField("_entities"); + assertEquals(1, entities.getArguments().size()); + assertEquals("[_Any!]!", entities.getArgument("representations").getType().toString()); + assertEquals("[_Entity]!", entities.getType().toString()); + + GraphQLFieldDefinition service = queryRoot.getField("_service"); + assertEquals(0, service.getArguments().size()); + assertEquals("_Service!", service.getType().toString()); + + GraphQLFieldDefinition query = queryRoot.getField("testTypeWithFederation"); + assertEquals(1, query.getArguments().size()); + assertEquals(GraphQLString, query.getArgument("arg").getType()); + assertEquals("TestTypeWithFederation", ((GraphQLObjectType) query.getType()).getName()); + + GraphQLObjectType type = graphQLSchema.getObjectType("TestTypeWithFederation"); + assertEquals(2, type.getDirectives().size()); + assertKeyDirective(type.getDirectives().get(0), "id"); + assertKeyDirective(type.getDirectives().get(1), "type id"); + assertEquals(3, type.getFields().size()); + assertEquals("id", type.getFields().get(0).getName()); + assertEquals(GraphQLString, type.getFields().get(0).getType()); + assertEquals("type", type.getFields().get(1).getName()); + assertEquals(GraphQLString, type.getFields().get(1).getType()); + assertEquals("value", type.getFields().get(2).getName()); + assertEquals(GraphQLString, type.getFields().get(2).getType()); + + GraphQLObjectType serviceType = graphQLSchema.getObjectType("_Service"); + assertEquals(1, serviceType.getFields().size()); + assertEquals("sdl", serviceType.getFields().get(0).getName()); + assertEquals("String!", serviceType.getFields().get(0).getType().toString()); } private static void assertKeyDirective(GraphQLDirective graphQLDirective, String value) {