Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion docs/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,9 @@ Exceptions
.. autoexception:: pynamodb.exceptions.TableError
.. autoexception:: pynamodb.exceptions.TableDoesNotExist
.. autoexception:: pynamodb.exceptions.DoesNotExist

.. autoexception:: pynamodb.exceptions.TransactWriteError
.. autoexception:: pynamodb.exceptions.TransactGetError
.. autoexception:: pynamodb.exceptions.InvalidStateError
.. autoexception:: pynamodb.exceptions.AttributeDeserializationError
.. autoexception:: pynamodb.exceptions.AttributeNullError
.. autoclass:: pynamodb.exceptions.CancellationReason
7 changes: 7 additions & 0 deletions docs/release_notes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@
Release Notes
=============

v5.4.0
----------
* Expose transaction cancellation reasons in
:meth:`~pynamodb.exceptions.TransactWriteError.cancellation_reasons` and
:meth:`~pynamodb.exceptions.TransactGetError.cancellation_reasons` (#1144).


v5.3.5
----------
* Fix message of some exceptions derived from :class:`~pynamodb.exceptions.PynamoDBException` (#1113).
Expand Down
4 changes: 4 additions & 0 deletions docs/transaction.rst
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ Now, say you make another attempt to debit one of the accounts when they don't h
# Because the condition check on the account balance failed,
# the entire transaction should be cancelled
assert e.cause_response_code == 'TransactionCanceledException'
# the first 'update' was a reason for the cancellation
assert e.cancellation_reasons[0].code == 'ConditionalCheckFailed'
# the second 'update' wasn't a reason, but was cancelled too
assert e.cancellation_reasons[1] is None

user1_statement.refresh()
user2_statement.refresh()
Expand Down
2 changes: 1 addition & 1 deletion pynamodb/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
"""
__author__ = 'Jharrod LaFon'
__license__ = 'MIT'
__version__ = '5.3.5'
__version__ = '5.4.0'
18 changes: 16 additions & 2 deletions pynamodb/connection/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
from pynamodb.exceptions import (
TableError, QueryError, PutError, DeleteError, UpdateError, GetError, ScanError, TableDoesNotExist,
VerboseClientError,
TransactGetError, TransactWriteError)
TransactGetError, TransactWriteError, CancellationReason,
)
from pynamodb.expressions.condition import Condition
from pynamodb.expressions.operand import Path
from pynamodb.expressions.projection import create_projection_expression
Expand Down Expand Up @@ -465,7 +466,20 @@ def _make_api_call(self, operation_name: str, operation_kwargs: Dict, settings:
verbose_properties['table_name'] = operation_kwargs.get(TABLE_NAME)

try:
raise VerboseClientError(botocore_expected_format, operation_name, verbose_properties)
raise VerboseClientError(
botocore_expected_format,
operation_name,
verbose_properties,
cancellation_reasons=(
(
CancellationReason(
code=d['Code'],
message=d.get('Message'),
) if d['Code'] != 'None' else None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why return None here? Shouldn't we instead not return anything at all?

The following code will break with AttributeError: 'NoneType' object has no attribute 'code':

        except pydb_exc.TransactWriteError as e:
            retry_err_types = (
                'ProvisionedThroughputExceededException',
                'ThrottlingError',
                'TransactionConflict',
            )
            for error in e.cancellation_reasons:
                if error.code in retry_err_types:
                    raise RuntimeError(f'transaction failed {error.code}')

Just seems like it'd be cleaner to not return anything vs returning None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cancellation reasons are 1:1 to the transaction items so you can know what item was the reason.

Would you rather the reason itself not to be none but for code to be none?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah to me that would feel cleaner but I guess it's subjective. Just negates the extra "if error and error.code"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, if it was "cleaner", it would leave users with no way of associating reasons with individual transaction items. Maybe a different interface would convey this better, e.g. mapping of item to reason.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that would make sense to me

)
for d in data.get('CancellationReasons', [])
),
)
except VerboseClientError as e:
if is_last_attempt_for_exceptions:
log.debug('Reached the maximum number of retry attempts: %s', attempt_number)
Expand Down
79 changes: 72 additions & 7 deletions pynamodb/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
"""
PynamoDB exceptions
"""

from typing import Any, Optional
from typing import Any
from typing import Dict
from typing import Iterable
from typing import List
from typing import Optional

import botocore.exceptions

Expand Down Expand Up @@ -99,18 +102,59 @@ def __init__(self, table_name: str) -> None:
super(TableDoesNotExist, self).__init__(msg)


class CancellationReason:
"""
A reason for a transaction cancellation.
"""
def __init__(self, *, code: str, message: Optional[str]) -> None:
self.code = code
self.message = message

def __eq__(self, other: Any) -> bool:
return isinstance(other, CancellationReason) and self.code == other.code and self.message == other.message


class TransactWriteError(PynamoDBException):
"""
Raised when a TransactWrite operation fails
"""
pass

@property
def cancellation_reasons(self) -> List[Optional[CancellationReason]]:
"""
When :attr:`.cause_response_code` is ``TransactionCanceledException``, this property lists
cancellation reasons in the same order as the transaction items (one-to-one).
Items which were not part of the reason for cancellation would have :code:`None` as the value.

For a list of possible cancellation reasons and their semantics,
see `TransactWriteItems`_ in the AWS documentation.

.. _TransactWriteItems: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_TransactWriteItems.html
"""
if not isinstance(self.cause, VerboseClientError):
return []
return self.cause.cancellation_reasons


class TransactGetError(PynamoDBException):
"""
Raised when a TransactGet operation fails
"""
pass
@property
def cancellation_reasons(self) -> List[Optional[CancellationReason]]:
"""
When :attr:`.cause_response_code` is ``TransactionCanceledException``, this property lists
cancellation reasons in the same order as the transaction items (one-to-one).
Items which were not part of the reason for cancellation would have :code:`None` as the value.

For a list of possible cancellation reasons and their semantics,
see `TransactGetItems`_ in the AWS documentation.

.. _TransactGetItems: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_TransactGetItems.html
"""
if not isinstance(self.cause, VerboseClientError):
return []
return self.cause.cancellation_reasons


class InvalidStateError(PynamoDBException):
Expand Down Expand Up @@ -141,8 +185,24 @@ def prepend_path(self, attr_name: str) -> None:


class VerboseClientError(botocore.exceptions.ClientError):
def __init__(self, error_response: Any, operation_name: str, verbose_properties: Optional[Any] = None):
""" Modify the message template to include the desired verbose properties """
def __init__(
self,
error_response: Dict[str, Any],
operation_name: str,
verbose_properties: Optional[Any] = None,
*,
cancellation_reasons: Iterable[Optional[CancellationReason]] = (),
) -> None:
"""
Like ClientError, but with a verbose message.

:param error_response: Error response in shape expected by ClientError.
:param operation_name: The name of the operation that failed.
:param verbose_properties: A dict of properties to include in the verbose message.
:param cancellation_reasons: For `TransactionCanceledException` error code,
a list of cancellation reasons in the same order as the transaction's items (one to one).
For items which were not a reason for the transaction cancellation, :code:`None` will be the value.
"""
if not verbose_properties:
verbose_properties = {}

Expand All @@ -152,4 +212,9 @@ def __init__(self, error_response: Any, operation_name: str, verbose_properties:
'operation: {{error_message}}'
).format(request_id=verbose_properties.get('request_id'), table_name=verbose_properties.get('table_name'))

super(VerboseClientError, self).__init__(error_response, operation_name)
self.cancellation_reasons = list(cancellation_reasons)

super(VerboseClientError, self).__init__(
error_response, # type:ignore[arg-type] # in stubs: botocore.exceptions._ClientErrorResponseTypeDef
operation_name,
)
30 changes: 27 additions & 3 deletions tests/integration/test_transaction_integration.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import uuid
from datetime import datetime

import botocore
import pytest

from pynamodb.connection import Connection
from pynamodb.exceptions import DoesNotExist, TransactWriteError, TransactGetError, InvalidStateError
from pynamodb.exceptions import CancellationReason
from pynamodb.exceptions import DoesNotExist, TransactWriteError, InvalidStateError


from pynamodb.attributes import (
Expand Down Expand Up @@ -160,12 +162,34 @@ def test_transact_write__error__transaction_cancelled__condition_check_failure(c
with TransactWrite(connection=connection) as transaction:
transaction.save(User(1), condition=(User.user_id.does_not_exist()))
transaction.save(BankStatement(1), condition=(BankStatement.user_id.does_not_exist()))
assert get_error_code(exc_info.value) == TRANSACTION_CANCELLED
assert 'ConditionalCheckFailed' in get_error_message(exc_info.value)
assert exc_info.value.cause_response_code == TRANSACTION_CANCELLED
assert 'ConditionalCheckFailed' in exc_info.value.cause_response_message
assert exc_info.value.cancellation_reasons == [
CancellationReason(code='ConditionalCheckFailed', message='The conditional request failed'),
CancellationReason(code='ConditionalCheckFailed', message='The conditional request failed'),
]
assert isinstance(exc_info.value.cause, botocore.exceptions.ClientError)
assert User.Meta.table_name in exc_info.value.cause.MSG_TEMPLATE
assert BankStatement.Meta.table_name in exc_info.value.cause.MSG_TEMPLATE


@pytest.mark.ddblocal
def test_transact_write__error__transaction_cancelled__partial_failure(connection):
User(2).delete()
BankStatement(2).save()

# attempt to do this as a transaction with the condition that they don't already exist
with pytest.raises(TransactWriteError) as exc_info:
with TransactWrite(connection=connection) as transaction:
transaction.save(User(2), condition=(User.user_id.does_not_exist()))
transaction.save(BankStatement(2), condition=(BankStatement.user_id.does_not_exist()))
assert exc_info.value.cause_response_code == TRANSACTION_CANCELLED
assert exc_info.value.cancellation_reasons == [
None,
CancellationReason(code='ConditionalCheckFailed', message='The conditional request failed'),
]


@pytest.mark.ddblocal
def test_transact_write__error__multiple_operations_on_same_record(connection):
BankStatement(1).save()
Expand Down