-
Notifications
You must be signed in to change notification settings - Fork 651
otelhttp: Remove custom wrapper after handling request #6914
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
a47db8c
to
25e2f1c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6914 +/- ##
=====================================
Coverage 75.5% 75.6%
=====================================
Files 207 207
Lines 19319 19362 +43
=====================================
+ Hits 14601 14651 +50
+ Misses 4283 4275 -8
- Partials 435 436 +1
🚀 New features to boost your workflow:
|
Could you add a changelog entry? |
6834b55
to
dc01b21
Compare
Should we do this every time we use the body wrapper? |
I wasn't sure in the beginning as I didn't know |
Could you do that in this PR? |
Yeah, it is done already :) |
|
||
// Restore the original body after the request is processed to avoid issues | ||
// with extra wrapper since `http/server.go` later checks type of `r.Body`. | ||
defer func() { r.Body = prevBody }() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is missing tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Turns out that we don't really need this code in gorilla/mux
as internally gorilla/mux
does the following:
...
if r.Match(req, &match) {
handler = match.Handler
req = requestWithVars(req, match.Vars)
req = requestWithRoute(req, match.Route)
}
...
func requestWithVars(r *http.Request, vars map[string]string) *http.Request {
ctx := context.WithValue(r.Context(), varsKey, vars)
return r.WithContext(ctx)
}
func requestWithRoute(r *http.Request, route *Route) *http.Request {
ctx := context.WithValue(r.Context(), routeKey, route)
return r.WithContext(ctx)
}
and r.WithContext(ctx)
does a shallow copy of the http.Request
so that the processed request is different than the original request and therefore r.Body
is a different pointer (but still pointing to the same memory). Thus, original r.Body
is left untouched.
But I would still leave this code just in case something changes in the future in gorilla/mux
and for consistency with net/http
code.
Signed-off-by: Janusz Marcinkiewicz <[email protected]>
Closes #6908