powertools-lambda-python: Bug: BaseBatchProcessingError.child_exceptions doesn't handle None case

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

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 2
Comments: 23 (22 by maintainers)

Commits related to this issue

fix(batch): allow child_exceptions to be none Change: - Allow for child_exceptions being none in format_exceptions fixes #1203 — committed to gyft/aws-lambda-powertools-python by michaelbrewer 2 years ago

Most upvoted comments

Hey @michaelbrewer. I haven’t spent any time looking deeper at the typing yet, but appreciate the extra context. As per @heitorlessa’s earlier message I’ve emailed to arrange a chat, just waiting on that as I don’t really want to put the time into it until I understand what the objectives/constraints are and how the work would fit into their roadmap.

However we can’t add it as part of the project as the library still supports Python 3.6.

I assume this library just has to track the lambda runtime environment support, which is August 17 per this (8 months after the true EOL of 3.6). Assuming that 3.7 is supported for as long after actual EOL, then we’ve probably got until early 2024 to support 3.7. Supporting 3.7+ with this work means we could use typing-extensions which has already dropped 3.6.

client: Any = None is a hack to make MyPy happy as the assignment would fail.

Mypy should just assume the type based on the self assignment in __init__() (ref). If I comment out the client: Any = None class variable assignment, mypy doesn’t raise any issue for me (using project’s mypy config). However, that is on line 63 in the develop branch, and on line 68 in your image above and the self.client assignment is different in the develop version to your image above, so maybe not comparing apples with apples.

I’ll talk about typing all day @michaelbrewer but we seem to have jumped away from the original issue here and prob. just making unnecessary noise for the maintainers. Feel free to email me.

peterschutt on May 19, 2022

@peterschutt sure, i update the PR to match all of the feedback. As we use powertools in production at Fiserv, this will likely help us there too.

michaelbrewer on May 18, 2022

If you rely on self.child_exceptions being None, then you are likely running into the same bug and have a runtime error.

… if the way you use the exception results in it getting serialized via __str__(), sure.

My PR is for your one.

If I was lodging the same bug again I’d definitely make the change in format_exceptions() and not in the constructor. It maintains the existing interface, ensures the class at least honors the existing interface, and eliminates any issues arising from the surface area of the change.

In any case, the impression that I get from @sthulb and @heitorlessa is that there are some higher level things worthy of more attention at the moment. I only lodged the report because it didn’t look right, not because I was directly affected by the issue in any meaningful way.

@michaelbrewer Thanks for doing the PR though, and if @sthulb or @heitorlessa want to merge it, I’ll be happy to make any changes at that point.

peterschutt on May 18, 2022

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

michaelbrewer on May 16, 2022