Repository navigation
test: add comprehensive validate middleware unit tests - #689
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR modifies validation middleware and request schemas to enforce guild context alongside user identification in management and moderation endpoints. The validate middleware now strips unknown fields from request objects and reassigns validated data. Supporting unit and integration tests are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
…asesParam/userIdParam schemas - Assign validated query/params to req.query/req.params to strip unknown fields - Fix moderation.ts userCasesParam schema to include guildId - Fix management.ts userIdParam schema to include guildId - All 775 tests now pass
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/backend/src/middleware/validate.ts (1)
33-38: Strip-then-merge logic is correct; one caveat on non-object schemas.The in-place mutation pattern is the right approach — Express 5 exposes
req.queryas a getter, so reassignment would throw, whereas key-level deletion andObject.assignsafely update the existing object. Since Zod object schemas strip unknown keys by default,result.dataalready lacks unknowns, so the delete loop onreq.queryis what actually enforces stripping at the HTTP-layer.Minor robustness note:
key in result.dataassumesresult.datais an object. If a caller ever passes a schema whose output is a primitive (e.g. a.transform(...)producing a string/number) tovalidateQuery/validateParams, this line will throwTypeError: Cannot use 'in' operator. Consider a guard liketypeof result.data === 'object' && result.data !== nullbefore the cleanup loop, or tighten the generic constraint toSchema<Record<string, unknown>>for query/params variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/middleware/validate.ts` around lines 33 - 38, The delete-then-assign logic iterates keys on req.query and uses "key in result.data", which will throw if result.data is a non-object; update the cleanup to first guard that result.data is an object (e.g. check typeof result.data === "object" && result.data !== null) before running the Object.keys(req.query).forEach loop and Object.assign, or alternatively tighten the schema/generic for validateQuery/validateParams to only accept object-shaped outputs; locate the logic in validate.ts around req.query and result.data and apply the guard or type constraint accordingly.packages/backend/tests/unit/middleware/validate.test.ts (1)
67-86: Good coverage of the new mutate-and-strip behavior.Optional enhancement: add a case that exercises coercion (e.g.,
z.object({ limit: z.coerce.number() })with input{ limit: '10' }) and assertsreq.query.limit === 10. That captures the main user-visible benefit of writingresult.databack ontoreq.query(type-coerced values reaching the route handler), which the current tests don't directly verify since the schema here keepslimitas a string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/unit/middleware/validate.test.ts` around lines 67 - 86, Add a new unit test that verifies coercion by defining a schema that uses z.coerce.number() for limit (e.g., const coercingSchema = z.object({ limit: z.coerce.number().optional() })) and then call validateQuery(coercingSchema) with req.query { limit: '10' } (using the existing setup helper); assert next was called and that req.query.limit === 10 (a number), so the test exercises validateQuery's mutate-and-strip/coercion behavior rather than keeping the value as a string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/backend/src/middleware/validate.ts`:
- Around line 33-38: The delete-then-assign logic iterates keys on req.query and
uses "key in result.data", which will throw if result.data is a non-object;
update the cleanup to first guard that result.data is an object (e.g. check
typeof result.data === "object" && result.data !== null) before running the
Object.keys(req.query).forEach loop and Object.assign, or alternatively tighten
the schema/generic for validateQuery/validateParams to only accept object-shaped
outputs; locate the logic in validate.ts around req.query and result.data and
apply the guard or type constraint accordingly.
In `@packages/backend/tests/unit/middleware/validate.test.ts`:
- Around line 67-86: Add a new unit test that verifies coercion by defining a
schema that uses z.coerce.number() for limit (e.g., const coercingSchema =
z.object({ limit: z.coerce.number().optional() })) and then call
validateQuery(coercingSchema) with req.query { limit: '10' } (using the existing
setup helper); assert next was called and that req.query.limit === 10 (a
number), so the test exercises validateQuery's mutate-and-strip/coercion
behavior rather than keeping the value as a string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86818112-3206-4bb0-a2ee-a589a5667960
📒 Files selected for processing (5)
packages/backend/src/middleware/validate.tspackages/backend/src/schemas/management.tspackages/backend/src/schemas/moderation.tspackages/backend/tests/integration/services/redisCaching.test.tspackages/backend/tests/unit/middleware/validate.test.ts
💤 Files with no reviewable changes (1)
- packages/backend/tests/integration/services/redisCaching.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (4)
packages/backend/src/schemas/management.ts (1)
72-74: LGTM — schema now correctly requiresguildIdalongsideuserId.Matches the route pattern
/api/guilds/:guildId/logs/users/:userIdand keeps validation consistent with the new middleware stripping behavior.packages/backend/src/middleware/validate.ts (1)
54-59: Same pattern asvalidateQuery— LGTM.Duplicate of the query variant; see the note on line 33–38 regarding the
inoperator and non-object schema outputs. Not a blocker given all current call sites usez.object(...)schemas.packages/backend/src/schemas/moderation.ts (1)
12-14: LGTM — aligns with route/api/guilds/:guildId/moderation/users/:userId/cases.Validation now correctly covers both path params, and the handler already reads
req.params.guildIdandreq.params.userId(seepackages/backend/src/routes/moderation.ts:72-73), so no behavioral regression.packages/backend/tests/unit/middleware/validate.test.ts (1)
123-141: LGTM — parallels thevalidateQueryassertions.Tests correctly verify both assignment and unknown-field stripping for
req.params.
|
* test: add comprehensive validate middleware unit tests * fix(tests): resolve 2 failing validate middleware tests and fix userCasesParam/userIdParam schemas - Assign validated query/params to req.query/req.params to strip unknown fields - Fix moderation.ts userCasesParam schema to include guildId - Fix management.ts userIdParam schema to include guildId - All 775 tests now pass * test: fix validate middleware test assertions * test: fix validate middleware test assertions * fix: add type casts for Object.assign in validate middleware * fix: use object cast instead of any in validate middleware
This branch was successfully deployed
1 participant
Summary
Adds comprehensive unit test coverage for the validate middleware.
Changes
Test Coverage
Related
Extracted from #678 validation work (now focusing on tests-only).
Summary by CodeRabbit
Bug Fixes
Refactor
Tests