clarkie / dynogels

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

Migration from vogels: undefined fields make update operation to fail #58

Closed endymion00 closed 6 years ago

endymion00 commented 7 years ago

I migrated from vogels to dynogels and I've noticed that one my unit tests is failing with this message: "Uncaught AssertionError: expected [ValidationException: Invalid UpdateExpression: An expression attribute value used in expression is not defined; attribute value: :hashtag] to not exist"

The problem is that when updating a model by using MyModel.update(obj, function....) is failing due to obj.hashtag is undefined. When used vogels it didn't fail.

The solution seems to be set the field to null.

Is this an expected behavior? If so, what should I do in order to manage 'undefined' fields? Thank you in advance

jkav77 commented 7 years ago

DynamoDB doesn't permit undefined values in the update expression. What's the desired behavior for undefined attributes? Should dynogels just filter them out and ignore them on the update or should they be deleted from Dynamo?

endymion00 commented 7 years ago

I suppose the driver should omit this values before sending them to DynamoDB. Vogels.js is doing that way because my tests didn't fail (neither with null values nor undefined). By using dynogels I see the only and more comfortable approach is to use a lodash 'omit' function (or similar module) before sending the data to the .update method.

aneilbaboo commented 7 years ago

I think this is a related issue, so I'll put it here.

When you set a field inside an object to null in Model.update, it returns the object with that field set to null, but a subsequent Model.get returns the object with the field missing. This is inconsistent and seems like it will result in hard to catch bugs. Vogels' Model.update returns the object with the field stripped, just like it does from a subsequence Model.get.

E.g.,

in Vogels, update and get return the same value

// Vogels (using vogels-promisified)

var myModel = await Model.updateAsync({vars:{foo:null});
console.log('%j', myModel.get('vars')); // {}

var freshModel = await Model.getAsync('theidofthemodel');
console.log('%j', freshModel.get('vars')); // {}

in Dynogels, update and get return different values

// Dynogels (using dynogels-promisified)

var myModel = await Model.updateAsync({vars:{foo:null});
console.log('%j', myModel.get('vars')); // {foo:null}

var freshModel = await Model.getAsync('theidofthemodel');
console.log('%j', freshModel.get('vars')); // {}
jkav77 commented 6 years ago

Closing as stale

aneilbaboo commented 6 years ago

@dangerginger - Should this issue stay open? The issue I was seeing (different results returned by update versus get) was serious enough that I didn't end up using dynogels.

jkav77 commented 6 years ago

@aneilbaboo I think your issue is totally separate. Regarding @endymion00's original issue I think he was relying on undocumented behavior of vogels to treat undefined properties as a noop.

I think what you described is a bug though. Dynogels doesn't allow null values so the result from update should be the same as in get as you showed in your example: {}.

aneilbaboo commented 6 years ago

Makes sense. I'll create a new issue and add a script that demonstrates the problem.

aneilbaboo commented 6 years ago

Actually, it looks like the issue is fixed in the latest version.