fix: support linked, repeatable, federation directives#7249
Conversation
🦋 Changeset detectedLatest commit: 1f1afd6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughSummary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis update enhances the handling of repeatable directives in GraphQL schema merging, particularly for federation-related directives imported via @link. It introduces deep argument equality checks for directive deduplication, tracks repeatable directives from external specifications, updates merging logic, and adds comprehensive tests for these scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant mergeTypeDefs
participant mergeDirectives
participant isRepeatableDirective
participant federation-composition
User->>mergeTypeDefs: Merge type sources (with federation/link imports)
mergeTypeDefs->>federation-composition: extractLinkImplementations
federation-composition-->>mergeTypeDefs: Return repeatable directive names
mergeTypeDefs->>mergeDirectives: Pass repeatableLinkImports in config
mergeDirectives->>isRepeatableDirective: Check if directive is repeatable (including link imports)
isRepeatableDirective-->>mergeDirectives: Result (repeatable or not)
mergeDirectives->>mergeDirectives: Deduplicate/merge directives using deep argument equality
mergeDirectives-->>mergeTypeDefs: Return merged directives
mergeTypeDefs-->>User: Return merged schema
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/merge/tests/merge-typedefs.spec.tsOops! Something went wrong! :( ESLint: 9.30.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. If you still have problems after following the migration guide, please stop by 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
💻 Website PreviewThe latest changes are available as preview in: https://pr-7249.graphql-tools.pages.dev |
| DirectiveDefinitionNode | ||
| >; | ||
|
|
||
| config.repeatableLinkImports = repeatableLinkImports; |
There was a problem hiding this comment.
This may not be how we want to pass this value around. I considered passing another argument to all the merge functions, but adding a config option was much simpler. Let me know if this would be better to do in a different way.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
.changeset/@graphql-tools_merge-7249-dependencies.md (1)
4-5: Fix Markdown list indentation (MD007).The dependency bullet is indented two spaces, violating the project’s lint rule. Align it with the front-matter header or add a leading dash only one space from the margin.
- - Added dependency [`@theguild/federation-composition@^0.16.0` ↗︎](...) +- Added dependency [`@theguild/federation-composition@^0.16.0` ↗︎](...)packages/import/tests/schema/import-schema.spec.ts (2)
470-474: Typo in comment.
"If this every changes"→"If this ever changes".-// If this every changes, then the expectedSDL here can have the duplicate +// If this ever changes, then the expectedSDL here can have the duplicate
482-486: Double @key directive may mask real deduplication regressions.Expecting two identical
@key(fields: "id")directives bakes current duplication into the gold file. When deduplication gets fixed this test will start failing for the wrong reason. Consider asserting on order-insensitive set equality or removing the duplicate fromexpectedSDLand usingtoBeSimilarGqlDocwith a comparison that ignores directive ordering/count.packages/merge/src/typedefs-mergers/schema-def.ts (1)
35-37: Guarding undefinedexistingNodeis correct, but update type hints for callers.The signature changed to allow
undefined; ensure every call site is re-compiled understrictNullChecksto avoid passingnullinadvertently. If any external types exported this function, bump at least a minor version.packages/merge/src/typedefs-mergers/directives.ts (1)
68-94:matchValuesis quadratic and verbose – consider a simpler deep-equality
matchValuesperforms nestedfindcalls for lists/objects, turning directive-deduplication into an O(n²) walk per comparison. Converting AST values to JS viavalueFromASTUntyped(orgraphql/jsutils) and then doing a shallowfast-deep-equalstyle compare would both simplify the code and reduce complexity.Not critical for small schemas but noticeable in large, heavily-annotated federated graphs.
packages/merge/tests/merge-typedefs.spec.ts (4)
486-498: Directive location is invalid & oneMyTypedefinition is redundant
@fieldsis declaredrepeatable on INTERFACE, yet it is attached totypedefinitions (CoreType,MyType).
The test still parses because validation is skipped, but the schema would be invalid once validated.
UseOBJECT, orOBJECT | INTERFACE, to avoid false–positive test coverage.The third array element duplicates the previous
type MyType @fields(name: "id") { … }verbatim.
The duplication is only needed to assert directive de-duplication, but repeating the whole type clutters the test – anextend type(or eventype MyType @fields(name: "id")once) is enough.- directive @fields(name: String!, args: [String]) repeatable on INTERFACE + directive @fields(name: String!, args: [String]) repeatable on OBJECT | INTERFACE @@ - `type MyType @fields(name: "id") { id: Int }`,
528-554: Same directive-location mismatch as aboveThe helper directive in this test is also limited to
INTERFACEbut is applied to the object typeCoreType.
Please adjust the location or the attachment site for consistency with the spec.
610-628: Minor API-consistency nitpick
mergeTypeDefsaccepts either a singleTypeSourceor an array.
All other tests pass an array; here you pass theDocumentNodedirectly.
Wrapping it in an array keeps the test style uniform and avoids an accidental “options” parameter shift if the call site is edited later.- const merged = mergeTypeDefs( - parse(` + const merged = mergeTypeDefs([ + parse(` @@ - `, - ), - ); + `, + ), + ]);
1796-1817:it.todocomment left in – consider tracking with an issueLeaving the TODO test stub is fine, but adding a link to an open issue (or creating one) will prevent it from being forgotten.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.changeset/@graphql-tools_merge-7249-dependencies.md(1 hunks)packages/import/tests/schema/fixtures/directive/i.graphql(1 hunks)packages/import/tests/schema/import-schema.spec.ts(3 hunks)packages/merge/package.json(1 hunks)packages/merge/src/typedefs-mergers/directives.ts(4 hunks)packages/merge/src/typedefs-mergers/merge-typedefs.ts(6 hunks)packages/merge/src/typedefs-mergers/schema-def.ts(1 hunks)packages/merge/tests/merge-typedefs.spec.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/merge/src/typedefs-mergers/merge-typedefs.ts (3)
packages/utils/src/Interfaces.ts (1)
TypeSource(270-277)packages/utils/src/comments.ts (1)
resetComments(24-26)packages/merge/src/typedefs-mergers/merge-nodes.ts (1)
mergeGraphQLNodes(32-135)
🪛 markdownlint-cli2 (0.17.2)
.changeset/@graphql-tools_merge-7249-dependencies.md
5-5: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (5)
packages/merge/package.json (1)
53-56: [web_search]According to the semantic versioning specification, what version range does the constraint "^0.16.0" match?packages/import/tests/schema/fixtures/directive/i.graphql (1)
1-8: LGTM – minimal fixture, covers repeatable @key directive.
No issues detected.packages/import/tests/schema/import-schema.spec.ts (1)
491-503: Great addition – validates repeatable directives context.The new test captures federation 2.6 behaviour and should prevent regressions.
packages/merge/tests/merge-typedefs.spec.ts (2)
1766-1779: Looks good – validates new federation behaviourThe test accurately captures the “do not merge unless arguments match” requirement for repeatable federation directives.
No issues spotted.
1781-1794: Good edge-case coverageNice addition verifying that
@linkimports are recognised even when theextend schemafollows type definitions.
No changes required.
| config.repeatableLinkImports = repeatableLinkImports; | ||
|
|
There was a problem hiding this comment.
Avoid mutating the caller’s config object
Assigning to config.repeatableLinkImports breaks referential transparency and leaks internal state:
config.repeatableLinkImports = repeatableLinkImports;Clone before modification, or return the computed set via the function result instead.
🤖 Prompt for AI Agents
In packages/merge/src/typedefs-mergers/merge-typedefs.ts around lines 258 to
259, avoid directly mutating the caller's config object by assigning to
config.repeatableLinkImports. Instead, create a clone of the config object
before modifying repeatableLinkImports or refactor the function to return the
updated config or the computed repeatableLinkImports value without altering the
original config. This preserves referential transparency and prevents leaking
internal state.
| /** | ||
| * Allow directives that are not defined in the schema, but are imported | ||
| * through federated @links, to be repeated. | ||
| */ | ||
| repeatableLinkImports?: Set<string>; | ||
| /** |
There was a problem hiding this comment.
Config is mutated later – treat repeatableLinkImports as read-only
repeatableLinkImports is declared on the public Config interface but later (line 258) the same object passed by the caller is mutated in-place. Public options are normally considered immutable; doing otherwise can create surprising side-effects for integrators who reuse a single config object across calls.
- config.repeatableLinkImports = repeatableLinkImports;
+ const nextCfg = { ...config, repeatableLinkImports };and pass nextCfg further down instead of config.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/merge/src/typedefs-mergers/merge-typedefs.ts around lines 81 to 86,
the repeatableLinkImports property on the Config interface is currently mutated
later in the code (around line 258), which breaks the expectation that public
config objects are immutable. To fix this, treat repeatableLinkImports as
read-only by creating a copy of it before any mutation occurs, and ensure that
the copied config object (nextCfg) is passed down in subsequent calls instead of
the original config to avoid side effects on the caller's object.
| } | ||
|
|
||
| function mergeArguments(a1: readonly ArgumentNode[], a2: readonly ArgumentNode[]): ArgumentNode[] { | ||
| const result: ArgumentNode[] = [...a2]; |
There was a problem hiding this comment.
To merge arguments correctly, even if defined within the same schema, we must not assume a2 is all clear here. Instead, we must process the entire list of arguments to build up the result set.
| return result; | ||
| } | ||
|
|
||
| function deduplicateDirectives( |
There was a problem hiding this comment.
no need for a separate call to dedupe if we dedupe as we go.
| }); | ||
|
|
||
| it.todo('supports multiple schema extensions'); | ||
| // , () => { |
There was a problem hiding this comment.
this is outside the scope of this PR but i think it is something we need in order to better support federation, because extend schema with directives is common
* fix: support linked, repeatable, federation directives * chore(dependencies): updated changesets for modified dependencies --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
2 participants
Description
Adds support for known repeatable federation directives without requiring they be defined in the SDL.
Previously, mergeTypeDefs used the directive definition node to check if a directive was repeatable. This is an issue with Federation because
@linkcan be used without specifying what is being imported for the default federation links. Rather than add the definitions, which may become stale, I made note of specifically which directives with federation are repeatable. This list is short and is unlikely to change frequently.I also adjusted the mergeTypeDefs logic for repeatable directives. If they are repeatable, then they are only merged if all the arguments match.
Related:
mergeTypeDefsthrows away therepeatabledefinition on a schema directive #6505Type of change
Please delete options that are not relevant.
How Has This Been Tested?
See test files.
Checklist:
CONTRIBUTING doc and the
style guidelines of this project