Merge pull request #53 from apollographql/sachin/better-schema-proces…

…sing

graphql-java-support: Remove federation directive and type definitions from SDL and use stricter SchemaGenerator checks

graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/Federation.java

@@ -1,19 +1,22 @@
 package com.apollographql.federation.graphqljava;
 
+import graphql.language.DirectiveDefinition;
 import graphql.language.ObjectTypeDefinition;
 import graphql.schema.GraphQLSchema;
 import graphql.schema.idl.RuntimeWiring;
 import graphql.schema.idl.SchemaGenerator;
 import graphql.schema.idl.SchemaParser;
 import graphql.schema.idl.TypeDefinitionRegistry;
+import graphql.schema.idl.TypeRuntimeWiring;
 import org.jetbrains.annotations.NotNull;
 
 import java.io.File;
 import java.io.Reader;
+import java.util.Comparator;
 
 public final class Federation {
     private static final SchemaGenerator.Options generatorOptions = SchemaGenerator.Options.defaultOptions()
-            .enforceSchemaDirectives(false);
+            .enforceSchemaDirectives(true);
 
     private Federation() {
     }
@@ -25,10 +28,11 @@ public static SchemaTransformer transform(final GraphQLSchema schema) {
 
     public static SchemaTransformer transform(final TypeDefinitionRegistry typeRegistry, final RuntimeWiring runtimeWiring) {
         ensureQueryTypeExists(typeRegistry);
+        RuntimeWiring newRuntimeWiring = ensureFederationDirectiveDefinitionsExist(typeRegistry, runtimeWiring);
         final GraphQLSchema original = new SchemaGenerator().makeExecutableSchema(
                 generatorOptions,
                 typeRegistry,
-                runtimeWiring);
+                newRuntimeWiring);
         return transform(original);
     }
 
@@ -76,4 +80,69 @@ private static void ensureQueryTypeExists(TypeDefinitionRegistry typeRegistry) {
             typeRegistry.add(ObjectTypeDefinition.newObjectTypeDefinition().name(queryName).build());
         }
     }
+
+    private static RuntimeWiring ensureFederationDirectiveDefinitionsExist(
+            TypeDefinitionRegistry typeRegistry,
+            RuntimeWiring runtimeWiring
+    ) {
+        // Add Federation directives if they don't exist.
+        FederationDirectives.allDefinitions
+                .stream()
+                .filter(def -> !typeRegistry.getDirectiveDefinition(def.getName()).isPresent())
+                .forEachOrdered(typeRegistry::add);
+
+        // Add scalar type for _FieldSet, since the directives depend on it.
+        if (!typeRegistry.getType(_FieldSet.typeName).isPresent()) {
+            typeRegistry.add(_FieldSet.definition);
+        }
+
+        // Also add the implementation for _FieldSet.
+        if (!runtimeWiring.getScalars().containsKey(_FieldSet.typeName)) {
+            return copyRuntimeWiring(runtimeWiring).scalar(_FieldSet.type).build();
+        } else {
+            return runtimeWiring;
+        }
+    }
+
+    private static RuntimeWiring.Builder copyRuntimeWiring(RuntimeWiring runtimeWiring) {
+        // Annoyingly graphql-java doesn't have a copy constructor for RuntimeWiring.Builder.
+        RuntimeWiring.Builder builder = RuntimeWiring.newRuntimeWiring();
+
+        runtimeWiring.getDataFetchers()
+                .entrySet()
+                .stream()
+                .map(entry -> {
+                    String name = entry.getKey();
+                    TypeRuntimeWiring.Builder typeWiring = TypeRuntimeWiring.newTypeWiring(name);
+                    typeWiring.dataFetchers(entry.getValue());
+                    if (runtimeWiring.getDefaultDataFetcherForType(name) != null) {
+                      typeWiring.defaultDataFetcher(runtimeWiring.getDefaultDataFetcherForType(name));
+                    }
+                    if (runtimeWiring.getTypeResolvers().get(name) != null) {
+                        typeWiring.typeResolver(runtimeWiring.getTypeResolvers().get(name));
+                    }
+                    if (runtimeWiring.getEnumValuesProviders().get(name) != null) {
+                        typeWiring.enumValues(runtimeWiring.getEnumValuesProviders().get(name));
+                    }
+                    return typeWiring.build();
+                })
+                .forEach(builder::type);
+
+        if (runtimeWiring.getWiringFactory() != null) {
+            builder.wiringFactory(runtimeWiring.getWiringFactory());
+        }
+        if (runtimeWiring.getCodeRegistry() != null) {
+            builder.codeRegistry(runtimeWiring.getCodeRegistry());
+        }
+        runtimeWiring.getScalars().forEach((name, scalar) -> builder.scalar(scalar));
+        if (runtimeWiring.getFieldVisibility() != null) {
+            builder.fieldVisibility(runtimeWiring.getFieldVisibility());
+        }
+        runtimeWiring.getRegisteredDirectiveWiring().forEach(builder::directive);
+        runtimeWiring.getDirectiveWiring().forEach(builder::directiveWiring);
+        builder.comparatorRegistry(runtimeWiring.getComparatorRegistry());
+        runtimeWiring.getSchemaTransformers().forEach(builder::transformer);
+
+        return builder;
+    }
 }

graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/FederationDirectives.java

@@ -5,15 +5,17 @@
 import graphql.language.DirectiveLocation;
 import graphql.language.InputValueDefinition;
 import graphql.language.NonNullType;
-import graphql.language.SDLDefinition;
 import graphql.language.TypeName;
 import graphql.schema.GraphQLArgument;
 import graphql.schema.GraphQLDirective;
 import graphql.schema.GraphQLNonNull;
 
 import java.util.Arrays;
-import java.util.HashSet;
+import java.util.Comparator;
+import java.util.LinkedHashSet;
 import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import static graphql.introspection.Introspection.DirectiveLocation.FIELD_DEFINITION;
 import static graphql.introspection.Introspection.DirectiveLocation.INTERFACE;
@@ -158,19 +160,35 @@ private FederationDirectives() {
 
     /* Sets */
 
-    public static final Set<GraphQLDirective> allDirectives = new HashSet<>();
-    public static final Set<SDLDefinition> allDefinitions = new HashSet<>();
+    public static final Set<String> allNames;
+    public static final Set<GraphQLDirective> allDirectives;
+    public static final Set<DirectiveDefinition> allDefinitions;
 
     static {
-        allDirectives.add(key);
-        allDirectives.add(external);
-        allDirectives.add(requires);
-        allDirectives.add(provides);
-        allDirectives.add(extends_);
-        allDefinitions.add(keyDefinition);
-        allDefinitions.add(externalDefinition);
-        allDefinitions.add(requiresDefinition);
-        allDefinitions.add(providesDefinition);
-        allDefinitions.add(extendsDefinition);
+        // We need to maintain sorted order here for tests, since SchemaPrinter doesn't sort
+        // directive definitions.
+        allDirectives = Stream.of(
+                key,
+                external,
+                requires,
+                provides,
+                extends_
+        )
+                .sorted(Comparator.comparing(GraphQLDirective::getName))
+                .collect(Collectors.toCollection(LinkedHashSet::new));
+        allDefinitions = Stream.of(
+                keyDefinition,
+                externalDefinition,
+                requiresDefinition,
+                providesDefinition,
+                extendsDefinition
+        )
+                .sorted(Comparator.comparing(DirectiveDefinition::getName))
+                .collect(Collectors.toCollection(LinkedHashSet::new));
+        allNames = allDefinitions
+                .stream()
+                .map(DirectiveDefinition::getName)
+                .collect(Collectors.toCollection(LinkedHashSet::new));
     }
+
 }

graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/FederationSdlPrinter.java

@@ -68,19 +68,27 @@ public static class Options {
 
         private final boolean includeDirectives;
 
+        private final Predicate<GraphQLDirective> includeDirectiveDefinition;
+
+        private final Predicate<GraphQLType> includeTypeDefinition;
+
         private final GraphqlTypeComparatorRegistry comparatorRegistry;
 
         private Options(boolean includeIntrospectionTypes,
                         boolean includeScalars,
                         boolean includeExtendedScalars,
                         boolean includeSchemaDefinition,
                         boolean includeDirectives,
+                        Predicate<GraphQLDirective> includeDirectiveDefinition,
+                        Predicate<GraphQLType> includeTypeDefinition,
                         GraphqlTypeComparatorRegistry comparatorRegistry) {
             this.includeIntrospectionTypes = includeIntrospectionTypes;
             this.includeScalars = includeScalars;
             this.includeExtendedScalars = includeExtendedScalars;
             this.includeSchemaDefinition = includeSchemaDefinition;
             this.includeDirectives = includeDirectives;
+            this.includeDirectiveDefinition = includeDirectiveDefinition;
+            this.includeTypeDefinition = includeTypeDefinition;
             this.comparatorRegistry = comparatorRegistry;
         }
 
@@ -106,6 +114,7 @@ public boolean isIncludeDirectives() {
 
         public static Options defaultOptions() {
             return new Options(false, false, false, false, true,
+                    directiveDefinition -> true, typeDefinition -> true,
                     DefaultGraphqlTypeComparatorRegistry.defaultComparators());
         }
 
@@ -117,7 +126,7 @@ public static Options defaultOptions() {
          * @return options
          */
         public Options includeIntrospectionTypes(boolean flag) {
-            return new Options(flag, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.comparatorRegistry);
+            return new Options(flag, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
         }
 
         /**
@@ -128,7 +137,7 @@ public Options includeIntrospectionTypes(boolean flag) {
          * @return options
          */
         public Options includeScalarTypes(boolean flag) {
-            return new Options(this.includeIntrospectionTypes, flag, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.comparatorRegistry);
+            return new Options(this.includeIntrospectionTypes, flag, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
         }
 
         /**
@@ -140,7 +149,7 @@ public Options includeScalarTypes(boolean flag) {
          * @return options
          */
         public Options includeExtendedScalarTypes(boolean flag) {
-            return new Options(this.includeIntrospectionTypes, this.includeScalars, flag, this.includeSchemaDefinition, this.includeDirectives, this.comparatorRegistry);
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, flag, this.includeSchemaDefinition, this.includeDirectives, this.includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
         }
 
         /**
@@ -154,7 +163,7 @@ public Options includeExtendedScalarTypes(boolean flag) {
          * @return options
          */
         public Options includeSchemaDefintion(boolean flag) {
-            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, flag, this.includeDirectives, this.comparatorRegistry);
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, flag, this.includeDirectives, this.includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
         }
 
         /**
@@ -166,7 +175,32 @@ public Options includeSchemaDefintion(boolean flag) {
          * @return new instance of options
          */
         public Options includeDirectives(boolean flag) {
-            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, flag, this.comparatorRegistry);
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, flag, this.includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
+        }
+
+        /**
+         * Filter printing of directive definitions. In Apollo Federation, some directive
+         * definitions need to be hidden, and this predicate allows filtering out such definitions.
+         * Prints all definitions by default. Note that both this predicate and the flag in
+         * {@link #includeDirectives(boolean)} must be true for a definition to be printed.
+         *
+         * @param includeDirectiveDefinition returns true if the definition should be printed
+         * @return new instance of options
+         */
+        public Options includeDirectiveDefinitions(Predicate<GraphQLDirective> includeDirectiveDefinition) {
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, includeDirectiveDefinition, this.includeTypeDefinition, this.comparatorRegistry);
+        }
+
+        /**
+         * Filter printing of type definitions. In Apollo Federation, some type definitions need to
+         * be hidden, and this predicate allows filtering out such definitions. Prints all
+         * definitions by default.
+         *
+         * @param includeTypeDefinition returns true if the definition should be printed
+         * @return new instance of options
+         */
+        public Options includeTypeDefinitions(Predicate<GraphQLType> includeTypeDefinition) {
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.includeDirectiveDefinition, includeTypeDefinition, this.comparatorRegistry);
         }
 
         /**
@@ -179,7 +213,7 @@ public Options includeDirectives(boolean flag) {
          * @return options
          */
         public Options setComparators(GraphqlTypeComparatorRegistry comparatorRegistry) {
-            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives,
+            return new Options(this.includeIntrospectionTypes, this.includeScalars, this.includeExtendedScalars, this.includeSchemaDefinition, this.includeDirectives, this.includeDirectiveDefinition, this.includeTypeDefinition,
                     comparatorRegistry);
         }
 
@@ -240,6 +274,7 @@ public String print(GraphQLSchema schema) {
         List<GraphQLType> typesAsList = schema.getAllTypesAsList()
                 .stream()
                 .sorted(Comparator.comparing(GraphQLType::getName))
+                .filter(options.includeTypeDefinition)
                 .collect(toList());
 
         printType(out, typesAsList, GraphQLInterfaceType.class, visibility);
@@ -523,7 +558,10 @@ private List<GraphQLDirective> getDirectives(GraphQLSchema schema) {
                 "deprecated");
 
         Predicate<GraphQLDirective> standard = directive -> standardDirectives.contains(directive.getName());
-        return schema.getDirectives().stream().filter(standard.negate()).collect(Collectors.toList());
+        return schema.getDirectives().stream()
+                .filter(standard.negate())
+                .filter(options.includeDirectiveDefinition)
+                .collect(Collectors.toList());
     }
 
     String typeString(GraphQLType rawType) {

graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/SchemaTransformer.java

@@ -15,6 +15,7 @@
 import org.jetbrains.annotations.NotNull;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -69,7 +70,7 @@ public final GraphQLSchema build() throws SchemaProblem {
                 GraphQLCodeRegistry.newCodeRegistry(originalSchema.getCodeRegistry());
 
         // Print the original schema as sdl and expose it as query { _service { sdl } }
-        final String sdl = sdl();
+        final String sdl = sdl(originalSchema);
         final GraphQLObjectType.Builder newQueryType = GraphQLObjectType.newObject(originalQueryType)
                 .field(_Service.field);
         newCodeRegistry.dataFetcher(FieldCoordinates.coordinates(
@@ -137,15 +138,34 @@ public final GraphQLSchema build() throws SchemaProblem {
                 .build();
     }
 
-    private String sdl() {
-        // Note that FederationSdlPrinter is a copy of graphql-java's SchemaPrinter that fixes a
-        // specific bug. Once graphql-java releases a bugfix (the PR in question is specifically
-        // graphql-java/graphql-java#1798 ) and backports it, we should revert to SchemaPrinter.
+    public static String sdl(GraphQLSchema schema) {
+        // Gather directive definitions to hide.
+        final Set<String> hiddenDirectiveDefinitions = new HashSet<>(FederationDirectives.allNames);
+
+        // Gather type definitions to hide.
+        final Set<String> hiddenTypeDefinitions = new HashSet<>();
+        hiddenTypeDefinitions.add(_Any.typeName);
+        hiddenTypeDefinitions.add(_Entity.typeName);
+        hiddenTypeDefinitions.add(_FieldSet.typeName);
+        hiddenTypeDefinitions.add(_Service.typeName);
+
+        // Note that FederationSdlPrinter is a copy of graphql-java's SchemaPrinter that:
+        // - fixes a specific bug in graphql-java that hasn't been backported yet, specifically
+        //   graphql-java/graphql-java#1798
+        // - adds the ability to filter out directive and type definitions, which is required
+        //   by federation spec.
+        //
+        // FederationSdlPrinter will need to be updated whenever graphql-java changes versions. It
+        // can be removed when the bug is fixed/backported, and when either graphql-java adds
+        // native support for filtering out directive and type definitions or federation spec
+        // changes to allow the currently forbidden directive and type definitions.
         final FederationSdlPrinter.Options options = FederationSdlPrinter.Options.defaultOptions()
                 .includeScalarTypes(true)
                 .includeExtendedScalarTypes(true)
                 .includeSchemaDefintion(true)
-                .includeDirectives(true);
-        return new FederationSdlPrinter(options).print(originalSchema);
+                .includeDirectives(true)
+                .includeDirectiveDefinitions(def -> !hiddenDirectiveDefinitions.contains(def.getName()))
+                .includeTypeDefinitions(def -> !hiddenTypeDefinitions.contains(def.getName()));
+        return new FederationSdlPrinter(options).print(schema);
     }
 }

graphql-java-support/src/main/java/com/apollographql/federation/graphqljava/_FieldSet.java

@@ -9,6 +9,7 @@ public final class _FieldSet {
 
     public static GraphQLScalarType type = GraphQLScalarType.newScalar(Scalars.GraphQLString)
             .name(typeName)
+            .description(null)
             .coercing(Scalars.GraphQLString.getCoercing())
             .build();