From 12ba1856158f6b7a414cbaaf05910400e92b28f0 Mon Sep 17 00:00:00 2001 From: Andreas Marek Date: Mon, 24 Aug 2026 09:02:06 +1000 Subject: [PATCH] Simplify directive cycle detection --- .../idl/SchemaTypeDirectivesChecker.java | 199 ++++++++---------- ...irectivesOnDirectiveDefinitionsTest.groovy | 22 ++ .../schema/idl/SchemaGeneratorTest.groovy | 39 +--- .../SchemaTypeDirectivesCheckerTest.groovy | 55 +++-- 4 files changed, 158 insertions(+), 157 deletions(-) diff --git a/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java b/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java index 35c58c525..e898a8054 100644 --- a/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java +++ b/src/main/java/graphql/schema/idl/SchemaTypeDirectivesChecker.java @@ -54,7 +54,6 @@ import static graphql.introspection.Introspection.DirectiveLocation.UNION; import static graphql.util.FpKit.getByName; import static graphql.util.FpKit.mergeFirst; -import static java.util.stream.Collectors.joining; /** * This is responsible for traversing EVERY type and field in the registry and ensuring that @@ -64,6 +63,8 @@ @Internal class SchemaTypeDirectivesChecker { + private static final String TYPE_PREFIX = "type:"; + private final TypeDefinitionRegistry typeRegistry; private final RuntimeWiring runtimeWiring; @@ -200,7 +201,6 @@ private static boolean isNoNullArgWithoutDefaultValue(InputValueDefinition defin private void commonCheck(Collection directiveDefinitions, List errors) { List directiveDefinitionsList = new ArrayList<>(directiveDefinitions); Map directiveDefinitionsByName = getByName(directiveDefinitionsList, DirectiveDefinition::getName, mergeFirst()); - Map>> referencesByName = referencesByName(directiveDefinitionsByName); directiveDefinitions.forEach(directiveDefinition -> { assertTypeName(directiveDefinition, errors); @@ -215,7 +215,7 @@ private void commonCheck(Collection directiveDefinitions, L .forEach(extension -> checkDirectReference(directiveDefinition, extension, extension.getDirectives(), errors)); }); - checkIndirectDirectiveCycles(directiveDefinitionsByName, referencesByName, errors); + checkIndirectDirectiveCycles(directiveDefinitionsByName, errors); } private static void checkDirectReference(DirectiveDefinition definition, @@ -228,55 +228,55 @@ private static void checkDirectReference(DirectiveDefinition definition, errors.add(new DirectiveIllegalReferenceError(definition, location)); } - private Map>> referencesByName( - Map directiveDefinitionsByName) { - Map>> result = new LinkedHashMap<>(); - directiveDefinitionsByName.values().forEach(definition -> - recordDirectiveDefinitionReferences(result, definition)); - recordInputTypeReferences(result); - return result; - } - - private void recordDirectiveDefinitionReferences(Map>> references, - DirectiveDefinition definition) { + private Map> directiveReferences(DirectiveDefinition definition) { + Map> references = new LinkedHashMap<>(); String source = definition.getName(); recordAppliedDirectiveReferences(references, source, definition, definition.getDirectives()); definition.getInputValueDefinitions().forEach(argument -> { recordAppliedDirectiveReferences(references, source, argument, argument.getDirectives()); - recordReference(references, source, typeKey(TypeUtil.unwrapAll(argument.getType()).getName()), argument); + recordReference(references, typeKey(TypeUtil.unwrapAll(argument.getType()).getName()), argument); }); typeRegistry.directiveExtensions() .getOrDefault(source, Collections.emptyList()) .forEach(extension -> recordAppliedDirectiveReferences(references, source, extension, extension.getDirectives())); + return references; } - private void recordInputTypeReferences(Map>> references) { - typeRegistry.getTypes(InputObjectTypeDefinition.class) - .forEach(definition -> recordInputObjectReferences(references, definition)); - typeRegistry.inputObjectTypeExtensions().values() - .forEach(extensions -> extensions.forEach(extension -> recordInputObjectReferences(references, extension))); - typeRegistry.getTypes(EnumTypeDefinition.class) - .forEach(definition -> recordEnumReferences(references, definition)); - typeRegistry.enumTypeExtensions().values() - .forEach(extensions -> extensions.forEach(extension -> recordEnumReferences(references, extension))); - typeRegistry.scalars().values() - .forEach(definition -> recordScalarReferences(references, definition)); - typeRegistry.scalarTypeExtensions().values() - .forEach(extensions -> extensions.forEach(extension -> recordScalarReferences(references, extension))); + private Map> typeReferences(String typeName) { + Map> references = new LinkedHashMap<>(); + TypeDefinition definition = findTypeDefFromRegistry(typeName, typeRegistry); + if (definition instanceof InputObjectTypeDefinition) { + recordInputObjectReferences(references, (InputObjectTypeDefinition) definition); + } else if (definition instanceof EnumTypeDefinition) { + recordEnumReferences(references, (EnumTypeDefinition) definition); + } else if (definition instanceof ScalarTypeDefinition) { + recordScalarReferences(references, (ScalarTypeDefinition) definition); + } + + typeRegistry.inputObjectTypeExtensions() + .getOrDefault(typeName, Collections.emptyList()) + .forEach(extension -> recordInputObjectReferences(references, extension)); + typeRegistry.enumTypeExtensions() + .getOrDefault(typeName, Collections.emptyList()) + .forEach(extension -> recordEnumReferences(references, extension)); + typeRegistry.scalarTypeExtensions() + .getOrDefault(typeName, Collections.emptyList()) + .forEach(extension -> recordScalarReferences(references, extension)); + return references; } - private static void recordInputObjectReferences(Map>> references, + private static void recordInputObjectReferences(Map> references, InputObjectTypeDefinition definition) { String source = typeKey(definition.getName()); recordAppliedDirectiveReferences(references, source, definition, definition.getDirectives()); definition.getInputValueDefinitions().forEach(field -> { recordAppliedDirectiveReferences(references, source, field, field.getDirectives()); - recordReference(references, source, typeKey(TypeUtil.unwrapAll(field.getType()).getName()), field); + recordReference(references, typeKey(TypeUtil.unwrapAll(field.getType()).getName()), field); }); } - private static void recordEnumReferences(Map>> references, + private static void recordEnumReferences(Map> references, EnumTypeDefinition definition) { String source = typeKey(definition.getName()); recordAppliedDirectiveReferences(references, source, definition, definition.getDirectives()); @@ -284,137 +284,120 @@ private static void recordEnumReferences(Map>> recordAppliedDirectiveReferences(references, source, value, value.getDirectives())); } - private static void recordScalarReferences(Map>> references, + private static void recordScalarReferences(Map> references, ScalarTypeDefinition definition) { recordAppliedDirectiveReferences(references, typeKey(definition.getName()), definition, definition.getDirectives()); } - private static void recordAppliedDirectiveReferences(Map>> references, + private static void recordAppliedDirectiveReferences(Map> references, String source, NamedNode location, List directives) { directives.forEach(directive -> { if (!directive.getName().equals(source)) { - recordReference(references, source, directive.getName(), location); + recordReference(references, directive.getName(), location); } }); } - private static void recordReference(Map>> references, - String source, + private static void recordReference(Map> references, String target, NamedNode location) { - references.computeIfAbsent(source, key -> new LinkedHashMap<>()) - .putIfAbsent(target, location); + references.putIfAbsent(target, location); } private static String typeKey(String typeName) { - return "type:" + typeName; + return TYPE_PREFIX + typeName; } - private static void checkIndirectDirectiveCycles( + private void checkIndirectDirectiveCycles( Map directiveDefinitionsByName, - Map>> referencesByName, List errors) { Set checked = new LinkedHashSet<>(); - Set visiting = new LinkedHashSet<>(); - List path = new ArrayList<>(); - for (String directiveName : directiveDefinitionsByName.keySet()) { - checkIndirectDirectiveCycles(directiveName, directiveDefinitionsByName, referencesByName, checked, visiting, path, errors); + LinkedHashSet currentPath = new LinkedHashSet<>(); + for (DirectiveDefinition directiveDefinition : directiveDefinitionsByName.values()) { + checkDirectiveReferencesForCycles(directiveDefinition.getName(), directiveDefinitionsByName, checked, currentPath, errors); } } - private static void checkIndirectDirectiveCycles(String nodeName, - Map directiveDefinitionsByName, - Map>> referencesByName, - Set checked, - Set visiting, - List path, - List errors) { + private void checkDirectiveReferencesForCycles(String nodeName, + Map directiveDefinitionsByName, + Set checked, + LinkedHashSet currentPath, + List errors) { if (checked.contains(nodeName)) { return; } - visiting.add(nodeName); - path.add(nodeName); - checkIndirectDirectiveCycleReferences(nodeName, directiveDefinitionsByName, referencesByName, checked, visiting, path, errors); - path.remove(path.size() - 1); - visiting.remove(nodeName); - checked.add(nodeName); - } - - private static void checkIndirectDirectiveCycleReferences(String nodeName, - Map directiveDefinitionsByName, - Map>> referencesByName, - Set checked, - Set visiting, - List path, - List errors) { - Map> references = referencesByName.getOrDefault(nodeName, Collections.emptyMap()); - for (Map.Entry> entry : references.entrySet()) { - checkIndirectDirectiveCycleReference(entry.getKey(), entry.getValue(), directiveDefinitionsByName, referencesByName, checked, visiting, path, errors); + currentPath.add(nodeName); + for (Map.Entry> reference : referencesFor(nodeName, directiveDefinitionsByName).entrySet()) { + String referencedNodeName = reference.getKey(); + if (currentPath.contains(referencedNodeName)) { + addIndirectDirectiveCycleError(referencedNodeName, reference.getValue(), directiveDefinitionsByName, currentPath, errors); + continue; + } + checkDirectiveReferencesForCycles(referencedNodeName, directiveDefinitionsByName, checked, currentPath, errors); } + currentPath.remove(nodeName); + checked.add(nodeName); } - private static void checkIndirectDirectiveCycleReference(String referencedNodeName, - NamedNode location, - Map directiveDefinitionsByName, - Map>> referencesByName, - Set checked, - Set visiting, - List path, - List errors) { - if (visiting.contains(referencedNodeName)) { - addIndirectDirectiveCycleError(referencedNodeName, location, directiveDefinitionsByName, path, errors); - return; + private Map> referencesFor( + String nodeName, + Map directiveDefinitionsByName) { + if (nodeName.startsWith(TYPE_PREFIX)) { + return typeReferences(nodeName.substring(TYPE_PREFIX.length())); } - if (!checked.contains(referencedNodeName)) { - checkIndirectDirectiveCycles(referencedNodeName, directiveDefinitionsByName, referencesByName, checked, visiting, path, errors); + DirectiveDefinition definition = directiveDefinitionsByName.get(nodeName); + if (definition == null) { + return Collections.emptyMap(); } + return directiveReferences(definition); } private static void addIndirectDirectiveCycleError(String repeatedNodeName, NamedNode location, Map directiveDefinitionsByName, - List path, + LinkedHashSet currentPath, List errors) { - List cyclePath = directiveCyclePath(repeatedNodeName, path); - String directiveName = cyclePath.stream() - .filter(directiveDefinitionsByName::containsKey) - .findFirst() - .orElse(null); + List path = new ArrayList<>(currentPath); + List cycle = path.subList(path.indexOf(repeatedNodeName), path.size()); + String directiveName = firstDirectiveName(cycle, directiveDefinitionsByName); if (directiveName == null) { return; } - cyclePath = rotateCyclePath(cyclePath, directiveName); - String cyclePathString = cyclePath.stream() - .map(SchemaTypeDirectivesChecker::displayNodeName) - .collect(joining(" -> ")); + List displayedCycle = rotateAndDisplayCycle(cycle, directiveName); + String cyclePath = String.join(" -> ", displayedCycle); DirectiveDefinition directiveDefinition = assertNotNull(directiveDefinitionsByName.get(directiveName)); - errors.add(new DirectiveIllegalReferenceError(directiveDefinition, location, cyclePathString)); + errors.add(new DirectiveIllegalReferenceError(directiveDefinition, location, cyclePath)); } - private static List directiveCyclePath(String repeatedDirectiveName, List path) { - int cycleStart = path.indexOf(repeatedDirectiveName); - List cyclePath = new ArrayList<>(path.subList(cycleStart, path.size())); - cyclePath.add(repeatedDirectiveName); - return cyclePath; + private static String firstDirectiveName( + List cycle, + Map directiveDefinitionsByName) { + for (String nodeName : cycle) { + if (directiveDefinitionsByName.containsKey(nodeName)) { + return nodeName; + } + } + return null; } - private static List rotateCyclePath(List cyclePath, String firstNode) { - List nodes = cyclePath.subList(0, cyclePath.size() - 1); - int firstNodeIndex = nodes.indexOf(firstNode); - List result = new ArrayList<>(nodes.size() + 1); - result.addAll(nodes.subList(firstNodeIndex, nodes.size())); - result.addAll(nodes.subList(0, firstNodeIndex)); - result.add(firstNode); + private static List rotateAndDisplayCycle(List cycle, String firstNode) { + int firstNodeIndex = cycle.indexOf(firstNode); + List result = new ArrayList<>(cycle.size() + 1); + for (int i = 0; i < cycle.size(); i++) { + String nodeName = cycle.get((firstNodeIndex + i) % cycle.size()); + result.add(displayNodeName(nodeName)); + } + result.add(displayNodeName(firstNode)); return result; } private static String displayNodeName(String nodeName) { - if (nodeName.startsWith("type:")) { - return nodeName.substring("type:".length()); + if (nodeName.startsWith(TYPE_PREFIX)) { + return nodeName.substring(TYPE_PREFIX.length()); } return nodeName; } diff --git a/src/test/groovy/graphql/schema/idl/DirectivesOnDirectiveDefinitionsTest.groovy b/src/test/groovy/graphql/schema/idl/DirectivesOnDirectiveDefinitionsTest.groovy index 1e1a1bd8c..598986603 100644 --- a/src/test/groovy/graphql/schema/idl/DirectivesOnDirectiveDefinitionsTest.groovy +++ b/src/test/groovy/graphql/schema/idl/DirectivesOnDirectiveDefinitionsTest.groovy @@ -219,6 +219,28 @@ class DirectivesOnDirectiveDefinitionsTest extends Specification { } type Query { field: String } ''' + "input extension cycle" | "loop -> LoopInput" | ''' + directive @loop(arg: LoopInput) on INPUT_FIELD_DEFINITION + input LoopInput { + base: String + } + extend input LoopInput { + field: String @loop + } + type Query { field: String } + ''' + "enum value cycle" | "loop -> LoopEnum" | ''' + directive @loop(arg: LoopEnum) on ENUM_VALUE + enum LoopEnum { + VALUE @loop + } + type Query { field: String } + ''' + "scalar cycle" | "loop -> LoopScalar" | ''' + directive @loop(arg: LoopScalar) on SCALAR + scalar LoopScalar @loop + type Query { field: String } + ''' "type-led cycle" | "b -> YInput -> XInput -> b" | ''' directive @a(arg: XInput) on FIELD_DEFINITION directive @b(arg: YInput) on INPUT_OBJECT diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy index a3d847be4..740579f94 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy @@ -2330,9 +2330,16 @@ class SchemaGeneratorTest extends Specification { schema != null } - def "#4201 indirect cyclical directive definitions are rejected without stack overflow - #name"() { + def "#4201 indirect cyclical directive definitions are rejected without stack overflow"() { given: - def registry = new SchemaParser().parse(sdl) + def registry = new SchemaParser().parse(''' + directive @foo(x: Int @bar(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + directive @bar(y: Int @foo(x: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION + + type Query { + field: String @foo(x: 10) @bar(y: 20) + } + ''') when: UnExecutableSchemaGenerator.makeUnExecutableSchema(registry) @@ -2341,33 +2348,7 @@ class SchemaGeneratorTest extends Specification { def e = thrown(SchemaProblem) e.errors.size() == 1 e.errors.get(0) instanceof DirectiveIllegalReferenceError - e.errors.get(0).getMessage().contains(cycleMessage) - - where: - name << ["two directives", "three directives"] - sdl << [ - ''' - directive @foo(x: Int @bar(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION - directive @bar(y: Int @foo(x: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION - - type Query { - field: String @foo(x: 10) @bar(y: 20) - } - ''', - ''' - directive @dirA(x: Int @dirB(y: 1)) on FIELD_DEFINITION | ARGUMENT_DEFINITION - directive @dirB(y: Int @dirC(z: 2)) on FIELD_DEFINITION | ARGUMENT_DEFINITION - directive @dirC(z: Int @dirA(x: 3)) on FIELD_DEFINITION | ARGUMENT_DEFINITION - - type Query { - field: String @dirA(x: 10) @dirB(y: 20) @dirC(z: 30) - } - ''' - ] - cycleMessage << [ - "'foo' must not reference itself via directive cycle 'foo -> bar -> foo'", - "'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'" - ] + e.errors.get(0).getMessage().contains("'foo' must not reference itself via directive cycle 'foo -> bar -> foo'") } def "code registry default data fetcher is respected"() { diff --git a/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy index 4debd015b..f8096c2b2 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaTypeDirectivesCheckerTest.groovy @@ -232,16 +232,8 @@ class SchemaTypeDirectivesCheckerTest extends Specification { errors.get(0).getMessage() == "'invalidExample' must not reference itself on 'arg''[@2:39]'" } - def "directive must not indirectly reference itself"() { + def "directive must not indirectly reference itself - #name"() { given: - def spec = ''' - directive @foo(arg: String @bar) on ARGUMENT_DEFINITION - directive @bar(arg: String @foo) on ARGUMENT_DEFINITION - - type Query { - f1 : String - } - ''' def registry = parse(spec) def errors = [] @@ -251,30 +243,53 @@ class SchemaTypeDirectivesCheckerTest extends Specification { then: errors.size() == 1 errors.get(0) instanceof DirectiveIllegalReferenceError - errors.get(0).getMessage().contains("'foo' must not reference itself via directive cycle 'foo -> bar -> foo'") + errors.get(0).getMessage().contains(cycleMessage) + + where: + name << ["two directives", "three directives"] + spec << [ + ''' + directive @foo(arg: String @bar) on ARGUMENT_DEFINITION + directive @bar(arg: String @foo) on ARGUMENT_DEFINITION + + type Query { + f1 : String + } + ''', + ''' + directive @dirA(x: Int @dirB(y: 1)) on ARGUMENT_DEFINITION + directive @dirB(y: Int @dirC(z: 2)) on ARGUMENT_DEFINITION + directive @dirC(z: Int @dirA(x: 3)) on ARGUMENT_DEFINITION + + type Query { + f1 : String + } + ''' + ] + cycleMessage << [ + "'foo' must not reference itself via directive cycle 'foo -> bar -> foo'", + "'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'" + ] } - def "directive must not indirectly reference itself through a longer cycle"() { + def "acyclic directive references are allowed"() { given: - def spec = ''' - directive @dirA(x: Int @dirB(y: 1)) on ARGUMENT_DEFINITION - directive @dirB(y: Int @dirC(z: 2)) on ARGUMENT_DEFINITION - directive @dirC(z: Int @dirA(x: 3)) on ARGUMENT_DEFINITION + def registry = parse(''' + directive @foo(arg: String @bar) on ARGUMENT_DEFINITION + directive @bar(arg: String @baz) on ARGUMENT_DEFINITION + directive @baz on ARGUMENT_DEFINITION type Query { f1 : String } - ''' - def registry = parse(spec) + ''') def errors = [] when: new SchemaTypeDirectivesChecker(registry, RuntimeWiring.newRuntimeWiring().build()).checkTypeDirectives(errors) then: - errors.size() == 1 - errors.get(0) instanceof DirectiveIllegalReferenceError - errors.get(0).getMessage().contains("'dirA' must not reference itself via directive cycle 'dirA -> dirB -> dirC -> dirA'") + errors.isEmpty() } def "directive must not begin with '__'"() {