fix: don't always convert bytes to text in streamToPromise

[ricellis]

streamToPromise calls toString() on the concated Buffer which defaults to utf-8 encoding and consequently can mangle non utf-8 content. This problem surfaced via IBM/cloudant-node-sdk#546.

• Remove toString() call when using Buffer type.

The problem is resolved by not calling toString() as the Buffer bytes may be passed as-is to the server for a byte stream. In cases where the stream is using string or objectMode not Buffer then the existing Buffer.isBuffer(results[0]) check puts the chunk as-is into the results array (i.e. it is an array of strings or objects).

• Update doc in streamToPromise to indicate it is not always text.
• Add test for binary type stream.
• Update assertions for Buffer streams to assert length and checksums.

Checklist

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

[dpopp07]

Looks good! 👍

[dpopp07]

@ricellis Before merging, I want to make sure you're aware of this invocation - https://github.com/IBM/node-sdk-core/blob/main/lib/request-wrapper.ts#L445

It is calling Buffer.from(...), which may not be necessary now that Buffers will be returned. Perhaps another conditional is needed there to avoid a bad type error? Let me know what you think

[ricellis]

It is calling Buffer.from(...), which may not be necessary now that Buffers will be returned. Perhaps another conditional is needed there to avoid a bad type error? Let me know what you think

Agreed. Though Buffer.from(buffer) is valid so it doesn't error (I did test streams going through the full core/client SDK). It just copies the Buffer again, but clearly it is wasteful to do so I'll add a check to avoid it.

[ricellis]

@dpopp07 I added the check in 1e9e691. FWIW I'm actually a little unsure about the "else" case - I don't understand what the expectation is for a returned Array when the stream is of string or in objectMode so I don't know that Buffer.from is actually what is wanted then, but the behaviour of that part hasn't been changed at all by this and I'm reluctant to make alterations outside of the scope of fixing the observed byte stream problem.

[ricellis]

@dpopp07 are you OK with the latest addition here? Would you like me to squash the commits?

[dpopp07]

@ricellis Sorry, I've been distracted by urgent work the last few days. I looked at that last change and it looks great.

but the behaviour of that part hasn't been changed at all by this and I'm reluctant to make alterations outside of the scope of fixing the observed byte stream problem

Agreed, no need to start tweaking existing behavior. If that becomes an issue, we will address it but it never has been in the past.

Unless there are any more changes you're planning to make, I will merge today.

[ricellis]

@dpopp07 np, I know the feeling! I'm not planning anything else for this so happy for it to merge whenever you get the chance, thanks so much.

[ibm-devx-sdk]

🎉 This PR is included in version 2.17.4 🎉

The release is available on:

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

Your semantic-release bot 📦🚀