Skip to content

fix(adapter): improve error propagation in Express-style handler#4250

Merged
ReneWerner87 merged 2 commits into
mainfrom
fix/adapter-case5-next-error-propagation
May 4, 2026
Merged

fix(adapter): improve error propagation in Express-style handler#4250
ReneWerner87 merged 2 commits into
mainfrom
fix/adapter-case5-next-error-propagation

Conversation

@ReneWerner87
Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings May 4, 2026 07:29
@ReneWerner87 ReneWerner87 requested a review from a team as a code owner May 4, 2026 07:29
@ReneWerner87 ReneWerner87 requested review from efectn, gaby and sixcolors May 4, 2026 07:29
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d187018f-186c-4a19-80b6-7a82dc56289c

📥 Commits

Reviewing files that changed from the base of the PR and between e0cac19 and 12acb9e.

📒 Files selected for processing (1)
  • adapter_test.go
✅ Files skipped from review due to trivial changes (1)
  • adapter_test.go

Walkthrough

The Express-style three-parameter handler adapter (func(Req, Res, func() error) error) now captures the result of c.Next() into a local nextErr and returns the handler's error if non-nil; otherwise it returns nextErr. Two tests were added to verify both precedence and propagation behavior.

Changes

Express Handler Error Propagation

Layer / File(s) Summary
Core Logic
adapter.go
In the Express-style (Req, Res, func() error) adapter, the provided next callback assigns c.Next() to a local nextErr and returns it. The adapter returns the handler's error immediately if non-nil; if the handler returns nil, the adapter returns nextErr.
Tests
adapter_test.go
Added two tests: one asserting the handler's returned error is preferred over a next() error (while still invoking next()), and one asserting a next() error is propagated when the handler returns nil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • sixcolors
  • efectn

Poem

🐰 I hopped through middleware, careful and spry,
I caught the next() whisper before it could fly,
If the handler shouts, I hand that reply,
Else I pass along what next lets slip by. 🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is completely missing. The repository requires a detailed description covering changes, type of change, and a comprehensive checklist. Add a complete pull request description following the template, including details of the changes, type of change classification, and completion of the required checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: improving error propagation in Express-style handler, which matches the code changes in adapter.go.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/adapter-case5-next-error-propagation

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes error propagation for Express-style handlers adapted through toFiberHandler, ensuring Fiber returns the handler's own error when both the handler and next() fail. It keeps adapter behavior consistent with the other Express-style adapter variants.

Changes:

  • Update the func(Req, Res, func() error) error adapter path to capture next() errors separately and only return them when the handler itself returns nil.
  • Add regression tests covering handler-error precedence over next() errors.
  • Add regression tests covering propagation of next() errors when the handler returns nil.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
adapter.go Adjusts Express-style adapter error propagation for the error-returning next variant.
adapter_test.go Adds tests validating handler-vs-next error precedence and next error propagation.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.25%. Comparing base (3e610ae) to head (12acb9e).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4250      +/-   ##
==========================================
+ Coverage   91.20%   91.25%   +0.05%     
==========================================
  Files         127      127              
  Lines       12551    12556       +5     
==========================================
+ Hits        11447    11458      +11     
+ Misses        692      687       -5     
+ Partials      412      411       -1     
Flag Coverage Δ
unittests 91.25% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the adaptExpressHandler function in adapter.go to correctly handle error propagation for Express-style handlers. It ensures that errors from the next callback are returned if the handler itself returns nil, while prioritizing errors returned by the handler if both occur. Two new test cases have been added to adapter_test.go to validate these scenarios. I have no feedback to provide.

@ReneWerner87 ReneWerner87 merged commit 6bf3888 into main May 4, 2026
21 checks passed
@ReneWerner87 ReneWerner87 deleted the fix/adapter-case5-next-error-propagation branch May 4, 2026 07:45
@github-project-automation github-project-automation Bot moved this to Done in v3 May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants