feat: ability to lock and unlock targets#590
Conversation
WalkthroughThis update introduces a locking mechanism for release targets, including database schema changes, new API endpoints for locking and unlocking, OpenAPI documentation, rule engine integration, and comprehensive end-to-end tests. The changes ensure only one active lock per release target and enforce access and concurrency controls at both the API and database levels. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant DB
participant RuleEngine
Client->>API: POST /v1/release-targets/{id}/lock
API->>DB: Check for existing active lock
alt Locked
API-->>Client: 409 Conflict (already locked)
else Not locked
API->>DB: Insert new lock record (transaction)
API-->>Client: 200 OK (lock record)
end
Client->>API: POST /v1/release-targets/{id}/unlock
API->>DB: Fetch latest lock record
alt Not locked
API-->>Client: 400 Bad Request
else Locked by another user
API-->>Client: 403 Forbidden
else Locked by current user
API->>DB: Update lock record (set unlockedAt)
API->>RuleEngine: Dispatch evaluation job
API-->>Client: 200 OK (updated lock record)
end
Suggested reviewers
Poem
✨ 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
e2e/api/schema.ts (1)
1183-1202:⚠️ Potential issueInconsistent enum – “rejected” allowed in operations but missing in core schemas
DeploymentVersion.status(line 1201) lists"building" | "ready" | "failed", while multiple operation request bodies now accept"rejected".
Similarly,Releasehas nostatusfield butupdateRelease/upsertReleaseallow it.This mismatch will break client code generated from the spec and may trigger runtime validation errors.
Update the component schemas to include the new
"rejected"value (and thestatusfield forRelease) so that the spec is self-consistent.Also applies to: 1783-1784, 1909-1910, 3544-3545, 3586-3587
🧹 Nitpick comments (13)
packages/rule-engine/src/manager/version-manager-rules.ts (1)
10-10: Import path suffix.jsis inconsistentEvery other import in this file omits the
.jsextension. Keeping a single style avoids friction for bundlers and Jest path mappers.-import { ReleaseTargetLockRule } from "../rules/release-target-lock-rule.js"; +import { ReleaseTargetLockRule } from "../rules/release-target-lock-rule";e2e/tests/api/release-target-lock.spec.yaml (1)
14-17: Minor YAML nit – final newlineMany linters (e.g.,
yamllint,prettier) flag a missing newline at EOF. Adding one avoids noisy diffs later.apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.ts (2)
58-65: Property overwrite:lockedByscalar → object
returning()yields{ lockedBy: <uuid> }. Spreading that object and later assigninglockedByagain replaces the scalar with{ id, name, email }. That works, but it’s easy to miss.Consider being explicit:
-const lock = await db.insert(...).returning().then(takeFirst); -const lockedBy = { id: user.id, name: user.name, email: user.email }; -return NextResponse.json({ ...lock, lockedBy }); +const [lock] = await db + .insert(schema.releaseTargetLockRecord) + .values({ releaseTargetId, lockedBy: user.id }) + .returning({ + id: schema.releaseTargetLockRecord.id, + releaseTargetId: schema.releaseTargetLockRecord.releaseTargetId, + lockedAt: schema.releaseTargetLockRecord.lockedAt, + }); + +return NextResponse.json({ + ...lock, + lockedBy: { id: user.id, name: user.name, email: user.email }, +});
71-75: Return 201 Created on successful lockThe operation creates a new resource (lock record). Using
201instead of the default200clarifies semantics and aligns with REST conventions.- return NextResponse.json({ - ...lock, - lockedBy, - }); + return NextResponse.json( + { ...lock, lockedBy }, + { status: 201 }, + );apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/openapi.ts (2)
28-34:unlockedAtshould not be a required property
unlockedAtis nullable and is expected to benulluntil the target is actually unlocked. Marking it as required forces callers to always send the key even when it provides no information and bloats the payload.required: [ "id", "releaseTargetId", "lockedAt", - "unlockedAt", "lockedBy", ],
62-64: Prefer status423 Lockedover409 ConflictHTTP 423 is expressly defined for “locked” resources and better conveys the semantics of this failure mode than a generic conflict.
- 409: { - description: "Release target is already locked", + 423: { + description: "Release target is already locked",packages/db/src/schema/release.ts (1)
154-177: Missing relations forreleaseTargetLockRecordAll other tables declare a relation-helper at the bottom of the file, but the new table does not. Omitting it means consumers must fall back to ad-hoc queries instead of the ergonomics Drizzle relations provide.
Add something similar to:
export const releaseTargetLockRecordRelations = relations( releaseTargetLockRecord, ({ one }) => ({ releaseTarget: one(releaseTarget, { fields: [releaseTargetLockRecord.releaseTargetId], references: [releaseTarget.id], }), user: one(user, { fields: [releaseTargetLockRecord.lockedBy], references: [user.id], }), }), );apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/route.ts (2)
80-90: Use a 409/423 status code for “not locked” instead of 400A bad request implies payload problems. In this context the request is syntactically valid; the error is state-based. Returning 409 Conflict (or 423 Locked) signals this more accurately to API consumers.
- { status: BAD_REQUEST }, + { status: CONFLICT }, // import CONFLICT from http-status
40-44:getIsAlreadyUnlockedunnecessarily recomputes “now” on every call
Date.now()once is enough; minor, but avoids micro-allocations inside tight request handlers.-const getIsAlreadyUnlocked = (unlockedAt: Date | null) => { - const now = new Date(); - return unlockedAt != null && isAfter(now, unlockedAt); -}; +const now = new Date(); +const getIsAlreadyUnlocked = (unlockedAt: Date | null) => + unlockedAt != null && isAfter(now, unlockedAt);packages/rule-engine/src/rules/release-target-lock-rule.ts (1)
39-45: Duplicate “is unlocked” logic – centralise to avoid driftThe exact same unlocked-determination logic exists in the API route. Pulling it into a shared util removes duplication and guarantees both code paths respect any future rule tweaks (e.g. time skew tolerance).
openapi.v1.json (1)
6447-6494: EnhanceReleaseTargetLockRecordschema
Add a top-level description for clarity and tighten validation by requiring thelockedBy.nameproperty and disallowing additional unknown properties.Example adjustments:
- "ReleaseTargetLockRecord": { + "ReleaseTargetLockRecord": { + "description": "Tracks when and by whom a release target was locked/unlocked.", "type": "object", "properties": { "id": { "type": "string", "format": "uuid" }, "releaseTargetId": { "type": "string", "format": "uuid" }, "lockedAt": { "type": "string", "format": "date-time" }, "unlockedAt": { "type": "string", "format": "date-time", "nullable": true }, "lockedBy": { "type": "object", "properties": { "id": { "type": "string", "format": "uuid" }, "name": { "type": "string" }, "email": { "type": "string" } }, - "required": [ - "id", - "email" - ] + "required": [ + "id", + "name", + "email" + ], + "additionalProperties": false } }, - "required": [ - "id", - "releaseTargetId", - "lockedAt", - "unlockedAt", - "lockedBy" - ] + "required": [ + "id", + "releaseTargetId", + "lockedAt", + "unlockedAt", + "lockedBy" + ], + "additionalProperties": false }e2e/tests/api/release-target-lock.spec.ts (2)
24-40: Factor out duplicated “create-resource” boilerplateEvery test repeats the same 20-line block to create a resource.
Extract a helper (e.g.createRandomResource()) or move it tobeforeEachto keep tests DRY and focused on lock behaviour.Also applies to: 102-118, 194-214, 259-279, 335-355
248-256: Assert error payloads, not just status codesThe negative-path tests only check HTTP status.
Add an assertion onresponse.data.error(or error code) to guarantee the API returns the documented error shape.Also applies to: 324-332, 371-379
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts(1 hunks)apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/route.ts(1 hunks)e2e/api/schema.ts(10 hunks)e2e/tests/api/release-target-lock.spec.ts(1 hunks)e2e/tests/api/release-target-lock.spec.yaml(1 hunks)openapi.v1.json(3 hunks)packages/db/drizzle/0114_military_daimon_hellstrom.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(2 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(2 hunks)packages/rule-engine/src/rules/release-target-lock-rule.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.
packages/rule-engine/src/manager/version-manager-rules.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/openapi.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.tsapps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/route.tspackages/rule-engine/src/rules/release-target-lock-rule.tspackages/db/src/schema/release.tse2e/tests/api/release-target-lock.spec.tse2e/api/schema.ts
🧬 Code Graph Analysis (5)
packages/rule-engine/src/manager/version-manager-rules.ts (1)
packages/rule-engine/src/rules/release-target-lock-rule.ts (1)
ReleaseTargetLockRule(9-47)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/openapi.ts (1)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts (1)
openapi(3-74)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts (1)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/openapi.ts (1)
openapi(3-90)
packages/rule-engine/src/rules/release-target-lock-rule.ts (3)
packages/rule-engine/src/types.ts (1)
PreValidationRule(43-46)packages/db/src/client.ts (1)
db(15-15)packages/db/src/common.ts (1)
takeFirstOrNull(15-20)
packages/db/src/schema/release.ts (1)
packages/db/src/schema/auth.ts (1)
user(27-44)
⏰ 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 (3)
packages/db/drizzle/meta/_journal.json (1)
802-808: Journal entry looks fineMeta-data addition is mechanically generated and consistent with the new migration tag.
packages/rule-engine/src/manager/version-manager-rules.ts (1)
68-73: Lock rule wisely placed first – good callPlacing
new ReleaseTargetLockRule(releaseTargetId)before concurrency & approval rules guarantees a fast fail for a locked target and avoids unnecessary queries.e2e/tests/api/release-target-lock.spec.ts (1)
74-86: Verify that the deployment chosen actually matches the release target
builder.refs.deployments.at(0)!assumes the first deployment created inbeforeAllis always the one linked to the freshly-created resource/environment. If the fixture order ever changes the test could silently pass/fail for the wrong reasons.Consider retrieving the deployment ID from
releaseTarget.deployment.idinstead of relying on a fixed array index.Also applies to: 152-164
| CREATE TABLE "release_target_lock_record" ( | ||
| "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, | ||
| "release_target_id" uuid NOT NULL, | ||
| "locked_at" timestamp with time zone DEFAULT now() NOT NULL, | ||
| "unlocked_at" timestamp with time zone, | ||
| "locked_by" uuid NOT NULL | ||
| ); | ||
| --> statement-breakpoint | ||
| ALTER TABLE "release_target_lock_record" ADD CONSTRAINT "release_target_lock_record_release_target_id_release_target_id_fk" FOREIGN KEY ("release_target_id") REFERENCES "public"."release_target"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "release_target_lock_record" ADD CONSTRAINT "release_target_lock_record_locked_by_user_id_fk" FOREIGN KEY ("locked_by") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action; No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add DB-level guarantees to prevent concurrent/invalid lock records
At the moment it is possible to insert:
- Multiple active rows (
unlocked_at IS NULL) for the samerelease_target_id, created by two concurrent requests that both pass theisCurrentlyLockedcheck. - A row whose
unlocked_atprecedeslocked_atdue to a bad clock or programming bug.
Both situations make the rule-engine’s logic ambiguous.
Consider extending the migration with:
-- Only one active lock per release_target
CREATE UNIQUE INDEX release_target_lock_active_idx
ON release_target_lock_record (release_target_id)
WHERE unlocked_at IS NULL;
-- Fast look-ups by release_target_id
CREATE INDEX release_target_lock_release_target_idx
ON release_target_lock_record (release_target_id);
-- Sanity check on the timestamps
ALTER TABLE "release_target_lock_record"
ADD CONSTRAINT release_target_lock_record_unlocked_after_locked
CHECK (unlocked_at IS NULL OR unlocked_at >= locked_at);These constraints make the consistency guarantees explicit and remove the need for a fragile application-side race check.
🤖 Prompt for AI Agents
In packages/db/drizzle/0114_military_daimon_hellstrom.sql lines 1 to 10, the
current schema allows multiple active lock records for the same
release_target_id and permits unlocked_at timestamps earlier than locked_at,
causing ambiguity. To fix this, add a partial unique index on release_target_id
where unlocked_at IS NULL to ensure only one active lock per release_target, add
a regular index on release_target_id for faster lookups, and add a CHECK
constraint to enforce unlocked_at is either null or not earlier than locked_at.
These changes enforce data consistency at the database level.
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.ts
Show resolved
Hide resolved
packages/db/src/schema/release.ts
Outdated
| export const releaseTargetLockRecord = pgTable("release_target_lock_record", { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| releaseTargetId: uuid("release_target_id") | ||
| .notNull() | ||
| .references(() => releaseTarget.id, { onDelete: "cascade" }), | ||
| lockedAt: timestamp("locked_at", { withTimezone: true }) | ||
| .notNull() | ||
| .defaultNow(), | ||
| unlockedAt: timestamp("unlocked_at", { withTimezone: true }), | ||
| lockedBy: uuid("locked_by") | ||
| .notNull() | ||
| .references(() => user.id, { onDelete: "cascade" }), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add index / uniqueness guarantee for active locks
Queries routinely filter on release_target_id and look for the latest record. Without an index this turns into a seq-scan as the table grows. Moreover, nothing currently prevents two concurrent “lock” writes, which would break the single-writer assumption and the business rules enforced by the API & rule engine.
export const releaseTargetLockRecord = pgTable("release_target_lock_record", {
id: uuid("id").primaryKey().defaultRandom(),
releaseTargetId: uuid("release_target_id")
.notNull()
.references(() => releaseTarget.id, { onDelete: "cascade" }),
+ /* NOTE: adding a plain index greatly speeds up the frequent
+ * `WHERE release_target_id = ? ORDER BY locked_at DESC LIMIT 1`
+ * query generated by the API and rule engine. */
+}, (t) => ({
+ idx_release_target: index("idx_release_target_lock_record_target").on(
+ t.releaseTargetId,
+ ),
+ /* Enforce at most one active lock at a time. */
+ uniq_active_lock: uniqueIndex("uniq_release_target_active_lock")
+ .on(t.releaseTargetId)
+ .where(isNull(t.unlockedAt)),
}));📝 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.
| export const releaseTargetLockRecord = pgTable("release_target_lock_record", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| releaseTargetId: uuid("release_target_id") | |
| .notNull() | |
| .references(() => releaseTarget.id, { onDelete: "cascade" }), | |
| lockedAt: timestamp("locked_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow(), | |
| unlockedAt: timestamp("unlocked_at", { withTimezone: true }), | |
| lockedBy: uuid("locked_by") | |
| .notNull() | |
| .references(() => user.id, { onDelete: "cascade" }), | |
| }); | |
| export const releaseTargetLockRecord = pgTable("release_target_lock_record", { | |
| id: uuid("id").primaryKey().defaultRandom(), | |
| releaseTargetId: uuid("release_target_id") | |
| .notNull() | |
| .references(() => releaseTarget.id, { onDelete: "cascade" }), | |
| lockedAt: timestamp("locked_at", { withTimezone: true }) | |
| .notNull() | |
| .defaultNow(), | |
| unlockedAt: timestamp("unlocked_at", { withTimezone: true }), | |
| lockedBy: uuid("locked_by") | |
| .notNull() | |
| .references(() => user.id, { onDelete: "cascade" }), | |
| /* NOTE: adding a plain index greatly speeds up the frequent | |
| * `WHERE release_target_id = ? ORDER BY locked_at DESC LIMIT 1` | |
| * query generated by the API and rule engine. */ | |
| }, (t) => ({ | |
| idx_release_target: index("idx_release_target_lock_record_target").on( | |
| t.releaseTargetId, | |
| ), | |
| /* Enforce at most one active lock at a time. */ | |
| uniq_active_lock: uniqueIndex("uniq_release_target_active_lock") | |
| .on(t.releaseTargetId) | |
| .where(isNull(t.unlockedAt)), | |
| })); |
🤖 Prompt for AI Agents
In packages/db/src/schema/release.ts around lines 140 to 152, the
releaseTargetLockRecord table lacks an index on releaseTargetId, causing
inefficient sequential scans during queries filtering by this column.
Additionally, there is no uniqueness constraint to prevent multiple active locks
for the same releaseTargetId, which violates the single-writer assumption. To
fix this, add an index on releaseTargetId to optimize query performance and
enforce a uniqueness constraint on releaseTargetId where unlockedAt is null to
guarantee only one active lock per release target.
| export const openapi: Swagger.SwaggerV3 = { | ||
| openapi: "3.0.0", | ||
| info: { | ||
| title: "Ctrlplane API", | ||
| version: "1.0.0", | ||
| }, | ||
| paths: { | ||
| "/v1/release-targets/{releaseTargetId}/unlock": { | ||
| post: { | ||
| summary: "Unlock a release target", | ||
| operationId: "unlockReleaseTarget", | ||
| parameters: [ | ||
| { | ||
| name: "releaseTargetId", | ||
| in: "path", | ||
| required: true, | ||
| schema: { type: "string", format: "uuid" }, | ||
| }, | ||
| ], | ||
| responses: { | ||
| 200: { | ||
| description: "Release target unlocked", | ||
| content: { | ||
| "application/json": { | ||
| schema: { | ||
| $ref: "#/components/schemas/ReleaseTargetLockRecord", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| 403: { | ||
| description: "Release target was not locked by the current user", | ||
| content: { | ||
| "application/json": { | ||
| schema: { | ||
| type: "object", | ||
| properties: { | ||
| error: { type: "string" }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| 400: { | ||
| description: "Release target is not locked", | ||
| content: { | ||
| "application/json": { | ||
| schema: { | ||
| type: "object", | ||
| properties: { | ||
| error: { type: "string" }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| 500: { | ||
| description: "Internal server error", | ||
| content: { | ||
| "application/json": { | ||
| schema: { | ||
| type: "object", | ||
| properties: { error: { type: "string" } }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Broken $ref – components.schemas section is missing
The spec references #/components/schemas/ReleaseTargetLockRecord, but the file defines no components block, making the OpenAPI document invalid in isolation and during bundling.
export const openapi: Swagger.SwaggerV3 = {
openapi: "3.0.0",
info: { … },
+ components: {
+ schemas: {
+ ReleaseTargetLockRecord: {
+ $ref: "../lock/openapi.ts#/components/schemas/ReleaseTargetLockRecord"
+ },
+ },
+ },
paths: {(or inline-copy the exact schema).
📝 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.
| export const openapi: Swagger.SwaggerV3 = { | |
| openapi: "3.0.0", | |
| info: { | |
| title: "Ctrlplane API", | |
| version: "1.0.0", | |
| }, | |
| paths: { | |
| "/v1/release-targets/{releaseTargetId}/unlock": { | |
| post: { | |
| summary: "Unlock a release target", | |
| operationId: "unlockReleaseTarget", | |
| parameters: [ | |
| { | |
| name: "releaseTargetId", | |
| in: "path", | |
| required: true, | |
| schema: { type: "string", format: "uuid" }, | |
| }, | |
| ], | |
| responses: { | |
| 200: { | |
| description: "Release target unlocked", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| $ref: "#/components/schemas/ReleaseTargetLockRecord", | |
| }, | |
| }, | |
| }, | |
| }, | |
| 403: { | |
| description: "Release target was not locked by the current user", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { type: "string" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 400: { | |
| description: "Release target is not locked", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { type: "string" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 500: { | |
| description: "Internal server error", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { error: { type: "string" } }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; | |
| export const openapi: Swagger.SwaggerV3 = { | |
| openapi: "3.0.0", | |
| info: { | |
| title: "Ctrlplane API", | |
| version: "1.0.0", | |
| }, | |
| components: { | |
| schemas: { | |
| ReleaseTargetLockRecord: { | |
| $ref: "../lock/openapi.ts#/components/schemas/ReleaseTargetLockRecord", | |
| }, | |
| }, | |
| }, | |
| paths: { | |
| "/v1/release-targets/{releaseTargetId}/unlock": { | |
| post: { | |
| summary: "Unlock a release target", | |
| operationId: "unlockReleaseTarget", | |
| parameters: [ | |
| { | |
| name: "releaseTargetId", | |
| in: "path", | |
| required: true, | |
| schema: { type: "string", format: "uuid" }, | |
| }, | |
| ], | |
| responses: { | |
| 200: { | |
| description: "Release target unlocked", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| $ref: "#/components/schemas/ReleaseTargetLockRecord", | |
| }, | |
| }, | |
| }, | |
| }, | |
| 403: { | |
| description: "Release target was not locked by the current user", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { type: "string" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 400: { | |
| description: "Release target is not locked", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { | |
| error: { type: "string" }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| 500: { | |
| description: "Internal server error", | |
| content: { | |
| "application/json": { | |
| schema: { | |
| type: "object", | |
| properties: { error: { type: "string" } }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts
lines 3 to 74, the OpenAPI spec references a schema ReleaseTargetLockRecord
under components.schemas, but the components block is missing entirely. To fix
this, add a components section with a schemas object defining
ReleaseTargetLockRecord according to its expected structure, or alternatively
inline the full schema definition directly where the $ref is used to ensure the
OpenAPI document is valid and self-contained.
There was a problem hiding this comment.
the ref exists in the lock one, and we merge the openapis into one so this is okay
There was a problem hiding this comment.
@adityachoudhari26 Got it! Thanks for clarifying your OpenAPI architecture. Since you merge all the individual OpenAPI specs into a single document, the $ref to ReleaseTargetLockRecord will be resolved during that merge process when the schema definition from the lock endpoint is included. That makes perfect sense for your modular approach.
/| /|
( :v: )
|(_)|
-"---"-
understanding nod
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#590
File: apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/unlock/openapi.ts:3-74
Timestamp: 2025-06-14T06:43:08.391Z
Learning: In the ctrlplane project, OpenAPI specifications are organized as separate files for each endpoint and then merged into a single OpenAPI document. Individual endpoint OpenAPI files can reference schemas using $ref even if the schema is not defined locally in that file, as long as the schema exists in another OpenAPI file that will be part of the merge process.
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.
| "/v1/release-targets/{releaseTargetId}/lock": { | ||
| "post": { | ||
| "summary": "Lock a release target", | ||
| "operationId": "lockReleaseTarget", | ||
| "parameters": [ | ||
| { | ||
| "name": "releaseTargetId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "Release target locked", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "409": { | ||
| "description": "Release target is already locked", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal server error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing security and tags for lock endpoint
The new /v1/release-targets/{releaseTargetId}/lock POST operation does not include a security definition (e.g., bearerAuth) or tags for grouping, unlike other protected endpoints.
Apply this diff to enforce authentication and group the operation:
"/v1/release-targets/{releaseTargetId}/lock": {
"post": {
"summary": "Lock a release target",
"operationId": "lockReleaseTarget",
+ "tags": ["ReleaseTargets"],
+ "security": [
+ { "bearerAuth": [] }
+ ],
"parameters": [
{
"name": "releaseTargetId",
"in": "path",
"required": true,
"schema": {
"type": "string",
"format": "uuid"
}
}
],
"responses": {
"200": { ... },
"409": { ... },
"500": { ... }
}
}
},📝 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.
| "/v1/release-targets/{releaseTargetId}/lock": { | |
| "post": { | |
| "summary": "Lock a release target", | |
| "operationId": "lockReleaseTarget", | |
| "parameters": [ | |
| { | |
| "name": "releaseTargetId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "description": "Release target locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "409": { | |
| "description": "Release target is already locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "/v1/release-targets/{releaseTargetId}/lock": { | |
| "post": { | |
| "summary": "Lock a release target", | |
| "operationId": "lockReleaseTarget", | |
| + "tags": ["ReleaseTargets"], | |
| + "security": [ | |
| + { "bearerAuth": [] } | |
| + ], | |
| "parameters": [ | |
| { | |
| "name": "releaseTargetId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "description": "Release target locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "409": { | |
| "description": "Release target is already locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In openapi.v1.json around lines 2942 to 3000, the POST operation for the
/v1/release-targets/{releaseTargetId}/lock endpoint is missing the security
definition and tags. To fix this, add a "security" field with the appropriate
authentication scheme (e.g., bearerAuth) and include a "tags" array to group the
operation consistently with other protected endpoints. This ensures the endpoint
requires authentication and is properly categorized in the API documentation.
| "/v1/release-targets/{releaseTargetId}/unlock": { | ||
| "post": { | ||
| "summary": "Unlock a release target", | ||
| "operationId": "unlockReleaseTarget", | ||
| "parameters": [ | ||
| { | ||
| "name": "releaseTargetId", | ||
| "in": "path", | ||
| "required": true, | ||
| "schema": { | ||
| "type": "string", | ||
| "format": "uuid" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "Release target unlocked", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "400": { | ||
| "description": "Release target is not locked", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "403": { | ||
| "description": "Release target was not locked by the current user", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "Internal server error", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "error": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing security and tags for unlock endpoint
The /v1/release-targets/{releaseTargetId}/unlock POST operation likewise omits a security section and tags.
Include authentication and grouping:
"/v1/release-targets/{releaseTargetId}/unlock": {
"post": {
"summary": "Unlock a release target",
"operationId": "unlockReleaseTarget",
+ "tags": ["ReleaseTargets"],
+ "security": [
+ { "bearerAuth": [] }
+ ],
"parameters": [ ... ],
"responses": {
"200": { ... },
"400": { ... },
"403": { ... },
"500": { ... }
}
}
},📝 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.
| "/v1/release-targets/{releaseTargetId}/unlock": { | |
| "post": { | |
| "summary": "Unlock a release target", | |
| "operationId": "unlockReleaseTarget", | |
| "parameters": [ | |
| { | |
| "name": "releaseTargetId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "description": "Release target unlocked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "400": { | |
| "description": "Release target is not locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Release target was not locked by the current user", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "/v1/release-targets/{releaseTargetId}/unlock": { | |
| "post": { | |
| "summary": "Unlock a release target", | |
| "operationId": "unlockReleaseTarget", | |
| "tags": ["ReleaseTargets"], | |
| "security": [ | |
| { "bearerAuth": [] } | |
| ], | |
| "parameters": [ | |
| { | |
| "name": "releaseTargetId", | |
| "in": "path", | |
| "required": true, | |
| "schema": { | |
| "type": "string", | |
| "format": "uuid" | |
| } | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "description": "Release target unlocked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "$ref": "#/components/schemas/ReleaseTargetLockRecord" | |
| } | |
| } | |
| } | |
| }, | |
| "400": { | |
| "description": "Release target is not locked", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "403": { | |
| "description": "Release target was not locked by the current user", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "500": { | |
| "description": "Internal server error", | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "error": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In openapi.v1.json around lines 3100 to 3173, the POST operation for the
/v1/release-targets/{releaseTargetId}/unlock endpoint is missing the security
and tags fields. Add a "security" section specifying the required authentication
method(s) and include a "tags" array to group this endpoint appropriately,
ensuring it aligns with other related endpoints for consistency and proper API
documentation.
| await page.waitForTimeout(5_000); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid brittle fixed waits – poll for the expected condition instead
Hard-coded page.waitForTimeout(5_000) sleeps make tests slower and flaky.
Use Playwright’s expect.poll, or query the API until the resource/release-target appears, e.g.:
await expect.poll(async () => {
const { data } = await api.GET("/v1/resources/{id}/release-targets", { … });
return data?.length;
}).toBe(1);This will speed the suite up and eliminate timing guesswork.
Also applies to: 87-88, 119-120, 179-180, 215-216, 280-281, 356-357
🤖 Prompt for AI Agents
In e2e/tests/api/release-target-lock.spec.ts around lines 41-42, replace the
fixed wait call page.waitForTimeout(5_000) with a polling mechanism using
Playwright's expect.poll to repeatedly check the API until the expected resource
or release-target appears. This involves querying the API endpoint in a loop and
asserting the desired condition (e.g., data length equals 1) instead of waiting
a fixed time. Apply the same change to the other specified line ranges (87-88,
119-120, 179-180, 215-216, 280-281, 356-357) to improve test reliability and
speed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
e2e/tests/api/release-target-lock.spec.ts (1)
41-42: Replace fixed sleeps with polling (duplicate of earlier review)
page.waitForTimeout(...)makes the suite slow and flaky. Please switch toexpect.poll(or similar polling against the API) as previously suggested.Also applies to: 87-88, 119-120, 179-180, 215-216, 280-281, 356-357, 403-404
🧹 Nitpick comments (2)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.ts (2)
18-32: Query can be simplified & pushed to the DBFetching the whole row only to re-evaluate timestamps in JS costs an extra network hop and leaves a tiny race window.
Consider a singleSELECT 1 FROM release_target_lock_record WHERE release_target_id = $1 AND (unlocked_at IS NULL OR unlocked_at > now()) LIMIT 1;and return a boolean.
Cleaner, faster, and no clock‐skew concerns.
40-53: Re-consider heavy row lock
SELECT … FOR UPDATEserialises all lock attempts on the release-target row and may cause 55P03 errors you later map to 409.
Since the migration adds a partial unique index, you can drop the row lock and attempt the insert first withonConflictDoNothing()(or handle23505). Throughput under load will improve and code becomes simpler.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.ts(1 hunks)e2e/tests/api/release-target-lock.spec.ts(1 hunks)packages/db/drizzle/0114_tiresome_korath.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/release.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/drizzle/meta/_journal.json
- packages/db/src/schema/release.ts
🧰 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/webservice/src/app/api/v1/release-targets/[releaseTargetId]/lock/route.tse2e/tests/api/release-target-lock.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/db/drizzle/0114_tiresome_korath.sql (1)
1-11: Schema looks solidColumns, FKs, and the partial unique index enforcing one active lock per target are correctly declared.
Summary by CodeRabbit