Skip to content

Backport 26.1: Fix schema rebuild after root type deletion - #4446

Open
andimarek wants to merge 1 commit into
26.xfrom
codex/backport-4390-to-26.x
Open

Backport 26.1: Fix schema rebuild after root type deletion#4446
andimarek wants to merge 1 commit into
26.xfrom
codex/backport-4390-to-26.x

Conversation

@andimarek

Copy link
Copy Markdown
Member

Backport of #4390 to the 26.x maintenance branch.

Summary

Fixes #3384.

The core issue is that a built GraphQLSchema can contain two views of the same relationship:

  • the original child list used for rebuild traversal, which may still contain GraphQLTypeReference("Node")
  • the resolved accessor view used by runtime/schema APIs, where Person.getInterfaces() returns the actual GraphQLInterfaceType Node instance

In the #3384 repro, Person was originally created with .withInterface(typeRef("Node")). During schema build, that reference is resolved and Person.getInterfaces() starts returning the real Node interface. Later, when the mutation root is removed, Node is no longer reachable through the deleted mutation field. The rebuilt schema still reaches Person from Query, and Person still implements Node, so Node must remain in the rebuilt type map.

The catch is that schema build collection intentionally traverses getChildrenWithTypeReferences(). For Person, that traversal sees the original GraphQLTypeReference("Node"), not the already-resolved Node object returned by Person.getInterfaces(). So the collector can visit Person without collecting the resolved Node interface. Later, GraphQLTypeResolvingVisitor uses Person.getInterfaces(), looks up Node in the rebuilt type map, and fails because Node was missed during collection.

This is what the code calls an indirect strong reference:

  • indirect: the traverser did not visit it as a normal child node, because the child list exposes the original type reference
  • strong: the schema element already holds the actual GraphQLNamedType object through an accessor such as getInterfaces() or getTypes()

GraphQLTypeCollectingVisitor already handled this pattern for field, argument, input field, and applied directive argument types. This PR extends the same collection logic to:

  • object implemented interfaces
  • interface implemented interfaces
  • union member types

The same original-reference versus resolved-accessor split exists for unions and interface inheritance:

  • GraphQLUnionType.getChildrenWithTypeReferences() exposes original possible types, while getTypes() can return resolved object types
  • GraphQLInterfaceType.getChildrenWithTypeReferences() exposes original implemented interfaces, while getInterfaces() can return resolved interfaces

This does not keep every deleted-path extra type alive. It only preserves resolved named types that are still referenced by a schema element that remains reachable in the rebuilt schema.

Repro Coverage

Adds #3384 Spock coverage for these schema rebuild cases:

  • object implementing an interface through a type reference
  • union containing a member through a type reference
  • interface implementing another interface through a type reference

The object/interface case is covered through both paths:

  • direct rebuild with GraphQLSchema.newSchema(schema).mutation((GraphQLObjectType) null).build()
  • traversal-based deletion with SchemaTransformer.transformSchemaWithDeletes(...)

The union and interface-inheritance cases specifically prove why GraphQLTypeCollectingVisitor needs to follow getTypes() and getInterfaces(), not just field/argument/input references.

Before the collector change, these cases fail during rebuild with an NPE in GraphQLTypeResolvingVisitor; they now pass.

Local Test Stability

This PR also keeps the local build cleanup from the previous version of #4390: markGeneratedEqualsHashCode now cleans classes-jacoco before rewriting and does not append duplicate graphql.coverage.Generated annotations. That prevents local-only ArchUnit failures in JMHForkArchRuleTest after interrupted or repeated incremental runs.

Verification

./gradlew test --tests "graphql.schema.impl.SchemaUtilTest.can rebuild schema after removing root type that made an implemented interface reachable" --tests "graphql.schema.impl.SchemaUtilTest.can transform schema after deleting root type that made an implemented interface reachable" --tests "graphql.schema.impl.SchemaUtilTest.can rebuild schema after removing root type that made a union member reachable" --tests "graphql.schema.impl.SchemaUtilTest.can rebuild schema after removing root type that made an interface implemented by another interface reachable"

./gradlew test --tests graphql.schema.impl.SchemaUtilTest

Also ran:

git diff --check

* Make generated annotation marking idempotent

* Add failing schema rebuild repros for issue 3384

* Fix type collection for resolved schema references

* Add schema rebuild coverage for union and interface references

* Remove issue annotations from schema rebuild tests

* Rename schema rebuild test helpers
@andimarek andimarek changed the title Fix schema rebuild after root type deletion Backport 26.1: Fix schema rebuild after root type deletion Aug 20, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5737 (-190 🔴) 5681 (-190 🔴) 0 (±0) 0 (±0) 56 (±0)
Java 17 5737 (-190 🔴) 5680 (-190 🔴) 0 (±0) 0 (±0) 57 (±0)
Java 21 5737 (-190 🔴) 5680 (-190 🔴) 0 (±0) 0 (±0) 57 (±0)
Java 25 5737 (-190 🔴) 5680 (-190 🔴) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 22980 (-760 🔴) 22753 (-760 🔴) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 28910 3119 90.3% -0.3% 🔴
Branches 8420 1504 84.8% -0.4% 🔴
Methods 7731 1222 86.4% -0.6% 🔴

Changed Class Coverage (35 classes)

Class Line Branch Method
g.l.AstSignature
$3
+100.0% 🟢 +100.0% 🟢 +100.0% 🟢
g.l.AstSignatureInputReferences removed removed removed
g.l.AstSignatureReferenceCollector removed removed removed
g.l.AstSignatureWithInputResult removed removed removed
g.l.AstSignatureWithInputResult
$Builder
removed removed removed
g.l.Directive -3.2% 🔴 ±0.0% -5.9% 🔴
g.l.DirectiveDefinition -19.2% 🔴 ±0.0% ±0.0%
g.l.DirectiveExtensionDefinition removed removed removed
g.l.DirectiveExtensionDefinition
$Builder
removed removed removed
g.l.DirectiveLocation -4.3% 🔴 ±0.0% -8.3% 🔴
g.l.FragmentSpread -16.1% 🔴 ±0.0% -27.8% 🔴
g.l.FragmentSpread
$Builder
-6.5% 🔴 ±0.0% -9.1% 🔴
g.l.PrettyAstPrinter ±0.0% -2.0% 🔴 ±0.0%
g.l.VariableDefinition -14.5% 🔴 ±0.0% -26.1% 🔴
g.l.VariableDefinition
$Builder
-6.2% 🔴 ±0.0% -8.8% 🔴
g.s.d.a.EditOperationAnalyzer -0.3% 🔴 ±0.0% -0.7% 🔴
g.s.d.a.SchemaDifference
$AppliedDirectiveDirectiveLocation
removed removed removed
g.s.GraphQLDirective -3.1% 🔴 ±0.0% -6.7% 🔴
g.s.GraphqlDirectivesContainerTypeBuilder -14.7% 🔴 -50.0% 🔴 -18.2% 🔴
g.s.GraphQLInterfaceType
$Builder
-1.4% 🔴 ±0.0% -3.8% 🔴
g.s.GraphQLObjectType -1.7% 🔴 ±0.0% -3.6% 🔴
g.s.i.e.DirectiveExtensionDirectiveRedefinitionError removed removed removed
g.s.i.e.DirectiveExtensionMissingBaseError removed removed removed
g.s.i.e.SchemaProblem -16.7% 🔴 ±0.0% -25.0% 🔴
g.s.i.TypeDefinitionRegistry ±0.0% -0.3% 🔴 ±0.0%
g.s.i.UnionTypesChecker ±0.0% -7.1% 🔴 ±0.0%
g.s.t.FieldVisibilitySchemaTransformation
$ElementRemovalVisitor
removed removed removed
g.s.t.FieldVisibilitySchemaTransformation
$FieldRemovalVisitor
+100.0% 🟢 +100.0% 🟢 +100.0% 🟢
g.s.t.VisibleInterfaceImplementationPredicateEnvironmentImpl removed removed removed
g.s.v.NoDefaultValueCircularRefs removed removed removed
g.s.v.NoUnbrokenInputCycles -2.6% 🔴 -4.4% 🔴 ±0.0%
g.v.OperationValidator ±0.0% -1.3% 🔴 ±0.0%
g.v.TraversalContext +99.4% 🟢 +94.1% 🟢 +100.0% 🟢
g.v.ValidationContext -2.8% 🔴 ±0.0% -5.0% 🔴
g.v.ValidationError
$Builder
±0.0% -50.0% 🔴 ±0.0%
Directive — method details
Method Line Branch
deepCopy 0.0% (-100.0% 🔴)
DirectiveDefinition — method details
Method Line Branch
<init> new 100.0%
withNewChildren 0.0% (-100.0% 🔴)
deepCopy 0.0% (-100.0% 🔴)
lambda$withNewChildren$0 0.0% (-100.0% 🔴)
<init> removed removed
getDirectives removed removed
getDirectivesByName removed removed
getDirectives removed removed
hasDirective removed removed
DirectiveLocation — method details
Method Line Branch
deepCopy 0.0% (-100.0% 🔴)
FragmentSpread — method details
Method Line Branch
getDirectivesByName 0.0% (-100.0% 🔴)
getDirectives 0.0% (-100.0% 🔴)
hasDirective 0.0% (-100.0% 🔴)
withNewChildren 0.0% (-100.0% 🔴)
lambda$withNewChildren$0 0.0% (-100.0% 🔴)
FragmentSpread.Builder — method details
Method Line Branch
directive 0.0% (-100.0% 🔴)
PrettyAstPrinter — method details
Method Line Branch
lambda$directiveDefinition$0 100.0% 50.0% (-50.0% 🔴)
directiveExtensionDefinition removed removed
lambda$directiveExtensionDefinition$0 removed removed
VariableDefinition — method details
Method Line Branch
<init> new 100.0%
getDirectivesByName 0.0% (-100.0% 🔴)
getDirectives 0.0% (-100.0% 🔴)
hasDirective 0.0% (-100.0% 🔴)
withNewChildren 0.0% (-100.0% 🔴)
newVariableDefinition 0.0% (-100.0% 🔴)
lambda$withNewChildren$0 0.0% (-100.0% 🔴)
<init> removed removed
VariableDefinition.Builder — method details
Method Line Branch
directive 0.0% (-100.0% 🔴)
description removed removed
EditOperationAnalyzer — method details
Method Line Branch
appliedDirectiveDeleted 85.5% 72.5% (-0.2% 🔴)
appliedDirectiveArgumentDeleted 82.6% (-0.3% 🔴) 73.5% (-0.6% 🔴)
appliedDirectiveArgumentAdded 79.6% (-0.4% 🔴) 69.4% (-0.8% 🔴)
appliedDirectiveArgumentChanged 99.1% 68.6% (-0.3% 🔴)
appliedDirectiveAdded 85.5% 72.5% (-0.2% 🔴)
isAppliedDirectiveAdded 80.4% (-8.9% 🔴) 78.6% (-4.8% 🔴)
lambda$isAppliedDirectiveAdded$6 0.0% (-100.0% 🔴)
GraphQLDirective — method details
Method Line Branch
<init> new 100.0%
copy 0.0% (-100.0% 🔴)
<init> removed removed
getExtensionDefinitions removed removed
isDeprecated removed removed
getDeprecationReason removed removed
getDirectives removed removed
getDirectivesByName removed removed
getAllDirectivesByName removed removed
getDirective removed removed
getAppliedDirectives removed removed
getAllAppliedDirectivesByName removed removed
getAppliedDirective removed removed
GraphqlDirectivesContainerTypeBuilder — method details
Method Line Branch
withDirectives 0.0% (-100.0% 🔴) 0.0% (-100.0% 🔴)
withDirective 0.0% (-100.0% 🔴)
GraphQLInterfaceType.Builder — method details
Method Line Branch
replaceInterfaces 0.0% (-100.0% 🔴)
GraphQLObjectType — method details
Method Line Branch
getDirective 0.0% (-100.0% 🔴)
SchemaProblem — method details
Method Line Branch
toString 0.0% (-100.0% 🔴)
TypeDefinitionRegistry — method details
Method Line Branch
<init> new 100.0%
add 95.6% (-0.3% 🔴) 92.9% (-0.5% 🔴)
remove 92.3% (-0.5% 🔴) 90.9% (-0.8% 🔴)
remove 74.1% (-1.8% 🔴) 68.2% (-2.7% 🔴)
isPossibleType 92.6% (+14.8% 🟢) 83.3% (+16.7% 🟢)
<init> removed removed
removeDirectiveExtension removed removed
defineDirectiveExt removed removed
directiveExtensions removed removed
implementsInterface removed removed
getImplementingTypeExtensions removed removed
isUnionMember removed removed
lambda$isUnionMember$0 removed removed
lambda$implementsInterface$0 removed removed
lambda$defineDirectiveExt$0 removed removed
lambda$merge$15 removed removed
lambda$merge$16 removed removed
UnionTypesChecker — method details
Method Line Branch
checkUnionType 100.0% 90.0% (-10.0% 🔴)
hasMemberTypes removed removed
lambda$hasMemberTypes$0 removed removed
NoUnbrokenInputCycles — method details
Method Line Branch
unwrapNonNull 83.3% (-16.7% 🔴) 75.0% (-25.0% 🔴)
OperationValidator — method details
Method Line Branch
checkVariable new 100.0% 100.0%
validateDeferDirectiveOnRootLevel 92.9% 68.8% (-12.5% 🔴)
validateDeferDirectiveOnValidOperation new 90.9% 80.0%
getOperationDefinition new 100.0%
lambda$getOperationDefinition$1 new 100.0%
lambda$getOperationDefinition$0 new 100.0%
enterTraversal removed removed
enterField removed removed
enterVariableDefinition removed removed
enterArgument removed removed
enterArrayValue removed removed
enterObjectField removed removed
leaveTraversal removed removed
resolveSelectionSetParent removed removed
getSelectionSetOwnerType removed removed
getOperationType removed removed
getFieldDefinition removed removed
getArgumentDefinition removed removed
getNullableType removed removed
getParentType removed removed
getFieldDefinition removed removed
getInputType removed removed
getDefaultValue removed removed
getDirectiveDefinition removed removed
enterName removed removed
leaveName removed removed
lastElement removed removed
pop removed removed
checkVariable removed removed
validateVariableNotAllowedInConstantDirective removed removed
getInlineFragmentType removed removed
validateDeferDirectiveOnValidOperation removed removed
ValidationContext — method details
Method Line Branch
getTraversalContext new 100.0%
getParentType new 100.0%
getInputType new 100.0%
getDefaultValue new 100.0%
getFieldDef new 100.0%
getDirective new 100.0%
getArgument new 100.0%
getOutputType new 100.0%
getQueryPath new 100.0%
toString new 0.0%
ValidationError.Builder — method details
Method Line Branch
sourceLocation 100.0% 50.0% (-50.0% 🔴)

Full HTML report: build artifact jacoco-html-report

Updated: 2026-08-20 00:25:55 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant