Dynamoid / dynamoid

Ruby ORM for Amazon's DynamoDB.
MIT License
576 stars 197 forks source link

transactions multi-table MVP #688

Closed ckhsponge closed 6 months ago

ckhsponge commented 11 months ago

This is a first attempt at dynamodb transactions that can span tables. Create, update, upsert and delete are supported. See README_transact.md for examples of the proposed API.

Tests have not yet been implemented.

ckhsponge commented 10 months ago

@andrykonchin I made some updates to the transaction API and now there is no longer a mess of hashes being passed around. Your feedback would be appreciated. Some minor cleanup and tests are still needed.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (4520343) 90.32% compared to head (a5b7640) 90.89%.

Files Patch % Lines
lib/dynamoid/transaction_write/update_upsert.rb 90.62% 3 Missing :warning:
lib/dynamoid/transaction_write/update.rb 88.23% 2 Missing :warning:
lib/dynamoid/transaction_write/action.rb 98.46% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #688 +/- ## ========================================== + Coverage 90.32% 90.89% +0.56% ========================================== Files 62 71 +9 Lines 3154 3414 +260 ========================================== + Hits 2849 3103 +254 - Misses 305 311 +6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ckhsponge commented 9 months ago

@andrykonchin Another round of feedback would be appreciated. I believe all issues were covered although adding block support for updates is something that could still be done. This might be a cleaner place to add add() expressions.

I'd like to get to a v1 candidate at some point with unit tests included.

ckhsponge commented 9 months ago

@andrykonchin Thanks for the feedback. There are now proper delete and destroys. Upsert is working like an upsert as well.

andrykonchin commented 9 months ago

Looks good so we can move on and and add specs.

ckhsponge commented 8 months ago

@andrykonchin There's now a collection of specs for the transaction writes. Can you run them on github or give me permission to run them?

What else is needed to get v1 released?

Thanks!

andrykonchin commented 8 months ago

The new specs look good, but I would prefer to have more granular test cases that check only one aspect of a method behaviour (ideally only one check/assert per case)

ckhsponge commented 8 months ago

The new specs look good, but I would prefer to have more granular test cases that check only one aspect of a method behaviour (ideally only one check/assert per case)

Yeah, I usually put 4 or 5 syntax checks in each case. I'll split those out.

ckhsponge commented 8 months ago

@andrykonchin Thanks for the specs review. What are your thoughts on this one? https://github.com/Dynamoid/dynamoid/pull/688#discussion_r1436639724

andrykonchin commented 8 months ago

A failing spec is fixed on master.

ckhsponge commented 7 months ago

@andrykonchin update() and upsert() no longer allow a key to be specified separate from the attributes. A key must always be included in the model or in the attributes. This simplifies the method signatures and makes them consistent with the other transaction actions.

I believe I covered all your feedback on the specs so another round of review would be good. Thanks!!!

ckhsponge commented 7 months ago

@andrykonchin More tests were added for rollbacks and callbacks. Instance usage tests for upsert were removed.

ckhsponge commented 7 months ago

I removed the named parameter for options. This simplifies the brackets for expected usage e.g.:

save!( instance, skip_callbacks: true)
create!( klass, name: 'hi')
create!( klass, {name: 'hi'}, {skip_callbacks: true})

instead of:

save!( instance, options: {skip_callbacks: true})
create!( klass, {name: 'hi'})
create!( klass, {name: 'hi'}, options: {skip_callbacks: true})
ckhsponge commented 7 months ago

@andrykonchin I believe all feedback has been addressed and further review would be appreciated.

ckhsponge commented 7 months ago

@andrykonchin Anything blocking this? Thanks.

andrykonchin commented 7 months ago

Could you please fix Rubocop issues (related to the changes only - there are some existing issues on master - I will address them myself) and squash all the working commits into one or more atomic ones?

andrykonchin commented 7 months ago

I want to release a new version without transactions now and release transactions in the next version. The last release was a year ago and I was already asked for new version.

ckhsponge commented 6 months ago

@andrykonchin I squashed the commits and I believe Rubocops will pass. Anything else I need to do? There are Codeclimate complexity issues.

andrykonchin commented 6 months ago

Merged! :tada: Great work!

ckhsponge commented 6 months ago

That's great news! Thanks for the support.