Support setting Request encoding for response

[bb010g]

Mostly useful for Node.js hooks that want to deal with binary data.

🚀 Why this change?

Request keeps on messing up higher bytes in some binary data I want to test in a hook.

📝 Related issues and Pull Requests

Epic: empty responses and binary files
#617, #87

✅ What didn't I forget?

• To write docs
• To write tests
• To put Conventional Changelog prefixes in front of all my commits and run npm run lint

[honzajavorek]

Thanks for the patch! If I understand this correctly, with this change we keep utf-8 as default encoding for the response, but we allow this to be overridden in hooks, so binary responses can be tested. Am I correct?

That's pretty neat. We need tests for this though so we can be sure it works as expected and that it really solves testing of the binary responses. Would you be willing to contribute those as well?

One of my concerns would also be whether this would work also for non-JavaScript hooks. In JS hooks you can set the encoding so the body is consumed as buffer, but I'm not sure what the result would be in other languages. Forcing Buffer means setting the encoding to null, which also might be tricky in some languages.

What do you think about this?

[honzajavorek]

I don't think this is necessary. Dredd is being published on CI exclusively, by Semantic Release. Also, the engines field clearly states "node": ">= 4".

[honzajavorek]

Ah, I did not notice the commit message during review:

See https://iamakulov.com/notes/npm-4-prepublish/ , npm/npm#16685 , and https://stackoverflow.com/a/44136814/1098906

Could you send this change in a separate PR, please? At the same time, I'm still not completely sure about it, because Dredd is being published on CI exclusively, so we could just make sure that package.json contents match the npm version which publishes the package. That basically means just pinning npm to a specific version in the Travis CI config and then having the package.json specific to that npm version.

[bb010g]

Sure, I can separate that out. I ended up sticking it in here because of the need to depend on my Git branch with Yarn, and yarnpkg/yarn#2875 / yarnpkg/yarn#3553 requiring an explicit prepare for that. The benefit of using this slightly more defensive syntax is that, if you need to do source compilation for whatever reason, you can still work with an older npm. If we don't care about npm 3 compatibility though, the prepublish script can be dropped entirely.

[honzajavorek]

Hmm makes sense. Let's do this, then!

[bb010g]

For non-JS hooks, you'd probably want to convert any Buffers you find to/from the native byte array type for that language before & after each hook. I'm not too familiar with how the hook architecture works, though, so I'd have to figure out where all that's handled. Since we're already wrapping the request options, having an extra 'bytes' option that maps to null should work.

As far as I've tested, Buffers are safe to have any time in request.body before sending. 'binary' encoded strings will also remain as-is until Request goes to send them, in which case they'll be converted to 'utf8'. Converting to a Buffer here at the end of a before* hook works fine. As for dealing with binary data on the real.body end, Gavel wants a string, so you have to convert back to a string at the end of a before*Validation hook. Automatically performing this step should be fine, but wouldn't be necessary to make this usable by other languages.

[honzajavorek]

I'm very sorry we were not able to get back to this. It's a significant change, which needs some design work and a lot of tests to be completely sure it works as it should be and that we're not breaking something.

I need to close this as we're going to work on #705 first, which requires having no pending Pull Requests. We'll get back to this proposal as soon as we're addressing the rest of the "empty responses and binary files" epic (the empty responses were implemented in #906).

Thanks for all the work! I'll do my best for this contribution not to be forgotten!

[honzajavorek]

@bb010g I'm trying to revisit this and I'm struggling to reproduce the problem you had:

Request keeps on messing up higher bytes in some binary data I want to test in a hook.

Would you be able to provide an example of such data so I can make it part of Dredd's test suite? I thought I'd be able to reproduce this with any binary data, but I wasn't successful with an arbitrary PNG image.

[bb010g]

Sorry, I'm not using Dredd currently and don't have access to the old files, but IIRC it was failing on bytestrings that weren't valid UTF-8. A single byte of 0xff should do it.

[honzajavorek]

This is going to get resolved, tested, and documented in #1094. Thank you @bb010g for pointing the issue out, for trying to provide a solution, and for discussing the issue. It was really helpful!