Fix schema rebuild after root type deletion#4390
Merged
Merged
Conversation
Contributor
Test ReportTest Results
Code Coverage (Java 25)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3384.
The core issue is that a built
GraphQLSchemacan contain two views of the same relationship:GraphQLTypeReference("Node")Person.getInterfaces()returns the actualGraphQLInterfaceType NodeinstanceIn the #3384 repro,
Personwas originally created with.withInterface(typeRef("Node")). During schema build, that reference is resolved andPerson.getInterfaces()starts returning the realNodeinterface. Later, when the mutation root is removed,Nodeis no longer reachable through the deleted mutation field. The rebuilt schema still reachesPersonfromQuery, andPersonstill implementsNode, soNodemust remain in the rebuilt type map.The catch is that schema build collection intentionally traverses
getChildrenWithTypeReferences(). ForPerson, that traversal sees the originalGraphQLTypeReference("Node"), not the already-resolvedNodeobject returned byPerson.getInterfaces(). So the collector can visitPersonwithout collecting the resolvedNodeinterface. Later,GraphQLTypeResolvingVisitorusesPerson.getInterfaces(), looks upNodein the rebuilt type map, and fails becauseNodewas missed during collection.This is what the code calls an indirect strong reference:
GraphQLNamedTypeobject through an accessor such asgetInterfaces()orgetTypes()GraphQLTypeCollectingVisitoralready handled this pattern for field, argument, input field, and applied directive argument types. This PR extends the same collection logic to:The same original-reference versus resolved-accessor split exists for unions and interface inheritance:
GraphQLUnionType.getChildrenWithTypeReferences()exposes original possible types, whilegetTypes()can return resolved object typesGraphQLInterfaceType.getChildrenWithTypeReferences()exposes original implemented interfaces, whilegetInterfaces()can return resolved interfacesThis 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:
The object/interface case is covered through both paths:
GraphQLSchema.newSchema(schema).mutation((GraphQLObjectType) null).build()SchemaTransformer.transformSchemaWithDeletes(...)The union and interface-inheritance cases specifically prove why
GraphQLTypeCollectingVisitorneeds to followgetTypes()andgetInterfaces(), 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:
markGeneratedEqualsHashCodenow cleansclasses-jacocobefore rewriting and does not append duplicategraphql.coverage.Generatedannotations. That prevents local-only ArchUnit failures inJMHForkArchRuleTestafter interrupted or repeated incremental runs.Verification
Also ran: