feat: add disable resolvability validation flag#2065
Conversation
Aenimus
requested review from
JivusAyrus,
Noroth,
StarpTech,
devsergiy,
endigma,
jensneuse and
wilsonrivera
as code owners
WalkthroughThis change introduces a new option to disable resolvability validation across the entire stack for federated GraphQL graph operations. The option is added to CLI commands, protobuf schemas, backend service handlers, composition logic, and test utilities. Type and function signatures are updated throughout to propagate and support the new Changes
✨ Finishing Touches
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 (
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (14)
proto/wg/cosmo/platform/v1/platform.proto (14)
95-95: Same observation as above – see main comment.
136-136: Same observation as above – see main comment.
177-177: Same observation as above – see main comment.
636-636: Same observation as above – see main comment.
655-655: Same observation as above – see main comment.
686-686: Same observation as above – see main comment.
2072-2072: Same observation as above – see main comment.
2200-2200: Same observation as above – see main comment.
2219-2219: Same observation as above – see main comment.
2249-2249: Same observation as above – see main comment.
2265-2265: Same observation as above – see main comment.
2279-2279: Same observation as above – see main comment.
2293-2293: Same observation as above – see main comment.
2599-2599: Same observation as above – see main comment.
🧹 Nitpick comments (3)
cli/src/commands/feature-flag/commands/enable.ts (1)
24-24: Consider the property ordering change.The
enabled: trueproperty was moved beforenameandnamespace. While this doesn't affect functionality, ensure this reordering was intentional and aligns with any API documentation or conventions.composition/tests/v1/types/unions.test.ts (1)
472-475: LGTM! Test function replacements are consistent.All test cases have been correctly updated to use the new helper functions. However, some calls retain explicit type assertions (e.g.,
as FederationResultSuccess) which are now redundant since the helper functions return correctly typed results.Consider removing these redundant type assertions for consistency:
- ) as FederationResultSuccess; + );Also applies to: 500-503, 528-531, 538-541, 582-585, 626-629, 638-641, 650-653, 733-736, 816-819, 853-856, 890-893, 920-923, 965-968, 1010-1013
proto/wg/cosmo/platform/v1/platform.proto (1)
58-58: Document & harmonisedisable_resolvability_validationflag across messagesGood to see the flag added, but it lacks any explanatory comment and the name diverges from the existing
skip_traffic_checkboolean.
Consider:
- Adding a short doc-comment before every occurrence to describe when it should be set and its default behaviour (validation ON when unset).
- Evaluating a more consistent verb such as
skip_resolvability_validation(mirrorsskip_traffic_check) to keep the API surface uniform.- If this option will be reused elsewhere, encapsulate it in a small
CompositionOptionsmessage and embed that instead of duplicating a scalar across 15+ request types (DRY).None of these is a blocker for wire-compatibility, but tightening them up now will save churn later.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
shared/test/router.config.test.ts (1)
33-33: UpdatefederateTestSubgraphsto use the new object‐based APIThe test helper still calls the old signature
federateSubgraphs([…,], version)but the runtime API now expects a single object. Update the implementation in
shared/test/testdata/utils.tsso that it matches the new pattern.Files to change:
- shared/test/testdata/utils.ts
Suggested diff:
--- a/shared/test/testdata/utils.ts +++ b/shared/test/testdata/utils.ts @@ -export function federateTestSubgraphs(): FederationResult { +export function federateTestSubgraphs(): FederationResult { const accounts: Subgraph = { … }; const inventory: Subgraph = { … }; // … - return federateSubgraphs([accounts, inventory, products, reviews], LATEST_ROUTER_COMPATIBILITY_VERSION); + return federateSubgraphs({ + subgraphs: [accounts, inventory, products, reviews], + version: LATEST_ROUTER_COMPATIBILITY_VERSION, + }); }This change will align your test helper with the updated
federateSubgraphs({ subgraphs, version })signature and resolve the “Cannot read properties of undefined (reading ‘length’)” errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (19)
cli/src/commands/graph/federated-graph/commands/check.ts(1 hunks)cli/src/commands/router/commands/compose.ts(3 hunks)cli/src/commands/subgraph/commands/fix.ts(2 hunks)cli/src/commands/subgraph/commands/update.ts(3 hunks)composition/src/federation/federation.ts(1 hunks)composition/src/v1/federation/federation-factory.ts(10 hunks)composition/tests/v1/directives/inaccessible.test.ts(13 hunks)composition/tests/v1/entities.test.ts(23 hunks)composition/tests/v1/types/enums.test.ts(10 hunks)composition/tests/v1/types/inputs.test.ts(7 hunks)composition/tests/v1/types/unions.test.ts(13 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(32 hunks)controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/subgraph/fixSubgraphSchema.ts(4 hunks)controlplane/src/core/repositories/FederatedGraphRepository.ts(9 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(6 hunks)proto/wg/cosmo/platform/v1/platform.proto(17 hunks)shared/src/router-config/builder.ts(3 hunks)shared/test/router.config.test.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- shared/src/router-config/builder.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- controlplane/src/core/bufservices/subgraph/fixSubgraphSchema.ts
- composition/tests/v1/directives/inaccessible.test.ts
- cli/src/commands/subgraph/commands/update.ts
- cli/src/commands/router/commands/compose.ts
- cli/src/commands/graph/federated-graph/commands/check.ts
- composition/tests/v1/types/inputs.test.ts
- composition/tests/v1/types/enums.test.ts
- composition/tests/v1/types/unions.test.ts
- composition/tests/v1/entities.test.ts
- controlplane/src/core/repositories/SubgraphRepository.ts
- composition/src/federation/federation.ts
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
- proto/wg/cosmo/platform/v1/platform.proto
- controlplane/src/core/repositories/FederatedGraphRepository.ts
- composition/src/v1/federation/federation-factory.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts (1)
controlplane/src/utils/utils.ts (1)
newCompositionOptions(3-10)
🪛 GitHub Actions: Shared CI
shared/test/router.config.test.ts
[error] 33-33: Test failure: TypeError: Cannot read properties of undefined (reading 'length') in 'Build Subgraph schema' test.
[error] 114-114: Test failure: TypeError: Cannot read properties of undefined (reading 'length') in 'Build config with plugin subgraph' test.
⏰ 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-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
🔇 Additional comments (6)
shared/test/router.config.test.ts (3)
202-205: LGTM: Correct API update for object-based parameters.The function call has been properly updated to use the new object-based parameter structure, which supports the new
disableResolvabilityValidationflag and improves API consistency.
256-259: LGTM: Consistent API update.This change correctly follows the same object-based parameter pattern as the other
federateSubgraphscalls in the file.
310-313: LGTM: Final API update completed.This change completes the consistent update of
federateSubgraphscalls to use the new object-based parameter structure.controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts (2)
19-19: LGTM: Import addition is correct.The import of
newCompositionOptionsutility function is properly added and will be used to handle the new disable resolvability validation feature.
103-103: LGTM: Correct integration of disable resolvability validation flag.The addition of the third parameter using
newCompositionOptions(req.disableResolvabilityValidation)properly integrates the new feature flag into the composition process. The utility function correctly handles the conversion from the boolean flag to composition options.cli/src/commands/subgraph/commands/fix.ts (1)
4-4: LGTM: Good cleanup of unused import.Removing the unused
joinimport while keepingresolveimproves code maintainability.
controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
Outdated
Show resolved
Hide resolved
JivusAyrus
left a comment
JivusAyrus
left a comment
There was a problem hiding this comment.
Should we write some tests?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/test/integrations.test.ts (1)
198-267: Well-structured integration test for resolvability validation feature.The test comprehensively verifies the new
disableResolvabilityValidationflag functionality by testing both enabled and disabled scenarios. The test logic is sound:
- Creates the necessary setup (namespace, federated graph, first subgraph)
- Verifies schema checking fails with validation enabled but succeeds when disabled
- Confirms publishing behavior matches the validation results
- Includes proper cleanup
The assertions correctly verify both status codes and composition error counts for each scenario.
Consider extracting the subgraph name generation and reusing it for better maintainability:
+ const subgraphNameOne = genID('one'); const publishResponseOne = await client.publishFederatedSubgraph({ - name: genID('one'), + name: subgraphNameOne, namespace, routingUrl: 'http://localhost:4001', schema: resolvabilitySDLOne, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
composition/src/v1/federation/federation-factory.ts(10 hunks)composition/tests/v1/resolvability.test.ts(38 hunks)controlplane/src/core/bufservices/contract/createContract.ts(2 hunks)controlplane/src/core/bufservices/contract/updateContract.ts(2 hunks)controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts(2 hunks)controlplane/src/core/bufservices/feature-flag/deleteFeatureFlag.ts(2 hunks)controlplane/src/core/bufservices/feature-flag/enableFeatureFlag.ts(2 hunks)controlplane/src/core/bufservices/feature-flag/updateFeatureFlag.ts(2 hunks)controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/federated-graph/createFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/federated-graph/updateFederatedGraph.ts(2 hunks)controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts(2 hunks)controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts(2 hunks)controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts(2 hunks)controlplane/src/core/bufservices/subgraph/fixSubgraphSchema.ts(4 hunks)controlplane/src/core/bufservices/subgraph/moveSubgraph.ts(2 hunks)controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts(2 hunks)controlplane/src/core/bufservices/subgraph/updateSubgraph.ts(2 hunks)controlplane/src/core/util.ts(3 hunks)controlplane/test/integrations.test.ts(2 hunks)controlplane/test/test-util.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- controlplane/test/test-util.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts
- controlplane/src/core/bufservices/feature-flag/deleteFeatureFlag.ts
- controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts
- controlplane/src/core/bufservices/subgraph/checkSubgraphSchema.ts
- controlplane/src/core/bufservices/subgraph/updateSubgraph.ts
- controlplane/src/core/bufservices/subgraph/moveSubgraph.ts
- controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts
- controlplane/src/core/bufservices/federated-graph/createFederatedGraph.ts
- controlplane/src/core/bufservices/feature-flag/enableFeatureFlag.ts
- controlplane/src/core/bufservices/contract/createContract.ts
- controlplane/src/core/bufservices/subgraph/fixSubgraphSchema.ts
- controlplane/src/core/bufservices/federated-graph/updateFederatedGraph.ts
- controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts
- controlplane/src/core/bufservices/contract/updateContract.ts
- controlplane/src/core/bufservices/feature-flag/updateFeatureFlag.ts
- controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts
- composition/tests/v1/resolvability.test.ts
- controlplane/src/core/util.ts
- composition/src/v1/federation/federation-factory.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
controlplane/test/integrations.test.ts (3)
controlplane/test/test-util.ts (4)
SetupTest(66-417)createNamespace(810-815)resolvabilitySDLOne(962-970)resolvabilitySDLTwo(972-976)controlplane/src/core/test-util.ts (1)
genID(53-55)connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts (1)
createNamespace(74-83)
⏰ 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-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
🔇 Additional comments (1)
controlplane/test/integrations.test.ts (1)
9-9: LGTM!The new imports for
resolvabilitySDLOneandresolvabilitySDLTwoare correctly added to support the resolvability validation test.
Aenimus
deleted the
david/eng-7635-add-disable-resolvability-validation-flag
branch
4 participants
Summary by CodeRabbit
New Features
--disable-resolvability-validationto various commands, allowing users to optionally disable resolvability validation for federated graphs, subgraphs, contracts, and feature flags. This is intended for troubleshooting scenarios only.Bug Fixes
Documentation
Style
Tests
Checklist