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
10 changes: 10 additions & 0 deletions apps/api/openapi/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -5006,6 +5006,16 @@
],
"type": "string"
}
},
{
"allowReserved": true,
"description": "CEL expression to filter the results",
"in": "query",
"name": "cel",
"required": false,
"schema": {
"type": "string"
}
}
],
"responses": {
Expand Down
1 change: 1 addition & 0 deletions apps/api/openapi/paths/deploymentversions.jsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ local openapi = import '../lib/openapi.libsonnet';
openapi.limitParam(),
openapi.offsetParam(),
openapi.orderParam(),
openapi.celParam(),
],
responses: openapi.paginatedResponse(openapi.schemaRef('DeploymentVersion'))
+ openapi.notFoundResponse()
Expand Down
69 changes: 54 additions & 15 deletions apps/api/src/routes/v1/workspaces/deployments.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { AsyncTypedHandler } from "@/types/api.js";
import { ApiError, asyncHandler } from "@/types/api.js";
import { evaluate } from "cel-js";
import { Router } from "express";
import { v4 as uuidv4 } from "uuid";

Expand Down Expand Up @@ -258,6 +259,19 @@ const deleteDeployment: AsyncTypedHandler<
.json({ id: deploymentId, message: "Deployment delete requested" });
};

function filterDeploymentVersions(
versions: (typeof schema.deploymentVersion.$inferSelect)[],
cel: string,
) {
return versions.filter((version) => {
try {
return evaluate(cel, { deploymentVersion: version });
} catch {
return false;
}
});
}
Comment on lines +262 to +273
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 | 🟡 Minor

Silent catch { return false } makes invalid CEL expressions indistinguishable from empty results.

validResourceSelector only validates parse-time syntax (via cel-js parse), not runtime evaluation. A syntactically valid expression that references an unknown field, mis-types a comparison, or otherwise throws at evaluate-time will cause every candidate to be dropped and the endpoint will return { items: [], total: 0 } with HTTP 200 — users will have no way to tell their expression is wrong.

Two options, either is fine:

  1. Log the evaluation error at least once per request so operators can diagnose, e.g.:
Proposed change
 function filterDeploymentVersions(
   versions: (typeof schema.deploymentVersion.$inferSelect)[],
   cel: string,
 ) {
+  let loggedError = false;
   return versions.filter((version) => {
     try {
       return evaluate(cel, { deploymentVersion: version });
-    } catch {
+    } catch (err) {
+      if (!loggedError) {
+        console.warn("CEL evaluation failed for deployment version", {
+          cel,
+          versionId: version.id,
+          error: err,
+        });
+        loggedError = true;
+      }
       return false;
     }
   });
 }
  1. Evaluate once up-front against a representative candidate and reject the whole request with a 400 if it throws, so the caller gets actionable feedback instead of an empty list.
📝 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.

Suggested change
function filterDeploymentVersions(
versions: (typeof schema.deploymentVersion.$inferSelect)[],
cel: string,
) {
return versions.filter((version) => {
try {
return evaluate(cel, { deploymentVersion: version });
} catch {
return false;
}
});
}
function filterDeploymentVersions(
versions: (typeof schema.deploymentVersion.$inferSelect)[],
cel: string,
) {
let loggedError = false;
return versions.filter((version) => {
try {
return evaluate(cel, { deploymentVersion: version });
} catch (err) {
if (!loggedError) {
console.warn("CEL evaluation failed for deployment version", {
cel,
versionId: version.id,
error: err,
});
loggedError = true;
}
return false;
}
});
}
🤖 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 262 - 273, The
current filterDeploymentVersions function swallows runtime CEL evaluation errors
(catch { return false }) making bad selectors return empty results; change this
by performing an up-front evaluation of the CEL expression (using evaluate)
against a representative candidate (e.g., the first element of versions) in the
request handling code (or inside filterDeploymentVersions) and if evaluate
throws, surface a 400 Bad Request (or throw an error that your route handler
converts to 400) with the evaluation error message; then update
filterDeploymentVersions to not silently swallow errors (remove the empty catch
so evaluation errors propagate) so valid evaluations filter normally while
invalid selectors return a clear error instead of an empty list.


const listDeploymentVersions: AsyncTypedHandler<
"/v1/workspaces/{workspaceId}/deployments/{deploymentId}/versions",
"get"
Expand All @@ -266,29 +280,54 @@ const listDeploymentVersions: AsyncTypedHandler<
const limit = req.query.limit ?? 50;
const offset = req.query.offset ?? 0;
const order = req.query.order ?? "desc";
const { cel } = req.query;

const orderBy =
order === "asc"
? asc(schema.deploymentVersion.createdAt)
: desc(schema.deploymentVersion.createdAt);

if (cel == null) {
const { total } = await db
.select({ total: count() })
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId))
.then(takeFirst);

const [countResult] = await db
.select({ total: count() })
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId));
const versions = await db
.select()
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId))
.orderBy(orderBy)
.limit(limit)
.offset(offset);

res.status(200).json({
items: versions.map(formatDeploymentVersion),
total,
limit,
offset,
});
return;
}

const total = countResult?.total ?? 0;
if (!validResourceSelector(cel))
throw new ApiError("Invalid CEL expression", 400);

const versions = await db
// CEL is evaluated in-memory, so cap the candidate set to bound cost.
// Filtering applies to the 1000 most-recent (or oldest, for asc) versions.
const candidates = await db
.select()
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId))
.orderBy(
order === "asc"
? asc(schema.deploymentVersion.createdAt)
: desc(schema.deploymentVersion.createdAt),
)
.limit(limit)
.offset(offset);
.orderBy(orderBy)
.limit(1000);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will this be sufficient? What if you looped pages of 1000 until you got enough results?


const filtered = filterDeploymentVersions(candidates, cel);

res.status(200).json({
items: versions.map(formatDeploymentVersion),
total,
items: filtered.slice(offset, offset + limit).map(formatDeploymentVersion),
total: filtered.length,
limit,
offset,
});
Comment on lines +317 to 333
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

Hard 1000-candidate cap silently truncates total and pagination.

When cel is supplied, candidates are capped at 1000 before filtering and total is reported as filtered.length. For any deployment with >1000 versions this is misleading in two ways:

  • total no longer reflects the real number of matches — it's bounded by how many of the top-1000 (by createdAt) happen to match. A client seeing total: 37 cannot tell whether there are genuinely 37 matches or 37 within the first 1000 of many thousands.
  • Clients paginating with offset/limit can never reach matches that live outside the 1000-candidate window, so the endpoint will silently omit results.

At minimum, surface the cap to the caller (e.g., an additional flag like truncated: true when candidates.length === 1000, or expose the candidate-window size separately from the match count) and document the behavior on the OpenAPI spec. Longer-term, consider pushing CEL → SQL translation for common predicates, or iterating in DB-ordered chunks until offset + limit post-filter results are found.

🤖 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 317 - 333, The
current flow fetches up to 1000 candidates into candidates (query on
schema.deploymentVersion), runs filterDeploymentVersions(candidates, cel) and
returns total: filtered.length which can mislead callers; change the response to
include a truncated indicator and the candidate window size so callers know
results were capped. Specifically, after fetching candidates, set a boolean
truncated = candidates.length === 1000 and include truncated (and optionally
candidateWindow: candidates.length) in the JSON response alongside items, total,
limit, offset; keep using filtered.length for total but surface the cap so
clients can detect truncation. Ensure you update the handler that constructs the
response (the block that calls filterDeploymentVersions and
res.status(200).json(...)) to add these fields and keep existing fields
unchanged.

Expand Down
2 changes: 2 additions & 0 deletions apps/api/src/types/openapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3285,6 +3285,8 @@ export interface operations {
offset?: number;
/** @description Sort order for results */
order?: "asc" | "desc";
/** @description CEL expression to filter the results */
cel?: string;
};
header?: never;
path: {
Expand Down
Loading