Repository navigation
Merge type extensions into the types they extend - #855
Open
oryan-block wants to merge 1 commit into
Open
oryan-block wants to merge 1 commit into
oryan-block wants to merge 1 commit into
Conversation
Union, enum, interface and scalar extensions subclass their base definitions, so they replaced the base type (whichever came last won) instead of being merged into it. Directives and implemented interfaces on object and input extensions were dropped. The scanner now keeps the base definitions and groups every extension by type name. It reads union members, interface fields and implemented interfaces from the extensions too. The parser merges members, values, fields, interfaces and directives like graphql-java's SchemaGenerator, and sets extensionDefinitions on every type. graphql-java rejects an extension that repeats a value, field or non-repeatable directive of its type. Here enum values, interface fields and directives keep the first one, so schemas that repeated them to work around this keep building. Object and input fields are handled as before. An extension of a different kind than its type is now an error. A union, enum, interface or scalar extension without a definition still builds as the type itself, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.



Fixes #552
Checklist
Description
Extending a union, enum, interface or scalar replaces the type instead of adding to it, and no error is raised. On master,
union FooBar = Foo | Barplusextend union FooBar = Bazgives members[Baz],enum Color { RED }plusextend enum Color { GREEN }gives values[GREEN], andinterface Node { id: ID }plusextend interface Node { name: String }gives fields[name]. With the usual base-then-extension order it's the base that gets lost, which is what Sceri hit: interface fields go missing and things break at runtime. Object and input extensions merge their fields, but their directives are dropped. So a federation entity declared withtype Product @key(fields: "upc")plusextend type Product @key(fields: "name")keeps only the first@key, which is the gap left by #345.extend type X implements Yis dropped too.UnionTypeExtensionDefinition,EnumTypeExtensionDefinition,InterfaceTypeExtensionDefinitionandScalarTypeExtensionDefinitionare subclasses of their base definitions.SchemaClassScanneronly filtered out object and input extensions, so the others stayed indefinitionsByNameand replaced the base definition (associateBy, last one wins).SchemaParser#createObjectandSchemaParser#createInputObjectonly readdirectivesfrom the base definition, andcreateObjectonly read itsimplements.The scanner now keeps only base definitions in
definitionsByNameand groups every extension by type name in a newtypeExtensionsmap, which goes to the parser throughScannedSchemaObjects. When discovering types it also reads union-extension members, interface-extension fields andextend type X implements Y. Everycreate*function inSchemaParsernow merges the extensions' members, values, fields and interfaces, setsextensionDefinitions, and applies the directives of the base followed by those of the extensions (SchemaParser#directivesOf), which is what graphql-java'sSchemaGeneratordoes. Scalars get their SDL description, their basedefinitionandextensionDefinitions. Object and input field handling is unchanged. There's a smallextensionsOf<T>(name)helper inUtils.kt.graphql-java rejects an extension that repeats an enum value, an interface field or a non-repeatable directive. I didn't copy that. Repeating the base values in an extension was the usual workaround for this bug, so tools keeps the first definition of a repeated enum value, interface field or union member and those schemas keep building. A declared non-repeatable directive (or a built-in one) that an extension applies again is kept once, the first one, with a WARN. Without that it'd be applied twice in the applied directives and once in the legacy ones, and
SchemaPrinterwould print invalid SDL. Undeclared directives (withallowUndeclaredDirectives) are all kept, since they might be repeatable, like an undeclared federation@key.A union, enum, interface or scalar extension with no base type isn't valid per the spec, but it built as the type on master, so I kept that. Several such extensions of one name are now merged instead of the last one winning. Object and input extensions without a base type are still ignored. An extension whose kind doesn't match its type (e.g.
type Fooplusextend union Foo) is now aSchemaClassScannerError.Left out, and worth separate issues:
@specifiedBy. Fixing that means moving scalar creation to the parser.This touches
SchemaClassScannerandSchemaParserlike #842, #843 and #838, butgit merge-treeagainst each of their branches merges cleanly. #842'sfindUsageerror hints won't name types that are only used inside union, enum, interface or scalar extensions. Whichever merges second can make it walktypeExtensionstoo. After #842 is in, the scanner's separateextensionDefinitionsandinputExtensionDefinitionslists can go in favour oftypeExtensions.The new
TypeExtensionTesthas 23 tests and runs queries where it matters. It covers merged union members (with the base declared first and with the extension declared first), enum values, interface fields andimplements, and scalar descriptions, plusdefinitionandextensionDefinitionson each. It checks that the scanner wants classes for extension union members and for objects that implement an interface only through an extension, and that a union used only by an interface-extension field is discovered. For directives it covers the federation@keycase (applied and legacy directives, andSchemaPrinteroutput), input extension directives, aSchemaDirectiveWiringfiring on extensions of every kind, a repeated non-repeatable directive, repeated undeclared directives, and an undeclared extension directive with and withoutallowUndeclaredDirectives. It also coversincludeUnusedTypes,getUnusedDefinitions(), repeated values, fields and members, extension-only enums and unions, and kind mismatches. On master 20 tests fail and 2 error. The repeated non-repeatable directive test passes there because master drops extension directives altogether.clean verifypasses with 281 tests, and so doesclean test -Dgraphql-java.version=25.0.Behaviour change: some schemas that built before now fail, correctly. With the usual order it's the base that's newly enforced. Every member of a union and of its extensions needs a class, every enum value from the base and the extensions must exist in the Java enum, implementors must declare the base interface's fields, and unions and interfaces used by the fields that used to be dropped are discovered, so their members and implementors need classes too.
extend type X implements Ynow takes effect. Directives onextend type,extend input,extend interface,extend unionandextend enumare now built and passed toSchemaDirectiveWiring, so an undeclared one fails with "Found applied directive ... without corresponding directive definition" unlessallowUndeclaredDirectivesis set. Where a repeat is dropped, the base's copy is now the one kept, where master kept the extension's for unions, enums and interfaces.getExtensionDefinitions()is filled on object, interface, union, enum and scalar types anddefinitionis always the base, soSchemaPrinterwithuseAstDefinitions(true)prints the extensions too.getUnusedDefinitions()and the "can never be accessed" WARN report the base definition. Kind mismatches are a new error, which also covers object and input extensions of a different kind that used to be ignored silently. Probably worth a minor version and a note in the release notes.🤖 Generated with Claude Code