feat: release target releases endpoint and tests #515
Conversation
…get-releases-endpoint
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update introduces a new API endpoint to retrieve the latest 100 releases for a given release target, complete with OpenAPI documentation and end-to-end tests. The backend workers and utilities are refactored to improve transaction safety, row-level locking, and job dispatch logic. Permission and RBAC systems are extended to support the new "releaseTarget" scope and "releaseTarget.get" permission, ensuring secure access to the new endpoint. The YAML loader and test schema are updated to support deployment versions, and a comprehensive test suite validates release creation behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Auth
participant DB
Client->>API: GET /v1/release-targets/{releaseTargetId}/releases
API->>Auth: Authenticate & authorize (ReleaseTargetGet)
Auth-->>API: Permission granted
API->>DB: Query release target existence
DB-->>API: Release target found/not found
API->>DB: Query latest 100 releases (with joins)
DB-->>API: Releases data
API->>Client: JSON array of releases (200 OK) or error (404/500)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
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:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (1)
70-99: 🛠️ Refactor suggestionLimit job-fan-out to new release targets to avoid redundant queue traffic
After the transaction, you query all release targets for the whole workspace (lines 101-115) and enqueue an
EvaluateReleaseTargetjob for every one of them.
In the common case, only a handful of release-target rows were (re)computed inside the transaction. Dispatching jobs for the entire workspace:
- needlessly floods the queue (potentially 1000s of jobs),
- adds avoidable round-trips to Redis (even with the 500 ms dedup-TTL),
- introduces extra load on workers that will immediately no-op.
A lighter alternative is to enqueue jobs only for the IDs you just inserted:
- const releaseTargets = await db // huge select - .select() - .from(schema.releaseTarget) - ... - .then((rows) => rows.map((row) => row.release_target)); + const releaseTargets = await db + .select() + .from(schema.releaseTarget) + .where(inArray( -- drizzle helper + schema.releaseTarget.id, + releaseTargetsIdsFromInsert, -- ‹uuid[]› built from `releaseTargets` + ));This keeps the lock-scope optimisation you added, but prevents unnecessary work.
Feel free to ping me if you’d like a full patch.e2e/api/yaml-loader.ts (1)
218-223:⚠️ Potential issueDo not forward test-only
versionsfield to the deployments API
deploymentobjects include aversionsarray that is only meaningful for the test harness, but the/v1/deploymentsendpoint is unlikely to understand or validate it.
Forwarding unknown attributes can lead to 400‐level errors or, worse, silent no-ops that mask test failures.-const deploymentResponse = await api.POST("/v1/deployments", { - body: { - ...deployment, // <- also forwards deployment.versions ❌ - systemId: result.system.id, - }, -}); +const { versions: _ignored, ...deploymentBody } = deployment; // strip test-only field +const deploymentResponse = await api.POST("/v1/deployments", { + body: { + ...deploymentBody, + systemId: result.system.id, + }, +});
🧹 Nitpick comments (8)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/openapi.ts (1)
1-86: Well-structured OpenAPI spec for the new endpoint.The OpenAPI specification is clear and comprehensive, properly documenting the new endpoint for retrieving releases by release target ID.
Consider for future enhancement:
- Adding pagination support rather than a fixed limit of 100 releases
- Ensuring
DeploymentandDeploymentVersioncomponent schemas are properly defined in the shared componentsapps/event-worker/src/workers/compute-systems-release-targets.ts (1)
147-155: Await queue writes to guarantee at-least-once dispatch
getQueue(...).add()returns a Promise. Withoutawait, the worker may acknowledge the
BullMQ job before the new job is actually enqueued, causing silent loss if the Node process
crashes immediately afterwards.- getQueue(Channel.ComputePolicyTargetReleaseTargetSelector).add( + await getQueue(Channel.ComputePolicyTargetReleaseTargetSelector).add((Repeat inside the loop.)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
234-239: Await the job-dispatch promise
As with the other worker, add anawaitto guarantee that the dispatch is persisted before
the worker finishes.- getQueue(Channel.DispatchJob).add(job.id, { jobId: job.id }); + await getQueue(Channel.DispatchJob).add(job.id, { jobId: job.id });e2e/api/schema.ts (1)
2715-2743: Consider widening thevariables[*].valuetype
The runtime allows variable values to be string, number, boolean, object, etc.
The schema narrows this to string only, which will cause client-side codegen to lose type
information and possibly reject valid payloads.If the API intentionally stringifies everything, add a note; otherwise change to
value: string | number | boolean | object | null.openapi.v1.json (1)
2396-2409: Consider documenting auth requirements for the new endpointMost sensitive endpoints in the file are protected via the
bearerAuthsecurity scheme. The new release-retrieval route omits
security,401, and403responses. If the handler does enforce
authentication/authorization (as stated in the PR description), the
spec should reflect that so client generation & docs stay accurate.e2e/tests/api/release.spec.ts (2)
84-86: Replacepage.waitForTimeout& sleeps with logical waitsThe tests don’t use the browser; loading a page merely to sleep is
wasteful. Remove thepagefixture and rely on the polling helper
recommended above. This will also speed up headless CI runs.Also applies to: 128-130, 190-192
30-33: Fixture parameters includepagewhen not requiredExcept for the artificial delay noted above, the tests never interact
with a browser page. You can droppagefrom the parameter list to
reduce Playwright’s startup time.Also applies to: 105-108, 167-170
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts (1)
43-69: Sub-query aggregation may explode memory on large variable sets
json_aggaggregates all variable key/values for every release row.
If a release target has many variables or a long history, this endpoint will:
- Materialise a huge JSON array in Postgres memory for each join.
- Transfer large payloads over the network.
Mitigations:
- Project only the variables you need (e.g., latest snapshot) or paginate at the SQL level.
- Fetch variables in a second, paginated call keyed by
releaseId.- Add a
LIMITorWHEREonvariableValueSnapshot(e.g., latestcreatedAt).Consider profiling with realistic data before this hits production.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/event-worker/src/utils/dispatch-evaluate-jobs.ts(1 hunks)apps/event-worker/src/workers/compute-deployment-resource-selector.ts(2 hunks)apps/event-worker/src/workers/compute-environment-resource-selector.ts(2 hunks)apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts(3 hunks)apps/event-worker/src/workers/compute-systems-release-targets.ts(3 hunks)apps/event-worker/src/workers/evaluate-release-target.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts(1 hunks)e2e/api/schema.ts(2 hunks)e2e/api/yaml-loader.ts(3 hunks)e2e/tests/api/release.spec.ts(1 hunks)e2e/tests/api/release.spec.yaml(1 hunks)openapi.v1.json(1 hunks)packages/auth/src/utils/rbac.ts(2 hunks)packages/db/src/schema/rbac.ts(1 hunks)packages/validators/src/auth/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/compute-deployment-resource-selector.tspackages/db/src/schema/rbac.tspackages/validators/src/auth/index.tsapps/event-worker/src/workers/compute-environment-resource-selector.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/openapi.tsapps/event-worker/src/utils/dispatch-evaluate-jobs.tspackages/auth/src/utils/rbac.tsapps/event-worker/src/workers/evaluate-release-target.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.tsapps/event-worker/src/workers/compute-policy-target-release-target-selector.tsapps/event-worker/src/workers/compute-systems-release-targets.tse2e/tests/api/release.spec.tse2e/api/schema.tse2e/api/yaml-loader.ts
🧠 Learnings (2)
packages/db/src/schema/rbac.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.185Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
packages/auth/src/utils/rbac.ts (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#181
File: packages/auth/src/utils/rbac.ts:102-118
Timestamp: 2024-10-29T02:05:46.185Z
Learning: The `releaseChannel` scope type is included in the `scopeType` enum in `packages/db/src/schema/rbac.ts`.
🧬 Code Graph Analysis (6)
apps/event-worker/src/workers/compute-deployment-resource-selector.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
packages/rule-engine/src/types.ts (1)
ReleaseTargetIdentifier(82-86)
apps/event-worker/src/workers/evaluate-release-target.ts (4)
packages/db/src/schema/release.ts (3)
release(113-124)releaseTarget(20-42)versionRelease(46-60)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirst(9-13)packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (3)
packages/db/src/client.ts (1)
db(15-15)packages/db/src/schema/policy.ts (1)
policyTarget(64-78)packages/events/src/index.ts (1)
getQueue(28-34)
apps/event-worker/src/workers/compute-systems-release-targets.ts (3)
packages/db/src/client.ts (1)
db(15-15)packages/events/src/index.ts (1)
getQueue(28-34)apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
dispatchEvaluateJobs(5-11)
e2e/tests/api/release.spec.ts (5)
e2e/tests/fixtures.ts (1)
test(11-26)e2e/api/yaml-loader.ts (3)
ImportedEntities(62-99)importEntitiesFromYaml(109-300)cleanupImportedEntities(307-355)packages/db/src/schema/workspace.ts (1)
workspace(18-27)packages/db/src/schema/resource.ts (1)
resource(59-87)packages/db/src/schema/release.ts (2)
releaseTarget(20-42)release(113-124)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (18)
packages/db/src/schema/rbac.ts (1)
53-53: Appropriate extension of scope types to support release targetsThe addition of the
"releaseTarget"enum value to thescopeTypeenum follows the existing pattern and properly extends the RBAC system to support the new release target functionality. This change aligns with the learnings showing similar scope types (likereleaseChannel) already exist in the system.e2e/tests/api/release.spec.yaml (1)
1-32: Well-structured test specification for release functionalityThis YAML test specification is well-organized with a clear hierarchy of entities (system → resources → environments → deployments). The use of template variables with
{{ prefix }}allows for dynamic test setup and reuse across different test contexts.packages/validators/src/auth/index.ts (1)
114-115: Correct extension of permission enum for release targetsThe addition of
ReleaseTargetGet = "releaseTarget.get"follows the established naming convention of[ResourceType].[Action]and properly supports the new API endpoint for retrieving release target releases. This permission will automatically be included in the admin role since it usesObject.values(Permission).apps/event-worker/src/workers/compute-deployment-resource-selector.ts (2)
23-24: Improved row-level locking precisionThe change to lock computedDeploymentResource rows rather than the entire deployment record is a good practice. This more granular locking approach reduces lock contention and improves database performance by only locking the specific rows being modified.
61-61: Enhanced transaction safety with awaited queue operationAdding
awaitto thegetQueue().add()method ensures the queue operation completes before the worker proceeds. This prevents potential race conditions and ensures proper sequencing of asynchronous operations in the event-processing pipeline.apps/event-worker/src/workers/compute-environment-resource-selector.ts (2)
41-42: Improved lock scope for reduced contention.This change refines the locking strategy to target only the specific computed environment resource rows associated with the environment, rather than locking the entire environment record. This approach reduces database contention by narrowing the lock scope.
82-82: Proper sequencing with await for queue operations.Adding the
awaitkeyword ensures that the job is successfully enqueued before the transaction completes, improving reliability of the job dispatch sequence.apps/event-worker/src/utils/dispatch-evaluate-jobs.ts (1)
1-5: LGTM! Good type refinement.Switching from the complete schema.ReleaseTarget to the more focused ReleaseTargetIdentifier type is a positive change. This makes the function signature more precise by specifically requiring only the identification fields needed for the operation.
packages/auth/src/utils/rbac.ts (2)
434-454: LGTM! Well-implemented scope resolution for release targets.The implementation follows the established pattern for scope resolution functions in this file, properly joining all necessary tables to establish the complete scope hierarchy from release target down to workspace.
478-478: Proper registration of release target scope handler.This addition ensures that the RBAC system correctly resolves permissions for the new release target scope, which is essential for the security of the new release targets endpoint.
apps/event-worker/src/workers/compute-systems-release-targets.ts (3)
80-81: Early-exit is a nice micro-optimisation
Short-circuiting when both id lists are empty prevents a DB round-trip – good catch.
157-158: 👍 Unified evaluate-job dispatcher
Delegating todispatchEvaluateJobskeeps the worker slim and re-usable.
161-165: Retry delay lowered to 500 ms – double-check back-pressure
Halving the delay increases contention retries under high load. Monitor lock-wait logs to
ensure this does not flood the queue.apps/event-worker/src/workers/evaluate-release-target.ts (1)
240-251: Good progressive retry strategy
Retrying on lock conflicts (55P03) and on FK-violation (23503) covers both concurrent
evaluations and race conditions during target creation. Nicely handled.e2e/api/schema.ts (1)
426-442: Endpoint stub looks correct
Path and method are wired into the OpenAPI spec; no immediate issues spotted.apps/event-worker/src/workers/compute-policy-target-release-target-selector.ts (2)
70-78: Lock may be a no-op when the association table is empty
FOR UPDATE NOWAITon the join will lock rows only returned by the query.
IfcomputedPolicyTargetReleaseTargetcurrently has zero rows for the
policyTarget, the select returns nothing → no rows are locked →
two workers can concurrently compute & insert, racing on the unique index.The race is benign (on-conflict-do-nothing) but still wastes work and may starve the queue.
Consider additionally locking thepolicyTargetrow itself (or using an advisory lock) to guarantee single-writer semantics.
116-129: AwaitingPromise.allis good 👍Switching from
queue.addBulkto per-item insertion withPromise.allmaintains parallelism while preserving dedup keys – nice improvement!e2e/tests/api/release.spec.ts (1)
159-160: Drop strayconsole.logstatements
console.login test output adds noise and makes CI logs harder to read.
[ suggest_nitpick ]Also applies to: 221-222
| SELECT ${schema.releaseTarget.id} FROM ${schema.releaseTarget} | ||
| WHERE ${or( | ||
| inArray(schema.releaseTarget.deploymentId, deploymentIds), | ||
| inArray(schema.releaseTarget.environmentId, environmentIds), | ||
| )} | ||
| FOR UPDATE NOWAIT | ||
| `, | ||
| ); | ||
|
|
||
| const previousReleaseTargets = await tx | ||
| .delete(schema.releaseTarget) | ||
| .where( | ||
| or( | ||
| inArray(schema.releaseTarget.deploymentId, deploymentIds), | ||
| inArray(schema.releaseTarget.environmentId, environmentIds), | ||
| ), | ||
| ) | ||
| .returning(); | ||
| await tx.execute( | ||
| sql` | ||
| SELECT * FROM ${schema.computedEnvironmentResource} | ||
| WHERE ${inArray(schema.computedEnvironmentResource.environmentId, environmentIds)} | ||
| FOR UPDATE NOWAIT | ||
| `, | ||
| ); | ||
|
|
||
| await tx.execute( | ||
| sql` | ||
| SELECT * FROM ${schema.computedDeploymentResource} | ||
| WHERE ${inArray(schema.computedDeploymentResource.deploymentId, deploymentIds)} | ||
| FOR UPDATE NOWAIT | ||
| `, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Guard against empty IN () clauses when locking rows
inArray(_, deploymentIds) / inArray(_, environmentIds) will generate … IN () if the
corresponding list is empty. PostgreSQL rejects that syntax, so an empty list will make the
whole transaction fail before we reach the retry logic.
- await tx.execute(sql`SELECT * FROM ${schema.computedEnvironmentResource}
- WHERE ${inArray(schema.computedEnvironmentResource.environmentId, environmentIds)}
- FOR UPDATE NOWAIT`);
+ if (environmentIds.length > 0) {
+ await tx.execute(sql`
+ SELECT * FROM ${schema.computedEnvironmentResource}
+ WHERE ${inArray(schema.computedEnvironmentResource.environmentId, environmentIds)}
+ FOR UPDATE NOWAIT
+ `);
+ }Apply the same conditional guard to all three lock statements (releaseTarget,
computedEnvironmentResource, computedDeploymentResource).
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
if (deploymentIds.length === 0 && environmentIds.length === 0) return;
we do an early return befdore the tx even happens if either is empty
There was a problem hiding this comment.
@adityachoudhari26 You're right that there's an early return if both arrays are empty. However, there's still a potential issue if only one of the arrays is empty:
- If
deploymentIdsis empty butenvironmentIdsis not, the lock statement forcomputedDeploymentResourcewould have an emptyIN ()clause - If
environmentIdsis empty butdeploymentIdsis not, the lock statement forcomputedEnvironmentResourcewould have an emptyIN ()clause
PostgreSQL rejects empty IN () syntax, so additional checks before each lock statement would prevent SQL errors in these cases. Something like:
if (environmentIds.length > 0) {
await tx.execute(sql`
SELECT * FROM ${schema.computedEnvironmentResource}
WHERE ${inArray(schema.computedEnvironmentResource.environmentId, environmentIds)}
FOR UPDATE NOWAIT
`);
}✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/event-worker/src/workers/compute-systems-release-targets.ts:86-110
Timestamp: 2025-04-28T18:38:21.152Z
Learning: In SQL queries that use `inArray()` with arrays like `deploymentIds` or `environmentIds`, if these arrays are empty, it will generate an invalid `IN ()` clause that PostgreSQL rejects. Adding condition checks (e.g., `if (array.length > 0)`) before executing such queries prevents SQL syntax errors.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
good catch, I should actually be using || for the early return
There was a problem hiding this comment.
@adityachoudhari26 Yes, using || for the early return is actually a more elegant solution! Changing to:
if (deploymentIds.length === 0 || environmentIds.length === 0) return;This would prevent execution of the transaction entirely if either array is empty, which elegantly avoids the empty IN () clause issue altogether. Much cleaner than adding individual checks before each SQL statement.
🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/event-worker/src/workers/compute-systems-release-targets.ts:86-110
Timestamp: 2025-04-28T18:38:21.152Z
Learning: In SQL queries that use `inArray()` with arrays like `deploymentIds` or `environmentIds`, if these arrays are empty, it will generate an invalid `IN ()` clause that PostgreSQL rejects. Adding condition checks (e.g., `if (array.length > 0)`) before executing such queries prevents SQL syntax errors.
| await new Promise((resolve) => setTimeout(resolve, 5_000)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid fixed sleeps – poll the API instead
Using setTimeout for 5 s here makes the suite slow locally and flaky
under load; CI variance may require even longer waits.
Refactor to poll until the worker has finished (example helper):
async function waitForReleaseTarget(api, resourceId: string, timeout = 10_000) {
const start = Date.now();
while (Date.now() - start < timeout) {
const r = await api.GET("/v1/resources/{resourceId}/release-targets", {
params: { path: { resourceId } },
});
if (r.data?.length) return r.data[0];
await new Promise((res) => setTimeout(res, 500));
}
throw new Error("release target not created in time");
}This keeps tests fast yet reliable.
There was a problem hiding this comment.
for now we will just keep the sleep intead of polling
There was a problem hiding this comment.
@adityachoudhari26 Understood, we'll keep the current approach with the sleep as is. Thank you for considering the suggestion.
| if (deployment.versions && deployment.versions.length > 0) { | ||
| for (const version of deployment.versions) { | ||
| const versionResponse = await api.POST("/v1/deployment-versions", { | ||
| body: { | ||
| ...version, | ||
| deploymentId: deploymentResponse.data!.id, | ||
| }, | ||
| }); | ||
|
|
||
| if (versionResponse.response.status !== 201) { | ||
| throw new Error( | ||
| `Failed to create deployment version: ${JSON.stringify(versionResponse.error)}`, | ||
| ); | ||
| } | ||
|
|
||
| const versionData = versionResponse.data!; | ||
| versions.push({ | ||
| id: versionData.id, | ||
| name: versionData.name, | ||
| tag: versionData.tag, | ||
| status: versionData.status ?? "ready", | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide sensible defaults & validation when creating deployment versions
nameis optional in the YAML, but the backend may require it. Defaultingnameto the tag keeps tests concise and prevents 422 responses.statusdefaults to"ready"on the response mapping, but you also need to supply a valid status when the field is omitted in the request body to avoid validation errors.- Consider short-circuiting if no versions are present instead of running an empty loop.
-if (deployment.versions && deployment.versions.length > 0) {
- for (const version of deployment.versions) {
+for (const version of deployment.versions ?? []) {
+ const {
+ tag,
+ name = tag, // default name
+ status = "ready", // default status
+ ...rest
+ } = version;
+
const versionResponse = await api.POST("/v1/deployment-versions", {
body: {
- ...version,
+ tag,
+ name,
+ status,
+ ...rest,
deploymentId: deploymentResponse.data!.id,
},
});
/* … */
}These guards make the import routine deterministic and reduce test flakiness.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (deployment.versions && deployment.versions.length > 0) { | |
| for (const version of deployment.versions) { | |
| const versionResponse = await api.POST("/v1/deployment-versions", { | |
| body: { | |
| ...version, | |
| deploymentId: deploymentResponse.data!.id, | |
| }, | |
| }); | |
| if (versionResponse.response.status !== 201) { | |
| throw new Error( | |
| `Failed to create deployment version: ${JSON.stringify(versionResponse.error)}`, | |
| ); | |
| } | |
| const versionData = versionResponse.data!; | |
| versions.push({ | |
| id: versionData.id, | |
| name: versionData.name, | |
| tag: versionData.tag, | |
| status: versionData.status ?? "ready", | |
| }); | |
| } | |
| } | |
| // Create a deployment version for each entry (no-op if versions is null/undefined or empty) | |
| for (const version of deployment.versions ?? []) { | |
| // Supply sensible defaults up front to avoid 422s | |
| const { | |
| tag, | |
| name = tag, // default name to the tag if omitted | |
| status = "ready", // default status to "ready" if omitted | |
| ...rest | |
| } = version; | |
| const versionResponse = await api.POST("/v1/deployment-versions", { | |
| body: { | |
| tag, | |
| name, | |
| status, | |
| ...rest, | |
| deploymentId: deploymentResponse.data!.id, | |
| }, | |
| }); | |
| if (versionResponse.response.status !== 201) { | |
| throw new Error( | |
| `Failed to create deployment version: ${JSON.stringify( | |
| versionResponse.error, | |
| )}`, | |
| ); | |
| } | |
| const versionData = versionResponse.data!; | |
| versions.push({ | |
| id: versionData.id, | |
| name: versionData.name, | |
| tag: versionData.tag, | |
| status: versionData.status ?? "ready", | |
| }); | |
| } |
| return { | ||
| deployment: release.deployment, | ||
| version: release.deployment_version, | ||
| variables, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Avoid leaking internal table columns in the public response
Returning the entire deployment and deployment_version records exposes internal IDs, timestamps, and potentially sensitive metadata.
Define and return a minimal DTO instead, e.g.:
return {
deployment: { id: release.deployment.id, name: release.deployment.name },
version: { id: release.deployment_version.id, tag: release.deployment_version.tag },
variables,
};This future-proofs the contract and prevents accidental PII leakage.
There was a problem hiding this comment.
there is nothing in these tables that we want to avoid sending to the public
There was a problem hiding this comment.
@adityachoudhari26 I understand. Thank you for clarifying that all the fields in these tables are intended to be publicly accessible. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#515
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/route.ts:103-108
Timestamp: 2025-04-28T18:41:58.802Z
Learning: In this project, full records from the `deployment` and `deployment_version` tables are considered safe for public API consumption, and there's no need to create restricted DTOs for them.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| authz(({ can, params }) => | ||
| can | ||
| .perform(Permission.ReleaseTargetGet) | ||
| .on({ type: "releaseTarget", id: params.releaseTargetId ?? "" }), | ||
| ), | ||
| ) | ||
| .handle<object, { params: Promise<{ releaseTargetId: string }> }>( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit 400 for missing releaseTargetId before RBAC check
params.releaseTargetId ?? "" silently converts an undefined path param to an empty string, which still passes to the RBAC layer and later SQL look-ups.
Returning 404/500 in that situation hides the real client error.
.use(
authz(({ can, params }) =>
- can
+ (params.releaseTargetId
+ ? can
.perform(Permission.ReleaseTargetGet)
.on({ type: "releaseTarget", id: params.releaseTargetId })
- ),
+ : false),
)Alternatively, short-circuit with a 400 Bad Request before invoking authz.
Committable suggestion skipped: line range outside the PR's diff.
| const { values } = release.variableSetReleaseSubquery; | ||
| const variables = values.map(({ key, value, sensitive }) => { | ||
| const strval = String(value); | ||
| const resolvedValue = sensitive | ||
| ? variablesAES256().decrypt(strval) | ||
| : strval; | ||
| return { key, value: resolvedValue }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Casting variable values to string loses original types
String(value) erases numeric/boolean/null information and may stringify complex JSON.
Down-stream consumers then need type inference or JSON.parse, which is brittle.
-const strval = String(value);
-const resolvedValue = sensitive
- ? variablesAES256().decrypt(strval)
- : strval;
-return { key, value: resolvedValue };
+const raw = sensitive
+ ? variablesAES256().decrypt(String(value))
+ : value;
+return { key, value: raw };Decrypt only when necessary and keep the original primitive/JSON types intact.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { values } = release.variableSetReleaseSubquery; | |
| const variables = values.map(({ key, value, sensitive }) => { | |
| const strval = String(value); | |
| const resolvedValue = sensitive | |
| ? variablesAES256().decrypt(strval) | |
| : strval; | |
| return { key, value: resolvedValue }; | |
| }); | |
| const { values } = release.variableSetReleaseSubquery; | |
| const variables = values.map(({ key, value, sensitive }) => { | |
| const raw = sensitive | |
| ? variablesAES256().decrypt(String(value)) | |
| : value; | |
| return { key, value: raw }; | |
| }); |
There was a problem hiding this comment.
this is fine
There was a problem hiding this comment.
@adityachoudhari26 Understood! I'll respect your decision to keep the original implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/event-worker/src/workers/evaluate-release-target.ts (1)
141-141:⚠️ Potential issueTransaction parameter added but DB clients are inconsistent
Similar to the
handleVersionReleasefunction, this function accepts a transaction parameter but still uses the globaldbclient in line 147.-const varrm = new VariableReleaseManager(db, { +const varrm = new VariableReleaseManager(tx, {Using the global
dbinstance defeats the purpose of passing the transaction and reintroduces the transaction isolation issues mentioned in the previous review.
🧹 Nitpick comments (2)
apps/event-worker/src/workers/evaluate-release-target.ts (2)
234-238: Feature flag for job creationGood practice using a feature flag to control the job creation behavior. This allows for gradual rollout and easier rollback if issues are discovered.
Consider using a constant for the environment variable name and possibly a default value for the check:
-if (process.env.ENABLE_NEW_POLICY_ENGINE === "true" && release != null) { +const ENABLE_NEW_POLICY_ENGINE = process.env.ENABLE_NEW_POLICY_ENGINE === "true"; +if (ENABLE_NEW_POLICY_ENGINE && release != null) {
240-251: Robust error handling with specific error codesExcellent error handling with specific PostgreSQL error codes to detect row locking and foreign key issues. The retry mechanism with delay is a good approach for handling temporary conflicts.
Consider extracting the error codes to named constants for better readability and maintainability:
-const isRowLocked = e.code === "55P03"; -const isReleaseTargetNotCommittedYet = e.code === "23503"; +const PG_ERROR_ROW_LOCKED = "55P03"; +const PG_ERROR_FOREIGN_KEY_VIOLATION = "23503"; + +const isRowLocked = e.code === PG_ERROR_ROW_LOCKED; +const isReleaseTargetNotCommittedYet = e.code === PG_ERROR_FOREIGN_KEY_VIOLATION;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/event-worker/src/workers/compute-systems-release-targets.ts(3 hunks)apps/event-worker/src/workers/evaluate-release-target.ts(3 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/openapi.ts(1 hunks)e2e/api/schema.ts(2 hunks)openapi.v1.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/event-worker/src/workers/compute-systems-release-targets.ts
- e2e/api/schema.ts
- apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/releases/openapi.ts
- openapi.v1.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/workers/evaluate-release-target.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/workers/evaluate-release-target.ts (4)
packages/db/src/common.ts (2)
Tx(22-22)takeFirst(9-13)packages/db/src/schema/release.ts (3)
releaseTarget(20-42)release(113-124)versionRelease(46-60)packages/db/src/client.ts (1)
db(15-15)packages/events/src/index.ts (1)
getQueue(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (4)
apps/event-worker/src/workers/evaluate-release-target.ts (4)
174-187: Improved transaction handling with row-level lockingGood restructuring to use a database transaction for the entire evaluation process. The code now properly retrieves and locks the release target within the transaction scope.
189-202: Efficient retrieval of existing releasesThe code now correctly retrieves the latest version and variable releases within the transaction, ensuring a consistent view of the data before making any changes.
208-215: Smart optimization to avoid redundant releasesExcellent optimization to avoid creating duplicate releases when nothing has changed. The comparison logic ensures that new releases are only created when either the version or variable release has changed.
216-232: Atomic release creationThe release creation is now properly atomic within the transaction. If any part of the process fails, the entire transaction will be rolled back, maintaining data consistency.
…get-releases-endpoint
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores