Repository navigation
feat: add delegation status filter validation (#113) - #226
Mikkelnice wants to merge 1 commit into
Conversation
- Add DelegationStatusFilter type and DELEGATION_STATUS_FILTERS constant to Delegation.ts - Validate ?status= query param in listDelegationsHandler before DB query - Reject unsupported values with 400 VALIDATION_ERROR - Add route tests covering valid filters, invalid filters, and unfiltered listing
📝 WalkthroughWalkthroughAdds delegation status filtering to the list route. The gateway validates the optional ChangesDelegation status filter validation
Sequence Diagram(s)sequenceDiagram
participant Client
participant listDelegationsHandler
participant Delegation
Client->>listDelegationsHandler: GET /api/v1/delegations?status=...
listDelegationsHandler->>listDelegationsHandler: parse status from URL
listDelegationsHandler->>listDelegationsHandler: validate against DELEGATION_STATUS_FILTERS
alt unsupported status
listDelegationsHandler-->>Client: 400 VALIDATION_ERROR
else supported or absent
listDelegationsHandler->>Delegation: findAll({ userId, status? })
Delegation-->>listDelegationsHandler: delegations
listDelegationsHandler-->>Client: 200 data[]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/backend/gateway/src/models/Delegation.ts (1)
4-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the union type from the constant to prevent contract drift.
Defining the same status literals twice creates a maintenance footgun. Make the array the source of truth and infer the union type from it.
Proposed refactor
-export type DelegationStatusFilter = 'pending' | 'active' | 'paused' | 'revoked' | 'expired'; -export const DELEGATION_STATUS_FILTERS: readonly DelegationStatusFilter[] = ['pending', 'active', 'paused', 'revoked', 'expired']; +export const DELEGATION_STATUS_FILTERS = ['pending', 'active', 'paused', 'revoked', 'expired'] as const; +export type DelegationStatusFilter = (typeof DELEGATION_STATUS_FILTERS)[number];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/gateway/src/models/Delegation.ts` around lines 4 - 5, The Delegation status contract is duplicated between DelegationStatusFilter and DELEGATION_STATUS_FILTERS, so make the constant the source of truth and derive the union from it. Update DelegationStatusFilter in Delegation.ts to infer from the literal array (using the existing DELEGATION_STATUS_FILTERS symbol) so the type and list stay in sync when statuses change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/gateway/routes/delegations.ts`:
- Line 147: The request parsing in delegations.ts uses req.headers.host as the
base for new URL, which is client-controlled and can cause parsing failures.
Update the URL construction near the req.url handling to use a ثابت fixed base
origin instead of req.headers.host, since only the query string needs to be
parsed, and keep the logic in the route handler that reads the URL query
parameters intact.
In `@tests/unit/package.json`:
- Line 7: The unit test command in package.json relies on
--experimental-test-module-mocks, which is not supported on all currently
allowed Node 20 runtimes. Update the repository’s Node version requirement and
CI/test runtime floor to a version that supports this flag (Node 20.18+ or
22.3+), and make sure the engine constraint and any version checks stay
consistent so tests won’t run on 20.0–20.17. Refer to the test script entry and
any engine/version configuration used for Node gating.
In `@tests/unit/src/delegation-status-filter.test.js`:
- Around line 17-25: The delegation status filter tests only verify response
codes, so they can miss a regression in the query filter. Update the relevant
cases in delegation-status-filter.test.js to assert that Delegation.findAll is
called with the expected where clause, using userId for the unfiltered request
and userId plus status for the filtered request, so the behavior is checked
through the Delegation.findAll mock as well as the response.
---
Nitpick comments:
In `@apps/backend/gateway/src/models/Delegation.ts`:
- Around line 4-5: The Delegation status contract is duplicated between
DelegationStatusFilter and DELEGATION_STATUS_FILTERS, so make the constant the
source of truth and derive the union from it. Update DelegationStatusFilter in
Delegation.ts to infer from the literal array (using the existing
DELEGATION_STATUS_FILTERS symbol) so the type and list stay in sync when
statuses change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a7f8d2f-e85d-43fb-b28a-c1ae37c82275
📒 Files selected for processing (4)
apps/backend/gateway/routes/delegations.tsapps/backend/gateway/src/models/Delegation.tstests/unit/package.jsontests/unit/src/delegation-status-filter.test.js
| return; | ||
| } | ||
|
|
||
| const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%26quot%3B%2F%26quot%3B%2C%2520%60http%3A%2F%2F%24%7Breq.headers.host%2520%3F%3F%2520%26quot%3Blocalhost%26quot%3B%7D%60); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid using untrusted Host header as URL base in request parsing.
req.headers.host is client-controlled; malformed values can make new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2F...) throw and turn a bad query into a 500. Use a fixed base since only query parsing is needed.
Proposed fix
- const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%2522%2F%2522%2C%2520%60http%3A%2F%2F%24%7Breq.headers.host%2520%3F%3F%2520%2522localhost%2522%7D%60);
+ const url = new url(/sitelet?url=https%3A%2F%2Fgithub.com%2FDelegoLabs%2FDelego%2Fpull%2Freq.url%2520%3F%3F%2520%2522%2F%2522%2C%2520%2522http%3A%2F%2Flocalhost%2522);📝 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.
| const url = new URL(req.url ?? "/", `http://${req.headers.host ?? "localhost"}`); | |
| const url = new URL(req.url ?? "/", "http://localhost"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/gateway/routes/delegations.ts` at line 147, The request parsing
in delegations.ts uses req.headers.host as the base for new URL, which is
client-controlled and can cause parsing failures. Update the URL construction
near the req.url handling to use a ثابت fixed base origin instead of
req.headers.host, since only the query string needs to be parsed, and keep the
logic in the route handler that reads the URL query parameters intact.
| "type": "module", | ||
| "scripts": { | ||
| "test": "node --test", | ||
| "test": "node --test --experimental-test-module-mocks", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Node version =="
node -v || true
echo "== tests/unit/package.json script =="
cat tests/unit/package.json | sed -n '1,80p'
echo "== engine/toolchain hints =="
fd -i 'package.json|.nvmrc|.node-version|.tool-versions|Dockerfile|ci|workflow' . \
| sed -n '1,120p'Repository: DelegoLabs/Delego
Length of output: 1145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root package.json (engines/scripts) =="
cat package.json | sed -n '1,220p'
echo "== Node/toolchain files =="
fd -HI '(^\.nvmrc$|^\.node-version$|^\.tool-versions$|^Dockerfile$|^package-lock\.json$|^pnpm-lock\.yaml$|^yarn\.lock$)' . -t f
echo "== CI/workflow files =="
fd -HI '(\.github/workflows/.*\.ya?ml$|ci|workflow)' . -t f
echo "== search for node version references =="
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' -e 'node\s*:?v?(\d+)' -e 'actions/setup-node' -e 'setup-node' -e 'engines' -e '18\.' -e '20\.' -e '22\.' .github . 2>/dev/null || trueRepository: DelegoLabs/Delego
Length of output: 20920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .github/workflows/ci.yml =="
cat -n .github/workflows/ci.yml | sed -n '1,260p'Repository: DelegoLabs/Delego
Length of output: 5452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .github/workflows/ci.yml =="
cat -n .github/workflows/ci.yml | sed -n '1,260p'Repository: DelegoLabs/Delego
Length of output: 5452
🌐 Web query:
Node.js --experimental-test-module-mocks introduced in which version supported Node 20
💡 Result:
The --experimental-test-module-mocks flag was introduced to the Node.js test runner in version 22.0.0 [1][2]. While it was developed and merged into the main branch in May 2024, it is not supported in Node.js 20 [1][2]. The feature, which enables mock.module functionality in the node:test runner, remains an experimental feature in newer releases [3][4][5]. Because module mocking relies on customization hooks that are also experimental, there have been no plans to backport this feature to Node.js 20 [2]. Users attempting to use mock.module in Node.js 20 will find the property undefined [6].
Citations:
- 1: nodejs/node@7a67ecf161
- 2: test_runner: support module mocking nodejs/node#52848
- 3: https://nodejs.org/api/test.html
- 4: https://nodejs.org/docs/v22.11.0/api/test.html
- 5: doc: cover --experimental-test-module-mocks flag nodejs/node#55021
- 6: https://stackoverflow.com/questions/79004444/mock-module-is-undefined-in-node-js-v22-9-0-nodetest-module
🌐 Web query:
Node.js --experimental-test-module-mocks introduced in which version supported Node 20
💡 Result:
The --experimental-test-module-mocks flag was introduced in Node.js v22.3.0 [1][2][3]. While there were discussions regarding backporting this feature to Node.js 20.x, it was explicitly marked as "backport-blocked" in the Node.js repository [4], and the feature is not natively supported in Node.js 20 versions. To use module mocking via the node:test module, you must use a Node.js version that includes the feature, starting from v22.3.0 [5][1].
Citations:
- 1: nodejs/node@5e25c2a79a
- 2: https://bhdouglass.com/blog/nodejs-test-runner/
- 3: doc: cover --experimental-test-module-mocks flag nodejs/node#55021
- 4: test_runner: support module mocking nodejs/node#52848
- 5: https://nodejs.org/docs/v22.11.0/api/test.html
🌐 Web query:
site:nodejs.org --experimental-test-module-mocks Node.js release notes
💡 Result:
The --experimental-test-module-mocks command-line flag is a feature in Node.js that enables module mocking within the built-in test runner [1][2]. Key details regarding this flag: Purpose: It allows developers to mock the exports of ECMAScript modules (ESM), CommonJS modules (CJS), and Node.js built-in modules during testing [2][3]. Stability: This feature is currently marked as having a stability index of 1.0 (Early development) [1]. Availability: The flag was introduced in Node.js v22.3.0 and v20.18.0 [1]. Usage: To utilize module mocking, you must start the Node.js process with this flag enabled [2][3]. If your environment uses the Node.js Permission Model, using this feature also requires the --allow-worker flag [1]. Implementation: Within tests, module mocking is accessed via the mock.module method provided by the test runner [2][4][5]. It is recommended to perform imports of the module to be tested dynamically after the mock has been configured to ensure the mock is correctly applied [4][5]. For detailed usage, you can refer to the official Node.js documentation for the test runner module [6][2].
Citations:
- 1: https://nodejs.org/api/cli.html
- 2: https://nodejs.org/download/release/v23.6.1/docs/api/test.html
- 3: https://nodejs.org/docs/v22.11.0/api/test.html
- 4: https://nodejs.org/learn/test-runner/using-test-runner
- 5: https://nodejs.org/learn/test-runner/mocking
- 6: https://nodejs.org/api/test.html
🌐 Web query:
site:nodejs.org v20.18.0 experimental-test-module-mocks node:test
💡 Result:
In Node.js v20.18.0, the --experimental-test-module-mocks command-line flag was introduced to enable experimental module mocking support in the built-in test runner [1][2][3]. Key details regarding this feature include: Experimental Status: As of its introduction, this feature is classified as Early Development (stability index 1.0) [2][4]. Usage: To use this feature, you must explicitly pass the --experimental-test-module-mocks flag when running your Node.js process [2][5]. If you are also using the Node.js Permission Model, this feature requires that you additionally pass the --allow-worker flag [2]. Functionality: This flag enables the mock.module(specifier[, options]) API within the node:test module [4][3]. This API allows for mocking the exports of ECMAScript modules (ESM), CommonJS modules (CJS), and Node.js built-in modules [4]. Important Implementation Note: When mocking modules, the mock must be configured before the module being mocked is imported or required by the code under test [6][5]. Because of this, it is common practice to use dynamic import statements after the mock has been set up to ensure the mocked version of the module is loaded instead of the original [6][5]. The mock.module API supports several configuration options, including: - cache: A boolean indicating whether to cache the mock module (default is false) [4]. - defaultExport: An optional value to serve as the module's default export [4]. - namedExports: An optional object to define the module's named exports [4]. This feature was part of the Node.js v20.18.0 release, which included several related commits to support module detection and refactor the mocking implementation [3][7].
Citations:
- 1: https://nodejs.org/download/release/v20.18.0/docs/api/all.html
- 2: https://nodejs.org/api/cli.html
- 3: https://nodejs.org/en/blog/release/v20.18.0
- 4: https://nodejs.org/docs/latest-v20.x/api/test.html
- 5: https://nodejs.org/learn/test-runner/using-test-runner
- 6: https://nodejs.org/learn/test-runner/mocking
- 7: https://nodejs.org/es/blog/release/v20.18.0
🌐 Web query:
site:nodejs.org "experimental-test-module-mocks" "v20.18.0" "v22.3.0"
💡 Result:
The flag --experimental-test-module-mocks is a Node.js command-line option used to enable module mocking functionality within the built-in test runner [1][2]. This feature was added in Node.js version 22.3.0 and backported to version 20.18.0 [1]. When enabled, it allows developers to mock the exports of ECMAScript modules (ESM), CommonJS modules (CJS), JSON modules, and Node.js built-in modules during testing [3][2]. This is typically achieved using the mock.module method provided by the node:test module [3][2]. If you are using the Permission Model in Node.js, using this feature also requires passing the --allow-worker flag [1].
Citations:
- 1: https://nodejs.org/api/cli.html
- 2: https://r2.nodejs.org/docs/v22.17.1/api/test.html
- 3: https://nodejs.org/download/release/latest-v20.x/docs/api/test.html
Raise the Node floor for --experimental-test-module-mocks
The flag requires Node 20.18+ (or 22.3+), but the repo still advertises >=20.0.0. Tighten the engine/CI version so 20.0–20.17 runtimes don’t break the unit tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/package.json` at line 7, The unit test command in package.json
relies on --experimental-test-module-mocks, which is not supported on all
currently allowed Node 20 runtimes. Update the repository’s Node version
requirement and CI/test runtime floor to a version that supports this flag (Node
20.18+ or 22.3+), and make sure the engine constraint and any version checks
stay consistent so tests won’t run on 20.0–20.17. Refer to the test script entry
and any engine/version configuration used for Node gating.
| await mock.module(`${root}/apps/backend/gateway/dist/src/models/index.js`, { | ||
| namedExports: { | ||
| Delegation: { findAll: async () => [] }, | ||
| DelegationPolicy: {}, | ||
| SpendLimit: {}, | ||
| PermissionLevel: {}, | ||
| Wallet: {}, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add assertions that Delegation.findAll receives the expected where filter.
Right now the suite checks status codes only, so it would still pass if query filtering regressed. Assert where: { userId } for unfiltered and where: { userId, status } for filtered requests.
Also applies to: 62-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/src/delegation-status-filter.test.js` around lines 17 - 25, The
delegation status filter tests only verify response codes, so they can miss a
regression in the query filter. Update the relevant cases in
delegation-status-filter.test.js to assert that Delegation.findAll is called
with the expected where clause, using userId for the unfiltered request and
userId plus status for the filtered request, so the behavior is checked through
the Delegation.findAll mock as well as the response.
|
Resolve conflicts |
|
Will be closed as conflicts weren't resolved |
Summary
Validates the
?status=query parameter onGET /api/v1/delegationsbefore querying the database.Changes
apps/backend/gateway/src/models/Delegation.ts— addedDelegationStatusFiltertype andDELEGATION_STATUS_FILTERSconstantapps/backend/gateway/routes/delegations.ts—listDelegationsHandlernow rejects unsupported status values with400 VALIDATION_ERROR; valid values are passed through to the DB query; no-param behavior is unchangedtests/unit/src/delegation-status-filter.test.js— route tests: unfiltered listing (200), all 5 valid statuses (200), two invalid status values (400 VALIDATION_ERROR)tests/unit/package.json— added--experimental-test-module-mocksflag to support ESM module mocking in Node's built-in test runnerTesting
Closes #113
Summary by CodeRabbit
New Features
Bug Fixes
Tests