Awkward API for handling `TransactionCanceled` from `transact_write_items`

[aclemons]

Overview

When using transact_write_items with multiple items, handling errors is inconsistent.

If the first item in the list caused the transaction to abort and it is one of the handled error types in aiodynamo:

https://github.com/HENNGE/aiodynamo/blob/922eb16fefcf94801897ccfe6c602fb063afa3c3/src/aiodynamo/errors.py#L172-L200

the cause is extracted and raised back up to the user:

https://github.com/HENNGE/aiodynamo/blob/922eb16fefcf94801897ccfe6c602fb063afa3c3/src/aiodynamo/errors.py#L211-L212

https://github.com/HENNGE/aiodynamo/blob/922eb16fefcf94801897ccfe6c602fb063afa3c3/src/aiodynamo/errors.py#L218-L223

If it is any other item in the list, it is unhandled and the TransactionCanceled is bubbled up.

This looks like it stems from this comment:

https://github.com/HENNGE/aiodynamo/pull/132#discussion_r939773600

This requires code using this API to handle errors in two different ways - if the code cares about the cause of the cancellation.

Say you want to update 3 items - each has a conditional check and but you want to handle the check failing differently for the three items.

To do this, you must have an except block for ConditionalCheckFailed - that handles the first item in the list, then you must also have an except block for TransactionCanceled and inspect the CancellationReasons - if the second or third reason has code ConditionalCheckFailed, then that item was the condition check which aborted the transaction.

For my use case, it would be clearer and simpler if we always raise the TransactionCanceled error. The other option, extending the inspection to all items in the CancellationReasons, could work, but has the disadvantage that there is not enough information to correlate back to the item which caused the rollback. For example, a rollback caused by the 5th item looks like this:

{'__type': 'com.amazonaws.dynamodb.v20120810#TransactionCanceledException', 'CancellationReasons': [{'Code': 'None'}, {'Code': 'None'}, {'Code': 'None'}, {'Code': 'None'}, {'Code': 'ConditionalCheckFailed', 'Message': 'The conditional request failed'}], 'Message': 'Transaction cancelled, please refer cancellation reasons for specific reasons [None, None, None, None, ConditionalCheckFailed]'}

If the existing conversion extends to all, you could handle the ConditionalCheckFailed in an except block, but would not know which item caused it.

Before I open a PR, I would like to ask, how do you handle this case in your code? Maybe you haven't needed to handle multiple items in different ways?

Depending on how we change this, it is potentially a breaking API change. One option to avoid that could be to make the client constructor take an option to configure the unwrapping behaviour. It defaults to this legacy behaviour, but also supports always unwrap and never unwrap.

Thoughts?

[ojii]

Thank you for this thorough bug report.

Before I open a PR, I would like to ask, how do you handle this case in your code? Maybe you haven't needed to handle multiple items in different ways?

we don't actually use transactions (yet) in our code.

Depending on how we change this, it is potentially a breaking API change. One option to avoid that could be to make the client constructor take an option to configure the unwrapping behaviour. It defaults to this legacy behaviour, but also supports always unwrap and never unwrap.

I'm okay with this being a breaking change. Making this a configurable behaviour would be quite awkward, complicate the code, tests and documentation.

[aclemons]

Excellent - that works for me. I'll open a PR for review.