fix: throw Koa HttpError from ctx.assert instead of nested http-errors instance - #1992
Open
marceli1404 wants to merge 2 commits into
Open
fix: throw Koa HttpError from ctx.assert instead of nested http-errors instance#1992marceli1404 wants to merge 2 commits into
marceli1404 wants to merge 2 commits into
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 (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Reviewer's GuideWraps ctx.assert and its helper methods to rethrow assertion failures as Koa.HttpError instances using Koa’s http-errors v2 createError, and extends tests to enforce instanceof Koa.HttpError and behavior of all assertion helpers. Sequence diagram for ctx.assert rethrowing Koa.HttpErrorsequenceDiagram
participant Context
participant assert_wrapper as assert
participant httpAssert
participant createError
Context->>assert_wrapper: assert(condition, status, message, props)
assert_wrapper->>httpAssert: httpAssert(condition, status, message, props)
httpAssert-->>assert_wrapper: throw err with status
assert_wrapper->>assert_wrapper: [err.status is number]
assert_wrapper->>createError: createError(err.status, err.message, props)
createError-->>Context: HttpError instance
Context-->>Context: err instanceof HttpError
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1992 +/- ##
==========================================
- Coverage 99.90% 99.81% -0.10%
==========================================
Files 9 9
Lines 2109 2135 +26
==========================================
+ Hits 2107 2131 +24
- Misses 2 4 +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 1 issue, and left some high level feedback:
- The
rethrowAssertionErrorwrapper creates newHttpErrorinstances and discards the original stack; consider propagatingerr.stackor using thecauseoption so debugging still points to the original assertion location. - When copying properties from the original assertion error, you skip some standard fields but also any custom non-enumerable properties; if you rely on metadata from http-assert/http-errors, double-check whether additional fields (e.g.
typeorcode) should be explicitly preserved.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `rethrowAssertionError` wrapper creates new `HttpError` instances and discards the original stack; consider propagating `err.stack` or using the `cause` option so debugging still points to the original assertion location.
- When copying properties from the original assertion error, you skip some standard fields but also any custom non-enumerable properties; if you rely on metadata from http-assert/http-errors, double-check whether additional fields (e.g. `type` or `code`) should be explicitly preserved.
## Individual Comments
### Comment 1
<location path="lib/context.js" line_range="15-17" />
<code_context>
const Cookies = require('cookies')
+function rethrowAssertionError (fn) {
+ return function wrapped (...args) {
+ try {
+ fn(...args)
+ } catch (err) {
+ if (err.status && typeof err.status === 'number') {
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrapped assert functions don't preserve the original `this` or return value.
The wrapper invokes `fn(...args)` without preserving `this` or returning the result, which can change behavior for `httpAssert` functions that rely on context or return values. Please call `fn.apply(this, args)` and return its result so the wrapper preserves the original semantics while still translating errors.
</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
+15
to
+17
| return function wrapped (...args) { | ||
| try { | ||
| fn(...args) |
There was a problem hiding this comment.
issue (bug_risk): Wrapped assert functions don't preserve the original this or return value.
The wrapper invokes fn(...args) without preserving this or returning the result, which can change behavior for httpAssert functions that rely on context or return values. Please call fn.apply(this, args) and return its result so the wrapper preserves the original semantics while still translating errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/context/assert.test.js (1)
18-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover custom-property preservation.
The wrapper explicitly transfers permitted properties, but no test verifies that contract. Add a failing assertion with a custom property and assert it survives reconstruction.
Proposed test
+ it('should retain custom assertion properties', () => { + const ctx = context() + + assert.throws( + () => ctx.assert(false, 418, 'teapot', { requestId: 'test-request' }), + err => { + assert.ok(err instanceof Koa.HttpError) + assert.strictEqual(err.requestId, 'test-request') + return true + } + ) + })🤖 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__/context/assert.test.js` around lines 18 - 97, Add coverage to the relevant HttpError reconstruction test around the existing ctx.assert cases by assigning a permitted custom property to the thrown error and asserting that property remains after reconstruction. Ensure the test verifies custom-property preservation alongside the existing status, message, and expose assertions.
🤖 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.
Nitpick comments:
In `@__tests__/context/assert.test.js`:
- Around line 18-97: Add coverage to the relevant HttpError reconstruction test
around the existing ctx.assert cases by assigning a permitted custom property to
the thrown error and asserting that property remains after reconstruction.
Ensure the test verifies custom-property preservation alongside the existing
status, message, and expose assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae34afaa-c153-4817-a3a8-94de2c96aa3f
📒 Files selected for processing (2)
__tests__/context/assert.test.jslib/context.js
Author
|
@fengmk2 @dead-horse — friendly ping. This PR fixes #1925: |
Member
|
@marceli1404 I think we should change the document description is better than change the code. |
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
2 participants
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.assert\ delegates to \http-assert, which depends on \http-errors@~1.8.0\ (installed in its own
ode_modules). Since Koa exports \HttpError\ from its own \http-errors@^2.0.0\ dependency, errors thrown by \ctx.assert()\ did not satisfy \�rr instanceof Koa.HttpError.
This change wraps \http-assert\ to re-throw assertion failures using Koa's \createError\ (from \http-errors\ v2), so both \ctx.assert\ and all its helper methods (.ok, .equal, .strictEqual, etc.) throw proper \HttpError\ instances.
Test plan:
ode --test\ — 446 tests, 0 failures
pm run build\ — passes
pm run lint\ — passes
Summary by Sourcery
Ensure ctx.assert and its helper methods throw Koa.HttpError instances instead of errors from the nested http-assert/http-errors dependency.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests