feat(controlplane): add CreateFederatedSubgraphs batch rpc - #3152
Conversation
Creates several subgraphs in a single request. Every entry is validated before anything is written and the writes share one transaction, so the request either creates all of the subgraphs or none of them. Creating a subgraph triggers no composition, so this is purely a batched write; schemas are published afterwards with PublishFederatedSubgraphs. The routing and subscription validation that create, publish and update each carried their own near-identical copy of now lives in a single helper. The wording of a few error messages is normalised as a result.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe platform API now supports atomic creation of multiple federated subgraphs. The control plane adds authorization, validation, transaction handling, audit logging, and structured responses. Shared routing validation is used by create, publish, and update operations. ChangesBatch federated subgraph creation
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
controlplane/test/subgraph/batch-create-subgraphs.test.ts (2)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for this variable.
Rename
dbnametodbNameand update its references. As per coding guidelines,**/*.{js,jsx,ts,tsx}requires camelCase for variable and function names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/test/subgraph/batch-create-subgraphs.test.ts` at line 14, Rename the variable dbname to dbName in the batch-create-subgraphs test and update every reference to use the camelCase name.Source: Coding guidelines
16-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAdd explicit callback types.
Import
type TestContextfromvitest. Annotate eachtestContextparameter and add explicit return types to thedescribe,beforeAll,afterAll, andtestcallbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/test/subgraph/batch-create-subgraphs.test.ts` around lines 16 - 17, Add the Vitest type-only TestContext import, annotate every testContext parameter with TestContext, and provide explicit return types for the describe, beforeAll, afterAll, and test callbacks in the Batch create subgraphs tests suite.Source: Coding guidelines
controlplane/src/core/util.ts (1)
947-956: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the input shape into an interface.
The coding guidelines prefer interfaces over type aliases for object shapes. The inline object literal for
inputis also reused conceptually by four call sites. An exported interface makes the contract easier to reference and document.♻️ Proposed refactor
+export interface SubgraphRoutingValidationInput { + isEventDrivenGraph: boolean; + routingUrl?: string; + subscriptionUrl?: string; + subscriptionProtocol?: GraphQLSubscriptionProtocol; + websocketSubprotocol?: GraphQLWebsocketSubprotocol; + routingUrlRequirement: RoutingUrlRequirement; + isGrpcService?: boolean; + isFeatureSubgraph?: boolean; +} + -export function validateSubgraphRouting(input: { - isEventDrivenGraph: boolean; - routingUrl?: string; - subscriptionUrl?: string; - subscriptionProtocol?: GraphQLSubscriptionProtocol; - websocketSubprotocol?: GraphQLWebsocketSubprotocol; - routingUrlRequirement: RoutingUrlRequirement; - isGrpcService?: boolean; - isFeatureSubgraph?: boolean; -}): string | undefined { +export function validateSubgraphRouting(input: SubgraphRoutingValidationInput): string | undefined {As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/util.ts` around lines 947 - 956, Extract the inline input object shape from validateSubgraphRouting into an exported interface, then use that interface as the function’s input parameter type. Preserve all existing properties, optionality, and types so the four call sites remain compatible.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controlplane/src/core/bufservices/subgraph/createFederatedSubgraphs.ts`:
- Around line 194-224: In the request validation near the existing empty
`req.subgraphs` check, reject batches exceeding a fixed maximum before entering
the `opts.db.transaction` callback. Reuse an appropriate existing
validation/error pattern and ensure oversized requests never reach the
sequential creation loop.
---
Nitpick comments:
In `@controlplane/src/core/util.ts`:
- Around line 947-956: Extract the inline input object shape from
validateSubgraphRouting into an exported interface, then use that interface as
the function’s input parameter type. Preserve all existing properties,
optionality, and types so the four call sites remain compatible.
In `@controlplane/test/subgraph/batch-create-subgraphs.test.ts`:
- Line 14: Rename the variable dbname to dbName in the batch-create-subgraphs
test and update every reference to use the camelCase name.
- Around line 16-17: Add the Vitest type-only TestContext import, annotate every
testContext parameter with TestContext, and provide explicit return types for
the describe, beforeAll, afterAll, and test callbacks in the Batch create
subgraphs tests suite.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a5aec3d-632e-45bc-bb22-2c1a2c17ec03
📒 Files selected for processing (14)
connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/src/core/bufservices/PlatformService.tscontrolplane/src/core/bufservices/subgraph/createFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/createFederatedSubgraphs.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/updateSubgraph.tscontrolplane/src/core/util.tscontrolplane/test/feature-subgraph/create-feature-subgraph.test.tscontrolplane/test/feature-subgraph/publish-feature-subgraph.test.tscontrolplane/test/subgraph/batch-create-subgraphs.test.tscontrolplane/test/subgraph/publish-subgraph.test.tscontrolplane/test/subgraph/update-subgraph.test.tsproto/wg/cosmo/platform/v1/platform.proto
Subgraph creation takes an array and issues one statement per table rather than one set per subgraph. Subgraphs sharing a label set within a namespace resolve their federated graphs once, and the audit logs for a batch are written in a single insert. Inserted rows are paired with their input by name, so the result never depends on the order postgres returns them in.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/src/core/repositories/SubgraphRepository.ts (1)
205-229: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKey the pairing map by namespace and name.
targetIdByNameuses onlyentry.data.nameas the key. Each entry carries its ownnamespaceId, and target names are unique per namespace, not per organization. If two entries share a name in different namespaces, the second entry overwrites the first. Both subgraph rows then receive the sametargetId, and the returned DTOs are wrong for one entry.All current callers pass a single namespace, so this is latent. A composite key removes the risk without changing behavior for single-namespace batches.
♻️ Proposed fix: composite pairing key
- // Paired by name rather than by position, so the result never depends on - // the order postgres returns the inserted rows in. - const targetIdByName = new Map(insertedTargets.map((target) => [target.name, target.id])); + // Paired by (namespaceId, name) rather than by position, so the result never depends on + // the order postgres returns the inserted rows in, and names are unique per namespace. + const targetKeyOf = (namespaceId: string, name: string): string => `${namespaceId}:${name}`; + const targetIdByKey = new Map( + insertedTargets.map((target) => [targetKeyOf(target.namespaceId, target.name), target.id]), + );Then replace every
targetIdByName.get(entry.data.name)!withtargetIdByKey.get(targetKeyOf(entry.data.namespaceId, entry.data.name))!.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/repositories/SubgraphRepository.ts` around lines 205 - 229, Update the pairing logic around targetIdByName to use a composite key of namespaceId and name, preventing entries with the same name in different namespaces from overwriting one another. Introduce or reuse a consistent targetKeyOf helper for both building the map from insertedTargets and looking up each entry, replacing targetIdByName.get(entry.data.name) while preserving single-namespace behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 205-229: Update the pairing logic around targetIdByName to use a
composite key of namespaceId and name, preventing entries with the same name in
different namespaces from overwriting one another. Introduce or reuse a
consistent targetKeyOf helper for both building the map from insertedTargets and
looking up each entry, replacing targetIdByName.get(entry.data.name) while
preserving single-namespace behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08105c87-29c8-4408-b8cd-6951e26e16d5
📒 Files selected for processing (7)
controlplane/src/core/bufservices/monograph/createMonograph.tscontrolplane/src/core/bufservices/plugin/validateAndFetchPluginData.tscontrolplane/src/core/bufservices/subgraph/createFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/createFederatedSubgraphs.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/services/ApolloMigrator.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts
- controlplane/src/core/bufservices/subgraph/createFederatedSubgraph.ts
- controlplane/src/core/bufservices/subgraph/createFederatedSubgraphs.ts
# Conflicts: # connect/src/wg/cosmo/platform/v1/platform_pb.ts
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3152 +/- ##
==========================================
+ Coverage 1.87% 43.96% +42.09%
==========================================
Files 330 1076 +746
Lines 46020 140553 +94533
Branches 522 7441 +6919
==========================================
+ Hits 861 61792 +60931
- Misses 44844 76910 +32066
- Partials 315 1851 +1536
🚀 New features to boost your workflow:
|
…ey hold Drops the lookup maps in favour of pairing inserted rows with their input by index, and migrates apollo graphs with a single create call rather than one per subgraph. The base subgraph mapping is now written only when a subgraph is actually a feature subgraph, matching the sibling checks, rather than whenever feature subgraph options are present.
Summary by CodeRabbit
New Features
Bug Fixes
Adds
CreateFederatedSubgraphs, which creates several subgraphs in a single request.Every entry is validated before anything is written, and the writes share one transaction, so the request either creates all of the subgraphs or none of them. Duplicate names within the request, entries that already exist in the namespace, and invalid labels, names or routing URLs are all rejected up front, leaving nothing behind.
Creating a subgraph does not trigger a composition — a subgraph carries no schema until it is published — so this is purely a batched write. Schemas are published afterwards with
PublishFederatedSubgraphs, which composes each affected graph once.The routing and subscription validation that create, publish and update each carried their own near-identical copy of now lives in a single helper, parameterised by whether the routing URL is required (create, publish), optional (update) or absent (plugins). A few error messages are normalised as a result:
Event-Driven GraphsbecomesAn Event-Driven Graph, some trailing periods are dropped, and publish's empty-routing-URL message now matches create's.Reviewer notes
controlplane/test/subgraph/batch-create-subgraphs.test.tscovers the happy path, label matching (including unlabelled subgraphs against a graph with no label matchers), Event-Driven graphs, and each rejection case, asserting that a rejected batch creates nothing.Checklist