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

Add a gunzipping option #192

Merged
merged 4 commits into from Sep 1, 2017
Merged

Add a gunzipping option #192

merged 4 commits into from Sep 1, 2017

Conversation

@Marsup
Copy link
Member

Marsup commented Aug 30, 2017

As discussed with you, here's a gunzip option. I think it's my 1st significant contribution to wreck so be careful ;)

@Marsup Marsup added the feature label Aug 30, 2017
@Marsup Marsup requested a review from geek Aug 30, 2017
@@ -9,6 +9,7 @@ const Https = require('https');
const Stream = require('stream');
const Hoek = require('hoek');
const Boom = require('boom');
const Zlib = require('zlib');

This comment has been minimized.

Copy link
@geek

geek Aug 30, 2017

Member

our sorting is odd, but zlib should go up above hoek

@@ -356,25 +360,54 @@ internals.Client.prototype.read = function (res, options, callback) {
return callback(null, null);
}

if (options.json === 'force') {
const parse = () => {

This comment has been minimized.

Copy link
@geek

geek Aug 30, 2017

Member

feel free to delete the // Parse JSON comment above this.. maybe rename the parse function to parseJSON ?

@@ -11,6 +11,7 @@ const Stream = require('stream');
const Hoek = require('hoek');
const Lab = require('lab');
const Reload = require('require-reload');
const Zlib = require('zlib');

This comment has been minimized.

Copy link
@geek

geek Aug 30, 2017

Member

since zlib is part of core, it should go above hoek

@geek

This comment has been minimized.

Copy link
Member

geek commented Aug 30, 2017

@Marsup For the build issues, maybe make the test more relaxed on the error message:

expect(err).to.be.an.error('unexpected end of file');

change to

expect(err).to.be.an.error();
expect(err).to.be.an.error('Unexpected token \u001f in JSON at position 0');

change to

expect(err).to.be.an.error();

either that or have custom node version checks in the test :/

Copy link
Member

kanongil left a comment

The gunzipping means that any Content-Encoding, Content-Length, Content-Type, Content-Range, and ETag headers are not valid for the returned buffer.

Either these need be corrected somehow, or at least documented that they have been invalidated. I suspect it will have to be the latter.

const contentType = (res.headers && internals.findHeader('content-type', res.headers)) || '';
const mime = contentType.split(';')[0].trim().toLowerCase();
if (contentEncoding === 'gzip') {
Zlib.gunzip(buffer, (err, gunzipped) => {

This comment has been minimized.

Copy link
@kanongil

kanongil Aug 31, 2017

Member

I would much prefer to use streaming gunzip, rather than the sync one that you use. Eg. create a passthrough gunzip stream and pipe through it. This might even be a simpler implementation.

@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Aug 31, 2017

I don't think Content-Type would be different, what can we do about fixing the others and is it actually useful ?

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Aug 31, 2017

It seems I was too quick regarding Content-Type. You are right.

Regarding fixes, I suspect nothing much can be done, since the headers are part of the native http.IncomingMessage which have already been exposed before the call to read().

if (options.gunzip) {
const contentEncoding = options.gunzip === 'force' ?
'gzip' :
(res.headers && internals.findHeader('content-encoding', res.headers)) || '';

This comment has been minimized.

Copy link
@kanongil

kanongil Aug 31, 2017

Member

This doesn't handle multiple encoding entries. Eg. gzip, identity can be supported.

This comment has been minimized.

Copy link
@Marsup

Marsup Aug 31, 2017

Author Member

Is it common ? Never encountered this one.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Aug 31, 2017

Looking at it, I'm not sure this feature is compatible with the hands off approach of wreck. We really need more control of the exposed output to handle this transparently.

Maybe it can work if it is only enabled for the helper methods, where we have tighter control of the output.

@Marsup Marsup force-pushed the gunzip branch from 0764cc6 to 0c6455e Aug 31, 2017
@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Aug 31, 2017

So you'd rather not have it in wreck ? I'll let Wyatt decide on this, but I think it's a pretty common thing to do, and it's an opt-in feature so I'm not forcing it on anyone.

Updated to stream mode as suggested and doc changes.

@geek

This comment has been minimized.

Copy link
Member

geek commented Aug 31, 2017

@kanongil I agree with @Marsup that since this is an opt-in feature that doesn't add any additional overhead we are fine to include it, especially because this is a common use-case.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Aug 31, 2017

A couple more issues:

If the server provides a Content-Range header, which is any less than the full content, it is incompatible with this feature. I suspect the best solution is to immediately throw an error.

Secondly, old servers can signal gzip encoding with x-gzip, which needs to be handled as well. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Aug 31, 2017

I also don't like that there is not way to know whether the buffer has been transparently gunzipped, or not.

@geek
geek approved these changes Aug 31, 2017
@Marsup

This comment has been minimized.

Copy link
Member Author

Marsup commented Aug 31, 2017

OK, then I'll ask for an option to completely disable JSON parsing, because right now the auto-detection is killing any attempt I do at handling the gzipping externally.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Aug 31, 2017

A completely different issues, is that I would expect this feature to somehow add a Accept-Encoding: gzip header to the outgoing request.

@geek

This comment has been minimized.

Copy link
Member

geek commented Aug 31, 2017

@kanongil

I also don't like that there is not way to know whether the buffer has been transparently gunzipped, or not.

Enabling the feature and checking the Content-Encoding header should be sufficient for detecting if a buffer was gunzipped.

If the server provides a Content-Range header, which is any less than the full content, it is incompatible with this feature. I suspect the best solution is to immediately throw an error.

This is a good point. Since this is an opt-in feature that is brand new, I don't think we need to address this particular use case yet. If someone is in need of this support we can address it then.

@geek geek added this to the 12.3.0 milestone Aug 31, 2017
@geek geek merged commit 12a42b6 into master Sep 1, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@geek geek deleted the gunzip branch Sep 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.