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

Add write transaction #468

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

btree1970
Copy link

Add transaction write feature to DynamoId.

  • Currently only works for adding multiple write operations

@andrykonchin
Copy link
Member

andrykonchin commented Aug 24, 2020

Looks pretty good 👍 .

The only issue I see is that transact method works just for the one model/table but DynamoDB's transaction can involve different tables in one call. I would like to have only one flexible API for write transactions. That's why I would like transact to work with several tables.

The only reason why transactions aren't added yet is that I am not sure what API should be.

I have following thoughts:

  • API should be high-level and operate model classes instead of table names
  • operations are hight-level and names match already existing methods like update_attributes/update_fields/upsert/...
  • callbacks should be called, at least some of them (e.g. for creation/deletion)

I considered following interface:

# Dyhamoid::Document.transaction or User.transaction

Dynamoid::Document.transaction do |txn|
  txn.create(User, attributes)
  txn.update(User, attributes)
  txn.update_fields(User, attributes, conditions)
  txn.upsert(User, attributes)
  txn.delete(User, id)
end

So please let me know what do you think about it.

@btree1970
Copy link
Author

Looks pretty good 👍 .

The only issue I see is that transact method works just for the one model/table but DynamoDB's transaction can involve different tables in one call. I would like to have only one flexible API for write transactions. That's why I would like transact to work with several tables.

The only reason why transactions aren't added yet is that I am not sure what API should be.

I have following thoughts:

  • API should be high-level and operate model classes instead of table names
  • operations are hight-level and names match already existing methods like update_attributes/update_fields/upsert/...
  • callbacks should be called, at least some of them (e.g. for creation/deletion)

I considered following interface:

# Dyhamoid::Document.transaction or User.transaction

Dynamoid::Document.transaction do |txn|
  txn.create(User, attributes)
  txn.update(User, attributes)
  txn.update_fields(User, attributes, conditions)
  txn.upsert(User, attributes)
  txn.delete(User, id)
end

So please let me know what do you think about it.

Hey thanks for the response. I admit i did this PR with a specific business case in mind. I like your suggestion for the new api design. Will work on it over the next few days.

@andrykonchin
Copy link
Member

Great.

I believe that proposed above approach isn't as good and consistent as it could be so there is room for improvement. For instance txn.create(User, attributes) seems ugly to me but I don't see better alternative.

@dpsk
Copy link

dpsk commented Jul 27, 2021

@andrykonchin what's the status on transact write support, is it planned? any help needed?

@andrykonchin
Copy link
Member

Transactions aren't supported right now. The main issue is that it isn't clear to me how the transactions API should look. Any opinion/proposition is welcomed.

@ckhsponge
Copy link
Contributor

ckhsponge commented Sep 12, 2023

Have there been any more thoughts on this? Below is an alternate API idea using an array of arrays. An array of hashes could work too. Personally I prefer single table usage of DynamoDB so don't have a need to span tables but I see why it should be supported.

Dynamoid::Document.transaction([
  [User, :create, attributes],
  [User, :update, attributes],
  [User, :update_fields, attributes, conditions],
  [User, :upsert, attributes],
  [User, :delete, id]
])

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 this pull request may close these issues.

None yet

5 participants