Clever / wag

sWAGger - Web API Generator
Apache License 2.0
77 stars 6 forks source link

Add transaction support to x-db #400

Closed nikhilb4a closed 3 years ago

nikhilb4a commented 3 years ago

This PR adds support to the auto-gen dynamo code for transactions. There is an EnableTransactions field on the x-db object that takes in a list of schemas that the table can write transactions with. The transactions also have conditions that must be met in order for the transaction to succeed in the case of updates.

One interesting decision is where the definition for which transactions are enabled should live - right now I have it on one of the two schemas involved in the transaction, as that is something that fits with the current design of x-db and is a place that felt natural when it comes to using the field (as I set up a table, I may think which other tables I will transact with). That being said, it means that it could end up being unnecessarily set on both tables and it means there are places we need to access the other tables config in order to have the template code follow the current pattern (eg. tests, accessing table name).

To show how this would be useful, I've created a PR in messaging-service that has the changes from our current state to one that uses these wag transactions. I've tested that it builds and the transactions work to fully replace our current approach.

The two situations it replaces are -

  1. Writing a new channel message while updating the corresponding channel object to increment the message count and update the newest message preview
  2. Deleting a channel message while updating the corresponding channel object to mark the newest message preview as deleted (if the deleted message is the newest message)

Todo:

This code still has some work to be done including more thorough tests, but Im hoping to get feedback on the approach before continuing down this path.

nikhilb4a commented 3 years ago

Thanks for the review, Taylor! And for our chat the other day to help me more forward.

I would also consider accepting the conditions in a more general format - with this API, it's hard to extend this to support anything other than "equals" conditions. Maybe the simplest thing to do is to have the API just accept an expression.ConditionBuilder rather than a map if the plan is not to make some type-safe version using the generated Go model.

This is an interesting point. I've gone back and forth a bit on this. I thought the approach I used might be a good starting point to abstract away the condition building for ease of use if it seemed that most conditions could be satisfied with an "equals" condition (which is the case for both of my use cases), thinking we could maybe expand on in the future if we do see other use cases come up. That being said, since this approach is already not type-safe, maybe it does make sense to go the more general way of accepting an ConditionBuilder.

nikhilb4a commented 3 years ago

Updated the interface to take in a *expression.ConditionBuilder instead of an equality condition map. More flexible and still pretty easy to use! Example here