Bug: BaseBatchProcessingError.child_exceptions doesn't handle None case

[peterschutt]

Expected Behaviour

This is a bit of a nitpick but something I noticed on my travels...

https://github.com/awslabs/aws-lambda-powertools-python/blob/1ca74d36af7165f41dc2d6b39a8f36caacd69677/aws_lambda_powertools/utilities/batch/exceptions.py#L11-L24

child_exceptions parameter is typed Optional[List[ExceptionInfo]] but the None case isn't handled anywhere before iteration over the attribute occurs in format_exceptions() method.

Current Behaviour

Instantiating the exception and calling format_exceptions() without providing a list for the child_exceptions param will raise an exception.

Code snippet

from aws_lambda_powertools.utilities.batch.exceptions import BaseBatchProcessingError

err = BaseBatchProcessingError(msg="something")  # types ok
err.format_exceptions("parent exception string")

Possible Solution

Most likely just make sure attrib is always a list without having to change the signature of the class.

class BaseBatchProcessingError(Exception):
    def __init__(self, msg="", child_exceptions: Optional[List[ExceptionInfo]] = None):
        ...
        self.child_exceptions = child_exceptions or []

Steps to Reproduce

See code snippet above.

AWS Lambda Powertools for Python version

latest

AWS Lambda function runtime

3.9

Packaging format used

PyPi

Debugging logs

No response

[boring-cyborg]

Thanks for opening your first issue here! We'll come back to you as soon as we can.

[michaelbrewer]

More likely to fail before this, so maybe child_exceptions should not be optional.

            raise SQSBatchProcessingError(
                msg=f"Not all records processed successfully. {len(self.exceptions)} individual errors logged "
                f"separately below.",
                child_exceptions=self.exceptions,
            )

[michaelbrewer]

@peterschutt if this is accepted as an 'issue', i made a PR (#1205) based on your fix.

[sthulb]

I can’t stress this enough @michaelbrewer, In an effort to prevent you from wasting time, you should wait for issues to prioritised, this won't be merged for a long time until we're ready to add new features.

[michaelbrewer]

I fully understand the risk of putting my unpaid time into this.

In this case, this is a bug and not a feature, so out of respect for my fellow Powertools users i submitted a fix PR for the one offending line

Feel free to drop these PRs or ignore until August 2022

[heitorlessa]

hey @peterschutt thank you for flagging another piece we can improve 🙏. We'll look into it after some major changes in exposing the roadmap, list of maintainers, and our new end-to-end testing framework (internal, but we might explore ways to expose to customers).

If you however ran into an issue with Batch without explicitly calling format_exceptions, please do let us know and either of the maintainers[1] can prioritize it ASAP.

From memory, the Optional was a leftover type hint after several refactoring attempts (legacy SQS -> new SQS), and while we're passing an actual exception as part of our failure_handler mechanics.... we can surely do better - besides this, I just spotted a few other areas for internal improvements so we can handle them altogether.

[1] Currrent maintainers: @am29d @mploski @sthulb @heitorlessa.

Thank you!

[michaelbrewer]

At Fiserv, our developers run into this kind of misleading typing of public apis and error handling within the library all of the time. Hopefully some stability comes through.

from aws_lambda_powertools.utilities.batch.exceptions import SQSBatchProcessingError
print(SQSBatchProcessingError("invalid input"))

Outputs:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/michaelbrewer/projects/python/aws-lambda-powertools-python/aws_lambda_powertools/utilities/batch/exceptions.py", line 37, in __str__
    return self.format_exceptions(parent_exception_str)
  File "/Users/michaelbrewer/projects/python/aws-lambda-powertools-python/aws_lambda_powertools/utilities/batch/exceptions.py", line 19, in format_exceptions
    for exception in self.child_exceptions:
TypeError: 'NoneType' object is not iterable

[michaelbrewer]

A typing based change would actually have to be a breaking change:

As msg becomes a required parameter to allow for child_exceptions to be an none-optional list

patch below:

diff --git a/aws_lambda_powertools/utilities/batch/exceptions.py b/aws_lambda_powertools/utilities/batch/exceptions.py
index d90c25f1..f33f6aef 100644
--- a/aws_lambda_powertools/utilities/batch/exceptions.py
+++ b/aws_lambda_powertools/utilities/batch/exceptions.py
@@ -9,7 +9,7 @@ ExceptionInfo = Tuple[Optional[Type[BaseException]], Optional[BaseException], Op


 class BaseBatchProcessingError(Exception):
-    def __init__(self, msg="", child_exceptions: Optional[List[ExceptionInfo]] = None):
+    def __init__(self, msg: str, child_exceptions: List[ExceptionInfo]):
         super().__init__(msg)
         self.msg = msg
         self.child_exceptions = child_exceptions
@@ -27,7 +27,7 @@ class BaseBatchProcessingError(Exception):
 class SQSBatchProcessingError(BaseBatchProcessingError):
     """When at least one message within a batch could not be processed"""

-    def __init__(self, msg="", child_exceptions: Optional[List[ExceptionInfo]] = None):
+    def __init__(self, msg: str, child_exceptions: List[ExceptionInfo]):
         super().__init__(msg, child_exceptions)

     # Overriding this method so we can output all child exception tracebacks when we raise this exception to prevent
@@ -40,7 +40,7 @@ class SQSBatchProcessingError(BaseBatchProcessingError):
 class BatchProcessingError(BaseBatchProcessingError):
     """When all batch records failed to be processed"""

-    def __init__(self, msg="", child_exceptions: Optional[List[ExceptionInfo]] = None):
+    def __init__(self, msg: str, child_exceptions: List[ExceptionInfo]):
         super().__init__(msg, child_exceptions)

     def __str__(self):
diff --git a/tests/functional/test_utilities_batch.py b/tests/functional/test_utilities_batch.py
index a5e1e706..3db2ec20 100644
--- a/tests/functional/test_utilities_batch.py
+++ b/tests/functional/test_utilities_batch.py
@@ -908,3 +908,7 @@ def test_batch_processor_error_when_entire_batch_fails(sqs_event_factory, record

     # THEN raise BatchProcessingError
     assert "All records failed processing. " in str(e.value)
+
+
+def test_child_exceptions_is_required():
+    assert SQSBatchProcessingError(msg="required", child_exceptions=[]).format_exceptions("example") == "example\n"

[peterschutt]

A typing based change would actually have to be a breaking change:

Quite possible that somewhere downstream people are relying on self.child_exceptions being None if not passed in. If you accept that, then my original suggestion of child_exceptions or [] in __init__() isn't really a safe change either.

The safest change would be to address it right at the point of iteration in format_exceptions(), e.g., :

     def format_exceptions(self, parent_exception_str): 
         ...
         for exception in self.child_exceptions or []: 
         ...

... then could use warnings and deprecate the behavior of not passing in the child_exceptions param when instantiating the error with the aim to eventually have it all nice and tidy.

Another point, the exception class doesn't really care that the exceptions are a list or not, just that they can be iterated over in format_exceptions(), so probably the constructor could be typed a little more liberally as well, Iterable or Collection.