fix: dont double encode cel expression#959
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![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)
318-556: Avoid repeated non-null assertions in new response checks.The new assertions repeatedly use
listRes.data!. Prefer a small guard/helper once per response and then assert on the narrowed value to keep failures clearer and types safer.Based on learnings: In e2e test files, prefer explicit null safety checks and validation over non-null assertions (!). When validating API responses in tests, use type guards and throw descriptive errors rather than assuming values are always present.
🤖 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 318 - 556, The tests repeatedly use non-null assertions on listRes.data! which is brittle; change each test to capture and validate the response once (e.g., const res = listRes.response; const data = listRes.data; if (!data) throw new Error("Missing response data");) and then use data.items and data.total for subsequent assertions so you remove repeated `listRes.data!` usages and provide a clear guard and error message; update all occurrences in the tests that reference listRes.data! (including checks for items, total, and .items.some) to use the guarded `data` variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/api/resources.spec.ts`:
- Around line 287-574: The tests starting at the new resource-filter cases
create and delete entities inline using api.PUT/api.DELETE; replace that manual
setup/teardown by declaring the resources in a sibling .spec.yaml fixture and
loading them via importEntitiesFromYaml before the tests and removing them with
cleanupImportedEntities after (or in an afterEach/afterAll). Update each test
that references created identifiers/kinds/names (the blocks using api.PUT with
identifier variables like identifier, identifier1, identifier2 and kinds like
kind/Kind+Plus-*/Kind%20Space-*) to instead rely on the imported fixture entity
identifiers and ensure tests call importEntitiesFromYaml and
cleanupImportedEntities in the same spec to manage lifecycle. Ensure the YAML
fixture names correspond to the generated identifiers used by the tests or make
the tests read identifiers from the imported fixture metadata.
---
Nitpick comments:
In `@e2e/tests/api/resources.spec.ts`:
- Around line 318-556: The tests repeatedly use non-null assertions on
listRes.data! which is brittle; change each test to capture and validate the
response once (e.g., const res = listRes.response; const data = listRes.data; if
(!data) throw new Error("Missing response data");) and then use data.items and
data.total for subsequent assertions so you remove repeated `listRes.data!`
usages and provide a clear guard and error message; update all occurrences in
the tests that reference listRes.data! (including checks for items, total, and
.items.some) to use the guarded `data` variable instead.
🪄 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: 483bd987-4436-4006-a598-9e7e6ac70b0a
📒 Files selected for processing (2)
apps/api/src/routes/v1/workspaces/resources.tse2e/tests/api/resources.spec.ts
| test("should filter resources with CEL containing + character", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const suffix = faker.string.alphanumeric(8); | ||
| const identifier = `res-plus-${suffix}`; | ||
| const kind = `Kind+Plus-${suffix}`; | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| body: { | ||
| name: "Plus Resource", | ||
| kind, | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { cel: `resource.kind == "${kind}"` }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); | ||
|
|
||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test("should filter resources with CEL containing % character", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const identifier = `res-pct-${faker.string.alphanumeric(8)}`; | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| body: { | ||
| name: "100% Complete", | ||
| kind: "TestKind", | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { cel: 'resource.name == "100% Complete"' }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); | ||
|
|
||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test("should filter resources with CEL containing literal %20", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const suffix = faker.string.alphanumeric(8); | ||
| const identifier = `res-pct20-${suffix}`; | ||
| const kind = `Kind%20Space-${suffix}`; | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| body: { | ||
| name: "Pct20 Resource", | ||
| kind, | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { cel: `resource.kind == "${kind}"` }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); | ||
|
|
||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test("should filter resources with CEL containing & and = characters", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const suffix = faker.string.alphanumeric(8); | ||
| const identifier = `res-amp-${suffix}`; | ||
| const name = `a&b=c-${suffix}`; | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| body: { | ||
| name, | ||
| kind: "TestKind", | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { cel: `resource.name == "${name}"` }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); | ||
|
|
||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier }, | ||
| }, | ||
| }, | ||
| ); | ||
| }); | ||
|
|
||
| test("should return 400 for invalid CEL expression", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { cel: "this is not valid cel ===!!!" }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(400); | ||
| }); | ||
|
|
||
| test("should return empty results when CEL matches no resources", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { | ||
| cel: `resource.kind == "NonExistentKind-${faker.string.alphanumeric(16)}"`, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items).toHaveLength(0); | ||
| expect(listRes.data!.total).toBe(0); | ||
| }); | ||
|
|
||
| test("should filter resources with compound CEL expression", async ({ | ||
| api, | ||
| workspace, | ||
| }) => { | ||
| const identifier1 = `res-comp-a-${faker.string.alphanumeric(8)}`; | ||
| const identifier2 = `res-comp-b-${faker.string.alphanumeric(8)}`; | ||
| const kind = `CompoundKind-${faker.string.alphanumeric(6)}`; | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier: identifier1 }, | ||
| }, | ||
| body: { | ||
| name: "Compound Match", | ||
| kind, | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| await api.PUT( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier: identifier2 }, | ||
| }, | ||
| body: { | ||
| name: "Compound NoMatch", | ||
| kind, | ||
| version: "1.0.0", | ||
| config: {}, | ||
| metadata: {}, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| const listRes = await api.GET("/v1/workspaces/{workspaceId}/resources", { | ||
| params: { | ||
| path: { workspaceId: workspace.id }, | ||
| query: { | ||
| cel: `resource.kind == "${kind}" && resource.name == "Compound Match"`, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier1)).toBe( | ||
| true, | ||
| ); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier2)).toBe( | ||
| false, | ||
| ); | ||
|
|
||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier: identifier1 }, | ||
| }, | ||
| }, | ||
| ); | ||
| await api.DELETE( | ||
| "/v1/workspaces/{workspaceId}/resources/identifier/{identifier}", | ||
| { | ||
| params: { | ||
| path: { workspaceId: workspace.id, identifier: identifier2 }, | ||
| }, | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Use YAML-backed entity fixtures for these new E2E cases.
The added tests in Line 287 onward manually create/delete entities via API calls. For this e2e path, please define entities in a sibling .spec.yaml and use importEntitiesFromYaml / cleanupImportedEntities instead of inline setup/teardown.
As per coding guidelines: **/e2e/**/*.spec.ts: E2E tests use YAML fixture files (.spec.yaml alongside .spec.ts) to declare test entities; use importEntitiesFromYaml to load them and cleanupImportedEntities to 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 287 - 574, The tests starting
at the new resource-filter cases create and delete entities inline using
api.PUT/api.DELETE; replace that manual setup/teardown by declaring the
resources in a sibling .spec.yaml fixture and loading them via
importEntitiesFromYaml before the tests and removing them with
cleanupImportedEntities after (or in an afterEach/afterAll). Update each test
that references created identifiers/kinds/names (the blocks using api.PUT with
identifier variables like identifier, identifier1, identifier2 and kinds like
kind/Kind+Plus-*/Kind%20Space-*) to instead rely on the imported fixture entity
identifiers and ensure tests call importEntitiesFromYaml and
cleanupImportedEntities in the same spec to manage lifecycle. Ensure the YAML
fixture names correspond to the generated identifiers used by the tests or make
the tests read identifiers from the imported fixture metadata.
There was a problem hiding this comment.
Pull request overview
This PR fixes CEL query handling for the Resource list API by removing an extra decode step that could corrupt CEL expressions (notably around +, %, and literal percent-encoded sequences), and adds E2E coverage for those cases.
Changes:
- Stop manually URL-decoding the
celquery param inGET /v1/workspaces/{workspaceId}/resources. - Add E2E tests for CEL filtering when the expression contains
+,%, literal%20, and&/=characters. - Add E2E tests for invalid CEL (400), empty matches, and compound CEL expressions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| e2e/tests/api/resources.spec.ts | Adds E2E tests to ensure CEL filtering works with special characters and edge cases. |
| apps/api/src/routes/v1/workspaces/resources.ts | Removes the extra CEL decoding step and evaluates/validates the query param as-is. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
This assertion only checks that the matching resource is included. If the API accidentally ignores the cel query and returns all resources, this test would still pass. Consider also asserting total/items.length (e.g., equals 1) or creating a known non-matching resource and asserting it is excluded to ensure the filter is actually applied.
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
This test only verifies that the matching identifier is present, which wouldn’t fail if the server ignored the cel filter and returned all resources. Add an exclusion assertion (or assert total/items.length) to make sure the filter is applied, not just parseable.
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
This checks inclusion but not exclusion. To ensure the CEL filter is actually being enforced (and not ignored), consider asserting total/items.length or adding a second resource that should not match and asserting it’s absent.
| expect(listRes.response.status).toBe(200); | ||
| expect(listRes.data!.items.some((r) => r.identifier === identifier)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
This verifies the matching resource is included, but it won’t catch a regression where the API ignores cel and returns all resources. Add a negative assertion (non-matching resource not returned) or assert total/items.length to confirm filtering happens.
Resolves #935
Summary by CodeRabbit
Bug Fixes
+,%,&,=) without unintended URL-decoding transformations.Tests