Skip to content
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

Conditional updates are incorrect in README #657

Closed
jplaisted opened this issue May 22, 2023 · 2 comments · Fixed by #660
Closed

Conditional updates are incorrect in README #657

jplaisted opened this issue May 22, 2023 · 2 comments · Fixed by #660

Comments

@jplaisted
Copy link

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
Copy link
Member

Nice catch! Will fix README soon.

@jplaisted
Copy link
Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants