Skip to content

fix(idempotency): Log nested exception message #1813

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

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
10 changes: 6 additions & 4 deletions aws_lambda_powertools/utilities/idempotency/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,9 @@ def _process_idempotency(self):
record = self._get_idempotency_record()
return self._handle_for_status(record)
except Exception as exc:
raise IdempotencyPersistenceLayerError("Failed to save in progress record to idempotency store") from exc
raise IdempotencyPersistenceLayerError(
"Failed to save in progress record to idempotency store", exc
) from exc

return self._get_function_response()

Expand Down Expand Up @@ -161,7 +163,7 @@ def _get_idempotency_record(self) -> DataRecord:
# Wrap remaining unhandled exceptions with IdempotencyPersistenceLayerError to ease exception handling for
# clients
except Exception as exc:
raise IdempotencyPersistenceLayerError("Failed to get record from idempotency store") from exc
raise IdempotencyPersistenceLayerError("Failed to get record from idempotency store", exc) from exc

return data_record

Expand Down Expand Up @@ -214,7 +216,7 @@ def _get_function_response(self):
self.persistence_store.delete_record(data=self.data, exception=handler_exception)
except Exception as delete_exception:
raise IdempotencyPersistenceLayerError(
"Failed to delete record from idempotency store"
"Failed to delete record from idempotency store", delete_exception
) from delete_exception
raise

Expand All @@ -223,7 +225,7 @@ def _get_function_response(self):
self.persistence_store.save_success(data=self.data, result=response)
except Exception as save_exception:
raise IdempotencyPersistenceLayerError(
"Failed to update record state to success in idempotency store"
"Failed to update record state to success in idempotency store", save_exception
) from save_exception

return response
38 changes: 30 additions & 8 deletions aws_lambda_powertools/utilities/idempotency/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,49 +3,71 @@
"""


class IdempotencyItemAlreadyExistsError(Exception):
from typing import Optional, Union


class BaseError(Exception):
"""
Base error class that overwrites the way exception and extra information is printed.
See https://github.com/awslabs/aws-lambda-powertools-python/issues/1772
"""

def __init__(self, *args: Optional[Union[str, Exception]]):
self.message = str(args[0]) if args else ""
self.details = "".join(str(arg) for arg in args[1:]) if args[1:] else None

def __str__(self):
"""
Return all arguments formatted or original message
"""
if self.message and self.details:
return f"{self.message} - ({self.details})"
return self.message


Copy link
Contributor

@leandrodamascena leandrodamascena Jan 5, 2023

Choose a reason for hiding this comment

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

Hello @mploski! This is a huge improvement indeed. I remember the first time I needed to use the idempotency utility, I got an error due to permissions and spent a lot of time figuring out the reason for the "Failed to save record in progress to idempotency store" message. Creating a specific class to capture and return detailed errors makes life and adoption easier!!

I was wondering if we really need to instantiate the parent Exception class or if we could change this code to directly return the error message and make the code more clean. But it's just an idea to think about, I'm not sure and would just like to hear your side.

class BaseError(Exception):
     """
    Base error class that overwrites the way exception and extra information is printed.
    See https://github.com/awslabs/aws-lambda-powertools-python/issues/1772
    """

    def __init__(self, *args: str):
        self.message = args[0] if args else ""
        self.args = args

    def __str__(self):
        """
        Return all arguments formatted or original message
        """
        if self.args:
            return f"{' - '.join(str(arg) for arg in self.args)}"
        return self.message

Copy link
Contributor Author

@mploski mploski Jan 7, 2023

Choose a reason for hiding this comment

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

Hey @leandrodamascena , thx for the feedback!
I like your proposal and I refactored my code to follow your suggestion i.e to modify how exception is printed in str dunder method. This approach also has additional benefit that we keep initial attributes ( in args list) in object itself so we can easily see how exception object was initialized during debugging if needed.

As for the super class calling. In theory it is suggested to call it as explained in Python Cookbook book: https://books.google.pl/books?id=f7wGeA71_eUC&pg=PA579&dq=To+illustrate+the+use+of+.args,+consider+this+interactive+session+with+the+built-in+RuntimeError+exception,+and+notice+how+any+number+of+arguments+can+be+used+with+the+raise+statement:&hl=en&sa=X&ved=2ahUKEwinm_Dy0bX8AhWMposKHQNHB3oQ6AF6BAgBEAI#v=onepage&q=To%20illustrate%20the%20use%20of%20.args%2C%20consider%20this%20interactive%20session%20with%20the%20built-in%20RuntimeError%20exception%2C%20and%20notice%20how%20any%20number%20of%20arguments%20can%20be%20used%20with%20the%20raise%20statement%3A&f=false.
I dove deep more to see what happens when we call Exception init method. It seems this behavior changed somewhere in newer python 3 releases and args list initialization was moved to new dunder method so all needed stuff is initialized either way even without calling superclass init method ( as oppose to python 2.x and 3.0 for example) . So I think we can safely omit it.
here is a c code for new method in exception class in python 3 main branch: https://github.com/python/cpython/blob/3.7/Objects/exceptions.c#L47 - you can see that we copy args to instance attribute
and in Python 3.0:
https://github.com/python/cpython/blob/v3.0/Objects/exceptions.c#L49 - you can see that this action is done only in init not in new

Can I ask you to review it once again? :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @mploski! Thank you very much for your feedback and all the links provided, it was very helpful to understand more deeply how the exception is handled and it arguments.

From my side everything is ok and we are ready to merge!

class IdempotencyItemAlreadyExistsError(BaseError):
"""
Item attempting to be inserted into persistence store already exists and is not expired
"""


class IdempotencyItemNotFoundError(Exception):
class IdempotencyItemNotFoundError(BaseError):
"""
Item does not exist in persistence store
"""


class IdempotencyAlreadyInProgressError(Exception):
class IdempotencyAlreadyInProgressError(BaseError):
"""
Execution with idempotency key is already in progress
"""


class IdempotencyInvalidStatusError(Exception):
class IdempotencyInvalidStatusError(BaseError):
"""
An invalid status was provided
"""


class IdempotencyValidationError(Exception):
class IdempotencyValidationError(BaseError):
"""
Payload does not match stored idempotency record
"""


class IdempotencyInconsistentStateError(Exception):
class IdempotencyInconsistentStateError(BaseError):
"""
State is inconsistent across multiple requests to persistence store
"""


class IdempotencyPersistenceLayerError(Exception):
class IdempotencyPersistenceLayerError(BaseError):
"""
Unrecoverable error from the data store
"""


class IdempotencyKeyError(Exception):
class IdempotencyKeyError(BaseError):
"""
Payload does not contain an idempotent key
"""
20 changes: 18 additions & 2 deletions tests/functional/idempotency/test_idempotency.py
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,19 @@ def test_idempotent_lambda_save_inprogress_error(persistence_store: DynamoDBPers
# GIVEN a miss configured persistence layer
# like no table was created for the idempotency persistence layer
stubber = stub.Stubber(persistence_store.table.meta.client)
stubber.add_client_error("put_item", "ResourceNotFoundException")
service_error_code = "ResourceNotFoundException"
service_message = "Custom message"

exception_message = "Failed to save in progress record to idempotency store"
exception_details = (
f"An error occurred ({service_error_code}) when calling the PutItem operation: {service_message}"
)

stubber.add_client_error(
"put_item",
service_error_code,
service_message,
)
stubber.activate()

@idempotent(persistence_store=persistence_store)
Expand All @@ -1083,9 +1095,13 @@ def lambda_handler(event, context):
lambda_handler({}, lambda_context)

# THEN idempotent should raise an IdempotencyPersistenceLayerError
# AND append downstream exception details
stubber.assert_no_pending_responses()
stubber.deactivate()
assert "Failed to save in progress record to idempotency store" == e.value.args[0]
assert exception_message == e.value.args[0]
assert isinstance(e.value.args[1], Exception)
assert exception_details in e.value.args[1].args
assert f"{exception_message} - ({exception_details})" in str(e.value)


def test_handler_raise_idempotency_key_error(persistence_store: DynamoDBPersistenceLayer, lambda_context):
Expand Down