Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bbakerman merged 1 commit into from
Sep 6, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/main/java/graphql/schema/idl/SchemaGeneratorHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,10 @@

import static graphql.Assert.assertNotNull;
import static graphql.Directives.DEPRECATED_DIRECTIVE_DEFINITION;
import static graphql.Directives.IncludeDirective;
import static graphql.Directives.NO_LONGER_SUPPORTED;
import static graphql.Directives.SPECIFIED_BY_DIRECTIVE_DEFINITION;
import static graphql.Directives.SkipDirective;
import static graphql.Directives.SpecifiedByDirective;
import static graphql.collect.ImmutableKit.emptyList;
import static graphql.introspection.Introspection.DirectiveLocation.ARGUMENT_DEFINITION;
Expand Down Expand Up @@ -1055,6 +1057,11 @@ Set<GraphQLDirective> buildAdditionalDirectiveDefinitions(BuildContext buildCtx)
TypeDefinitionRegistry typeRegistry = buildCtx.getTypeRegistry();

for (DirectiveDefinition directiveDefinition : typeRegistry.getDirectiveDefinitions().values()) {
if (IncludeDirective.getName().equals(directiveDefinition.getName())
Copy link
Member

@bbakerman bbakerman Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add all the system defined directives

@deprecated / @specfiedBy / @include / @Skip

Copy link
Member

@bbakerman bbakerman Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise I approve of this PR and its approach

Copy link
Contributor Author

@tinnou tinnou Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@bbakerman bbakerman Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|| SkipDirective.getName().equals(directiveDefinition.getName())) {
// skip and include directives are added by default to the GraphQLSchema via the GraphQLSchema builder.
continue;
}
GraphQLDirective directive = buildDirectiveDefinitionFromAst(buildCtx, directiveDefinition, inputTypeFactory(buildCtx));
buildCtx.addDirectiveDefinition(directive);
additionalDirectives.add(directive);
Expand Down
31 changes: 31 additions & 0 deletions src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2500,4 +2500,35 @@ class SchemaGeneratorTest extends Specification {
then:
noExceptionThrown()
}

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
}
}