fix(api): coerce numeric query params, which the new global pipe rejected - #204
Merged
Conversation
…cted Installing the global ValidationPipe in #199 made `@IsNumber()` run against query strings for the first time. Query values are always strings, so `GET /v1/payments?limit=2` started coming back 400 with "limit must be a number conforming to the specified constraints". Every paginated list endpoint using PaymentFiltersDto was affected — page, limit, minAmount and maxAmount alike. It broke precisely because validation started working: before the pipe, nothing ran, and the string went through untouched. `@Type(() => Number)` converts the four query fields before validation. Done on the DTO rather than by enabling `enableImplicitConversion` globally, because that flag also coerces request bodies — and a body field that quietly accepts "5" for 5 is not what you want on the endpoints that move money. Found by exercising the running API while auditing leftover worktree branches; the existing e2e suites never passed a numeric query parameter, so nothing caught it. They do now. Tests: 287 unit, 18 e2e (3 new). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The regression
#199 installed a global
ValidationPipe. That made@IsNumber()run against query strings for the first time — and query values are always strings, so:Every paginated list endpoint using
PaymentFiltersDtowas affected:page,limit,minAmount,maxAmount.It broke because validation started working. Before the pipe, nothing ran and the string went through untouched, so this was invisible until the pipe made the constraint real.
The fix
@Type(() => Number)on the four query fields, converting before validation.Deliberately not
enableImplicitConversion: trueon the global pipe: that flag also coerces request bodies, and an amount field that quietly accepts"5"for5is not what you want on the endpoints that move money. Query DTOs are where the coercion is genuinely needed, so that is where it goes.Verified against the running API
?limit=2?limit=2&page=1?limit=abc?status=NOPEHow it was found
Auditing leftover worktree branches for unmerged work — one of them touched the payments list, so I exercised the endpoint directly. No existing e2e case passed a numeric query parameter, which is why CI stayed green through the break. Three cases added to close that gap.
Tests: 287 unit, 18 e2e (3 new).