wip: feat: add gzip and brotli middleware #232

mauri870 · 2023-07-04T21:39:32Z

goravel/goravel#208

Closes goravel/goravel#208

📑 Description

✅ Checks

My pull request adheres to the code style of this project
My code requires changes to the documentation
I have updated the documentation as required
All the tests have passed

ℹ Additional Information

What is missing:

Update docs, add gzip and brotli to Routing
Create config files for gzip and brotli

Thx @Marian0 for the help getting gzip compression to work ✌️

mauri870 · 2023-07-04T21:43:15Z

@hwbrzzl One thing to take into consideration is that the brotli middleware I choose does not handle fallback to gzip. We can't also chain .GlobalMiddleware(middlewares.Brotli(), middlewares.Gzip()) since we'll end up with a brotli compressed gzip payload (or a gzip compressed brotli payload xD).

My take on it is that we should aim for a global Compress middleware with a respective compress.go config file where you can adjust the options and also enable brotli, gzip, deflate or maybe all of them at once. That would make things more easier than maintaining multiple middlewares. Please let me know what you think about it.

hwbrzzl · 2023-07-05T00:12:40Z

@hwbrzzl One thing to take into consideration is that the brotli middleware I choose does not handle fallback to gzip. We can't also chain .GlobalMiddleware(middlewares.Brotli(), middlewares.Gzip()) since we'll end up with a brotli compressed gzip payload (or a gzip compressed brotli payload xD).

My take on it is that we should aim for a global Compress middleware with a respective compress.go config file where you can adjust the options and also enable brotli, gzip, deflate or maybe all of them at once. That would make things more easier than maintaining multiple middlewares. Please let me know what you think about it.

Yes, it's better to add a configuration to control it. There's a problem, we want to implement multiple HTTP drivers: goravel/gin, goravel/filber, they are developing, and we have another PR to refactor HTTP and Route modules, so can we make these two middleware in this PR more generic? Change gin to net/http, we will also try to change the CORS middleware.

mauri870 · 2023-07-05T00:17:26Z

Since we are aiming to support different http drivers it makes sense to try to make the compression midlewares generic. I will work on it 🙏

hwbrzzl

Great PR! I wonder if we can add some unit tests for this PR. If it's hard to add it, please add some screenshots that test this feature locally, and please list the new configurations added in this PR here.

http/middleware/brotli.go

hwbrzzl · 2023-07-05T00:14:28Z

http/middleware/gzip.go

+	return func(ctx httpcontract.Context) {
+		switch ctx := ctx.(type) {
+		case *http.GinContext:
+			newGzip(facades.Config().GetInt("gzip.compression_level"))(ctx.Instance())


Marian0 · 2023-07-05T11:18:16Z

@mauri870 great job! Thanks for creating this PR - I tested it locally on a sample project and both compressions are working as expected. Thanks again 🙌

codecov · 2023-07-05T22:19:13Z

Codecov Report

Patch coverage: 96.25% and project coverage change: +0.18 🎉

Comparison is base (989ae8b) 62.76% compared to head (b380a98) 62.94%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   62.76%   62.94%   +0.18%     
==========================================
  Files         128      130       +2     
  Lines        8314     8347      +33     
==========================================
+ Hits         5218     5254      +36     
+ Misses       2735     2733       -2     
+ Partials      361      360       -1

Impacted Files	Coverage Δ
schedule/service_provider.go	`0.00% <0.00%> (ø)`
support/database/database.go	`79.31% <0.00%> (ø)`
contracts/foundation/service_provider.go	`50.00% <50.00%> (ø)`
schedule/application.go	`94.33% <100.00%> (+1.15%)`	⬆️
schedule/event.go	`100.00% <100.00%> (ø)`
support/debug/dump.go	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

hwbrzzl · 2023-07-06T00:48:41Z

http/middleware/compression.go

+	gbrotli "github.com/anargu/gin-brotli"
+	"github.com/andybalholm/brotli"
+	"github.com/gin-contrib/gzip"
+	"github.com/gin-gonic/gin"


Q: Can we remove the gin dependency totally?

Yes it should be possible I think. I found this package but it requires an http.Handler as input. Any clue how to use this here instead of the gin one?

We may need to read the source code to understand how to implement Gzip.

By the way, I missed one thing, we can use Gzip by nginx, is it necessary to implement Gzip through the framework? Most time, we need to use Nginx to make a request forwarding. cc @Marian0

My 2 cents is that it is something worth it to have in the framework. There was a time until ~2016 that it was preferred to always expose go services being a proxy, (such as nginx),. But then companies started to expose their go services directly and I believe notw it is actually safe to do that. Ofc you do not get the benefits of a true proxy but compression is a nice to have for such cases.

Make sense, let's go ahead. 👍

mauri870 · 2023-07-13T23:52:36Z

@hwbrzzl I was unable to get this working without relying on gin. I think we should wait for the http refactoring before moving forward with this task.

hwbrzzl · 2023-07-17T04:22:43Z

Hi @mauri870 Sorry for the late, I have a travel last week. Sure, make sense, let's wait a bit until the HTTP refactor is finished.

hwbrzzl · 2023-08-29T07:10:12Z

Hi @mauri870 The HTTP refactor is completed, we can move on now.

feat: add gzip and brotli middleware

aad32c7

hwbrzzl reviewed Jul 5, 2023

View reviewed changes

hwbrzzl mentioned this pull request Jul 5, 2023

✨ [Feature] The HTTP middleware can be move to goravel/http-middleware goravel/goravel#204

Closed

1 task

mauri870 changed the title ~~feat: add gzip and brotli middleware~~ WIP: feat: add gzip and brotli middleware Jul 5, 2023

mauri870 added 2 commits July 5, 2023 17:09

add missing brotli dependencies

8b9178f

Add compression middleware

b380a98

hwbrzzl reviewed Jul 6, 2023

View reviewed changes

devhaozi changed the title ~~WIP: feat: add gzip and brotli middleware~~ wip: feat: add gzip and brotli middleware Jul 6, 2023

devhaozi added the 🛠 In Progress label Jul 6, 2023

hwbrzzl closed this Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: feat: add gzip and brotli middleware #232

wip: feat: add gzip and brotli middleware #232

mauri870 commented Jul 4, 2023

mauri870 commented Jul 4, 2023 •

edited

Loading

hwbrzzl commented Jul 5, 2023

mauri870 commented Jul 5, 2023

hwbrzzl left a comment

hwbrzzl Jul 5, 2023

Marian0 commented Jul 5, 2023

codecov bot commented Jul 5, 2023

hwbrzzl Jul 6, 2023

mauri870 Jul 6, 2023

hwbrzzl Jul 6, 2023

mauri870 Jul 6, 2023 •

edited

Loading

hwbrzzl Jul 7, 2023

mauri870 commented Jul 13, 2023

hwbrzzl commented Jul 17, 2023

hwbrzzl commented Aug 29, 2023

wip: feat: add gzip and brotli middleware #232

wip: feat: add gzip and brotli middleware #232

Conversation

mauri870 commented Jul 4, 2023

📑 Description

✅ Checks

ℹ Additional Information

mauri870 commented Jul 4, 2023 • edited Loading

hwbrzzl commented Jul 5, 2023

mauri870 commented Jul 5, 2023

hwbrzzl left a comment

Choose a reason for hiding this comment

hwbrzzl Jul 5, 2023

Choose a reason for hiding this comment

Marian0 commented Jul 5, 2023

codecov bot commented Jul 5, 2023

Codecov Report

hwbrzzl Jul 6, 2023

Choose a reason for hiding this comment

mauri870 Jul 6, 2023

Choose a reason for hiding this comment

hwbrzzl Jul 6, 2023

Choose a reason for hiding this comment

mauri870 Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

hwbrzzl Jul 7, 2023

Choose a reason for hiding this comment

mauri870 commented Jul 13, 2023

hwbrzzl commented Jul 17, 2023

hwbrzzl commented Aug 29, 2023

mauri870 commented Jul 4, 2023 •

edited

Loading

mauri870 Jul 6, 2023 •

edited

Loading