Fix @skip definition is added twice to the GraphQLSchema when defined in sdl #2940

tinnou · 2022-09-02T01:21:11Z

Description

When @skip/@include is defined in a SDL, schema generation will create a GraphQLSchema with multiple skip/include definitions. This can cause problems when a schema is serialized and parsed multiple times.

A little far fetched example:

    @Test
    fun parse() {
        val schema: GraphQLSchema = """
            type Query {
                hello: String
            }
        """.trimIndent().mockSchema()
        val sdl = schema.printSdl()
        val schema2 = sdl.mockSchema()
        val sdl2 = schema2.printSdl()
        sdl2.mockSchema() // throws
    }

    fun GraphQLSchema.printSdl(): String {
        return SchemaPrinter(SchemaPrinter.Options.defaultOptions()
            .includeDirectiveDefinitions(true)
        ).print(this)
    }

will throw:

Caused by: SchemaProblem{errors=['include' type [@13:1] tried to redefine existing directive 'include' type [@7:1], 'skip' type [@25:1] tried to redefine existing directive 'skip' type [@19:1]]}
	at graphql.schema.idl.SchemaParser.buildRegistry(SchemaParser.java:156)
	at graphql.schema.idl.SchemaParser.parseImpl(SchemaParser.java:124)
	at graphql.schema.idl.SchemaParser.parse(SchemaParser.java:90)
	at graphql.schema.idl.SchemaParser.parse(SchemaParser.java:75)

We could add a predicate to avoid printing directives skip and include but it would sit better if we could ensure printing the sdl generates a valid SDL (that can be parsed back into a valid GraphQLSchema).

The actual issue comes from the fact @skip and @include are added by default on the GraphQLSchema builder.

Expected Behavior

To preserve the behavior of being able to redefine the skip and include directives without raising an error, we should probably just ignore any directive definition named skip or include and rely on the added by default definitions provided by the GraphQL library implementation.

I'm on the fence as to add validation to check if a provided skip/include definition is semantically equivalent to the ones defined by the server. Since these are guaranteed to be provided by the GraphQL server implementation, I don't feel this is really needed and it will likely be an artifact of a tool having printed them in the SDL.

Changes

The fix consists of skipping any directive defined in the sdl with the name skip/include with the expectation graphql-java will use its own.

Testing

The following test fails and show that redefining the skip/include directive leads to multiple definitions added to the GraphQLSchema instance.

def "skip and include should be added to the schema only if not already defined"() {
        def sdl = '''
            "Directs the executor to skip this field or fragment when the `if`'argument is true."
            directive @skip(
                "Skipped when true."
                if: Boolean!
              ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
              
            "Directs the executor to include this field or fragment only when the `if` argument is true"
            directive @include(
                "Included when true."
                if: Boolean!
              ) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT
              
            type Query {
                hello: String
            }
        '''
        when:
        def schema = TestUtil.schema(sdl)
        then:
        schema.getDirectives().findAll { it.name == "skip" }.size() == 1
        schema.getDirectives().findAll { it.name == "include" }.size() == 1

        and:
        def newSchema = GraphQLSchema.newSchema(schema).build()
        then:
        newSchema.getDirectives().findAll { it.name == "skip" }.size() == 1
        newSchema.getDirectives().findAll { it.name == "include" }.size() == 1
    }

…irective definitions on the GraphQLSchema

bbakerman · 2022-09-02T01:54:55Z

src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java

        TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();

        for (DirectiveDefinition directiveDefinition : typeRegistry.getDirectiveDefinitions().values()) {
+            if (IncludeDirective.getName().equals(directiveDefinition.getName())


we should add all the system defined directives

@deprecated / @specfiedBy / @include / @Skip

Otherwise I approve of this PR and its approach

It's a little different for @deprecated and @specifiedBy.
Let me explain and tell me what you think.

@deprecated and @specifiedBy are schema directives, and the current behavior is to only add them if not present in the supplied SDL.
I think this is fair since one could imagine a case where we are reading an older spec SDL and it would be nice if we can keep the spec behavior defined in the SDL (e.g deprecated locations missing the enum location on older version of the spec) although not super important in my opinion since spec evolution is backward compatible, hence using the engine definitions is still OK.

Additionally @deprecated and @specifiedBy are added in the typeRegistry rather than the GraphQLSchema.Builder since @deprecated and @specifiedBy are schema directives, and the applied directives locations should be checked while performing the registry checks.

Also we could add some validation, such that if you attempt to define @deprecated and @specifiedBy on your own, then they need to be "compatible" with the spec standard defined directives. Not super convinced we need that kind of validation, it feels a little over defensive.

For @skip and @include, they are execution (query) directives, i.e there can't be any applied directives in the schema, so they are being added by default only later in the GraphQLSchema builder. The behavior I added here "ignores" any directive in the SDL that attempts to redefines them.

To make things consistent w.r.t to all standard directives, we could follow the same pattern as with deprecated and specifiedBy, meaning adding them all only if not present in the typeRegistry, but we would need to either remove them from the GraphQLSchema.Builder (potentially a behavior breaking change and this might cause problems when building a schema programmatically without a SDL) or only add them to the GraphQLSchem.Builder if not present (although there is not a good way today as GraphQLSchema.builder.additionalDirectives adds to the set even though it might share the same name so we might see double adding when a schema is merged and cloned from another schema).

Thoughts?

Lets go with this for now - progress over perfection. You make a good point so lets go with it

bbakerman

Please include all the standard directives as being skipped

@Skip

Fix bug where parsing a sdl with @Skip definition would result in 2 directive definitions on the GraphQLSchema (#2940)

@Skip

Fix bug where parsing a sdl with @Skip definition would result in 2 directive definitions on the GraphQLSchema (#2940)

* 19.3: (709 commits) use class loader in getResource (graphql-java#3038) Stable port of graphql-java#2940 (graphql-java#2947) Stable port of Diff counts are the same (graphql-java#2946) Stable port of Fix printing directives when they contain something like a formatting specifier (graphql-java#2919) (graphql-java#2920) (graphql-java#2945) Stable port of Fix field visibility bug with enum with enum args (graphql-java#2926) (graphql-java#2944) Stable fix for graphql-java#2943 (graphql-java#2943) Added test fore intersection Xuorig Fix PR - Edge case with GraphQLTypeReference and Schema Transforms (graphql-java#2906) Fix typo in description of skip directive (graphql-java#2915) Add smaller set first optimisation Cheaper calculation for narrowing down possible objects in ENO Handles isDeprecated not being present in the json Defaults Locale when calling validation (graphql-java#2908) DF SelectionSet Benchmark (graphql-java#2893) Test stability (graphql-java#2903) Donna's catch! (graphql-java#2900) Deprecate Apollo Cache Control READY - Stop DOS attacks by making the lexer stop early on evil input. (graphql-java#2892) Bump java-dataloader ahead of release State is passed explicitly to instrumentation and parameters are NOT mutated (graphql-java#2769) ... # Conflicts: # README.md # build.gradle # src/main/java/graphql/GraphQL.java # src/main/java/graphql/Scalars.java # src/main/java/graphql/execution/ValuesResolver.java # src/main/java/graphql/relay/SimpleListConnection.java # src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java

Fix bug where parsing a sdl with @Skip definition would result in 2 d…

7a245cd

…irective definitions on the GraphQLSchema

bbakerman reviewed Sep 2, 2022

View reviewed changes

bbakerman requested changes Sep 2, 2022

View reviewed changes

bbakerman added this to the 20.0 milestone Sep 2, 2022

bbakerman merged commit be2de88 into graphql-java:master Sep 6, 2022

bbakerman added a commit that referenced this pull request Sep 7, 2022

Stable port of #2940

0a76c76

Fix bug where parsing a sdl with @Skip definition would result in 2 directive definitions on the GraphQLSchema (#2940)

bbakerman mentioned this pull request Sep 7, 2022

Stable port of #2940 #2947

Merged

bbakerman added a commit that referenced this pull request Sep 7, 2022

Stable port of #2940 (#2947)

db56490

Fix bug where parsing a sdl with @Skip definition would result in 2 directive definitions on the GraphQLSchema (#2940)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix @skip definition is added twice to the GraphQLSchema when defined in sdl #2940

Fix @skip definition is added twice to the GraphQLSchema when defined in sdl #2940

Uh oh!

tinnou commented Sep 2, 2022 •

edited

Loading

Uh oh!

bbakerman Sep 2, 2022

Uh oh!

bbakerman Sep 2, 2022

Uh oh!

tinnou Sep 5, 2022 •

edited

Loading

Uh oh!

bbakerman Sep 6, 2022

Uh oh!

bbakerman left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix @skip definition is added twice to the GraphQLSchema when defined in sdl #2940

Fix @skip definition is added twice to the GraphQLSchema when defined in sdl #2940

Uh oh!

Conversation

tinnou commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Expected Behavior

Changes

Testing

Uh oh!

bbakerman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

bbakerman Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

tinnou Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbakerman Sep 6, 2022

Choose a reason for hiding this comment

Uh oh!

bbakerman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tinnou commented Sep 2, 2022 •

edited

Loading

tinnou Sep 5, 2022 •

edited

Loading