Fix data corruption when storing binary data

[peecky]

Hello, I fixed a bug of storing corrupted binary data.

For example, following code did not properly store the value.

const schema = new dynamoose.Schema({
    key: { type: String },
    value: { type: Buffer }
});
const Model = dynamoose.model('KVS', schema);

const s = Buffer.from([0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x13, 0xd3, 0x61, 0x60, 0x60]);
console.log(s); // <Buffer 1f 8b 08 00 00 00 00 00 00 13 d3 61 60 60>

await Model.batchPut([{ key: 'key1', value: s }]);
const c = (await Model.get({ key: 'key1' })).value;
console.log(c); // <Buffer 1f ef bf bd 08 00 00 00 00 00 00 13 ef bf bd 61 60 60>

Because not every binary data can be converted to UTF-8 string, also the AWS SDK supports both Buffer and string for type B, it is better not to convert the value to string when the type is B and the value is Buffer instance.

[coveralls]

Pull Request Test Coverage Report for Build 584

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.3%) to 84.256%

---

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	1	78.14%

Totals
Change from base Build 581:	0.3%
Covered Lines:	1849
Relevant Lines:	2124

---

💛 - Coveralls

[fishcharlie]

@peecky Shouldn't this include some tests as well?

[peecky]

@fishcharlie Thanks. :)

[fishcharlie]

You reported this as a bug in Dynamoose. So the broken version should be made into a separate (new) test. Without your fix this test should fail. With your fix the test should pass. It should be a 100% fully unique test.

[fishcharlie]

These two lines should be included within a test and not globally scoped.

[fishcharlie]

This whole section should probably be included in a separate test, not this one.

[fishcharlie]

@peecky Thanks for this! Just submitted a review with some more changes 😃

[fishcharlie]

@peecky This looks good to me. My only other concern is that this could be a breaking change to some users. What are your thoughts on this?

[peecky]

I can’t imagine any cases that the current version is useful or works well but new version breaks the working.

Strictly speaking, however, it is a breaking change. It might be better to apply this change to the next major version. The bug is still avoidable on the current version by setting toDynamo.

const schema = new dynamoose.Schema({
    key: { type: String },
    value: { type: Buffer, toDynamo: x => ({ B: x }) }
});

[fishcharlie]

LGTM to be released in v1.0

[fishcharlie]

@peecky We will get this in for version 1.0. Thanks!