-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix 'in (false)' condition expression #159
base: main
Are you sure you want to change the base?
Conversation
|
Oh, wait, that line of code is indeed it prevents item field values |
ConditionExpression: '#a IN (:a)', | ||
UpdateExpression: 'SET #a = :b', | ||
ExpressionAttributeNames: {'#a': 'active'}, | ||
ExpressionAttributeValues: {':a': {BOOL: false}, ':b': {BOOL: true}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make test cases comprehensive, consider adding all kinds of JS falsy things: false, null, 0, 0.0, ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added another test for those values. null
and 0
/0.0
don't fail because they're encoded as {NULL:true}
and {N: "0"}
respectively, so their "attrVal" becomes true
and "0"
.
The one that confuses me as to how it didn't fail is ""
({S:""}
) because I feel like that should have the same issue but doesn't for reasons I cannot understand.
ExpressionAttributeValues: {':t': {BOOL: true}, ':v': value}, | ||
}), function(err, res) { | ||
if (err) return cb(err) | ||
res.statusCode.should.equal(200, `Update failed when checking for {${Object.keys(value)[0]}:${Object.values(value)[0]}}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use JSON.stringify(value)
over custom keys/values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me.
@mhart ?
dynalite incorrectly rejects condition expressions when using
IN
withfalse
.For example, if we have an item
{key: {S: "key"}, val: {BOOL: false}}
and do an update with a condition expression{ConditionExpression: "#v IN (:f)", ExpressionAttributeNames: {"#v": "val"}, ExpressionAttributeValues: {":f": {BOOL: false}}
it will fail with conditional check failed, even though it should match (tested against real AWS DynamoDB and dynamodb-local).I tried to fix it myself but unfortunately my JS skills proved to be insufficient, but I did manage to write a test that illustrates the issue, so hopefully someone else can write the actual fix for it.I figured out a fix, but I don't fully understand this whole code base so I'm not sure if the line I removed to fix this is somehow needed for other things to work