Skip to content

Merge type extensions into the types they extend - #855

Open
oryan-block wants to merge 1 commit into
masterfrom
bugfix/552
Open

oryan-block wants to merge 1 commit into
masterfrom
bugfix/552

Conversation

@oryan-block

Copy link
Copy Markdown
Collaborator

Fixes #552

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

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 | Bar plus extend union FooBar = Baz gives members [Baz], enum Color { RED } plus extend enum Color { GREEN } gives values [GREEN], and interface Node { id: ID } plus extend 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 with type Product @key(fields: "upc") plus extend type Product @key(fields: "name") keeps only the first @key, which is the gap left by #345. extend type X implements Y is dropped too.

UnionTypeExtensionDefinition, EnumTypeExtensionDefinition, InterfaceTypeExtensionDefinition and ScalarTypeExtensionDefinition are subclasses of their base definitions. SchemaClassScanner only filtered out object and input extensions, so the others stayed in definitionsByName and replaced the base definition (associateBy, last one wins). SchemaParser#createObject and SchemaParser#createInputObject only read directives from the base definition, and createObject only read its implements.

The scanner now keeps only base definitions in definitionsByName and groups every extension by type name in a new typeExtensions map, which goes to the parser through ScannedSchemaObjects. When discovering types it also reads union-extension members, interface-extension fields and extend type X implements Y. Every create* function in SchemaParser now merges the extensions' members, values, fields and interfaces, sets extensionDefinitions, and applies the directives of the base followed by those of the extensions (SchemaParser#directivesOf), which is what graphql-java's SchemaGenerator does. Scalars get their SDL description, their base definition and extensionDefinitions. Object and input field handling is unchanged. There's a small extensionsOf<T>(name) helper in Utils.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 SchemaPrinter would print invalid SDL. Undeclared directives (with allowUndeclaredDirectives) 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 Foo plus extend union Foo) is now a SchemaClassScannerError.

Left out, and worth separate issues:

  • Scalar directives. Scalars are still built in the scanner without any directives, base or extension, including @specifiedBy. Fixing that means moving scalar creation to the parser.
  • Object fields still keep the extension's definition when a name is repeated, and input objects still list extension fields before base fields. Changing either would change existing behaviour or the order of printed SDL.
  • A nested union that's only reachable as a member of another union still fails in the scanner with "No object type found". That's not new.

This touches SchemaClassScanner and SchemaParser like #842, #843 and #838, but git merge-tree against each of their branches merges cleanly. #842's findUsage error hints won't name types that are only used inside union, enum, interface or scalar extensions. Whichever merges second can make it walk typeExtensions too. After #842 is in, the scanner's separate extensionDefinitions and inputExtensionDefinitions lists can go in favour of typeExtensions.

The new TypeExtensionTest has 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 and implements, and scalar descriptions, plus definition and extensionDefinitions on 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 @key case (applied and legacy directives, and SchemaPrinter output), input extension directives, a SchemaDirectiveWiring firing on extensions of every kind, a repeated non-repeatable directive, repeated undeclared directives, and an undeclared extension directive with and without allowUndeclaredDirectives. It also covers includeUnusedTypes, 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 verify passes with 281 tests, and so does clean 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 Y now takes effect. Directives on extend type, extend input, extend interface, extend union and extend enum are now built and passed to SchemaDirectiveWiring, so an undeclared one fails with "Found applied directive ... without corresponding directive definition" unless allowUndeclaredDirectives is 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 and definition is always the base, so SchemaPrinter with useAstDefinitions(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

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>
@sonarqubecloud

Copy link
Copy Markdown

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.

Type extensions aren't merged: union/enum/interface/scalar extensions replace the base type, directives on object/input extensions are dropped

1 participant