Implement hijacker interface for Negroni integration

[ribice]

Resolves #837 (comment)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.81%. Comparing base (11d3c79) to head (e7df055).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   82.83%   82.81%   -0.03%     
==========================================
  Files          51       51              
  Lines        4597     4580      -17     
==========================================
- Hits         3808     3793      -15     
+ Misses        636      633       -3     
- Partials      153      154       +1     

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

[cleptric]

There are some breaking changes in here we need to call out on the changelog. Besides this, LGTM!

[ribice]

There are some breaking changes in here we need to call out on the changelog. Besides this, LGTM!

Are you referring to moving the WrapResponseWriter from sentryhttp to sentry?

That wasn't supposed to be exposed externally, so doubt anyone used it. To prevent a breaking change, we can keep it at sentryhttp as well (and just call the one at sentry internally)?

[cleptric]

It’s ok, we just need to call it out 🙂

[ribice]

@cleptric Perhaps we should move the Hijacker interface to /internal?

[aldy505]

Perhaps we should move the Hijacker interface to /internal?

@ribice I'd say you should move it to internal package, but keep the one at sentryhttp to avoid breaking change, then add DEPRECATION notice (the one that's according to godoc format).

The one on sentryhttp should be a passthrough to internal package. Then after 2-3 minor version release, you can safely remove it.

[kydemy-fran]

Hi there,
I reported the originar issue with negroni + websockets: the response writter does not implement http.Hijacker interface. #837 (comment)
Today we have upgraded from v0.27.0 to v0.29.0 and the error persists.

I've reviewed the code and I've found the issue:

sentry-go/internal/httputils/wrap_writer.go

Lines 35 to 57 in a6acd05

	func NewWrapResponseWriter(w http.ResponseWriter, protoMajor int) WrapResponseWriter {
	_, fl := w.(http.Flusher)
	bw := basicWriter{ResponseWriter: w, code: http.StatusOK}
	if protoMajor == 2 {
	_, ps := w.(http.Pusher)
	if fl && ps {
	return &http2FancyWriter{bw}
	}
	} else {
	_, hj := w.(http.Hijacker)
	_, rf := w.(io.ReaderFrom)
	if fl && hj && rf {
	return &httpFancyWriter{bw}
	}
	}
	if fl {
	return &flushWriter{bw}
	}
	return &bw
	}

The w is of type *negroni.responseWriter in negroni v1, which is the one referenced by the sentry-go package.
This type implements http.Flusher, http.Hijacker, but not io.ReaderFrom.
So fl and hj are true, but rf is not. So in line 48 does not return the httpFancyWriter.
And it returns the basic flushWriter in line 53. (which does not support hijacker / websockets.

We have had to rollback our releases today as this was failing.

As mentioned before, negroni v1 uses a limited writer. This is solved in negroni v3, which is easy to migrate to, but it would be nice if go-sentry was upgraded to v3 too, as v1 was released in 2018. Having our project requiring both versions is a nightmare.

From my point of view, if go-sentry imports negroni v1, then the library should work with v1 (and this is not happening at the moment). If you expect people to be using v3 (which works with go-sentry v0.29.0) then you should import negroni v3 in your go.mod.

Please let me know your thoughts

Thank you!

[cleptric]

@ribice can you take a look please?

[ribice]

Yes I can. Before I change the interface to support v1, I'm curious if we can update to v3? Is that an option?

[cleptric]

Fine with me.

[kydemy-fran]

Thank you for looking into this guys :)

[ribice]

Upgraded negroni to latest version in #885. This worked just fine: https://gist.github.com/ribice/95934b3c209b9692efc4b50b91d5dd4a