Skip to content

fix: dont insert into resource_variable table if no variables set on resource #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adityachoudhari26 merged 2 commits into from
Mar 27, 2026
+39 −29
Merged

fix: dont insert into resource_variable table if no variables set on resource #880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8821c5e
fix: dont insert into resource_variable table if no variables set on …
adityachoudhari26 Mar 27, 2026
4818964
fix
adityachoudhari26 Mar 27, 2026
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
63 changes: 34 additions & 29 deletions apps/api/src/routes/v1/workspaces/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
import * as schema from "@ctrlplane/db/schema";

import { validResourceSelector } from "../valid-selector.js";
import { extractMessageFromError } from "./utils.js";

const listResources: AsyncTypedHandler<
"/v1/workspaces/{workspaceId}/resources",
Expand Down Expand Up @@ -150,12 +151,7 @@ const upsertResourceByIdentifier: AsyncTypedHandler<
} catch (error) {
res.status(500).json({
error: "Failed to upsert resource",
message:
error instanceof Error
? error.message
: typeof error === "string"
? error
: String(error),
message: extractMessageFromError(error),
});
}
};
Expand Down Expand Up @@ -220,31 +216,40 @@ const updateVariablesForResource: AsyncTypedHandler<
"/v1/workspaces/{workspaceId}/resources/identifier/{identifier}/variables",
"patch"
> = async (req, res) => {
const { workspaceId, identifier } = req.params;
const { body } = req;

const resource = await findResource(workspaceId, identifier);
const { id: resourceId } = resource;

await db.transaction(async (tx) => {
await tx
.delete(schema.resourceVariable)
.where(eq(schema.resourceVariable.resourceId, resource.id));
await tx.insert(schema.resourceVariable).values(
Object.entries(body).map(([key, value]) => ({
resourceId,
key,
value,
})),
);
});
try {
const { workspaceId, identifier } = req.params;
const { body } = req;

const resource = await findResource(workspaceId, identifier);
const { id: resourceId } = resource;

await db.transaction(async (tx) => {
await tx
.delete(schema.resourceVariable)
.where(eq(schema.resourceVariable.resourceId, resource.id));
const entries = Object.entries(body);
if (entries.length > 0)
await tx.insert(schema.resourceVariable).values(
entries.map(([key, value]) => ({
resourceId,
key,
value,
})),
);
});

enqueueReleaseTargetsForResource(db, workspaceId, resourceId);
enqueueReleaseTargetsForResource(db, workspaceId, resourceId);

res.status(202).json({
id: resource.id,
message: "Resource variables update requested",
});
res.status(202).json({
id: resource.id,
message: "Resource variables update requested",
});
} catch (error) {
res.status(error instanceof ApiError ? error.statusCode : 500).json({
error: "Failed to update resource variables",
message: extractMessageFromError(error),
});
}
Comment on lines +219 to +252
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

The conditional insert correctly fixes #878, but the try/catch swallows ApiError status codes.

The entries.length > 0 check correctly prevents the empty .values() call. However, wrapping findResource inside the try/catch causes ApiError exceptions (e.g., 404 Not Found) to be caught and returned as 500 errors.

Other handlers in this file (e.g., getVariablesForResource, deleteResourceByIdentifier) let ApiError propagate to asyncHandler which preserves the correct status code.

🛠️ Proposed fix: Re-throw ApiError to preserve status codes 
   } catch (error) {
+    if (error instanceof ApiError) throw error;
     res.status(500).json({
       error: "Failed to update resource variables",
       message: extractMessageFromError(error),
     });
   }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/api/src/routes/v1/workspaces/resources.ts` around lines 219 - 252, The
try/catch around the update-resource-variables handler is catching ApiError
(from findResource) and turning it into a 500; change the catch to re-throw
ApiError so its status code is preserved: inside the catch check if (error
instanceof ApiError) throw error; otherwise keep the existing
res.status(500).json({ error: "...", message: extractMessageFromError(error) });
this preserves behavior like other handlers (e.g., getVariablesForResource,
deleteResourceByIdentifier) while keeping enqueueReleaseTargetsForResource and
the transactional insert logic intact.

};

const parseSelector = (raw: string | null | undefined): string | undefined => {
Expand Down
5 changes: 5 additions & 0 deletions apps/api/src/routes/v1/workspaces/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export function extractMessageFromError(error: unknown): string {
if (error instanceof Error) return error.message;
if (typeof error === "string") return error;
return String(error);
}
Loading