search

clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

.destroy(x) fails if x does not exist #52

Closed jkav77 closed 7 years ago

jkav77 commented 7 years ago

fixes #44

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.003%) to 98.636% when pulling 701275f68dfa4272f322527ed80049df28868d01 on dangerginger:destroy-require-exists into cd503bd5d975c68ccf14f63df39c64dd5802a578 on clarkie:master.

deedw commented 7 years ago

I believe this change will affect code that relies on existing behaviour and departs from the documented functionality of underlying DeleteItem DynamoDb call.

As written in this pull request, there is a new conditional check that the item exists which cannot be adjusted using passed parameters.

If a change is to be made then I believe it should be an optional parameter that indicates that the item should exist. Default behaviour would be as existing code.

jkav77 commented 7 years ago

I agree this breaks existing behavior and the current behavior is consistent with Dynamo DB. Checking for the existence of the item with a conditional check is easily achieved with undocumented behavior, which is why I submitted #53.