chore: include version dependencies in version list endpoint#1124
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (7)
✨ 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. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the deployment version list endpoint to include each version’s dependency selectors in the response, and aligns OpenAPI/types plus E2E coverage to validate the new shape.
Changes:
- Add
dependenciesmap to the deployment versions list API response (including{}when none). - Introduce a new OpenAPI schema (
DeploymentVersionWithDependencies) and switch the list endpoint to return it. - Add an E2E test asserting dependency maps are returned per version.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| e2e/tests/api/deployment-version-list.spec.ts | Adds E2E coverage to assert the list endpoint returns a per-version dependencies map. |
| e2e/api/schema.ts | Updates generated E2E OpenAPI client types to include the new list response schema. |
| apps/api/src/types/openapi.ts | Updates generated API OpenAPI types to reflect the new list response schema. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Implements dependencies aggregation in the list endpoint via a raw SQL query returning dependency maps. |
| apps/api/openapi/schemas/deploymentversions.jsonnet | Defines DeploymentVersionWithDependencies schema. |
| apps/api/openapi/paths/deploymentversions.jsonnet | Switches list response item schema to DeploymentVersionWithDependencies. |
| apps/api/openapi/openapi.json | Updates compiled OpenAPI spec to include the new schema and response references. |
Comments suppressed due to low confidence (1)
apps/api/src/routes/v1/workspaces/deployments.ts:392
- In the CEL paging loop, each batch fetch now also computes dependencies for every scanned version (up to CEL_MAX_SCAN). If dependencies aren't needed for CEL evaluation, consider scanning versions without dependencies and only fetching/attaching dependencies for the matched page (or adding a flag to selectVersionsWithDeps) to avoid unnecessary DB work.
const batch = await selectVersionsWithDeps(
whereSql,
orderBySql,
CEL_BATCH_SIZE,
scanned,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| COALESCE( | ||
| (SELECT jsonb_object_agg( | ||
| d.dependency_deployment_id, | ||
| jsonb_build_object('versionSelector', d.version_selector) | ||
| ) |
There was a problem hiding this comment.
Pushing back. The correlated subquery filters on d.deployment_version_id = v.id, which is the leading column of deployment_version_dependency's primary key (primaryKey({ columns: [deploymentVersionId, dependencyDeploymentId] }) in packages/db/src/schema/deployment-version.ts). So each row resolves to an indexed range scan on the PK, not a sequential scan.
Page sizes are bounded (default 50, max 1000) and the CEL scan batches at CEL_BATCH_SIZE = 500. Worst case is ~500 fast PK seeks per batch — comparable in plan cost to a hash-aggregated LEFT JOIN, and we get a single round-trip without a post-query grouping step in JS.
Suggesting a separate query was actually the original draft; we deliberately chose the inlined subquery here to keep it to one query per fetch and to make version.dependencies available inside the CEL evaluator during scan (otherwise we'd need to refetch deps after filtering). Keeping as-is.
There was a problem hiding this comment.
The pushback is sound. The correlated subquery resolves against the leading column of deployment_version_dependency's composite PK, so each execution is an indexed range scan rather than a seq scan. With page sizes bounded at ≤1000 rows, the worst case is O(page_size) fast PK seeks — no worse in practice than a hash-aggregated LEFT JOIN, and it avoids a post-query merge step in JS.
The other key reason to keep it inline: the dependencies field needs to be present on each version row during the CEL scan pass (inside the batch loop). Pulling it out into a second query would require fetching deps after filtering, which either means two round-trips or pre-fetching for all candidates before filtering — neither is clearly better. Single query, keeping as-is makes sense.
| openapi.orderParam(), | ||
| openapi.celParam(), | ||
| ], | ||
| responses: openapi.paginatedResponse(openapi.schemaRef('DeploymentVersion')) | ||
| responses: openapi.paginatedResponse(openapi.schemaRef('DeploymentVersionWithDependencies')) |
There was a problem hiding this comment.
Pushing back. The split is intentional:
GET .../versionsis the only endpoint where the caller doesn't already know the dependencies — it's the list view, where carrying the deps inline saves a per-row follow-up toGET .../deployment-versions/{id}/dependencies.POST .../versionsreturnsDeploymentVersionbecause the caller just sent the dependencies in the request body; echoing them back adds nothing.- Promoting
dependenciesonto the baseDeploymentVersionschema would force every endpoint that returns a version (release, job, etc.) to either populate it or document it as optional/empty. That turns a targeted feature into a fan-out change and silently makes other endpoints slower or misleading.
The dedicated DeploymentVersionWithDependencies shape was an explicit design choice for this exact reason. Keeping it.
There was a problem hiding this comment.
Agreed with the reasoning. The split is the right call:
GET /versionsis the discovery path — callers don't already have the deps, so inlining saves N follow-up requests.POST /versionsis a write — the caller provided the deps; echoing them back is redundant noise.- Merging
dependenciesonto the baseDeploymentVersionschema would force every downstream endpoint (release, job, etc.) to either populate it (query cost on unrelated paths) or leave it absent/null (misleading schema contract). A dedicatedDeploymentVersionWithDependenciesshape scopes the change precisely where it's needed. Keeping as-is.
No description provided.