Dynamoid / dynamoid

Ruby ORM for Amazon's DynamoDB.
MIT License
580 stars 195 forks source link

Conditional updates are incorrect in README #657

Closed jplaisted closed 1 year ago

jplaisted commented 1 year ago

I've discovered, that at least for upsert, the readme's examples of using conditions are incorrect. Namely; there is no if: keyword argument to that method. Instead, there is a positional argument at the end.

# bad (from readme)
Address.upsert(id, { city: 'Chicago' }, if: { unless_exists: [:id] })
# good 
Address.upsert(id, { city: 'Chicago' }, { unless_exists: [:id] })

I see this if keyword elsewhere in the README and I sadly don't have time to track down all instances where it may be correct (or incorrect).

Something that may help here is, in addition to fixing the readme, adding more tests. I don't see unless_exists in any spec. Everything in the README aught to be in a test :)

What happens if you run the bad example?

Dynamoid will never update items. This isn't a syntax error; I think its some ruby implicit kwargs to positional hash sillyness. Here's the definition of upsert

def upsert(hash_key_value, range_key_value = nil, attrs = {}, conditions = {})

So what ends up happening is the conditions variable ends up as {:if=>{:unless_exists=>[:id]}}. The rest of the code never expects this if, and treats it literally. The update item call to dynamo will have a expected:{"unless_exists"=>{value:{l:[{s:"id"}]}}}, which is not correct, and will cause a failure condition.

What happens if you run the good example?

Things work as intended. Code here knows to extract unless_exists from the conditions hash, and use that to form the dynamo call, which now will have expected:{"id"=>{exists:false}} instead.

andrykonchin commented 1 year ago

Nice catch! Will fix README soon.

jplaisted commented 1 year ago

Thanks!