fix: handle missing metadata keys in CEL resource filter#960
fix: handle missing metadata keys in CEL resource filter#960adityachoudhari26 merged 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughAdded try/catch error handling around CEL evaluation in resource listing to gracefully exclude resources on evaluation failure instead of propagating exceptions. Updated related e2e tests to better cover CEL metadata filtering scenarios with special characters and key presence validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
Updates resource listing CEL filtering to avoid request failures when selectors reference missing metadata keys, and adjusts E2E coverage to exercise metadata-based selectors and missing-key behavior.
Changes:
- Treat CEL evaluation errors during resource filtering as non-matches (skip the resource instead of failing the request).
- Update E2E tests to filter on
resource.metadata[...](including+,&,=) and add a missing-metadata-key scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| e2e/tests/api/resources.spec.ts | Shifts several CEL filter tests to metadata-based selectors and adds coverage for missing metadata keys. |
| apps/api/src/routes/v1/workspaces/resources.ts | Wraps cel-js evaluate() in a try/catch so evaluation failures don’t crash the list endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return evaluate(cel, { resource }); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The catch-all around evaluate() will silently treat any runtime evaluation error as a non-match. That prevents 500s for missing metadata keys, but it also masks unexpected server-side bugs or selectors that parse successfully but are not evaluable (e.g., unsupported functions/fields), returning a misleading 200/empty result. Consider narrowing this to the specific evaluation error(s) you expect for missing keys (or checking error type/message), and rethrow/500 (or 400) for other failures; at minimum, capture the error and add bounded logging/telemetry so issues aren’t silently hidden.
| @@ -401,20 +414,31 @@ test.describe("Resource API", () => { | |||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | |||
| params: { | |||
| path: { workspaceId: workspace.id }, | |||
| query: { cel: `resource.kind == "${kind}"` }, | |||
| query: { cel: 'resource.metadata["env"] == "production"' }, | |||
| }, | |||
There was a problem hiding this comment.
This test previously covered CEL filters containing a literal "%20" in the selector value, but it has been replaced with the missing-metadata-key scenario. There are no remaining "%20"-related assertions in this file, so the URL-decoding regression the prior test guarded against is no longer covered. If handling literal "%20" is still required, keep that case as a separate test (or add a new one) rather than overwriting it here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/tests/api/resources.spec.ts (1)
375-445: Use YAML fixtures for this new e2e case.This adds ad-hoc resource setup/teardown inside the test body instead of declaring the entities in the adjacent
.spec.yamland using the fixture helpers. Moving it there would keep the new regression aligned with the repo’s e2e conventions.As per coding guidelines, "E2E tests use YAML fixture files (
.spec.yamlalongside.spec.ts) to declare test entities; useimportEntitiesFromYamlto load them andcleanupImportedEntitiesto tear them down".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/api/resources.spec.ts` around lines 375 - 445, The test "should skip resources where CEL references a missing metadata key" currently creates and deletes resources inline; instead add those resources to the adjacent .spec.yaml fixture and load/teardown them using the e2e helpers: declare both resources in the .spec.yaml fixture file (matching identifiers used in the test), then in the .spec.ts replace the inline PUT/DELETE calls with importEntitiesFromYaml(...) at test setup (or beforeEach) and call cleanupImportedEntities(...) in teardown (or afterEach); remove the ad-hoc API calls and keep the list/expect assertions using the fixture identifiers so the test follows the repository's e2e fixture conventions.
🤖 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/resources.ts`:
- Around line 52-56: The catch in validResourceSelector currently swallows all
exceptions from evaluate(cel, { resource }) causing runtime missing-field errors
(e.g., resource.metadata.nonexistent) to be treated as false; change the handler
to only catch/handle missing-key/runtime-lookup errors (or rethrow unexpected
exceptions) and/or log them: specifically, in the function that calls
evaluate(cel, { resource }) (validResourceSelector), detect the error
type/message for missing metadata keys and return false for that case, but for
any other exception rethrow or log and propagate an error response so the
endpoint does not silently succeed.
---
Nitpick comments:
In `@e2e/tests/api/resources.spec.ts`:
- Around line 375-445: The test "should skip resources where CEL references a
missing metadata key" currently creates and deletes resources inline; instead
add those resources to the adjacent .spec.yaml fixture and load/teardown them
using the e2e helpers: declare both resources in the .spec.yaml fixture file
(matching identifiers used in the test), then in the .spec.ts replace the inline
PUT/DELETE calls with importEntitiesFromYaml(...) at test setup (or beforeEach)
and call cleanupImportedEntities(...) in teardown (or afterEach); remove the
ad-hoc API calls and keep the list/expect assertions using the fixture
identifiers so the test follows the repository's e2e fixture conventions.
🪄 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: 6c0c444d-b1e2-44c7-9b78-d85e3f9b9bc6
📒 Files selected for processing (2)
apps/api/src/routes/v1/workspaces/resources.tse2e/tests/api/resources.spec.ts
| try { | ||
| return evaluate(cel, { resource }); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect selector validation and the new catch block side by side.
sed -n '1,220p' apps/api/src/routes/v1/valid-selector.ts
printf '\n--- resources.ts ---\n'
sed -n '39,60p' apps/api/src/routes/v1/workspaces/resources.tsRepository: ctrlplanedev/ctrlplane
Length of output: 955
🏁 Script executed:
# Find the evaluate function definition and its documentation
rg -B 3 -A 10 "export.*evaluate|function evaluate" apps/api/src --type ts --type tsx
# Check for test cases around resource filtering and CEL evaluation
fd -e test.ts -e spec.ts apps/api/src | xargs rg -l "evaluate\|resources" | head -5
# Look for any comments or context around the try/catch addition
git log --oneline -S "catch {" apps/api/src/routes/v1/workspaces/resources.ts | head -3Repository: ctrlplanedev/ctrlplane
Length of output: 530
🏁 Script executed:
# Find evaluate function definition
fd . apps/api --full-path | xargs rg -l "export.*evaluate|function evaluate" | head -3
# Search for evaluate in the codebase more broadly
rg "evaluate\(" apps/api/src --type ts -B 2 -A 8 | head -50
# Check if there's any specific error handling for missing keys elsewhere
rg "missing.*key|key.*missing|undefined.*field" apps/api/src --type tsRepository: ctrlplanedev/ctrlplane
Length of output: 836
🏁 Script executed:
# Find where evaluate is imported from in resources.ts
head -40 apps/api/src/routes/v1/workspaces/resources.ts | grep -E "import|from"
# Find all files that export evaluate
rg "export.*evaluate" apps/ --type ts
# Find the evaluate function implementation
rg -B 5 -A 15 "const evaluate|function evaluate" apps/ --type tsRepository: ctrlplanedev/ctrlplane
Length of output: 533
🏁 Script executed:
# Look for tests involving evaluate or CEL resource filtering
fd . apps/api --full-path -name "*.test.ts" -o -name "*.spec.ts" | xargs rg "evaluate|CEL|resource.*filter" 2>/dev/null | head -20
# Check if evaluate is used elsewhere in the codebase with different error handling
rg "evaluate\(" apps/ --type ts -B 3 -A 3
# Check for any comments or documentation about CEL evaluation errors
rg "cel-js|evaluate" apps/api/src --type ts -B 2 -A 2 | grep -i "error\|exception\|missing\|undefined"
# Look at the full resources.ts file around the filter to understand context
sed -n '30,75p' apps/api/src/routes/v1/workspaces/resources.tsRepository: ctrlplanedev/ctrlplane
Length of output: 5747
🏁 Script executed:
# Find cel-js package info and version
rg "cel-js" apps/ packages/ --type json
# Look for any tests involving CEL evaluation errors
fd . apps/api --full-path -type f | xargs grep -l "evaluate" | xargs grep -l "test\|spec" | head -3
# Check workspace-engine for similar patterns (from learnings, there's CEL usage there)
rg "evaluate\|cel" apps/workspace-engine/pkg/workspace/store/relationship_indexes.go -B 2 -A 5
# Look for error types or error messages related to missing keys
rg "missing\|undefined\|field\|key" apps/api/src/routes/v1/workspaces/resources.tsRepository: ctrlplanedev/ctrlplane
Length of output: 502
🏁 Script executed:
# Search for tests of the resources endpoint
fd . apps/api --full-path -type f \( -name "*.test.ts" -o -name "*.spec.ts" \) | xargs rg -l "resources" 2>/dev/null | head -5
# Look for any logging or error reporting around evaluate
rg "console\|log\|error" apps/api/src/routes/v1/workspaces/resources.ts
# Check if there are any comments or documentation in resources.ts about error handling
head -100 apps/api/src/routes/v1/workspaces/resources.ts | grep -E "//|/\*"
# Search entire resources.ts file for context
cat apps/api/src/routes/v1/workspaces/resources.tsRepository: ctrlplanedev/ctrlplane
Length of output: 13718
Broaden scope or narrow error handling in CEL evaluation catch block.
The commit targets "missing metadata keys," but the current implementation catches all exceptions from evaluate(). Since validResourceSelector only validates CEL syntax (not runtime evaluation), expressions like resource.metadata.nonexistent == "value" will parse fine, fail at runtime when the field is missing, and silently return false instead of surfacing an error. The endpoint then returns 200 with partial or empty results instead of an error response. Either explicitly handle only missing-key errors, log unexpected failures, or update the PR scope.
🤖 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 52 - 56, The
catch in validResourceSelector currently swallows all exceptions from
evaluate(cel, { resource }) causing runtime missing-field errors (e.g.,
resource.metadata.nonexistent) to be treated as false; change the handler to
only catch/handle missing-key/runtime-lookup errors (or rethrow unexpected
exceptions) and/or log them: specifically, in the function that calls
evaluate(cel, { resource }) (validResourceSelector), detect the error
type/message for missing metadata keys and return false for that case, but for
any other exception rethrow or log and propagate an error response so the
endpoint does not silently succeed.
Resolves #958
Summary by CodeRabbit
Bug Fixes
Tests