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

Support request body inflate and gunzip when Content-Encoding header is appropriately set #38

Open
bminer opened this issue Mar 18, 2016 · 16 comments

Comments

@bminer
Copy link
Contributor

bminer commented Mar 18, 2016

If a client sends an HTTP request with "Content-Encoding" header set, co-body should be able to quickly pipe that to zlib and inflate the compressed request body prior to parsing the body.

I'd be happy to help and submit a PR, but I'd like to discuss how to implement first. Any thoughts? Is this something that needs to be done in raw-body or another lib? raw-body needs the stream only, but each "buddy" in co-body needs the full HTTP request Object. Might require a bit of refactoring?

@bminer
Copy link
Contributor Author

bminer commented Mar 18, 2016

I should also mention that the old Express body-parser lib supports HTTP request inflation:

https://github.com/expressjs/body-parser/blob/master/lib/read.js#L143-L176

@haoxins
Copy link
Member

haoxins commented Mar 19, 2016

SGTM

we can dep https://github.com/stream-utils/inflation

@bminer
Copy link
Contributor Author

bminer commented Mar 21, 2016

@coderhaoxin - I like it. I'll submit a PR that modifies each "buddy" (i.e. lib/json.js) to use the inflation lib.

@bminer
Copy link
Contributor Author

bminer commented Mar 21, 2016

Just waiting for this to be merged...
stream-utils/inflation#14

@felixsanz
Copy link

felixsanz commented Dec 31, 2016

@bminer , @fengmk2

If a client sends an HTTP request with "Content-Encoding" header set, co-body should be able to quickly pipe that to zlib and inflate the compressed request body prior to parsing the body.

This PR should be reverted, as stated in #51, not all Content-Encodings are gzip. For example DeflateRaw and Brotli will make the parser fail and the body will not be available.

@fengmk2 fengmk2 reopened this Jan 3, 2017
@fengmk2
Copy link
Member

fengmk2 commented Jan 3, 2017

@bminer can you fix @felixsanz's bug too?

@felixsanz
Copy link

@fengmk2 I think the inflate/decompress thing should be bundled in it's own middleware, like this:

app.use(bodyparser)
app.use(decompressBody) // some inflate middleware
// or app.use(brotli) <- decompress brotli bodies

This is a bodyparser, and that is the only thing that it should do. If the body is compressed or not I don't think it has anything to do with this middleware. It'll be a mess to support all encodings here (and the upcoming ones like the facebook's zstd).

@bminer
Copy link
Contributor Author

bminer commented Jan 3, 2017

I'm not so sure that I agree. If the HTTP body is compressed, the bodyparser should be able to decompress it in order to parse it. I see the argument for separate middleware, but gzip is the "standard".

What is wrong about trying to support other inflation algorithms?

@bminer
Copy link
Contributor Author

bminer commented Jan 3, 2017

Also deflate and gzip are supported by the inflation lib. Maybe Brotli can be added?

@felixsanz
Copy link

felixsanz commented Jan 3, 2017

and what about deflateRaw? and zopli? and zstd? :/

Anyway, supporting brotli is more than nothing. Hell, even if you don't support brotli i'm happy as long as you remove the default inflate() process that is giving me errors because i'm not using deflate :D

@bminer
Copy link
Contributor Author

bminer commented Jan 3, 2017

Hmm, what errors do you get with an unsupported content encoding?

@bminer
Copy link
Contributor Author

bminer commented Jan 3, 2017

Nvm, looks like the inflation lib raises an "unsupported content encoding" error 415. Can you use a middleware to inflate the body and then set the content-encoding header to "identity"? Long term solution could be to add support for brotli et. al. to the inflation lib.

@felixsanz
Copy link

felixsanz commented Jan 3, 2017

I can't inflate the body because there is no body :P The bodyparser doesn't parse the body because error 415.

I should close the other issue and continue just here, right?

@bminer
Copy link
Contributor Author

bminer commented Jan 17, 2017

OK, here's my understanding of co-body. First, we inflate the req stream using the inflation lib. Then, we pass that stream to raw-body to get the size-validated raw HTTP body Buffer. Finally, the body is parsed using the logic in this lib.

The problem is that co-body expects req to be the stream from which the HTTP body should be read. Rather, co-body should read from the inflated stream. Is there a way in Koa to replace req with the stream returned from inflate(req)?

If not, it would be rather difficult to break the HTTP body inflation into a separate module. Thus, the best way forward might be to add support for more compression algorithms into the inflation lib. Right now, it supports zlib stuff, but it could be modified to add support for others.

@felixsanz
Copy link

@bminer As i said above, there is more compression methods than inflate/gunzip. You can't inflate a body that is not deflated. It's like trying to decompress a zip file with rar specific tools.

And i don't think modify the inflate lib is a good idea because inflate is inflate and brotli is brotli. Or at least the library should be renamed because naming it "inflate" when it decompress brotli is confusing as hell

@bminer
Copy link
Contributor Author

bminer commented Jan 17, 2017

@felixsanz - Fair point about the inflation lib being called "inflation". Might be confusing if it supported brotli or zstd. Maybe a better name would be decompress-stream?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants