feat: version list endpoint can filter with CEL expression#1047
feat: version list endpoint can filter with CEL expression#1047adityachoudhari26 merged 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughAdds CEL expression filtering support to the deployment versions list endpoint. When a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a cel query parameter to the deployment versions listing endpoint to support filtering results via a CEL expression (fixes #1046).
Changes:
- Adds
celas an optional query parameter forGET /v1/workspaces/{workspaceId}/deployments/{deploymentId}/versionsin OpenAPI types/spec. - Implements server-side CEL validation and in-memory filtering of deployment versions using
cel-js. - Updates the generated OpenAPI JSON output to include the new parameter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/src/types/openapi.ts | Adds cel query param typing for listDeploymentVersions. |
| apps/api/src/routes/v1/workspaces/deployments.ts | Validates and applies CEL filtering to deployment versions; changes pagination behavior. |
| apps/api/openapi/paths/deploymentversions.jsonnet | Documents the new cel query parameter for the endpoint. |
| apps/api/openapi/openapi.json | Includes the generated OpenAPI parameter definition for cel. |
Comments suppressed due to low confidence (1)
apps/api/src/routes/v1/workspaces/deployments.ts:292
- The query is only filtered by
deploymentIdand does not verify that the deployment belongs to the{workspaceId}in the path. This can allow cross-workspace access if a caller knows a deploymentId. Consider joiningdeploymentVersiontodeploymentand addingdeployment.workspaceId = workspaceId(similar toapps/api/src/routes/v1/workspaces/deployment-versions.ts:18-30).
const allVersions = await db
.select()
.from(schema.deploymentVersion)
.where(eq(schema.deploymentVersion.deploymentId, deploymentId))
.orderBy(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -283,11 +294,16 @@ const listDeploymentVersions: AsyncTypedHandler< | |||
| ? asc(schema.deploymentVersion.createdAt) | |||
| : desc(schema.deploymentVersion.createdAt), | |||
| ) | |||
| .limit(limit) | |||
| .offset(offset); | |||
| .limit(1000); | |||
|
|
|||
There was a problem hiding this comment.
.limit(1000) hard-caps the candidate set, so pagination and total become incorrect whenever a deployment has >1000 versions (and offset > 999 will always return an empty page). If this limit is meant as a safety cap, it should be enforced via API validation and reflected in the response/error; otherwise, fetch all versions for accurate totals or implement chunked fetching until offset + limit items are collected and the full filtered total is known.
| const filtered = | ||
| cel != null ? filterDeploymentVersions(allVersions, cel) : allVersions; | ||
|
|
||
| const total = filtered.length; | ||
| const items = filtered.slice(offset, offset + limit); |
There was a problem hiding this comment.
When cel is not provided, the handler still loads up to 1000 rows and paginates in memory. This ignores the requested limit/offset at the database level and can add unnecessary DB/network load compared to applying .limit(limit).offset(offset) in the query for the non-CEL case.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/src/routes/v1/workspaces/deployments.ts (1)
266-268: CEL receives raw row withDateobject instead of the formatted shape.
versionhere isschema.deploymentVersion.$inferSelect, sodeploymentVersion.createdAtis a JSDateandmessageisstring | null(rather than the API'sstring | undefined). CEL comparisons likedeploymentVersion.createdAt > "2026-01-01"or string ops onmessagewill silently fail (caught by thereturn falseabove) and give users counter-intuitive results versus what they see in the response body.Consider evaluating against the same shape the API returns so the CEL contract matches the documented response:
- return evaluate(cel, { deploymentVersion: version }); + return evaluate(cel, { deploymentVersion: formatDeploymentVersion(version) });This also documents implicitly (via
formatDeploymentVersion) which fields are queryable. Worth calling out the available fields in the OpenAPI description for thecelparam as well.🤖 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 266 - 268, The filter currently calls evaluate(cel, { deploymentVersion: version }) with the DB row (Date and nullable message) which breaks CEL expectations; instead pass the API-shaped object produced by formatDeploymentVersion so fields like createdAt are formatted and message becomes the API's optional string. Replace the evaluate call in the versions.filter block to use formatDeploymentVersion(version) (or a small adapter that also converts message null→undefined) and update the CEL param OpenAPI description to list which fields are queryable to match formatDeploymentVersion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In `@apps/api/src/routes/v1/workspaces/deployments.ts`:
- Around line 266-268: The filter currently calls evaluate(cel, {
deploymentVersion: version }) with the DB row (Date and nullable message) which
breaks CEL expectations; instead pass the API-shaped object produced by
formatDeploymentVersion so fields like createdAt are formatted and message
becomes the API's optional string. Replace the evaluate call in the
versions.filter block to use formatDeploymentVersion(version) (or a small
adapter that also converts message null→undefined) and update the CEL param
OpenAPI description to list which fields are queryable to match
formatDeploymentVersion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b361bc37-5548-4b13-9648-a55d087a5728
📒 Files selected for processing (4)
apps/api/openapi/openapi.jsonapps/api/openapi/paths/deploymentversions.jsonnetapps/api/src/routes/v1/workspaces/deployments.tsapps/api/src/types/openapi.ts
| function filterDeploymentVersions( | ||
| versions: (typeof schema.deploymentVersion.$inferSelect)[], | ||
| cel: string, | ||
| ) { | ||
| return versions.filter((version) => { | ||
| try { | ||
| return evaluate(cel, { deploymentVersion: version }); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
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:
- 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;
}
});
}- 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.
| 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.
| // 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); | ||
|
|
||
| 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, | ||
| }); |
There was a problem hiding this comment.
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:
totalno longer reflects the real number of matches — it's bounded by how many of the top-1000 (by createdAt) happen to match. A client seeingtotal: 37cannot tell whether there are genuinely 37 matches or 37 within the first 1000 of many thousands.- Clients paginating with
offset/limitcan 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.
| .limit(limit) | ||
| .offset(offset); | ||
| .orderBy(orderBy) | ||
| .limit(1000); |
There was a problem hiding this comment.
Will this be sufficient? What if you looped pages of 1000 until you got enough results?
fixes #1046
Summary by CodeRabbit