fix: compression handling of string streams

[ricellis]

Checklist

• npm test passes (tip: npm run lint-fix can correct most style issues)
• tests are included
• documentation is changed or added

There is a bug where a stream of string chunks does not get correctly compressed.
The reason is that streamToPromise can return either a Buffer or an array of string chunks.
For the string array stream case request-wrapper, for streamData that isn't Buffer,request-wrapper calls Buffer.from(streamData) for the array, but that doesn't provide the content of the stream because Buffer.from(array) expects an array of utf8 bytes, not an array string .

I added a new test assertion to gzipRequestBody › should compress stream data into a buffer to demonstrate this:

 FAIL  test/unit/request-wrapper.test.js
  ● gzipRequestBody › should compress stream data into a buffer

    expect(received).toBe(expected) // Object.is equality

    Expected: "request body"
    Received: ""

This assertion passes correctly when request wrapper is changed to call Buffer.from(streamData.join('') to convert the array of strings into a single string suitable for Buffer.from(string).

Happy for any feedback on a nicer way to do this than join.

As far as I could see gzipSync requires a Buffer and this is the only place streamToPromise is called in the core so it might be better to remove this ternary and streamToPromise entirely and just call buffer(stream) from stream/consumers. However, it wasn't clear whether streamToPromise was considered API and might be used by external callers so I tried for a fix that didn't change the contract of that function at all.

[pyrooka]

Wow, looks good thanks a lot for fixing this! I'm surprised that we didn't check the actual value in this scenario thus we had an incorrect code for years. Nice catch!

Happy for any feedback on a nicer way to do this than join.

I've no better idea than this.

However, it wasn't clear whether streamToPromise was considered API and might be used by external callers so I tried for a fix that didn't change the contract of that function at all.

AFAIK, we don't use it anywhere else but I'm also unsure about the external usage of this function. I agree with you, let's keep it for now. It shouldn't hurt us.

[ibm-devx-sdk]

🎉 This PR is included in version 5.4.6 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀