chore: include dependencies on version post endpoint#1087
chore: include dependencies on version post endpoint#1087adityachoudhari26 merged 2 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR extends deployment version creation to accept optional dependency specifications. Each dependency maps a deployment ID to a CEL expression selector. The handler validates dependencies (syntax, self-reference, workspace membership) and persists them atomically with the version via new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 35 minutes and 49 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR extends the deployment-version create (POST) API to support declaring dependency edges inline at version creation time, updates the OpenAPI schema accordingly, and adds E2E coverage for the dependency CRUD endpoints and the new inline-create behavior.
Changes:
- Add
dependenciesto the deployment version POST request body and insert dependency edges in the same DB transaction. - Update OpenAPI specs/types to include the new request field and dependency endpoints.
- Add Playwright E2E tests covering inline dependency creation and dependency list/upsert/delete endpoints.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/tests/api/deployment-version-dependencies.spec.ts | Adds E2E coverage for dependency endpoints and inline dependency creation on version POST. |
| e2e/package.json | Adds a dedicated script to run the new dependency E2E spec. |
| e2e/api/schema.ts | Regenerates/updates the E2E OpenAPI client schema with dependency routes/types and related additions. |
| apps/api/src/types/openapi.ts | Updates API OpenAPI-derived TS types to include dependencies on version creation. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Implements inline dependency validation + atomic insert during deployment version creation. |
| apps/api/openapi/schemas/deploymentversions.jsonnet | Adds dependencies to the deployment version create schema. |
| apps/api/openapi/paths/deploymentversions.jsonnet | Updates version-create responses to 200 + adds 400/404 responses. |
| apps/api/openapi/openapi.json | Updates generated OpenAPI JSON to reflect the schema changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const dependencyEntries = Object.entries(dependencies ?? {}); | ||
| if (dependencyEntries.length > 0) { | ||
| for (const [depDeploymentId, edge] of dependencyEntries) { | ||
| if (depDeploymentId === deploymentId) | ||
| throw new ApiError( | ||
| "A deployment version cannot depend on its own deployment", | ||
| 400, | ||
| ); | ||
| if (!validResourceSelector(edge.versionSelector)) | ||
| throw new ApiError( | ||
| `Invalid versionSelector CEL expression for dependency ${depDeploymentId}`, | ||
| 400, | ||
| ); |
There was a problem hiding this comment.
dependencies entries can omit versionSelector and still pass validation because validResourceSelector(undefined) returns true. That would allow creating dependency edges with an unintended default/undefined selector (or trigger a DB error depending on how undefined is serialized). Add an explicit runtime check that edge.versionSelector is a non-empty string before calling validResourceSelector (and return 400 when missing).
| const dependencyDeploymentIds = dependencyEntries.map(([id]) => id); | ||
| const found = await db | ||
| .select({ id: schema.deployment.id }) | ||
| .from(schema.deployment) | ||
| .where( | ||
| and( | ||
| eq(schema.deployment.workspaceId, workspaceId), | ||
| inArray(schema.deployment.id, dependencyDeploymentIds), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
dependencyEntries keys are treated as deployment UUIDs and passed directly into inArray(schema.deployment.id, dependencyDeploymentIds). If a client supplies a non-UUID key (or dependencies isn’t an object), Postgres will throw invalid input syntax for type uuid, resulting in a 500. Validate each depDeploymentId is a UUID (and that dependencies is a plain object) and return 400 for invalid IDs before querying.
| const { workspaceId, deploymentId } = req.params; | ||
| const { body } = req; | ||
| const { dependencies, ...versionFields } = body; | ||
|
|
There was a problem hiding this comment.
This handler does not verify that deploymentId belongs to workspaceId (or even exists) before inserting a deployment version and enqueuing work. That allows cross-workspace writes if a caller can guess a deployment ID from another workspace. Add a lookup on schema.deployment scoped by (workspaceId, deploymentId) and return 404 when not found before proceeding.
| const deployment = await db | |
| .select({ id: schema.deployment.id }) | |
| .from(schema.deployment) | |
| .where( | |
| and( | |
| eq(schema.deployment.workspaceId, workspaceId), | |
| eq(schema.deployment.id, deploymentId), | |
| ), | |
| ) | |
| .then(takeFirst); | |
| if (!deployment) | |
| throw new ApiError(`Deployment ${deploymentId} not found`, 404); |
| const inserted = await tx | ||
| .insert(schema.deploymentVersion) | ||
| .values(data) | ||
| .onConflictDoNothing() | ||
| .returning() | ||
| .then(takeFirst); | ||
|
|
There was a problem hiding this comment.
onConflictDoNothing().returning().then(takeFirst) will throw when a (deploymentId, tag) conflict occurs because returning() returns 0 rows on DO NOTHING; takeFirst then raises an exception, producing a 500. Decide on intended behavior and handle it explicitly (e.g., return 409 conflict, or fetch and return the existing version).
| const inserted = await tx | |
| .insert(schema.deploymentVersion) | |
| .values(data) | |
| .onConflictDoNothing() | |
| .returning() | |
| .then(takeFirst); | |
| const insertedRows = await tx | |
| .insert(schema.deploymentVersion) | |
| .values(data) | |
| .onConflictDoNothing() | |
| .returning(); | |
| const inserted = insertedRows[0]; | |
| if (inserted == null) | |
| throw new ApiError( | |
| `Deployment version with tag ${data.tag} already exists for deployment ${deploymentId}`, | |
| 409, | |
| ); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/openapi/paths/deploymentversions.jsonnet`:
- Around line 35-40: The OpenAPI spec change switched the create deployment
version response to 200 (openapi.okResponse for DeploymentVersion) but the
generated client types still model createDeploymentVersion as 202; regenerate
the OpenAPI artifacts so generated types (the createDeploymentVersion return
type in the generated openapi.ts) reflect the new 200 response. Run the
project's OpenAPI generation script/command used to produce
apps/api/src/types/openapi.ts (the generator that converts the
deploymentversions.jsonnet spec into types), commit the regenerated artifacts,
and verify createDeploymentVersion now expects a 200 DeploymentVersion response
instead of 202.
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 403-446: The current dependency existence check runs before the
db.transaction and can race with deletes; move the lookup into the same
transaction (use tx.select(...) instead of db.select(...)) and validate
dependencyDeploymentIds inside the transaction before inserting edges, i.e.
perform the found = await tx.select({ id: schema.deployment.id
}).from(schema.deployment).where(... inArray(dependencyDeploymentIds) ...) and
build foundIds to throw ApiError for missing ids prior to the tx.insert of
schema.deploymentVersionDependency, or alternatively catch the FK/db error from
the tx insert and translate it to a 404; locate usage of dependencyEntries,
dependencyDeploymentIds, found, tx, inserted, and
tx.insert(schema.deploymentVersionDependency) to apply the change.
In `@e2e/api/schema.ts`:
- Around line 141-181: The OpenAPI-generated types in e2e/api/schema.ts are
incorrect: the path entries for
"/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies",
"/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies/{dependencyDeploymentId}",
and "/v1/workspaces/{workspaceId}/deployments/name/{name}" declare
parameters.path as never while the OpenAPI spec and the corresponding operations
(e.g., operations["listDeploymentVersionDependencies"],
operations["requestDeploymentVersionDependencyUpsert"],
operations["requestDeploymentVersionDependencyDeletion"]) require path params
(workspaceId, deploymentVersionId, dependencyDeploymentId, name); fix by
regenerating the OpenAPI TypeScript types from the canonical OpenAPI source so
those path parameter definitions are emitted correctly (ensuring the paths
entries include the required path parameter shapes) rather than hand-editing the
file.
In `@e2e/tests/api/deployment-version-dependencies.spec.ts`:
- Around line 12-72: Replace the manual provisioning/teardown in test.beforeAll
and test.afterAll with YAML fixture helpers: remove the API POST/DELETE blocks
that create systemId, downstreamId, upstreamId, secondUpstreamId and instead
call importEntitiesFromYaml at the start of the suite to load a .spec.yaml
fixture that declares those entities (names/slugs), and call
cleanupImportedEntities in test.afterAll to tear them down; update references to
systemId/downstreamId/upstreamId/secondUpstreamId to use the IDs returned by
importEntitiesFromYaml (or the fixture loader’s result) so
test.beforeAll/test.afterAll no longer perform direct API calls.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90e12a5d-c799-4786-a68a-4f8eda3c1ec6
📒 Files selected for processing (8)
apps/api/openapi/openapi.jsonapps/api/openapi/paths/deploymentversions.jsonnetapps/api/openapi/schemas/deploymentversions.jsonnetapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.tse2e/api/schema.tse2e/package.jsone2e/tests/api/deployment-version-dependencies.spec.ts
| "/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies": { | ||
| parameters: { | ||
| query?: never; | ||
| header?: never; | ||
| path?: never; | ||
| cookie?: never; | ||
| }; | ||
| /** | ||
| * List deployment-version dependencies | ||
| * @description Returns the dependency edges declared by this deployment version. | ||
| */ | ||
| get: operations["listDeploymentVersionDependencies"]; | ||
| put?: never; | ||
| post?: never; | ||
| delete?: never; | ||
| options?: never; | ||
| head?: never; | ||
| patch?: never; | ||
| trace?: never; | ||
| }; | ||
| "/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies/{dependencyDeploymentId}": { | ||
| parameters: { | ||
| query?: never; | ||
| header?: never; | ||
| path?: never; | ||
| cookie?: never; | ||
| }; | ||
| get?: never; | ||
| /** | ||
| * Upsert deployment-version dependency | ||
| * @description Declare or update a version-selector dependency from this deployment version to another deployment. Identified by the (deploymentVersionId, dependencyDeploymentId) pair. | ||
| */ | ||
| put: operations["requestDeploymentVersionDependencyUpsert"]; | ||
| post?: never; | ||
| /** Delete deployment-version dependency */ | ||
| delete: operations["requestDeploymentVersionDependencyDeletion"]; | ||
| options?: never; | ||
| head?: never; | ||
| patch?: never; | ||
| trace?: never; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'deployment-versions/\{deploymentVersionId\}/dependencies|deployment-versions/\{deploymentVersionId\}/dependencies/\{dependencyDeploymentId\}|deployments/name/\{name\}' apps/api/openapi e2e/api/schema.tsRepository: ctrlplanedev/ctrlplane
Length of output: 6094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the parameters defined for the first endpoint in openapi.json
echo "=== Parameters for /v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies ==="
rg -A 30 '"/v1/workspaces/\{workspaceId\}/deployment-versions/\{deploymentVersionId\}/dependencies"' apps/api/openapi/openapi.json | head -50
echo ""
echo "=== Check operations section for listDeploymentVersionDependencies ==="
rg -A 5 '"listDeploymentVersionDependencies"' e2e/api/schema.ts
echo ""
echo "=== Check operations section for requestDeploymentVersionDependencyUpsert ==="
rg -A 5 '"requestDeploymentVersionDependencyUpsert"' e2e/api/schema.tsRepository: ctrlplanedev/ctrlplane
Length of output: 1960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the operation definitions in the schema.ts to see if they have proper path parameters
echo "=== Full operation definition for listDeploymentVersionDependencies ==="
rg -B 2 -A 20 'listDeploymentVersionDependencies:' e2e/api/schema.ts
echo ""
echo "=== Check parameters for second endpoint in openapi.json ==="
rg -A 35 '"/v1/workspaces/\{workspaceId\}/deployment-versions/\{deploymentVersionId\}/dependencies/\{dependencyDeploymentId\}"' apps/api/openapi/openapi.json | head -55Repository: ctrlplanedev/ctrlplane
Length of output: 2225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the third endpoint parameters in openapi.json
echo "=== Parameters for /v1/workspaces/{workspaceId}/deployments/name/{name} ==="
rg -A 30 '"/v1/workspaces/\{workspaceId\}/deployments/name/\{name\}"' apps/api/openapi/openapi.json | head -50
echo ""
echo "=== Check if paths entries have any path param definitions ==="
rg -B 5 -A 10 '"/v1/workspaces/\{workspaceId\}/deployment-versions/\{deploymentVersionId\}/dependencies":' e2e/api/schema.ts | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 1751
Regenerate the OpenAPI types; path entries are missing path parameter definitions.
The paths entries for the new dependency and deployment endpoints declare parameters.path?: never, but the OpenAPI source and the corresponding operations definitions both include required path parameters (workspaceId, deploymentVersionId, dependencyDeploymentId, and/or name). This breaks type safety for callers relying on the paths interface. Regenerate this file from the OpenAPI source rather than manually patching.
Affected lines:
- Line 141:
/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies - Line 161:
/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies/{dependencyDeploymentId} - Line 217:
/v1/workspaces/{workspaceId}/deployments/name/{name}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/api/schema.ts` around lines 141 - 181, The OpenAPI-generated types in
e2e/api/schema.ts are incorrect: the path entries for
"/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies",
"/v1/workspaces/{workspaceId}/deployment-versions/{deploymentVersionId}/dependencies/{dependencyDeploymentId}",
and "/v1/workspaces/{workspaceId}/deployments/name/{name}" declare
parameters.path as never while the OpenAPI spec and the corresponding operations
(e.g., operations["listDeploymentVersionDependencies"],
operations["requestDeploymentVersionDependencyUpsert"],
operations["requestDeploymentVersionDependencyDeletion"]) require path params
(workspaceId, deploymentVersionId, dependencyDeploymentId, name); fix by
regenerating the OpenAPI TypeScript types from the canonical OpenAPI source so
those path parameter definitions are emitted correctly (ensuring the paths
entries include the required path parameter shapes) rather than hand-editing the
file.
| test.beforeAll(async ({ api, workspace }) => { | ||
| const systemRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/systems", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: `dep-test-system-${faker.string.alphanumeric(8)}` }, | ||
| }, | ||
| ); | ||
| expect(systemRes.response.status).toBe(202); | ||
| systemId = systemRes.data!.id; | ||
|
|
||
| const downstreamName = `downstream-${faker.string.alphanumeric(8)}`; | ||
| const downstreamRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/deployments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: downstreamName, slug: downstreamName }, | ||
| }, | ||
| ); | ||
| expect(downstreamRes.response.status).toBe(202); | ||
| downstreamId = downstreamRes.data!.id; | ||
|
|
||
| const upstreamName = `upstream-${faker.string.alphanumeric(8)}`; | ||
| const upstreamRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/deployments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { name: upstreamName, slug: upstreamName }, | ||
| }, | ||
| ); | ||
| expect(upstreamRes.response.status).toBe(202); | ||
| upstreamId = upstreamRes.data!.id; | ||
|
|
||
| const secondUpstreamName = `upstream2-${faker.string.alphanumeric(8)}`; | ||
| const secondUpstreamRes = await api.POST( | ||
| "/v1/workspaces/{workspaceId}/deployments", | ||
| { | ||
| params: { path: { workspaceId: workspace.id } }, | ||
| body: { | ||
| name: secondUpstreamName, | ||
| slug: secondUpstreamName, | ||
| }, | ||
| }, | ||
| ); | ||
| expect(secondUpstreamRes.response.status).toBe(202); | ||
| secondUpstreamId = secondUpstreamRes.data!.id; | ||
| }); | ||
|
|
||
| test.afterAll(async ({ api, workspace }) => { | ||
| await api.DELETE("/v1/workspaces/{workspaceId}/systems/{systemId}", { | ||
| params: { path: { workspaceId: workspace.id, systemId } }, | ||
| }); | ||
| for (const id of [downstreamId, upstreamId, secondUpstreamId]) { | ||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/deployments/{deploymentId}", | ||
| { | ||
| params: { path: { workspaceId: workspace.id, deploymentId: id } }, | ||
| }, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use YAML fixtures for this e2e suite.
This file hand-rolls entity provisioning and teardown. As per coding guidelines, **/e2e/**/*.spec.ts should declare test entities in a .spec.yaml fixture and load/clean them with importEntitiesFromYaml / cleanupImportedEntities.
As per coding guidelines: **/e2e/**/*.spec.ts: E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to tear them down.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/api/deployment-version-dependencies.spec.ts` around lines 12 - 72,
Replace the manual provisioning/teardown in test.beforeAll and test.afterAll
with YAML fixture helpers: remove the API POST/DELETE blocks that create
systemId, downstreamId, upstreamId, secondUpstreamId and instead call
importEntitiesFromYaml at the start of the suite to load a .spec.yaml fixture
that declares those entities (names/slugs), and call cleanupImportedEntities in
test.afterAll to tear them down; update references to
systemId/downstreamId/upstreamId/secondUpstreamId to use the IDs returned by
importEntitiesFromYaml (or the fixture loader’s result) so
test.beforeAll/test.afterAll no longer perform direct API calls.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes