🔥 Feature: Add support for zstd compression #3041
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Warning Review failedThe pull request is closed. WalkthroughThe updates introduce support for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberServer
participant CompressionMiddleware
Client->>FiberServer: Request with Accept-Encoding header
FiberServer->>CompressionMiddleware: Process request
CompressionMiddleware->>FiberServer: Return compressed response with zstd/gzip/deflate/brotli
FiberServer->>Client: Response with Content-Encoding header
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3041 +/- ##
==========================================
+ Coverage 82.78% 82.87% +0.08%
==========================================
Files 115 115
Lines 8227 8234 +7
==========================================
+ Hits 6811 6824 +13
+ Misses 1086 1081 -5
+ Partials 330 329 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- .github/README.md (1 hunks)
- .gitignore (1 hunks)
- Makefile (1 hunks)
- app.go (3 hunks)
- bind_test.go (2 hunks)
- constants.go (1 hunks)
- ctx.go (2 hunks)
- docs/api/fiber.md (1 hunks)
- docs/middleware/compress.md (1 hunks)
- docs/middleware/static.md (1 hunks)
- docs/whats_new.md (1 hunks)
- helpers_test.go (1 hunks)
- middleware/compress/compress_test.go (5 hunks)
- middleware/static/static.go (2 hunks)
- middleware/static/static_test.go (2 hunks)
Files skipped from review due to trivial changes (3)
- .gitignore
- Makefile
- constants.go
Additional context used
LanguageTool
docs/middleware/compress.md
[uncategorized] ~61-~61: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...evel" field are: -LevelDisabled (-1): Compression is disabled. - `LevelDefaul...
[uncategorized] ~62-~62: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ession is disabled. -LevelDefault (0): Default compression level. - `LevelBest...
[uncategorized] ~63-~63: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...ompression level. -LevelBestSpeed (1): Best compression speed. - `LevelBestCom...
[uncategorized] ~64-~64: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...sion speed. -LevelBestCompression (2): Best compression. ## Default Config `...docs/whats_new.md
[grammar] ~51-~51: It looks like there is a word missing here. Did you mean “listen to config”? (LISTEN_TO_ME)
Context: ...ic.md) * app.Config properties moved to listen config * DisableStartupMessage * EnablePre...
[style] ~55-~55: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym. (ADVERB_REPETITION_PREMIUM)
Context: ...nablePrintRoutes * ListenerNetwork -> previously Network ### new methods * RegisterCus...
[uncategorized] ~192-~192: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ... ::: ### new methods * AutoFormat -> ExpressJs like * Host -> ExpressJs like * Port ->...
[uncategorized] ~193-~193: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ... AutoFormat -> ExpressJs like * Host -> ExpressJs like * Port -> ExpressJs like * IsProxy...
[uncategorized] ~194-~194: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ...like * Host -> ExpressJs like * Port -> ExpressJs like * IsProxyTrusted * Reset * Schema ...
[uncategorized] ~197-~197: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ...ke * IsProxyTrusted * Reset * Schema -> ExpressJs like * SendStream -> ExpressJs like * S...
[uncategorized] ~198-~198: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ...chema -> ExpressJs like * SendStream -> ExpressJs like * SendString -> ExpressJs like * S...
[uncategorized] ~199-~199: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ...tream -> ExpressJs like * SendString -> ExpressJs like * String -> ExpressJs like * ViewB...
[uncategorized] ~200-~200: The official spelling of this programming framework is “Express.js”. (NODE_JS)
Context: ...endString -> ExpressJs like * String -> ExpressJs like * ViewBind -> instead of Bind ###...
[style] ~253-~253: Consider rephrasing this to strengthen your wording. (MAKE_CHANGES)
Context: ...::: ## 🧬 Middlewares ### CORS We've made some changes to the CORS middleware to improve its f...
[uncategorized] ~261-~261: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...updated fields: -Config.AllowOrigins: Now accepts a slice of strings, each re...
[uncategorized] ~262-~262: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... allowed origin. -Config.AllowMethods: Now accepts a slice of strings, each re...
[uncategorized] ~263-~263: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ... allowed method. -Config.AllowHeaders: Now accepts a slice of strings, each re...
[uncategorized] ~264-~264: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION)
Context: ...allowed header. -Config.ExposeHeaders: Now accepts a slice of strings, each re...docs/api/fiber.md
[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space. (NOUN_VERB_CONFUSION)
Context: ... | This allows to setup app name for the app ...
[uncategorized] ~61-~61: The preposition ‘to’ seems more likely in this position. (AI_HYDRA_LEO_REPLACE_ON_TO)
Context: ...tained by the top-level app and applied on prefix associated requests. ...
[uncategorized] ~68-~68: Did you mean: “By default,”? (BY_DEFAULT_COMMA)
Context: ...turn the value of the given header key. By defaultc.IP()will return the Remote IP from ...
[typographical] ~78-~78: The conjunction “so that” does not require a comma. (SO_THAT_UNNECESSARY_COMMA)
Context: ... before setting the path for the context, so that the routing can also work with URL enco...
[style] ~106-~106: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_WANT_TO_VB)
Context: ... | Path of the certificate file. If you want to use TLS, you must enter this field. ...
[style] ~107-~107: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. (REP_WANT_TO_VB)
Context: ...f the certificate's private key. If you want to use TLS, you must enter this field. ...
[grammar] ~115-~115: Did you mean “customizing”? Or maybe you should add a pronoun? In active voice, ‘allow’ + ‘to’ takes an object, usually a pronoun. (ALLOW_TO)
Context: ...func()| Allows to customize success behavior when gracefully shutti...
[style] ~156-~156: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym. (ENGLISH_WORD_REPEAT_BEGINNING_RULE)
Context: ..., CertKeyFile: "./cert.key"})#### TLS with certificatego title="Examples...
Markdownlint
docs/middleware/static.md
22-22: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank linesdocs/whats_new.md
49-49: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
50-50: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
51-51: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
52-52: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
53-53: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
54-54: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
55-55: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
59-59: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
60-60: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
61-61: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
65-65: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
66-66: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
67-67: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
68-68: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
69-69: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
73-73: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
74-74: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
75-75: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
192-192: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
193-193: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
194-194: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
195-195: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
196-196: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
197-197: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
198-198: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
199-199: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
200-200: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
201-201: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
205-205: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
206-206: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
207-207: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
208-208: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
209-209: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
210-210: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
211-211: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
212-212: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
213-213: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
214-214: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
215-215: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
219-219: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
220-220: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
221-221: Expected: dash; Actual: asterisk (MD004, ul-style)
Unordered list style
229-229: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
278-278: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
189-189: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
242-242: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
299-299: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
255-255: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
258-258: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
143-143: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
171-171: null (MD031, blanks-around-fences)
Fenced code blocks should be surrounded by blank lines
23-23: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
256-256: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linesdocs/api/fiber.md
85-85: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
118-118: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
218-218: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
Additional comments not posted (13)
docs/middleware/compress.md (1)
7-7: Updated Compression Middleware DocumentationThe documentation now correctly lists
zstdas a supported compression algorithm alongsidegzip,deflate, andbrotli. This update aligns with the new feature implementation in the middleware.middleware/static/static.go (1)
62-72: Updated Configuration for Static MiddlewareThe
fasthttp.FSstruct has been updated to include new fields such asCompressBrotliandCompressedFileSuffixes, enhancing the middleware's flexibility in handling compressed files. This update is crucial for supporting multiple compression formats and is consistent with the PR's objective to supportzstdcompression.docs/middleware/static.md (1)
151-151: Updated Documentation for Static MiddlewareThe documentation now reflects the new capability of the static middleware to use
gzip,brotli, orzstdfor compression, based on theAccept-Encodingheader. This update is well-explained and aligns with the functionality changes in the middleware.middleware/compress/compress_test.go (1)
138-160: New Unit Tests forzstdCompressionThe added tests for
zstdcompression are comprehensive, testing the basic functionality to ensure that the compressed output is smaller than the original data. This is crucial for validating the correct implementation of the new compression algorithm.docs/whats_new.md (1)
268-268: Documentation on New Compression SupportThe section on compression correctly highlights the addition of
zstdalongside existing methods likegzip,deflate, andbrotli. This inclusion is essential for informing users about the new capabilities of the framework.helpers_test.go (1)
319-323: Updated benchmark to includezstdcompression type.The benchmark test now includes
zstdalongside other compression types (deflate,gzip,br,brotli). This aligns well with the PR's objective to introduce support forzstdcompression. The test is designed to ensure that the utility function can handle the new compression type effectively.middleware/static/static_test.go (1)
762-807: Consider improving test isolation and robustness.
- Test Isolation: Using
time.Sleepto wait for cache expiration is not reliable and can lead to flaky tests. Consider using a mock or a setup teardown method that can more reliably control the environment.- File Existence Checks: The test checks for the existence of compressed files. It would be beneficial to also check that the contents are correct and not just the existence.
[REFACTOR_SUGGESTion]
.github/README.md (1)
584-584: Update to compression middleware documentation is accurate.The addition of
zstdto the supported compression methods in thecompressmiddleware is correctly documented. This change aligns with the PR's objective to enhance compression capabilities.app.go (1)
479-484: Approved the initialization ofCompressedFileSuffixes.The initialization logic for
CompressedFileSuffixesin theNew()function is correctly implemented. It checks if the map is nil and sets the default values appropriately. This approach prevents nil map panics and ensures that the configuration is correctly initialized.bind_test.go (1)
Line range hint
827-951: Comprehensive testing for new compression support and improved bind functionalitiesThe additions to
Test_Bind_Bodyand related helper functions (testCompressedBody,testDecodeParser,testDecodeParserError) are well-implemented. These functions effectively extend the testing to cover the newzstdcompression along with existing ones, and they introduce tests for different content types which is crucial for ensuring robustness in data handling.
- The use of helper functions to modularize the testing of compressed bodies (
testCompressedBody) and content type parsing (testDecodeParserandtestDecodeParserError) enhances the readability and maintainability of the test suite.- The parallel execution setup in sub-tests ensures that tests do not interfere with each other, which is good practice in concurrent environments.
However, I noticed a potential area for enhancement:
- Error Handling: In the
testDecodeParserErrorfunction, the use ofrequire.Erroris appropriate, but it might be beneficial to also check the type or message of the error to ensure that it's failing for the right reasons, not just any error.Overall, the test coverage looks solid, and these tests should effectively validate the functionality of the bindings with various compression and content types. Great job on making sure the new features are well-tested!
docs/api/fiber.md (1)
51-51: Update documentation to reflect the new configuration for compressed file suffixes.The documentation now correctly describes the
CompressedFileSuffixesconfiguration, which uses a map to handle multiple compression types. This is an important update for clarity and functionality, ensuring that users are aware of how to configure different file suffixes for various compression types.ctx.go (2)
221-222: Addition ofzstdcompression support intryDecodeBodyInOrderfunction.The addition of a case for
StrZstdin thetryDecodeBodyInOrderfunction is aligned with the PR's objective to supportzstdcompression. This change allows the function to handlezstdencoded bodies by callingc.fasthttp.Request.BodyUnzstd(). This is a necessary update to ensure that the application can decode requests compressed withzstd.
1434-1442: Configuration ofsendFileFSwithzstdcompression support.The modifications to the
sendFileFSinitialization block to includeCompressBrotliandCompressedFileSuffixesare crucial for handling static files compressed withzstd. This configuration ensures that the server can serve static files efficiently by utilizing the appropriate compression based on the client'sAccept-Encodingheader. This change aligns with the PR's enhancements to the static file middleware.
gaby
changed the title
gaby
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- middleware/compress/compress_test.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/compress/compress_test.go
|
As of right now, the priority seems to be br > gzip > deflate > zstd, is there a way to change this so that zstd is preferred? |
|
@vovler The problem is that fasthttp also has an order. According to some research I was doing the order nowdays should be: br > zstd > gzip > deflate Can you create a separate ticket so we can address this? |
|
@vovler The problem is that fasthttp also has an order. According to some research I was doing the order nowdays should be:
I just created some middleware to rewrite the request header to force the order that I want, since it is indeed fasthttp that sets the order not gofiber, no point in creating a ticket. gofiber uses fasthttp.CompressHandlerBrotliLevel to set the compression. And in the fasthttp repo we can find it func CompressHandlerBrotliLevel(h RequestHandler, brotliLevel, otherLevel int) RequestHandler {
return func(ctx *RequestCtx) {
h(ctx)
switch {
case ctx.Request.Header.HasAcceptEncodingBytes(strBr):
ctx.Response.brotliBody(brotliLevel)
case ctx.Request.Header.HasAcceptEncodingBytes(strGzip):
ctx.Response.gzipBody(otherLevel)
case ctx.Request.Header.HasAcceptEncodingBytes(strDeflate):
ctx.Response.deflateBody(otherLevel)
case ctx.Request.Header.HasAcceptEncodingBytes(strZstd):
ctx.Response.zstdBody(otherLevel)
}
}
}and it seems to follow the hardcoded order that I found out earlier by playing with curl and accept encoding headers |
Description
zstdcompression in core Fiber, static middleware, and compression middlewareMakefileBindbody.CompressedFileSuffixesCompressedFileSuffix, addedCompressedFileSuffixes, setting both of these values causes issues withfasthttp.FSnaming of compressed files.CompressBrotli=TruewhenCompressis set toTrue, else it won't usebrotlicompression.Staticmiddleware for files below and above 200 bytes.Fixes #3001
Type of change