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

fix gzip not sending response code for no content responses (404, 301/302 redirects etc) #2481

Merged
merged 1 commit into from Jul 16, 2023

Conversation

aldas
Copy link
Contributor

@aldas aldas commented Jul 16, 2023

This is fix for #2480

Response code is not written from buffered writer to the actual writer when handler sent no-content response - ala 404 or redirect

func main() {
	e := echo.New()

	e.Use(middleware.Gzip())

	e.GET("/404", func(ctx echo.Context) error {
		return ctx.NoContent(http.StatusNotFound)
	})

	e.GET("/redirect", func(ctx echo.Context) error {
		return ctx.Redirect(http.StatusTemporaryRedirect, "/login")
	})

	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		log.Fatal(err)
	}
}

Example output after fix:

x@x:~/$ curl -v --compressed "http://localhost:8080/404"
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /404 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.88.1
> Accept: */*
> Accept-Encoding: deflate, gzip, br, zstd
> 
< HTTP/1.1 404 Not Found
< Vary: Accept-Encoding
< Date: Sun, 16 Jul 2023 17:20:11 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact

x@x:~/$ curl -v --compressed "http://localhost:8080/redirect"
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /redirect HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.88.1
> Accept: */*
> Accept-Encoding: deflate, gzip, br, zstd
> 
< HTTP/1.1 307 Temporary Redirect
< Location: /login
< Vary: Accept-Encoding
< Date: Sun, 16 Jul 2023 17:20:16 GMT
< Content-Length: 0
< 
* Connection #0 to host localhost left intact

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (60af056) 92.81% compared to head (ad61e72) 92.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2481   +/-   ##
=======================================
  Coverage   92.81%   92.82%           
=======================================
  Files          39       39           
  Lines        4618     4624    +6     
=======================================
+ Hits         4286     4292    +6     
  Misses        241      241           
  Partials       91       91           
Impacted Files Coverage Δ
middleware/compress.go 86.99% <100.00%> (+0.66%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aldas aldas merged commit 130be07 into labstack:master Jul 16, 2023
13 checks passed
@aldas aldas mentioned this pull request Jul 16, 2023
@ganigeorgiev
Copy link

@aldas If not abandoned or there are no other plans, is it possible to sync the latest master fixes with the v5_alpha branch?

@aldas
Copy link
Contributor Author

aldas commented Jul 16, 2023

v5 is not fully abandoned but you really should not use v5 today. For example we have briefly discussed switching back to Context being struct which is not in v5 yet.

At the moment and I am stalling on v5 because I want slog to land in Go standard library (Go 1.21 I think).

Are there specific features that you need from v5 that v4 does not have? I could check if those are backportable for you.


TLDR: I will promise you to find time to merge things from master to v5_alpha next week (I am using weeks starting on monday :) ). But you really should not use that branch for production-worthy things.

@ganigeorgiev
Copy link

Thank you @aldas for the quick response.

There is no specific feature from v5 that v4 doesn't have and we need, but I'm using v5_alpha deliberately in one of my open source projects (github.com/pocketbase) with the thinking that this way it should minimize the breaking changes and eventual manual migration steps that will be required by my users because we expose the echo router directly (basically last year I assumed that v5 would have been released before the stable version of my project and that's why we use v5_alpha). In hindsight, maybe it wasn't a good idea to rely on alpha branch in the first place, but things are already public anyway, so I will wait a little longer to see in what direction the echo development will take before making changes to my project.

@aldas aldas deleted the fix_gzip_2480 branch July 22, 2023 15:45
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

Successfully merging this pull request may close these issues.

None yet

2 participants