Add cowboy_decompress_h stream handler

[jdamanalo]

Closes #946.

I added documentation with tentative 2.X as the version where this module is introduced. Alternatively, the documentation could also be left out much like when the other stream handlers were still experimental.

[essen]

Please rebase onto current master so that CI runs properly. I am planning to do a release with this and other things soon.

[essen]

Just to be sure since copyright is important: no problem with me adding this to the top of the file?

diff --git a/src/cowboy_decompress_h.erl b/src/cowboy_decompress_h.erl
index ffbec25..f256223 100644
--- a/src/cowboy_decompress_h.erl
+++ b/src/cowboy_decompress_h.erl
@@ -1,3 +1,17 @@
+%% Copyright (c) 2023, jdamanalo <[email protected]>
+%%
+%% Permission to use, copy, modify, and/or distribute this software for any
+%% purpose with or without fee is hereby granted, provided that the above
+%% copyright notice and this permission notice appear in all copies.
+%%
+%% THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+%% WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+%% MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+%% ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+%% WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+%% ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+%% OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+
 -module(cowboy_decompress_h).
 -behavior(cowboy_stream).
 

Probably going to change to 2024 since 2.11 won't release this year.

[jdamanalo]

Sure, no problem! [email protected] for my email should be more long-lived.

[essen]

I won't have time to finish today so I'll leave a few questions. I'll continue early January.

Overall this is solid. Great job! Thanks. I did a few simplifications and whatnot, with the biggest change being making read_body_buffer a list of binaries, to avoid doing unneeded binary concatenations and memory allocations. We can give this list of binaries directly to the inflate functions and they won't need to convert them to a single binary. Win win. We do convert when passing the data forward to the next stream handler (whether we inflate or not).

I haven't reviewed the tests yet but after a quick look you get points for completeness.

Merry Christmas, cheers.

[essen]

One of the problems I see is that the handler cannot currently know that the content was decompressed because we do not modify the headers of the Req before passing it forward, and we do not add additional metadata to indicate we did so.

My immediate thought is that we should do both: update the headers to remove the content-encoding as well as add metadata to indicate what we decoded (basically copying the header value elsewhere). Another header field, content-length, will be inaccurate until the body is read fully, but cowboy_req then updates the Req with the right information.

If the plugin is enabled, we should remove the content-encoding, at least when we start reading the body. That's awkward though because you would have to read at least a little first to know whether you have content to decode afterwards...

So alternatively we could remove it in init but in that case it doesn't make sense to have a decompress_ignore option.

Another alternative would be to have a field in Req that the decompress handler always sets, even when not decompressing. This field would then help the user because:

• if it is missing, the decompress handler was not used; no problem
• if present and set to []: we know no content decoding took place (and we don't touch the content-encoding header)
• if present and set to the content-encoding value (inside a list, see below): we know content decoding happened/will happen (and the content-encoding header was removed, its value we now have in this field)

If for some reason the user wants to disable content decoding between init and the reading of the body, they can easily reconstruct the headers as no information was lost (only moved around). Users that do not care about that will see the decoding happen transparently (the content-encoding header is removed).

I am thinking this needs proper documentation as well as a better name for decompress_ignore since we aren't just ignoring it, we are more "aborting" it but I'll think about a better name than that.

For the field in Req I am thinking of content_decoded that would be an ordered list of decoded encodings (so it can be expanded later on if needed). In our case we'll just create it or if it exists do [<<"gzip">>|Value].

[essen]

I've done the above changes and renamed the option to decompress_enabled. I am updating the documentation now. All that's left after that is checking the tests, perhaps consolidating a thing or two, and we're good.

[essen]

OK I am pushing the changes. A review would be appreciated, my changes are in fd9711d. Closing, thanks!

[jdamanalo]

Reviewed and everything seems in order to me. I learned a lot from your extensive guidance. Thanks again, I really appreciate it! 🙌