Skip to content

chore: propagate federated directive definitions, subgraph schema nod…#2325

Merged
Aenimus merged 4 commits intomainfrom
david/eng-8464-propagate-missing-data
Nov 12, 2025
Merged

chore: propagate federated directive definitions, subgraph schema nod…#2325
Aenimus merged 4 commits intomainfrom
david/eng-8464-propagate-missing-data

Conversation

@Aenimus
Copy link
Copy Markdown
Member

@Aenimus Aenimus commented Nov 12, 2025

…e, default deprecated arg

Summary by CodeRabbit

  • New Features

    • Composition now exposes directive definition mappings in federation results.
    • @deprecated directives are normalized to always include a deprecation reason when missing.
    • Schema building better handles operation root types for more accurate composed schemas.
  • Chores

    • Dependency updates for composition tooling.
  • Tests

    • Expanded directive-related tests and fixtures to cover new directive handling and federation outputs.

Checklist

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 12, 2025

Walkthrough

Adds tracking and propagation of directive definitions via a new directiveDefinitionByName map across federation and normalization flows, ensures @deprecated directives always include a deprecation reason, tweaks schema/operation-type construction and build options, updates a dependency, and adjusts tests to exercise directive-related behavior.

Changes

Cohort / File(s) Change Summary
Dependency update
composition/package.json
Bumped @graphql-tools/utils from ^10.1.0 to ^10.10.2.
Build options & small reorder
composition/src/buildASTSchema/buildASTSchema.ts
Reordered placement of addInvalidExtensionOrphans?: boolean within BuildASTSchemaOptions (declaration moved).
Type surface & federation types
composition/src/federation/types.ts
Added DirectiveName, TypeName, SubgraphName imports; refined map key types and added directiveDefinitionByName: Map<DirectiveName, DirectiveDefinitionNode> to relevant federation result types.
Directive handling in schema utils
composition/src/schema-building/utils.ts
When assembling child directive maps, ensure @deprecated directives include a REASON argument (inject default if missing); iterate directives as [name, nodes] and added related imports.
Federation wiring & result payloads
composition/src/v1/federation/federation-factory.ts
Added directiveDefinitionByName: Map<DirectiveName, DirectiveDefinitionNode> to FederationFactory, populated it during per-directive processing, standardized array typings, and surfaced directiveDefinitionByName in federation result objects (with conditional subgraphConfigBySubgraphName inclusion in some paths).
Normalization & schema-node logic
composition/src/v1/normalization/normalization-factory.ts
Build operationTypes dynamically (include default root types), added #addSchemaDefinitionNode helper to append schema definition/extension as needed, use addInvalidExtensionOrphans: true when building AST schema, and tightened directive array typing.
Tests — directives
composition/tests/v1/directives/directives.test.ts
Switched helpers to normalizeSubgraphSuccess, added/tested directive name/definition exports and fixtures, and added assertions verifying directiveDefinitionByName mappings.
Tests — formatting & minor ordering
composition/tests/v1/directives/fieldset-directives.test.ts, composition/tests/v1/events.test.ts
Converted multiline @requires inline-fragments to single-line strings and reordered fields in a subscription streamConfiguration object (ordering only).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Areas needing extra attention:
    • FederationFactory: correct population and emission of directiveDefinitionByName across multiple return paths and interplay with subgraphConfigBySubgraphName.
    • NormalizationFactory: synthetic OPERATION_TYPE_DEFINITION creation and #addSchemaDefinitionNode behavior.
    • Schema utils: augmentation of @deprecated directives must preserve existing arguments/semantics.
    • Tests: updated/added tests around directive propagation and exported names.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly identifies the main changes: propagating federated directive definitions and subgraph schema nodes, which directly aligns with the substantial modifications across federation types, factory implementations, and normalization logic.
✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 12, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-c9a0bbc3389f667901ee7c7fb2c28562af34d130

Copy link
Copy Markdown
Contributor

@wilsonrivera wilsonrivera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
composition/src/federation/types.ts (1)

42-52: Add directiveDefinitionByName to the contracts success result.

federateSubgraphsWithContracts now spreads a FederationSuccess object (which includes directiveDefinitionByName) into an object literal typed as FederationResultWithContractsSuccess. Because this type doesn’t list directiveDefinitionByName, TypeScript flags the return with “Object literal may only specify known properties”. Please add the field so the contracts success type stays aligned.

 export type FederationResultWithContractsSuccess = {
+  directiveDefinitionByName: Map<DirectiveName, DirectiveDefinitionNode>;
   fieldConfigurations: Array<FieldConfiguration>;
   federatedGraphAST: DocumentNode;
   federatedGraphClientSchema: GraphQLSchema;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37ee2bf and eae1631.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • composition/package.json (1 hunks)
  • composition/src/buildASTSchema/buildASTSchema.ts (1 hunks)
  • composition/src/federation/types.ts (3 hunks)
  • composition/src/schema-building/utils.ts (3 hunks)
  • composition/src/v1/federation/federation-factory.ts (5 hunks)
  • composition/src/v1/normalization/normalization-factory.ts (8 hunks)
  • composition/tests/v1/directives/directives.test.ts (8 hunks)
  • composition/tests/v1/directives/fieldset-directives.test.ts (2 hunks)
  • composition/tests/v1/events.test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2142
File: helm/cosmo/Chart.yaml:0-0
Timestamp: 2025-08-15T10:21:45.838Z
Learning: In the WunderGraph Cosmo project, helm chart version upgrades and README badge synchronization are handled in separate helm release PRs, not in the initial version bump PRs.
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.

Applied to files:

  • composition/src/v1/normalization/normalization-factory.ts
  • composition/tests/v1/directives/fieldset-directives.test.ts
  • composition/tests/v1/directives/directives.test.ts
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Applied to files:

  • composition/tests/v1/directives/directives.test.ts
🧬 Code graph analysis (5)
composition/src/federation/types.ts (1)
composition/src/types/types.ts (1)
  • DirectiveName (3-3)
composition/src/v1/normalization/normalization-factory.ts (4)
composition/src/utils/utils.ts (1)
  • getOrThrowError (26-32)
composition/src/ast/utils.ts (2)
  • operationTypeNodeToDefaultType (164-168)
  • stringToNamedTypeNode (100-105)
composition/src/v1/normalization/walkers.ts (2)
  • upsertDirectiveSchemaAndEntityDefinitions (43-215)
  • upsertParentsAndChildren (217-572)
composition/src/buildASTSchema/buildASTSchema.ts (1)
  • buildASTSchema (26-72)
composition/src/schema-building/utils.ts (2)
composition/src/utils/string-constants.ts (2)
  • DEPRECATED (27-27)
  • REASON (120-120)
composition/src/ast/utils.ts (1)
  • stringToNameNode (77-82)
composition/tests/v1/directives/directives.test.ts (8)
composition/tests/utils/utils.ts (4)
  • normalizeSubgraphSuccess (36-46)
  • schemaToSortedNormalizedString (23-25)
  • normalizeString (19-21)
  • federateSubgraphsSuccess (58-71)
composition/src/router-compatibility-version/router-compatibility-version.ts (1)
  • ROUTER_COMPATIBILITY_VERSION_ONE (3-3)
composition/tests/v1/utils/utils.ts (3)
  • SCHEMA_QUERY_DEFINITION (158-162)
  • INACCESSIBLE_DIRECTIVE (50-53)
  • TAG_DIRECTIVE (115-117)
composition/src/types/types.ts (1)
  • DirectiveName (3-3)
composition/src/utils/string-constants.ts (2)
  • INACCESSIBLE (67-67)
  • TAG (153-153)
composition/src/v1/constants/directive-definitions.ts (2)
  • INACCESSIBLE_DEFINITION (446-462)
  • TAG_DEFINITION (709-735)
composition/src/subgraph/types.ts (1)
  • Subgraph (11-15)
composition/src/ast/utils.ts (1)
  • parse (272-274)
composition/src/v1/federation/federation-factory.ts (3)
composition/src/types/types.ts (2)
  • DirectiveName (3-3)
  • TypeName (11-11)
composition/src/schema-building/ast.ts (1)
  • MutableDefinitionNode (266-273)
composition/src/warnings/types.ts (1)
  • Warning (9-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
  • GitHub Check: build_push_image
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: integration_test (./events)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: Analyze (go)

Comment thread composition/tests/v1/directives/directives.test.ts Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 805c5c8 and 68077fd.

📒 Files selected for processing (1)
  • composition/tests/v1/directives/directives.test.ts (8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-08T20:57:07.946Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: controlplane/src/core/repositories/SubgraphRepository.ts:1746-1763
Timestamp: 2025-09-08T20:57:07.946Z
Learning: The checkSubgraphSchema.ts file already correctly implements linked subgraph functionality, using byName(linkedSubgraph.name, linkedSubgraph.namespace) to fetch target subgraphs and properly handles parse(newSchemaSDL) for schema building. The implementation doesn't need fixes for byId usage or schema parsing as it's already correct.

Applied to files:

  • composition/tests/v1/directives/directives.test.ts
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.

Applied to files:

  • composition/tests/v1/directives/directives.test.ts
🧬 Code graph analysis (1)
composition/tests/v1/directives/directives.test.ts (7)
composition/tests/utils/utils.ts (4)
  • normalizeSubgraphSuccess (36-46)
  • schemaToSortedNormalizedString (23-25)
  • normalizeString (19-21)
  • federateSubgraphsSuccess (58-71)
composition/src/router-compatibility-version/router-compatibility-version.ts (1)
  • ROUTER_COMPATIBILITY_VERSION_ONE (3-3)
composition/src/types/types.ts (1)
  • DirectiveName (3-3)
composition/src/utils/string-constants.ts (2)
  • INACCESSIBLE (67-67)
  • TAG (153-153)
composition/src/v1/constants/directive-definitions.ts (2)
  • INACCESSIBLE_DEFINITION (446-462)
  • TAG_DEFINITION (709-735)
composition/src/subgraph/types.ts (1)
  • Subgraph (11-15)
composition/src/ast/utils.ts (1)
  • parse (272-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: build_test
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: image_scan
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)

Comment thread composition/tests/v1/directives/directives.test.ts
@Aenimus Aenimus merged commit b14468a into main Nov 12, 2025
56 of 57 checks passed
@Aenimus Aenimus deleted the david/eng-8464-propagate-missing-data branch November 12, 2025 13:56
@coderabbitai coderabbitai Bot mentioned this pull request Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants