Fix adapter.Unwrap() not working after huma.WithContext() #829
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue with adapter.Unwrap() after using huma.WithContext() by exposing the original context through a new API. Key changes include the addition of HumaContext and OriginalContext functions in api.go, and updating each adapter’s Unwrap method to use OriginalContext. The tests have also been updated to ensure that using huma.WithContext() does not cause any panics when unwrapping.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api.go | Added HumaContext and OriginalContext methods to extract the original context. |
| adapters/humamux/humamux.go | Updated Unwrap to call OriginalContext for correct type assertion. |
| adapters/humahttprouter/humahttprouter.go | Updated Unwrap using OriginalContext for httprouter adapter. |
| adapters/humago/humago.go | Updated Unwrap using OriginalContext for humago adapter. |
| adapters/humagin/humagin.go | Updated Unwrap using OriginalContext for gin adapter. |
| adapters/humaflow/humaflow.go | Updated Unwrap using OriginalContext for humaflow adapter. |
| adapters/humafiber/humafiber.go | Updated Unwrap using OriginalContext for humafiber adapter. |
| adapters/humaecho/humaecho.go | Updated Unwrap using OriginalContext for humaecho adapter. |
| adapters/humachi/humachi.go | Updated Unwrap using OriginalContext for chi adapter. |
| adapters/humabunrouter/humabunrouter.go | Updated Unwrap using OriginalContext (with separate handling in compat mode). |
| adapters/adapters_test.go | Modified test setup to include an unwrapper function and added a new humago test case. |
There was a problem hiding this comment.
Sorry for the drive-by review 🙈
Personally I'm not sure about increasing the package API for this.
Wouldn't extending the Context interface with Context.WithValue (requiring the individual adapters to implement their own self-wrapping) be a bit simpler for the consumer?
I actually thought about that, but I was worried about introducing a breaking change, both for avoiding annoyances to other lib users and to increase the likelyhood of a merge Since the Unwrap issue is currently affecting at least me and a couple of other people, I'd love to have some patch-level release in the near future, then a more well-thought solution in a later release I didn't look too far into such a solution, but if @danielgtaylor is OK with changing the Context interface without having to bump to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #829 +/- ##
==========================================
- Coverage 93.04% 93.01% -0.04%
==========================================
Files 23 23
Lines 5392 5394 +2
==========================================
Hits 5017 5017
- Misses 319 321 +2
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Okay, good feedback @costela and I agree for now we can do this without the new public function call. I've updated the code to mimic the standard library |
|
Interesting implementation, yeah, it works for me. Thank you! |
Closes #790
Closes #784
Since I was also affected by this issue I decided to open a PR
I used the (very naive) strategy of exposing a func to extract the original
huma.Contextfrom asubContext, and made use of it in everyadapter.Unwrap(ctx)func.I updated the adapters test to make sure all adapters don't panic while adding a simple context value (and added the missing
humagotest case)While updating the test I also discovered that
humabunrouter.Unwrap(ctx)simply doesn't work with Bun in compat mode. I didn't fix it since it's not really related to this change, and I wasn't sure on the preferred way (duplicated func? new package? return *http.Request for both context implementations?)