-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Decompress request body when multi Content-Encoding sent on request headers #2555
Conversation
Add docs to functions to inform correctly what the change is
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
@Jictyvoo Thank you for the PR, can you take a look at all the issues raised by golangci-lint. GoFiber currently supports go 1.17-1.20 and some of the functions added in the unit-test are only available in 1.20 |
@gaby Oh, I see, I forgot about this lint rules. Already applied all changes needed to pass in the linter. Also, for the Thanks for the tip |
Also, with the previous fix to problems detected by tests, it becomes really hard to make the linter happy, so this change also helps in it
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.
LGTM, although this probably requires updating the version given there's quite a lot changing in Body()
@Jictyvoo can you also update docs, i forgot mentioning it |
@Jictyvoo can you share the times of the benchmark for before and after |
Done some benchmarks using the same test methods on both sources, to ensure that it will not affect the benchmark itself. Before the change:goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_Ctx_Body_With_Compression
Benchmark_Ctx_Body_With_Compression/gzip
Benchmark_Ctx_Body_With_Compression/gzip-16 1882047 658.6 ns/op 145 B/op 7 allocs/op
Benchmark_Ctx_Body_With_Compression/deflate
Benchmark_Ctx_Body_With_Compression/deflate-16 2226450 497.1 ns/op 129 B/op 4 allocs/op
Benchmark_Ctx_Body
Benchmark_Ctx_Body-16 100000000 13.36 ns/op 0 B/op 0 allocs/op
PASS After the Change:goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v2
cpu: AMD Ryzen 7 5800X 8-Core Processor
Benchmark_Ctx_Body_With_Compression
Benchmark_Ctx_Body_With_Compression/gzip
Benchmark_Ctx_Body_With_Compression/gzip-16 1445175 728.0 ns/op 145 B/op 7 allocs/op
Benchmark_Ctx_Body_With_Compression/gzip,invalid
Benchmark_Ctx_Body_With_Compression/gzip,invalid-16 1521243 806.5 ns/op 194 B/op 8 allocs/op
Benchmark_Ctx_Body_With_Compression/deflate
Benchmark_Ctx_Body_With_Compression/deflate-16 1965168 591.5 ns/op 129 B/op 4 allocs/op
Benchmark_Ctx_Body_With_Compression/gzip,deflate
Benchmark_Ctx_Body_With_Compression/gzip,deflate-16 663676 1508 ns/op 377 B/op 12 allocs/op
Benchmark_Ctx_Body
Benchmark_Ctx_Body-16 89043600 13.13 ns/op 0 B/op 0 allocs/op
PASS |
thx, looks almost the same -> no performance lose |
Have updated some, I don't know if it has others than |
This failed test, I don't see as something that I change, any of you know something about it? |
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
Currently the Ctx.Body() implementation doesn't parse multi content-encoding body, so in order to attend RFC9110, the Body method was changed, and a new wrapper method to body raw was made.
Fixes #2085
Type of change
Checklist: