transactions multi-table MVP #688

ckhsponge · 2023-10-12T06:10:54Z

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.

andrykonchin

I've left some comments. It isn't a call to action to make any change but is a way to get you more familiar with the project structure and its conventions.

lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb

ckhsponge · 2023-11-09T00:02:21Z

@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.

andrykonchin

Looks pretty good!

lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb

lib/dynamoid/transaction/executor.rb

lib/dynamoid/transaction/create.rb

lib/dynamoid/transaction/update.rb

andrykonchin · 2023-11-12T15:30:17Z

lib/dynamoid/transaction/upsert.rb

+            table_name: model_class.table_name,
+          }
+        }
+      end


Class-level methods like create/update/upsert accepts keys and attributes as separate parameters:

User.upsert('1', age: 26)

This way we can find the proper item in DynamoDB even when user updates sort key for instance (can he change a partition key? probably he can)

Yes! I changed update and delete to take an id which can be a hash_key or an [hash_key, range_key] like is used by find(). I don't think id is needed for create and upsert since those are both putting complete records. Should we include id anyways for those?

@andrykonchin I assumed that the partition key could be changed but that is not true. A record must be deleted and recreated if you wish to change the partition key. How would you feel about simplifying the APIs for update and upsert?

Instead of:
txn.update!(User, '1', {name: 'bob'} )
it would be:
txn.update!(User, {id: '1', name: 'bob'} )

When there's a range key:
txn.update!(User, ['1', 26], {name: 'bob'} )
would be:
txn.update!(User, {id: '1', age: 26, name: 'bob'} )

This makes the API similar to create which I like but perhaps it's too inconsistent with the other update and upserts.

This makes the API similar to create which I like but perhaps it's too inconsistent with the other update and upserts.

It seems to me it will not work for updating a range key attribute. Its current value should be used to find an item and a new value to set.

Current approach for transaction methods is to be as close to the existing methods as possible. If there are reasons to change method signature - it makes sense to change both existing method and a transactional one (but it would be a breaking change so it would require a major release).

I don't think id is needed for create and upsert since those are both putting complete records. Should we include id anyways for those?

So create shouldn't accept id as a separate parameter, but upsert should.

No part of a primary key can be updated. The record must be deleted and re-created to change the key. If it's a composite key this includes the range key which cannot be changed.

I see your point about wanting to be consistent with the non-transaction methods however not allowing a separate key to be specified may be a better direction to go in. This will discourage people from trying to update primary keys among other things. I like the shorter method signature and that all the transaction method signatures are more similar as well.

No part of a primary key can be updated. The record must be deleted and re-created to change the key. If it's a composite key this includes the range key which cannot be changed.

Indeed, exception is raised:

One or more parameter values were invalid: Cannot update attribute age. This attribute is part of the key (Aws::DynamoDB::Errors::ValidationException)

lib/dynamoid/transaction/executor.rb

lib/dynamoid/transaction/create.rb

codecov · 2023-11-29T21:43:45Z

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 ⚠️
lib/dynamoid/transaction_write/update.rb	88.23%	2 Missing ⚠️
lib/dynamoid/transaction_write/action.rb	98.46%	1 Missing ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ckhsponge · 2023-11-30T00:04:47Z

@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.

andrykonchin

Looks good!

andrykonchin · 2023-11-30T14:36:47Z

lib/dynamoid/adapter_plugin/aws_sdk_v3.rb

@@ -8,6 +8,7 @@
 require_relative 'aws_sdk_v3/item_updater'
 require_relative 'aws_sdk_v3/table'
 require_relative 'aws_sdk_v3/until_past_table_status'
+require_relative 'aws_sdk_v3/transact'


minor: Not the perfect name for a class.

Any suggestions? This class wraps TransactWriteItems and someday maybe TransactGetItems as named by AWS.

TBH I would move transact_write_items into the main plugin class. It's small and simple enough (what was a reason to move some methods into separate classes)

lib/dynamoid/transaction.rb

lib/dynamoid/transaction/action.rb

lib/dynamoid/transaction/delete.rb

lib/dynamoid/transaction/update.rb

lib/dynamoid/transaction/upsert.rb

lib/dynamoid/transaction/delete.rb

ckhsponge · 2023-11-30T21:30:11Z

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

andrykonchin · 2023-12-01T15:35:33Z

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

ckhsponge · 2023-12-21T01:48:20Z

@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 · 2023-12-24T22:13:27Z

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 · 2023-12-26T22:43:35Z

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.

spec/dynamoid/transaction_write_spec.rb

andrykonchin · 2023-12-24T21:56:24Z

spec/dynamoid/transaction_write_spec.rb

+        expect(obj1_found.name).to eql("one")
+        expect(obj2_found.name).to eql("two")
+        expect(obj3_found.name).to eql("three")
+      end


It makes sense to test transaction rollback - non of changes should be persisted.

Also makes sense to check a case when some model isn't valid - it shouldn't rollback transaction.

Validation tests now check there is no rollback with non-bang saves and that there is a rollback with bang! saves.

Yeah, I see. So there are still cases to check that a transaction is rolled back when some of operations fails on DynamoDB side, e.g. deleting/updating not existing item, creation of item which primary key already exists.

So it makes sense to add a case for each transactional method (create/update/delete etc) to ensure that transaction is atomic.

spec/dynamoid/transaction_write_spec.rb

ckhsponge · 2023-12-29T11:27:45Z

@andrykonchin Thanks for the specs review. What are your thoughts on this one?
#688 (comment)

andrykonchin · 2023-12-31T23:21:34Z

A failing spec is fixed on master.

ckhsponge · 2024-01-17T21:11:23Z

@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!!!

andrykonchin · 2024-01-21T14:42:19Z

spec/dynamoid/transaction_write/put_spec.rb

+          expect(obj1_found.name).to eql("oneone")
+          expect(obj2_found.name).to eql("two")
+        end
+      end


~~I believe validation should be tested here as well~~

spec/dynamoid/transaction_write/upsert_spec.rb

andrykonchin · 2024-01-21T14:56:10Z

spec/dynamoid/transaction_write_spec.rb

+        expect(obj1_found.name).to eql("one")
+        expect(obj2_found.name).to eql("two")
+        expect(obj3_found.name).to eql("three")
+      end


Yeah, I see. So there are still cases to check that a transaction is rolled back when some of operations fails on DynamoDB side, e.g. deleting/updating not existing item, creation of item which primary key already exists.

So it makes sense to add a case for each transactional method (create/update/delete etc) to ensure that transaction is atomic.

andrykonchin

Specs look much better. Thank you!

spec/dynamoid/transaction_write/create_spec.rb

spec/dynamoid/transaction_write/upsert_spec.rb

andrykonchin · 2024-01-21T15:14:51Z

spec/dynamoid/transaction_write/put_spec.rb

+    include_context 'transaction_write'
+
+    # a 'put' is a create that overwrites existing records if present
+    context 'puts' do


The create method doesn't overwrite existing document with the same primary key and raises Dynamoid::Errors::RecordNotUnique instead.

https://github.com/Dynamoid/dynamoid/blob/master/lib/dynamoid/persistence/save.rb#L50

I'm allowing create() to behave like a "put" when skip_existence_check: true is used. This does an overwrite if a previous record exists. The relatively short put_spec tests this. Maybe renaming to allow_overwrite: true would be better? Or add a put()?

Currently in a transaction when trying to create a record when it already exists raises Aws::DynamoDB::Errors::TransactionCanceledException.

spec/dynamoid/transaction_write/context.rb

spec/dynamoid/transaction_write/save_spec.rb

andrykonchin · 2024-01-21T15:57:55Z

spec/dynamoid/transaction_write/create_spec.rb

+          expect(klass_with_validation.exists?(obj1.id)).to be_falsey
+          expect(obj2.id).to be_present
+          expect(klass_with_validation.exists?(obj2.id)).to be_falsey
+        end


I would check rolling back of the whole transaction in a separate test case with corresponding title. Here and in specs for other methods.

Added explicit rollback specs.

andrykonchin · 2024-01-21T16:00:47Z

spec/dynamoid/transaction_write/update_spec.rb

+          obj1 = klass.create!(name: "one")
+          obj1.name = "oneone"
+          Dynamoid::TransactionWrite.execute do |txn|
+            txn.update! obj1


update with instance has different signature:

dynamoid/lib/dynamoid/persistence.rb

Lines 748 to 750 in 398f065

# user.update(unless_exists: [:age]) do |t|

# t.set(age: 18)

# end

It's OK to skip this method for now.

OK, skipping support for blocks in update() for now. I would like to support increment eventually.

spec/dynamoid/transaction_write/update_spec.rb

spec/dynamoid/transaction_write/upsert_spec.rb

ckhsponge · 2024-01-22T19:47:13Z

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

ckhsponge · 2024-01-29T18:33:54Z

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 · 2024-01-29T18:34:18Z

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

ckhsponge · 2024-02-07T16:29:00Z

@andrykonchin Anything blocking this? Thanks.

andrykonchin · 2024-02-10T13:19:36Z

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 · 2024-02-10T13:20:59Z

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 · 2024-02-12T23:40:01Z

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

andrykonchin · 2024-02-13T19:01:09Z

Merged! 🎉 Great work!

ckhsponge · 2024-02-13T19:25:09Z

That's great news! Thanks for the support.

ckhsponge marked this pull request as draft October 12, 2023 06:30

andrykonchin reviewed Oct 25, 2023

View reviewed changes

lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb Outdated Show resolved Hide resolved

lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb Outdated Show resolved Hide resolved

lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb Outdated Show resolved Hide resolved

ckhsponge requested a review from andrykonchin November 9, 2023 00:00

andrykonchin reviewed Nov 12, 2023

View reviewed changes

andrykonchin reviewed Nov 13, 2023

View reviewed changes

lib/dynamoid/transaction/executor.rb Outdated Show resolved Hide resolved

andrykonchin reviewed Nov 13, 2023

View reviewed changes

lib/dynamoid/transaction/create.rb Outdated Show resolved Hide resolved

andrykonchin reviewed Nov 30, 2023

View reviewed changes

lib/dynamoid/transaction/delete.rb Outdated Show resolved Hide resolved

ckhsponge changed the title ~~transactions multi-table MVP WIP~~ transactions multi-table MVP Dec 21, 2023

ckhsponge marked this pull request as ready for review December 21, 2023 01:38

andrykonchin reviewed Dec 28, 2023

View reviewed changes

andrykonchin reviewed Jan 21, 2024

View reviewed changes

write transactions that can span tables

a5b7640

ckhsponge force-pushed the transact branch from f508203 to a5b7640 Compare February 12, 2024 21:32

andrykonchin merged commit 6b96d1d into Dynamoid:master Feb 13, 2024
58 of 60 checks passed

	# user.update(unless_exists: [:age]) do \|t\|
	# t.set(age: 18)
	# end

transactions multi-table MVP #688

transactions multi-table MVP #688

Conversation

ckhsponge commented Oct 12, 2023

andrykonchin left a comment

Choose a reason for hiding this comment

ckhsponge commented Nov 9, 2023

andrykonchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrykonchin Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

ckhsponge commented Nov 30, 2023

andrykonchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckhsponge commented Nov 30, 2023

andrykonchin commented Dec 1, 2023

ckhsponge commented Dec 21, 2023

andrykonchin commented Dec 24, 2023 • edited Loading

ckhsponge commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrykonchin Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

ckhsponge commented Dec 29, 2023

andrykonchin commented Dec 31, 2023

ckhsponge commented Jan 17, 2024

andrykonchin Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

andrykonchin Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

andrykonchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckhsponge commented Jan 22, 2024

ckhsponge commented Jan 29, 2024

ckhsponge commented Jan 29, 2024

ckhsponge commented Feb 7, 2024

andrykonchin commented Feb 10, 2024

andrykonchin commented Feb 10, 2024

ckhsponge commented Feb 12, 2024

andrykonchin commented Feb 13, 2024

ckhsponge commented Feb 13, 2024

andrykonchin Dec 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

andrykonchin commented Dec 24, 2023 •

edited

Loading

andrykonchin Jan 21, 2024 •

edited

Loading

andrykonchin Jan 21, 2024 •

edited

Loading

andrykonchin Jan 21, 2024 •

edited

Loading