Skip to content

Commit 4d59fdc

Browse files
oryan-blockclaude
andcommitted
Simplify the directive lookup and the nested input tests
schemaDirectives is now a map by name, so finishing a directive replaces its entry instead of rebuilding the whole set, and findDirective is a plain lookup. The comments say in plainer words why directives are built twice. The WiringWrapper location change for input fields and enum values is reverted. directiveLocation isn't read anywhere, so it changed nothing. Tests: the #731 case is now an onInputObjectField override in the existing EmailDirective test. The two SchemaParserTest cases are one test with a list, a non-null and a back-pointing field. The two tests for input objects used by directive arguments are one, which records argument types at wiring time, in both declaration orders. Dropped the query-executing validation test: the type and legacy directive checks already cover what it walked, and the probe runs the issue's schema through extended-validation itself. None of the tests call deprecated API anymore, so the build passes with -Werror. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 568e21d commit 4d59fdc

4 files changed

Lines changed: 75 additions & 388 deletions

File tree

‎src/main/kotlin/graphql/kickstart/tools/SchemaParser.kt‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,9 @@ class SchemaParser internal constructor(
6262
private val codeRegistryBuilder = GraphQLCodeRegistry.newCodeRegistry()
6363
private val directiveWiringHelper = DirectiveWiringHelper(runtimeWiring, codeRegistryBuilder)
6464

65-
private lateinit var schemaDirectives : Set<GraphQLDirective>
66-
// replaces the first definition of a directive with the real one while they're being created, see createDirectives
65+
// by name, in the order they're declared
66+
private val schemaDirectives = mutableMapOf<String, GraphQLDirective>()
67+
// set while createDirectives runs, see there
6768
private var finishDirective: (String) -> Unit = {}
6869

6970
/**
@@ -114,7 +115,7 @@ class SchemaParser internal constructor(
114115
// Directives applied to the schema itself, e.g. `extend schema @link(...)`. Only GraphQLAppliedDirectives are built here, not
115116
// the deprecated GraphQLDirective ones: graphql-java doesn't resolve type references in those, so enum arguments would fail.
116117
val schemaAppliedDirectives = buildAppliedDirectives(rootInfo.getDirectives()).toList()
117-
return SchemaObjects(query, mutation, subscription, types, schemaDirectives, codeRegistryBuilder, rootInfo.getDescription(), schemaAppliedDirectives)
118+
return SchemaObjects(query, mutation, subscription, types, schemaDirectives.values.toSet(), codeRegistryBuilder, rootInfo.getDescription(), schemaAppliedDirectives)
118119
}
119120

120121
/**
@@ -325,32 +326,31 @@ class SchemaParser internal constructor(
325326
}
326327

327328
private fun createDirectives(inputObjects: MutableList<GraphQLInputObjectType>) {
328-
// Input objects used by directive arguments get built here, and building them needs the definitions of their own applied
329-
// directives. So a first set of definitions only refers to input objects by type reference, and each one is replaced by the
330-
// real definition the first time it's looked up, so that input objects see the real definitions of the directives applied
331-
// to them. Only where a directive is applied within its own argument types, i.e. a cycle, do they see the first definition.
332-
schemaDirectives = directiveDefinitions.map { definition ->
333-
createDirective(definition) { determineType(GraphQLInputType::class, it, permittedTypesForInputObject, inputObjects) as GraphQLInputType }
334-
}.toSet()
329+
// The input objects used by directive arguments are built here, and they need the directives applied to them. So every
330+
// directive is first built with type references to input objects, then rebuilt with the input objects the first time it's
331+
// looked up. Only a directive applied within its own argument types gets its first build there.
332+
directiveDefinitions.forEach { definition ->
333+
schemaDirectives[definition.name] =
334+
createDirective(definition) { determineType(GraphQLInputType::class, it, permittedTypesForInputObject, inputObjects) as GraphQLInputType }
335+
}
335336
val unfinishedDefinitions = directiveDefinitions.associateByTo(mutableMapOf()) { it.name }
336-
// shared by all definitions, so that an input object that's still being built isn't built again for another directive
337+
// shared, so that an input object that's still being built isn't built again for another directive
337338
val referencingInputObjects = mutableSetOf<String>()
338339
finishDirective = { name ->
339340
unfinishedDefinitions.remove(name)?.let { definition ->
340-
val directive = createDirective(definition) { determineInputType(it, inputObjects, referencingInputObjects) }
341-
schemaDirectives = schemaDirectives.map { if (it.name == name) directive else it }.toSet()
341+
schemaDirectives[name] = createDirective(definition) { determineInputType(it, inputObjects, referencingInputObjects) }
342342
}
343343
}
344344
directiveDefinitions.forEach { finishDirective(it.name) }
345345
finishDirective = {}
346346
// because the arguments can have directives too, we attach them only after the directives themselves are created
347-
schemaDirectives = schemaDirectives.map { d ->
347+
schemaDirectives.replaceAll { _, d ->
348348
val arguments = d.arguments.map { a -> a.transform {
349349
it.withAppliedDirectives(*buildAppliedDirectives(a.definition!!.directives))
350350
.withDirectives(*buildDirectives(a.definition!!.directives, Introspection.DirectiveLocation.OBJECT))
351351
} }
352352
d.transform { it.replaceArguments(arguments) }
353-
}.toSet()
353+
}
354354
}
355355

356356
private fun createDirective(definition: DirectiveDefinition, determineArgumentType: (Type<*>) -> GraphQLInputType): GraphQLDirective {
@@ -373,7 +373,7 @@ class SchemaParser internal constructor(
373373

374374
private fun findDirective(name: String): GraphQLDirective? {
375375
finishDirective(name)
376-
return schemaDirectives.find { it.name == name }
376+
return schemaDirectives[name]
377377
}
378378

379379
private fun createDirectiveArgument(definition: InputValueDefinition, determineArgumentType: (Type<*>) -> GraphQLInputType): GraphQLArgument {

‎src/main/kotlin/graphql/kickstart/tools/directive/DirectiveWiringHelper.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,13 @@ internal class DirectiveWiringHelper(
5959

6060
private fun wireInputFields(fieldsContainer: GraphQLInputFieldsContainer): List<GraphQLInputObjectField> {
6161
return fieldsContainer.fieldDefinitions.map { field ->
62-
wireDirectives(WiringWrapper(field, INPUT_FIELD_DEFINITION, SchemaDirectiveWiring::onInputObjectField, inputFieldsContainer = fieldsContainer))
62+
wireDirectives(WiringWrapper(field, FIELD_DEFINITION, SchemaDirectiveWiring::onInputObjectField, inputFieldsContainer = fieldsContainer))
6363
}
6464
}
6565

6666
private fun wireEnumValues(enumType: GraphQLEnumType): List<GraphQLEnumValueDefinition> {
6767
return enumType.values.map { value ->
68-
wireDirectives(WiringWrapper(value, ENUM_VALUE, SchemaDirectiveWiring::onEnumValue, enumType = enumType))
68+
wireDirectives(WiringWrapper(value, FIELD_DEFINITION, SchemaDirectiveWiring::onEnumValue, enumType = enumType))
6969
}
7070
}
7171

0 commit comments

Comments
 (0)