Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions apps/api/openapi/lib/openapi.libsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,17 @@
},
},

conflictResponse(description='Resource already exists'):: {
'409': {
description: description,
content: {
'application/json': {
schema: { '$ref': '#/components/schemas/ErrorResponse' },
},
},
},
},

internalServerError():: {
'500': {
description: 'Internal server error',
Expand Down
70 changes: 70 additions & 0 deletions apps/api/openapi/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -4542,6 +4542,26 @@
}
},
"description": "Accepted response"
},
"400": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Invalid request"
},
"409": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Deployment name already exists in this workspace"
}
},
"summary": "Create deployment"
Expand Down Expand Up @@ -4702,6 +4722,26 @@
}
},
"description": "Accepted response"
},
"400": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Invalid request"
},
"409": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Deployment name already exists in this workspace"
}
},
"summary": "Upsert deployment"
Expand Down Expand Up @@ -5297,6 +5337,26 @@
}
},
"description": "Accepted response"
},
"400": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Invalid request"
},
"409": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Environment name already exists in this workspace"
}
},
"summary": "Create environment"
Expand Down Expand Up @@ -5477,6 +5537,16 @@
}
},
"description": "Resource not found"
},
"409": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/ErrorResponse"
}
}
},
"description": "Environment name already exists in this workspace"
}
},
"summary": "Upsert environment"
Expand Down
8 changes: 6 additions & 2 deletions apps/api/openapi/paths/deployments.jsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ local openapi = import '../lib/openapi.libsonnet';
},
},
},
responses: openapi.acceptedResponse(openapi.schemaRef('DeploymentRequestAccepted')),
responses: openapi.acceptedResponse(openapi.schemaRef('DeploymentRequestAccepted'))
+ openapi.badRequestResponse()
+ openapi.conflictResponse('Deployment name already exists in this workspace'),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
},
'/v1/workspaces/{workspaceId}/deployments/{deploymentId}': {
Expand Down Expand Up @@ -57,7 +59,9 @@ local openapi = import '../lib/openapi.libsonnet';
},
},
},
responses: openapi.acceptedResponse(openapi.schemaRef('DeploymentRequestAccepted')),
responses: openapi.acceptedResponse(openapi.schemaRef('DeploymentRequestAccepted'))
+ openapi.badRequestResponse()
+ openapi.conflictResponse('Deployment name already exists in this workspace'),
},
delete: {
summary: 'Delete deployment',
Expand Down
7 changes: 5 additions & 2 deletions apps/api/openapi/paths/environments.jsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ local openapi = import '../lib/openapi.libsonnet';
},
},
},
responses: openapi.acceptedResponse(openapi.schemaRef('EnvironmentRequestAccepted')),
responses: openapi.acceptedResponse(openapi.schemaRef('EnvironmentRequestAccepted'))
+ openapi.badRequestResponse()
+ openapi.conflictResponse('Environment name already exists in this workspace'),
},
},
'/v1/workspaces/{workspaceId}/environments/{environmentId}': {
Expand Down Expand Up @@ -69,7 +71,8 @@ local openapi = import '../lib/openapi.libsonnet';
},
responses: openapi.acceptedResponse(openapi.schemaRef('EnvironmentRequestAccepted'))
+ openapi.notFoundResponse()
+ openapi.badRequestResponse(),
+ openapi.badRequestResponse()
+ openapi.conflictResponse('Environment name already exists in this workspace'),
},
},
}
80 changes: 50 additions & 30 deletions apps/api/src/routes/v1/workspaces/deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,16 +176,26 @@ const postDeployment: AsyncTypedHandler<

const id = uuidv4();

await db.insert(schema.deployment).values({
id,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
jobAgentSelector: body.jobAgentSelector ?? "false",
jobAgentConfig: body.jobAgentConfig ?? {},
metadata: body.metadata ?? {},
workspaceId,
});
try {
await db.insert(schema.deployment).values({
id,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
jobAgentSelector: body.jobAgentSelector ?? "false",
jobAgentConfig: body.jobAgentConfig ?? {},
metadata: body.metadata ?? {},
workspaceId,
});
} catch (error: any) {
if (error.code === "23505")
throw new ApiError(
"Deployment name already exists in this workspace",
409,
"DUPLICATE_NAME",
);
throw error;
}

enqueueReleaseTargetsForDeployment(db, workspaceId, id);

Expand All @@ -204,30 +214,40 @@ const upsertDeployment: AsyncTypedHandler<

const jobAgentConfig = body.jobAgentConfig ?? {};

await db
.insert(schema.deployment)
.values({
id: deploymentId,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
jobAgentSelector: body.jobAgentSelector ?? "false",
jobAgentConfig,
metadata: body.metadata ?? {},
workspaceId,
})
.onConflictDoUpdate({
target: schema.deployment.id,
set: {
try {
await db
.insert(schema.deployment)
.values({
id: deploymentId,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
jobAgentSelector: body.jobAgentSelector ?? "false",
jobAgentConfig,
metadata: body.metadata ?? {},
...(body.jobAgentSelector != null
? { jobAgentSelector: body.jobAgentSelector, jobAgentConfig }
: {}),
},
});
workspaceId,
})
.onConflictDoUpdate({
target: schema.deployment.id,
set: {
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
metadata: body.metadata ?? {},
...(body.jobAgentSelector != null
? { jobAgentSelector: body.jobAgentSelector, jobAgentConfig }
: {}),
},
Comment on lines +230 to +240
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This PUT can no longer clear or independently update job-agent settings.

The update branch only writes jobAgentSelector / jobAgentConfig when body.jobAgentSelector != null. That means an existing deployment keeps stale job-agent settings when the selector is omitted, and a request that only changes jobAgentConfig is silently ignored. Since the insert path still defaults omitted values to "false" / {}, the upsert behavior is now inconsistent and can preserve the wrong dispatch config.

Suggested fix
       .onConflictDoUpdate({
         target: schema.deployment.id,
         set: {
           name: body.name,
           description: body.description ?? "",
           resourceSelector: body.resourceSelector ?? "false",
+          jobAgentSelector: body.jobAgentSelector ?? "false",
+          jobAgentConfig,
           metadata: body.metadata ?? {},
-          ...(body.jobAgentSelector != null
-            ? { jobAgentSelector: body.jobAgentSelector, jobAgentConfig }
-            : {}),
         },
       });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/routes/v1/workspaces/deployments.ts` around lines 230 - 240, The
upsert in onConflictDoUpdate only writes jobAgentSelector/jobAgentConfig when
body.jobAgentSelector != null, which prevents clearing or updating job-agent
settings independently; change the logic to detect presence of request keys
(e.g., using Object.prototype.hasOwnProperty.call(body, 'jobAgentSelector') and
...call(body, 'jobAgentConfig')) and include those fields in the set payload
when present: if jobAgentSelector is present set jobAgentSelector to
body.jobAgentSelector ?? "false" (allow explicit null to clear), and if
jobAgentConfig is present set jobAgentConfig to body.jobAgentConfig ?? {} so
onConflictDoUpdate (target: schema.deployment.id) correctly updates or clears
job-agent settings even when only one of the two is provided.

});
} catch (error: any) {
if (error.code === "23505")
throw new ApiError(
"Deployment name already exists in this workspace",
409,
"DUPLICATE_NAME",
);
Comment on lines +243 to +248
throw error;
}

enqueueReleaseTargetsForDeployment(db, workspaceId, deploymentId);

Expand Down
60 changes: 40 additions & 20 deletions apps/api/src/routes/v1/workspaces/environments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { AsyncTypedHandler } from "@/types/api.js";
import { ApiError, asyncHandler } from "@/types/api.js";
import { Router } from "express";

import { and, count, eq } from "@ctrlplane/db";
import { and, count, eq, takeFirst } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
enqueueAllReleaseTargetsDesiredVersion,
Expand Down Expand Up @@ -129,7 +129,7 @@ const createEnvironment: AsyncTypedHandler<
const isValid = validResourceSelector(body.resourceSelector);
if (!isValid) throw new ApiError("Invalid resource selector", 400);

const [created] = await db
const created = await db
.insert(schema.environment)
.values({
name: body.name,
Expand All @@ -138,13 +138,23 @@ const createEnvironment: AsyncTypedHandler<
metadata: body.metadata ?? {},
workspaceId,
})
.returning();
.returning()
.then(takeFirst)
.catch((error: any) => {
if (error.code === "23505")
throw new ApiError(
"Environment name already exists in this workspace",
409,
"DUPLICATE_NAME",
);
throw error;
});

enqueueReleaseTargetsForEnvironment(db, workspaceId, created!.id);
enqueueReleaseTargetsForEnvironment(db, workspaceId, created.id);

res
.status(202)
.json({ id: created!.id, message: "Environment creation requested" });
.json({ id: created.id, message: "Environment creation requested" });
};

export const upsertEnvironmentById: AsyncTypedHandler<
Expand All @@ -157,25 +167,35 @@ export const upsertEnvironmentById: AsyncTypedHandler<
const isValid = validResourceSelector(body.resourceSelector);
if (!isValid) throw new ApiError("Invalid resource selector", 400);

await db
.insert(schema.environment)
.values({
id: environmentId,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
metadata: body.metadata ?? {},
workspaceId,
})
.onConflictDoUpdate({
target: schema.environment.id,
set: {
try {
await db
.insert(schema.environment)
.values({
id: environmentId,
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
metadata: body.metadata ?? {},
},
});
workspaceId,
})
.onConflictDoUpdate({
target: schema.environment.id,
set: {
name: body.name,
description: body.description ?? "",
resourceSelector: body.resourceSelector ?? "false",
metadata: body.metadata ?? {},
},
});
} catch (error: any) {
if (error.code === "23505")
throw new ApiError(
"Environment name already exists in this workspace",
409,
"DUPLICATE_NAME",
);
Comment on lines +191 to +196
throw error;
}

enqueueReleaseTargetsForEnvironment(db, workspaceId, environmentId);

Expand Down
Loading
Loading