ryanfitz / vogels

DynamoDB data mapper for node.js
Other
698 stars 125 forks source link

On table.create, the ConditionExpression isn't merged correctly #108

Open asargento opened 8 years ago

asargento commented 8 years ago

Assume that it is necessary to specify a Condition Expression for create and also the option overwrite:

var params = {};
params.ConditionExpression = '#n <> :x';
params.ExpressionAttributeNames = {'#n' : 'name'};
params.ExpressionAttributeValues = {':x' : 'Kurt Warner'};
params.overwrite = false;

User.create({id : 123, name : 'Kurt Warner' }, params, function (error, acc) { ... });

Then, ConditionExpression is not set correctly because of the following line in file table.js:

189 params = _.merge({}, params, options);

The merge overwrites the ConditionExpression and instead of "(id <> :id) AND #n <> :x, you get #n <> :x. The lodash merge doesn't merge correctly the ConfitionExpression. And the following error is issue by AWS SDK:

'Error: ValidationException: Value provided in ExpressionAttributeNames unused in expressions: keys: {#id}'.

RodH257 commented 8 years ago

Is there any workaround known for this? Or is this stopping Vogels from supporting conditional writes completely?

asargento commented 8 years ago

As far as I know, this stops Vogels from supporting Condition Expressions (I don't know any workaround).

clarkie commented 8 years ago

Hi @asargento & @RodH257, I have raised this as an issue on https://github.com/clarkie/dynogels/issues/7 and plan to look at it later today or tomorrow. I'd be interested to know how far you have got with your investigations.

Thanks

Clarkie

asargento commented 8 years ago

Hello. I had performed some debugging session to find the problem. The issue description summarises what I found during this debugging.

Regards,

António Sargento

On 29 Jun 2016, at 09:46 , Clarkie notifications@github.com wrote:

Hi @asargento https://github.com/asargento & @RodH257 https://github.com/RodH257, I have raised this as an issue on clarkie/dynogels#7 https://github.com/clarkie/dynogels/issues/7 and plan to look at it later today or tomorrow. I'd be interested to know how far you have got with your investigations.

Thanks

Clarkie

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ryanfitz/vogels/issues/108#issuecomment-229295138, or mute the thread https://github.com/notifications/unsubscribe/ABquqF-jpOpnVhvEZWJlBMU0PEa68zeoks5qQjD9gaJpZM4Gn6Oh.

RodH257 commented 8 years ago

@clarkie I realised that the issue we were investigating wasn't actually related to this. We were just trying to use 'ConditionExpression' without changing the setting for overwrite and had an unrelated bug.

However I'm a little confused as to what @asargento use case is - correct me if I'm wrong, but I think condition expressions run within the context of a hash/range key combination (otherwise it would probably have to do a table scan right?). So if overwrite is false, your condition expression should be meaningless?

asargento commented 8 years ago

I understood the functioning of the .create function as explain below. When creating an item, the condition that would evaluated is “id <> :id”. What I was expecting is if I want to add an extra condition to this one, I should specify it and said to that I don’t want to overwrite the default (in other words, “id <> :id AND #n <> :x”). If overwrite is set to true, then the default condition should be overwritten by the one specified.

Is this correct?

António Sargento

On 30 Jun 2016, at 01:40 , Rod Howarth notifications@github.com wrote:

@clarkie https://github.com/clarkie I realised that the issue we were investigating wasn't actually related to this. We were just trying to use 'ConditionExpression' without changing the setting for overwrite and had an unrelated bug.

However I'm a little confused as to what @asargento https://github.com/asargento use case is - correct me if I'm wrong, but I think condition expressions run within the context of a hash/range key combination (otherwise it would probably have to do a table scan right?). So if overwrite is false, your condition expression should be meaningless?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ryanfitz/vogels/issues/108#issuecomment-229529811, or mute the thread https://github.com/notifications/unsubscribe/ABquqPtDyQ8EdHg8na8bkVto2jPd8NkMks5qQxBngaJpZM4Gn6Oh.

RodH257 commented 8 years ago

I think the key thing (which isn't clear in their docs, and is what was confusing me when I commented on this ticket initially) is that the conditional expressions are conditions on a certain item that you must identify. Ie they run within the context of a partition key. So the #n <> :x part of your statement can be thought of as (id = :id AND #n <> :x).

So your condition ends up as something more like id <> :id AND (id = :id AND #n <> :x) which doesn't work.

Think of it as you saying 'update this specific item with this id, but only if this property of the item doesn't match a certain condition'. That condition can be that the id doesn't exist, or it can be that a certain property of the item with that id doesn't meet some certain criteria, but it cannot look at other items.

I guess the reason is that AWS doesn't support this might be that to do the #n <> :x part of your expression, dynamodb would have to do a whole table scan - which it won't do as part of conditional expressions.

This is my understanding anyway!

asargento commented 8 years ago

On 01 Jul 2016, at 06:03 , Rod Howarth notifications@github.com wrote:

I think the key thing (which isn't clear in their docs, and is what was confusing me when I commented on this ticket initially) is that the conditional expressions are conditions on a certain item that you must identify. Ie they run within the context of a partition key. So the #n <> :x part of your statement can be thought of as (id = :id AND #n <> :x).

So your condition ends up as something more like id <> :id AND (id = :id AND #n <> :x) which doesn't work.

But probably what is wrong is the condition. It should be id <> :id OR (id = :id AND #n <> :x). And this case, the all condition should be overwritten.

Think of it as you saying 'update this specific item with this id, but only if this property of the item doesn't match a certain condition'. That condition can be that the id doesn't exist, or it can be that a certain property of the item with that id doesn't meet some certain criteria, but it cannot look at other items.

I guess the reason is that AWS doesn't support this might be that to do the #n <> :x part of your expression, dynamodb would have to do a whole table scan - which it won't do as part of conditional expressions.

This is my understanding anyway!

I think that you are correct and my mistake was forgetting to consider that the range key is always on the context of a partition key. The condition that I need is id <> :id OR (id = :id AND #n <> :x). I will update the issue with what we have been discussing...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ryanfitz/vogels/issues/108#issuecomment-229854496, or mute the thread https://github.com/notifications/unsubscribe/ABquqIG2QUS5v884X7nP1y6ebRxYUfHcks5qRJ-YgaJpZM4Gn6Oh.

António Sargento

RodH257 commented 8 years ago

In that case, would you be able to do overwrite = true with just a condition expression of #n <> :x? so it will overwrite it only if that key doesn't exist, or if the key exists & n <> x ?

asargento commented 8 years ago

Its one way to do it. But it should also work with the following:

var params = {}; params.ConditionExpression = ‘id = :id AND #n <> :x'; params.ExpressionAttributeNames = {'#n' : 'name'}; params.ExpressionAttributeValues = {':x' : 'Kurt Warner’}; params.overwrite = false;

User.create({id : 123, name : 'Kurt Warner' }, params, function (error, acc) { ... });

And in some way to control the usage of AND and OR on line 335 of table.js (probably passing it on params, but I need to check the DynamoDb documentation)

334 if (_.isString(params.ConditionExpression)) { 335 params.ConditionExpression = ${params.ConditionExpression} AND (${condition.statement}); 336 } else { 337 params.ConditionExpression = (${condition.statement}); 338 }

But as I said, I will update this issue and probably close it

António Sargento

On 01 Jul 2016, at 08:46 , Rod Howarth notifications@github.com wrote:

In that case, would you be able to do overwrite = true with just a condition expression of #n <> :x? so it will overwrite it only if that key doesn't exist, or if the key exists & n <> x ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

asargento commented 8 years ago

After analysing a little better, I realize that if there isn't any way to optionally control the AND operator on line 335, then if we to do something interesting with the ConditionExpression, we need to define it and then set the params.overwrite = true (the condition expression, with overwrite set to false is always added to condition id <> :id). So, if we can't choose between AND or OR operators, then issue is not a bug and we should close it.