fix: throw koa http errors from ctx.assert#1977
Open
puneetdixit200 wants to merge 2 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes ctx.assert() to throw Koa.HttpError instances as documented. The implementation wraps http-assert calls through a new rethrowKoaHttpError helper that intercepts errors with numeric status codes and converts them into http-errors instances while preserving original error properties. The wrapped assert function and its method variants replace the direct httpAssert reference on the context prototype. Tests verify that both direct and method-based assertions throw the correct error type with expected status, message, and expose fields. sequenceDiagram
participant Context
participant rethrowKoaHttpError
participant httpAssert
participant createHttpError
Context->>rethrowKoaHttpError: Context.assert(...args)
rethrowKoaHttpError->>httpAssert: call underlying httpAssert method
httpAssert-->>rethrowKoaHttpError: throws Error(status, message)
rethrowKoaHttpError->>createHttpError: createHttpError(status, message)
createHttpError-->>rethrowKoaHttpError: Koa.HttpError instance
rethrowKoaHttpError-->>Context: throw Koa.HttpError (preserve props & stack)
🚥 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. Comment |
Reviewer's GuideWraps ctx.assert and its helper methods so that assertion failures thrown by http-assert are re-thrown as Koa’s own HttpError instances, and adds tests verifying that both direct assertions and helper assertions now produce Koa HttpError objects. Sequence diagram for ctx.assert rethrowing Koa HttpErrorsequenceDiagram
participant App
participant Context
participant rethrowKoaHttpError
participant httpAssert
participant createError
App->>Context: assert(false, 404, message)
Context->>rethrowKoaHttpError: rethrowKoaHttpError(httpAssert, args)
rethrowKoaHttpError->>httpAssert: httpAssert(false, 404, message)
httpAssert-->>rethrowKoaHttpError: throw err (from http-errors@1.x)
alt err has numeric status
rethrowKoaHttpError->>createError: createError(err.status, err.message, err)
createError-->>App: throw HttpError (from http-errors@2.x)
else other error
rethrowKoaHttpError-->>App: throw err
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1977 +/- ##
=======================================
Coverage 99.90% 99.90%
=======================================
Files 9 9
Lines 2109 2130 +21
=======================================
+ Hits 2107 2128 +21
Misses 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
rethrowKoaHttpErrorhelper currently wraps any thrown error that happens to have a numericstatusfield; consider narrowing this check (e.g., by also checking for a known marker property or constructor) so that arbitrary user errors with astatusfield are not silently converted intoKoa.HttpErrorinstances. - When re-wrapping the assertion error, the original stack trace is lost because
stackis non-enumerable and not copied byObject.assign; if preserving debuggability is important, consider explicitly copyingerr.stackonto the newHttpErrorinstance. - The assertion helper methods are re-exposed via arrow functions, which change metadata such as
nameandlengthcompared to the originalhttp-assertfunctions; if any downstream code relies on these, you may want to forward them in a way that preserves their observable shape (e.g., simple wrapper functions rather than arrow methods overObject.keys).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `rethrowKoaHttpError` helper currently wraps any thrown error that happens to have a numeric `status` field; consider narrowing this check (e.g., by also checking for a known marker property or constructor) so that arbitrary user errors with a `status` field are not silently converted into `Koa.HttpError` instances.
- When re-wrapping the assertion error, the original stack trace is lost because `stack` is non-enumerable and not copied by `Object.assign`; if preserving debuggability is important, consider explicitly copying `err.stack` onto the new `HttpError` instance.
- The assertion helper methods are re-exposed via arrow functions, which change metadata such as `name` and `length` compared to the original `http-assert` functions; if any downstream code relies on these, you may want to forward them in a way that preserves their observable shape (e.g., simple wrapper functions rather than arrow methods over `Object.keys`).
## Individual Comments
### Comment 1
<location path="lib/context.js" line_range="16-25" />
<code_context>
+ return fn(...args)
+ } catch (err) {
+ if (err && typeof err.status === 'number') {
+ throw createError(err.status, err.message, Object.assign({}, err))
+ }
+ throw err
</code_context>
<issue_to_address>
**suggestion:** Rewrapping errors like this may drop important non-enumerable properties such as the original stack.
`Object.assign({}, err)` only copies enumerable own properties, so non-enumerable fields like `stack` are lost, which makes debugging harder.
If you’re just normalizing errors with a `status`, consider either:
- Passing the original error as props and adjusting only what you need:
```js
const props = err instanceof Error ? err : { ...err }
throw createError(err.status, err.message, props)
```
- Preserving existing HTTP/createError instances and only wrapping non-HTTP errors:
```js
if (err && typeof err.status === 'number') {
if (err.expose || err instanceof Error) throw err
throw createError(err.status, err.message, { ...err })
}
```
This keeps the original error details while still normalizing behavior.
```suggestion
function rethrowKoaHttpError (fn, args) {
try {
return fn(...args)
} catch (err) {
if (err && typeof err.status === 'number') {
// Preserve existing HTTP/createError-style errors so we don't lose
// important non-enumerable properties like stack, expose, etc.
if (err.expose || err instanceof Error) {
throw err
}
// For non-HTTP-ish errors with a status, wrap them while copying
// enumerable properties for compatibility.
const props = err instanceof Error ? err : Object.assign({}, err)
throw createError(err.status, err.message, props)
}
throw err
}
}
```
</issue_to_address>
### Comment 2
<location path="__tests__/context/assert.test.js" line_range="25-34" />
<code_context>
assert(assertionRan)
})
+
+ it('should throw Koa HttpError instances', () => {
+ const ctx = context()
+
+ assert.throws(() => {
+ ctx.assert(false, 404, 'custom message')
+ }, err => {
+ assert.strictEqual(err instanceof Koa.HttpError, true)
+ assert.strictEqual(err.status, 404)
+ assert.strictEqual(err.message, 'custom message')
+ assert.strictEqual(err.expose, true)
+ return true
+ })
+ })
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `ctx.assert(false)` without an explicit status to cover the default 500 error case.
The new test only exercises `ctx.assert(false, 404, ...)`. Please add a sibling test for `ctx.assert(false)` (and/or `ctx.assert(false, 'message')`) that checks the thrown `Koa.HttpError` has the default 500 status and correct `expose` flag, so the non-explicit-status path is also covered for regressions.
</issue_to_address>
### Comment 3
<location path="__tests__/context/assert.test.js" line_range="39-51" />
<code_context>
+ })
+ })
+
+ it('should throw Koa HttpError instances from assertion helpers', () => {
+ const ctx = context()
+
+ assert.throws(() => {
+ ctx.assert.equal('actual', 'expected', 400, 'custom message')
+ }, err => {
+ assert.strictEqual(err instanceof Koa.HttpError, true)
+ assert.strictEqual(err.status, 400)
+ assert.strictEqual(err.message, 'custom message')
+ assert.strictEqual(err.expose, true)
+ return true
+ })
+ })
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for an additional assertion helper or the default-status path for helpers.
This currently exercises only `ctx.assert.equal` with an explicit 400 status. Since all `http-assert` helpers are wrapped, add a test that either uses a different helper (e.g. `ctx.assert.ok` / `ctx.assert.notEqual`) or omits the status argument, then assert the thrown error is still a `Koa.HttpError`. That will show the wrapper behaves correctly across helpers and argument shapes.
```suggestion
it('should throw Koa HttpError instances from assertion helpers', () => {
const ctx = context()
assert.throws(() => {
ctx.assert.equal('actual', 'expected', 400, 'custom message')
}, err => {
assert.strictEqual(err instanceof Koa.HttpError, true)
assert.strictEqual(err.status, 400)
assert.strictEqual(err.message, 'custom message')
assert.strictEqual(err.expose, true)
return true
})
})
it('should throw Koa HttpError instances from assertion helpers without explicit status', () => {
const ctx = context()
assert.throws(() => {
ctx.assert.ok(false, 'custom message without status')
}, err => {
assert.strictEqual(err instanceof Koa.HttpError, true)
assert.strictEqual(err.status, 500)
assert.strictEqual(err.message, 'custom message without status')
assert.strictEqual(err.expose, false)
return true
})
})
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+25
to
+34
| it('should throw Koa HttpError instances', () => { | ||
| const ctx = context() | ||
|
|
||
| assert.throws(() => { | ||
| ctx.assert(false, 404, 'custom message') | ||
| }, err => { | ||
| assert.strictEqual(err instanceof Koa.HttpError, true) | ||
| assert.strictEqual(err.status, 404) | ||
| assert.strictEqual(err.message, 'custom message') | ||
| assert.strictEqual(err.expose, true) |
There was a problem hiding this comment.
suggestion (testing): Add a test for ctx.assert(false) without an explicit status to cover the default 500 error case.
The new test only exercises ctx.assert(false, 404, ...). Please add a sibling test for ctx.assert(false) (and/or ctx.assert(false, 'message')) that checks the thrown Koa.HttpError has the default 500 status and correct expose flag, so the non-explicit-status path is also covered for regressions.
Comment on lines
+39
to
+51
| it('should throw Koa HttpError instances from assertion helpers', () => { | ||
| const ctx = context() | ||
|
|
||
| assert.throws(() => { | ||
| ctx.assert.equal('actual', 'expected', 400, 'custom message') | ||
| }, err => { | ||
| assert.strictEqual(err instanceof Koa.HttpError, true) | ||
| assert.strictEqual(err.status, 400) | ||
| assert.strictEqual(err.message, 'custom message') | ||
| assert.strictEqual(err.expose, true) | ||
| return true | ||
| }) | ||
| }) |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for an additional assertion helper or the default-status path for helpers.
This currently exercises only ctx.assert.equal with an explicit 400 status. Since all http-assert helpers are wrapped, add a test that either uses a different helper (e.g. ctx.assert.ok / ctx.assert.notEqual) or omits the status argument, then assert the thrown error is still a Koa.HttpError. That will show the wrapper behaves correctly across helpers and argument shapes.
Suggested change
| it('should throw Koa HttpError instances from assertion helpers', () => { | |
| const ctx = context() | |
| assert.throws(() => { | |
| ctx.assert.equal('actual', 'expected', 400, 'custom message') | |
| }, err => { | |
| assert.strictEqual(err instanceof Koa.HttpError, true) | |
| assert.strictEqual(err.status, 400) | |
| assert.strictEqual(err.message, 'custom message') | |
| assert.strictEqual(err.expose, true) | |
| return true | |
| }) | |
| }) | |
| it('should throw Koa HttpError instances from assertion helpers', () => { | |
| const ctx = context() | |
| assert.throws(() => { | |
| ctx.assert.equal('actual', 'expected', 400, 'custom message') | |
| }, err => { | |
| assert.strictEqual(err instanceof Koa.HttpError, true) | |
| assert.strictEqual(err.status, 400) | |
| assert.strictEqual(err.message, 'custom message') | |
| assert.strictEqual(err.expose, true) | |
| return true | |
| }) | |
| }) | |
| it('should throw Koa HttpError instances from assertion helpers without explicit status', () => { | |
| const ctx = context() | |
| assert.throws(() => { | |
| ctx.assert.ok(false, 'custom message without status') | |
| }, err => { | |
| assert.strictEqual(err instanceof Koa.HttpError, true) | |
| assert.strictEqual(err.status, 500) | |
| assert.strictEqual(err.message, 'custom message without status') | |
| assert.strictEqual(err.expose, false) | |
| return true | |
| }) | |
| }) |
![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
🤖 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 `@lib/context.js`:
- Around line 20-21: When recreating HTTP errors in rethrowKoaHttpError (the
block calling createError with Object.assign({}, err)), include the original
error.stack so the original stack trace is preserved; update the code that
builds the options for createError to copy err's enumerable properties and
explicitly set stack: err.stack (or otherwise ensure the created error's stack
is the original err.stack) while keeping status and message intact.
🪄 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: 8ccfb998-7a0d-4d7f-8893-4e36220eaafa
📒 Files selected for processing (2)
__tests__/context/assert.test.jslib/context.js
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.
Fixes #1925.
ctx.assertdelegates tohttp-assert, which currently throws errors created byhttp-assert's nestedhttp-errors@1.xdependency. Since Koa exportsHttpErrorfrom its ownhttp-errors@2.xdependency, errors thrown byctx.assert()do not satisfyerr instanceof Koa.HttpError.This keeps
http-assert's assertion behavior and helper methods, but converts thrown assertion errors through Koa'shttp-errorsdependency so direct assertions and helper assertions return KoaHttpErrorinstances.Verification:
node --test __tests__/context/assert.test.jsfailed onerr instanceof Koa.HttpErrorforctx.assert(false, 404, ...).node --test __tests__/context/assert.test.jsnpm run buildnpm run lintgit diff --checknpm test(442 tests, 0 failures)Summary by Sourcery
Ensure context assertions throw Koa HttpError instances instead of raw http-assert errors.
Enhancements:
Tests:
Summary by CodeRabbit
Tests
Bug Fixes