search

mapbox / dyno

simple dynamodb client
MIT License
78 stars 28 forks source link

Catch undefined value #89

Closed jakepruitt closed 8 years ago

jakepruitt commented 9 years ago

When a value is undefined for an item property, calling toDynamoTypes throws an error Cannot read property 'datatype' of undefined that originates in the call to formatDataType in the dynamo-doc code (https://github.com/mapbox/dyno/blob/master/lib/types.js#L17).

As per discussion, an undefined value should be ignored and not sent to dynamo, unless the attribute is a database key, in which case it should callback with an error rather than throwing.

rclark commented 9 years ago

So I added these tests that fail: https://github.com/mapbox/dyno/commit/520bf849ecca4689c6e1df2f6f14d7720675aeb1

With a trivial change in dynamodb-doc, these tests pass: https://github.com/rclark/dynamodb-document-js-sdk/commit/9e4e3aaa1ed31d51563d559b23d17f63fc99a96f

How about I try to get that PR into dynamodb-doc, and in the meantime if we need to can we get away with using a dynamodb-doc fork? cc @jakepruitt @willwhite

rclark commented 9 years ago

Hah maybe I am a complete liar. need more time.

rclark commented 9 years ago

I am only sort of a liar: I still think we should stopgap while trying to land a PR on dynamodb-doc.

The weird part is, look at the assertions in https://github.com/mapbox/dyno/commit/f1f7f8cc1be242a94283bac115abcc3f8c426cd4 and note that the undefined property appears in the object after we run the type conversion. This worried me: would the aws-sdk accept this?

So I wrote a quick test that proves that yes, it is okay to provide the aws-sdk with undefined properties: see https://gist.github.com/rclark/1cc2aa3e960801a900f8 if you want to check my proof.

rclark commented 9 years ago

https://github.com/awslabs/dynamodb-document-js-sdk/pull/34