Skip to content

Conversation

@wilsonrivera
Copy link
Contributor

@wilsonrivera wilsonrivera commented Jun 30, 2025

Motivation and Context

The purpose of this PR is to introduce the isVersionTwo field on the SubgraphConfig type so it can be exposed publicly.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

Summary by CodeRabbit

  • New Features

    • Added a new property to subgraph configuration to indicate version two subgraphs.
  • Tests

    • Added tests to verify correct assignment of the version two flag in subgraph configurations.
    • Refactored tests to use more specific helper functions for success and failure scenarios.

@wilsonrivera wilsonrivera requested a review from Aenimus June 30, 2025 16:56
@github-actions
Copy link

github-actions bot commented Jun 30, 2025

Router image scan passed

✅ No security vulnerabilities found in image:

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jun 30, 2025

Walkthrough

A new boolean property, isVersionTwo, was added to the SubgraphConfig type and is now populated from InternalSubgraph instances in the federation factory logic. Corresponding tests were updated to use helper functions for success and failure cases, and new tests were added to verify the correct assignment of the isVersionTwo flag.

Changes

File(s) Change Summary
composition/src/subgraph/types.ts Added isVersionTwo: boolean to SubgraphConfig type.
composition/src/v1/federation/federation-factory.ts Populated isVersionTwo property in SubgraphConfig within two methods of FederationFactory.
composition/tests/v1/federation-factory.test.ts Refactored tests to use helper functions; added tests for isVersionTwo assignment in subgraph config.

Sequence Diagram(s)

sequenceDiagram
    participant InternalSubgraph
    participant FederationFactory
    participant SubgraphConfig

    InternalSubgraph->>FederationFactory: Provide subgraph data (including isVersionTwo)
    FederationFactory->>SubgraphConfig: Construct SubgraphConfig with isVersionTwo
    FederationFactory->>Tests: Expose SubgraphConfig for verification
Loading

Poem

A hop and a skip, a flag to pursue,
Now subgraphs declare if they're version two!
The tests have been tidied, helpers in tow,
Ensuring our configs are ready to go.
With each little change, the code grows anew—
🐇 Cheers to the team for this versioning coup!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cdfbc9a and 471426d.

📒 Files selected for processing (3)
  • composition/src/subgraph/types.ts (1 hunks)
  • composition/src/v1/federation/federation-factory.ts (2 hunks)
  • composition/tests/v1/federation-factory.test.ts (17 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: build_push_image
  • GitHub Check: image_scan
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: build_test
  • GitHub Check: Analyze (go)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
composition/src/subgraph/types.ts (1)

16-21: New required field may introduce downstream compilation errors

isVersionTwo is added as a required property on SubgraphConfig.
Every call-site that instantiates a SubgraphConfig literal must now supply this flag or fail compilation. You updated the two factory locations, but external code (tests, utilities, plugins) may construct the type directly and now break.

Consider:

-export type SubgraphConfig = {
-  configurationDataByTypeName: Map<string, ConfigurationData>;
-  isVersionTwo: boolean;
-  parentDefinitionDataByTypeName: Map<string, ParentDefinitionData>;
-  schema: GraphQLSchema;
-};
+export type SubgraphConfig = {
+  configurationDataByTypeName: Map<string, ConfigurationData>;
+  /** `true` when the source subgraph is declared as v2. */
+  isVersionTwo: boolean;
+  parentDefinitionDataByTypeName: Map<string, ParentDefinitionData>;
+  schema: GraphQLSchema;
+};
  1. Document the semantics with a JSDoc comment (as above) so public consumers understand it.
  2. If you cannot guarantee all external instantiations are covered, downgrade to isVersionTwo?: boolean and default to false inside the factory.
composition/src/v1/federation/federation-factory.ts (2)

2682-2689: LGTM – factory now propagates the new flag

buildFederationResult() correctly injects isVersionTwo into the produced SubgraphConfig. This keeps the factory conformant with the updated type definition.


2969-2976: LGTM – contract result path updated as well

buildFederationContractResult() mirrors the change ensuring contract-specific flows return the flag too. Good consistency.

composition/tests/v1/federation-factory.test.ts (5)

26-26: LGTM: Clean import addition.

The addition of federateSubgraphsFailure import supports the test refactoring and follows the existing import pattern.


38-38: Excellent refactoring to use helper functions.

The migration from direct federateSubgraphs calls to federateSubgraphsFailure makes the test intent much clearer and eliminates the need for explicit type assertions. This addresses the previous review suggestion perfectly.

Also applies to: 44-44, 58-61


574-580: Well-structured test for version one subgraph validation.

The test correctly verifies that subgraphE (which uses basic GraphQL without federation v2 features) is properly identified as not being a version two subgraph.


582-588: Comprehensive test for version two subgraph validation.

The test appropriately uses subgraphJ (which contains federation v2 directives like @inaccessible) to verify that version two subgraphs are correctly flagged with isVersionTwo: true.


436-436: Consistent application of helper function pattern.

All remaining test calls have been successfully migrated to use the appropriate helper functions (federateSubgraphsSuccess or federateSubgraphsFailure). This creates a uniform, maintainable test structure throughout the file.

Also applies to: 520-520, 555-555, 561-561, 591-591, 605-605, 623-623, 709-709, 795-795, 812-812, 848-848, 868-868, 874-874, 881-881, 887-887, 893-893, 900-900, 906-906, 932-932, 958-958, 985-985

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Member

@Aenimus Aenimus left a comment

Choose a reason for hiding this comment

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

LGTM!

@Aenimus Aenimus enabled auto-merge (squash) June 30, 2025 17:48
@Aenimus Aenimus merged commit 09baaa8 into main Jun 30, 2025
32 of 33 checks passed
@Aenimus Aenimus deleted the wilson/eng-7441-composition-add-isversiontwo-to-subgraphconfig branch June 30, 2025 17:52
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.

3 participants