Skip to content
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

Compression Middleware needs to check status code #382

Closed
RXminuS opened this issue Jan 11, 2019 · 11 comments
Closed

Compression Middleware needs to check status code #382

RXminuS opened this issue Jan 11, 2019 · 11 comments

Comments

@RXminuS
Copy link

RXminuS commented Jan 11, 2019

Right now the compression middleware compresses any response and subsequently writes a 200 status code. However if for instance a 400 is returned we want to not send a 200 status code down; other compression middlewares seem to not compress if this is the case.

@RXminuS
Copy link
Author

RXminuS commented Jan 11, 2019

@pkieltyka
Copy link
Member

@RXminuS good catch and thx for the report

RXminuS added a commit to RXminuS/chi that referenced this issue Jan 11, 2019
Status code apparently is important to determine whether the response data should be compressed. go-chi#382
@RXminuS
Copy link
Author

RXminuS commented Jan 11, 2019

@pkieltyka I posted a quick'n dirty fix :-)

@RXminuS
Copy link
Author

RXminuS commented Jan 11, 2019

...mmm actually, there seems to be something else going on as well :-/ I need to investigate more

@RXminuS
Copy link
Author

RXminuS commented Jan 11, 2019

AH! defer w.WriteHeader(code) doesn't work... defer w.ResponseWriter.WriteHeader(code) DOES work...don't quite understand why though, must be something that goes wrong in the encapsulation...

@RXminuS
Copy link
Author

RXminuS commented Jan 11, 2019

My best guess is because of the naming conflict (overloading the WriteHeader method)...similar to what they are discussing here https://stackoverflow.com/questions/38513779/extending-golangs-http-responsewriter-functionality-to-pre-post-process-respons

@AdamHepner
Copy link

Ohhh, this had me going chase my tail for days now!

If this is any consolation, here's a minimal reproduction: https://gist.github.com/AdamHepner/03b9addac10c47534bd3392cb0e069e1

@anurag
Copy link

anurag commented Jan 17, 2019

This issue will probably going to cause a lot of people a lot of pain if they upgrade to v4. Perhaps worth pointing out in the release notes until it's fixed?

@lrstanley
Copy link

Agreed -- this completely breaks redirects if I load the compression middleware.

@pkieltyka
Copy link
Member

fixed in 1a6bb10 -- thanks for the tip on the fix @RXminuS -- I'm not sure why this works, but I've tested it and resolves the issue

@VojtechVitek
Copy link
Contributor

Released in https://github.com/go-chi/chi/tree/v4.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants