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

deprecated encoding 'binary' is used #7

Closed
iamdoron opened this issue Jul 3, 2013 · 18 comments
Closed

deprecated encoding 'binary' is used #7

iamdoron opened this issue Jul 3, 2013 · 18 comments
Assignees
Labels
bug
Milestone

Comments

@iamdoron
Copy link
Contributor

@iamdoron iamdoron commented Jul 3, 2013

in here:

var output = rawBuffer.toString('binary');

from node buffer documentation:

'binary' - A way of encoding raw binary data into strings by using only the first 8 bits of each character. This encoding method is deprecated and should be avoided in favor of Buffer objects where possible. This encoding will be removed in future versions of Node.

I was also wondering what happens to the returned payload, since its original encoding may be different.
I would expect something similar to:

res.payload = (new Buffer(res.payload.toString(), 'binary')).toString()

(for example, the default utf8 encoding will be returned)

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Aug 28, 2013

Binary is required for supporting zipped payloads... see this test: https://github.com/spumko/shot/blob/master/test/index.js#L108

@geek geek closed this Aug 28, 2013
@iamdoron

This comment has been minimized.

Copy link
Contributor Author

@iamdoron iamdoron commented Aug 28, 2013

@wpreul, I'm not sure it is required; you could use Buffer.
If I understand correctly, you want res.payload to be string[binary], but in this way you are returning to the user a 'binary' encoding, which means utf-8 characters that are not shown correctly - most users will expect a 'utf-8' string, not a deprecated 'binary' string. You could just decide the payload is a Buffer, then everything is well defined (but then the user will have to .toString it with its preferred encoding - if the user decides to treat it as a string).

Or, you could add
res.payloadBuffer as a Buffer
and keep
res.payload as a 'utf-8' encoded string

that way, if you believe the common case is that the user will expect a string payload, it will be the default.

Anyhow - I think it is still an issue, as there will come a day when 'binary' won't be there anymore :-)
& binary strings are not the default 'utf-8' string encoding

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

@iamdoron iamdoron commented Aug 29, 2013

@wpreul, this is a rough example for not using 'binary' (though it might not be optimal):

internals.finish = function (response, req, onEnd) {

    return function () {

        // Prepare response object

        var res = {
            raw: {
                req: req,
                res: response
            },
            headers: response._headers,
            statusCode: response.statusCode
        };

        // When done, call callback

        process.nextTick(function () {

            onEnd(res);
        });

        // Read payload

        var raw = [];
        var rawLength = 0;
        if (response.output &&
            response.output instanceof Array &&
            response.output.length) {

            for (var i = 0, il = response.output.length; i < il; ++i) {
                var chunk = (response.output[i] instanceof Buffer ? response.output[i] : new Buffer(response.output[i], response.outputEncodings[i]));
                raw.push(chunk);
                rawLength += chunk.length;
            }
        }

        var rawBuffer = Buffer.concat(raw, rawLength);

        // Parse payload

        var CRLF = '0d0a';

        var output = rawBuffer.toString('hex');
        var sep = output.indexOf(CRLF + CRLF);
        var payloadBlock = output.slice(sep + 4*2);
        var headerBlock = output.slice(0, sep);


        res.rawPayload = new Buffer(rawBuffer.length - sep/2 - 4);
        rawBuffer.copy(res.rawPayload, 0, sep/2 + 4);

        if (!res.headers['transfer-encoding']) {
            payloadBlock = new Buffer(payloadBlock, 'hex').toString();
            res.payload = payloadBlock;
            return;
        }

        var rest = payloadBlock;

        res.payload = '';

        res.payloadBuffer = '';

        while (rest) {
            var next = rest.indexOf(CRLF);
            var size = parseInt(new Buffer(rest.slice(0, next), 'hex').toString(), 16);
            if (size === 0) {
                rest = rest.slice(3*2);
                break;
            }
            var currentHex = rest.substr(next + 2*2, size*2);
            var buffer = new Buffer(currentHex, 'hex');
            res.payload += buffer.toString();
            res.payloadBuffer += currentHex;
            rest = rest.slice(next + 2*2 + 2*size + 2*2);
        }
        res.payloadBuffer = new Buffer(res.payloadBuffer, 'hex');

        rest = new Buffer(rest, 'hex').toString();

        if (rest) {
            var headers = rest.split(CRLF);
            headers.forEach(function (header) {

                var parts = header.split(':');
                if (parts.length === 2) {
                    response._headers[parts[0].trim().toLowerCase()] = parts[1].trim();
                }
            });
        }
    };
};
it('parses zipped payload', function (done) {

            var dispatch = function (req, res) {

                res.writeHead(200, 'OK');
                var stream = Fs.createReadStream('./package.json');
                stream.pipe(Zlib.createGzip()).pipe(res);
            };

            Shot.inject(dispatch, { method: 'get', url: '/' }, function (res) {

                Fs.readFile('./package.json', { encoding: 'utf-8' }, function (err, file) {

                    // Zlib.unzip(new Buffer(res.payload, 'binary'), function (err, unzipped) {
                    Zlib.unzip(res.payloadBuffer, function (err, unzipped) {

                        expect(err).to.not.exist;
                        expect(unzipped.toString('utf-8')).to.deep.equal(file);
                        done();
                    });
                });
            });
        });
@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Mar 16, 2015

@geek I've wasted spent 2 days on this problem, I'd appreciate either a fix or an advice on how to go about it.

Using a template (reply.view) having accentuated characters and using server.inject from hapi, prénom becomes prénom in response.result. While it would be OK in response.payload because it is supposed to be binary (which I didn't know and it being a string doesn't help coming to this conclusion), I don't see why response.result should be that way as well.
Honestly I can't determine if it's hapi or shot's fault, what am I doing wrong ?

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Mar 16, 2015

Does the same test pass if you start the server and make a real request?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Mar 16, 2015

Yes, the characters are perfectly fine in a real browser.

@bnjmnt4n

This comment has been minimized.

Copy link

@bnjmnt4n bnjmnt4n commented Mar 17, 2015

👍 for this. I’ve encountered the same issue, and had to use res.rawPayload.toString('utf8') to get my tests to past. This really confused me at the start, and I’m sure others will be confused too.

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Mar 27, 2015

@chrisdickinson do you have an opinion about that ?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 14, 2015

I took over the module for now. What's the question again?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Apr 15, 2015

@hueniverse when you use server.inject in hapi, you get a res.result that looks like a string you'd get from the server as a client, when in fact it's the binary form of that string. (eg. if I reply éèàù I get éèàù as payload)
I don't want to new Buffer(res.result, 'binary').toString('utf8') everywhere in my tests "just in case".

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Apr 21, 2015

We can add another property on res that will include the utf8 encoded version. Would that solve it? I am concerned about a breaking change.

@bnjmnt4n

This comment has been minimized.

Copy link

@bnjmnt4n bnjmnt4n commented Apr 21, 2015

That seems great! 👍

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented Apr 21, 2015

This looks like a bug to me, can you explain the documentation (e.g. when not a stream or a view) ? If I reply the same string from a handler it works, if from a view it doesn't, why ?

@hueniverse hueniverse reopened this May 21, 2015
@hueniverse hueniverse removed this from the 1.0.0 milestone May 21, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented May 21, 2015

@Marsup what documentation?

@Marsup

This comment has been minimized.

Copy link
Member

@Marsup Marsup commented May 21, 2015

@hueniverse Look for that specific sentence in hapi's docs, it's in server.inject.

iamdoron added a commit to iamdoron/shot that referenced this issue May 22, 2015
iamdoron added a commit to iamdoron/shot that referenced this issue May 22, 2015
iamdoron added a commit to iamdoron/shot that referenced this issue May 22, 2015
@hueniverse hueniverse closed this in d9dd737 Jun 4, 2015
hueniverse added a commit that referenced this issue Jun 4, 2015
fix #7
@hueniverse hueniverse added the bug label Jun 4, 2015
@hueniverse hueniverse added this to the 1.5.1 milestone Jun 4, 2015
@hueniverse hueniverse self-assigned this Jun 4, 2015
@merrihew

This comment has been minimized.

Copy link

@merrihew merrihew commented Jun 9, 2015

We've encountered an issue that have been tracked down to this set of changes to shot, specifically payloadBuffer.toString() in d9dd737 (commit diff).

Why was this not considered a breaking change with a major version bump (despite @hueniverse's comment bringing it up)? Bug or not, it's been the way shot has returned res.payload and people have accommodated for it. I think it warrants a revert and a major version bump.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jun 10, 2015

@merrihew probably should have been but too late now. In general bug fixes are an exception to the major version rule but this was a case of unexpected impact. The hapi change does flag it with breaking changes label.

@merrihew

This comment has been minimized.

Copy link

@merrihew merrihew commented Jun 10, 2015

@hueniverse Agreed on the fact that it's already out in the wild, so too late to revert. Opens up an interesting semver discussion when a bug has existed for an extended period of time... in our case, we had taken steps to handle the payload "bug", therefore the "fix" actually created a new bug. There's no clear right or wrong way to handle that, and ultimately something we can just work around on our side. Thanks for all of the continued work on hapi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.