Skip to content
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

fix(parser): overload parse when using envelope #885

Merged

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Dec 10, 2021

Issue #, if available:

Description of changes:

When using parse function with an envelope, Mypy doesn't allow setting an explicit type as parse returns a Model while an envelope parse could return various types ranging from List[Model], Dict[str, Model], List[Optional[Model], and this can grow over time as more event types are defined too.

This PR uses type function overload to ensure parse continues to return a Model when no envelope is used, and when an envelope is used it'll propagate a generic type allowing an explicit type to be set at runtime.

Gotcha

If you don't set an explicit type when using parse with envelope, Mypy will detect as Any since MyPy can't have a generic return type in this case.

Example

This uses a sample valid SNS payload as per the issue with an empty payload and with non-empty payload.

Mypy output

mypy check.py
check.py:59:13: note: Revealed type is "builtins.list[check.Item]"
check.py:60:13: note: Revealed type is "builtins.list[check.NoItem]"

Python type evaluation

Item list type: <class 'list'>
No item list type: <class 'list'>
from aws_lambda_powertools.utilities.parser import parse, BaseModel, envelopes

from typing import List


class Item(BaseModel):
    message: str


class NoItem(BaseModel):
    ...


payload = {
    "Records": [
        {
            "EventVersion": "1.0",
            "EventSubscriptionArn": "arn:aws:sns:us-east-2:123456789012:sns-lambda",
            "EventSource": "aws:sns",
            "Sns": {
                "SignatureVersion": "1",
                "Timestamp": "2019-01-02T12:45:07.000Z",
                "Signature": "tcc6faL2yUC6dgZdmrwh1Y4cGa/ebXEkAi6RibDsvpi+tE/1+82j...65r==",
                "SigningCertUrl": "https://sns.us-east-2.amazonaws.com/SimpleNotification",
                "MessageId": "95df01b4-ee98-5cb9-9903-4c221d41eb5e",
                "Message": '{"message": "Hello from SNS!"}',
                "Type": "Notification",
                "UnsubscribeUrl": "https://sns.us-east-2.amazonaws.com/?Action=Unsubscribe",
                "TopicArn": "arn:aws:sns:us-east-2:123456789012:sns-lambda",
            },
        }
    ]
}

no_item_payload = {
    "Records": [
        {
            "EventVersion": "1.0",
            "EventSubscriptionArn": "arn:aws:sns:us-east-2:123456789012:sns-lambda",
            "EventSource": "aws:sns",
            "Sns": {
                "SignatureVersion": "1",
                "Timestamp": "2019-01-02T12:45:07.000Z",
                "Signature": "tcc6faL2yUC6dgZdmrwh1Y4cGa/ebXEkAi6RibDsvpi+tE/1+82j...65r==",
                "SigningCertUrl": "https://sns.us-east-2.amazonaws.com/SimpleNotification",
                "MessageId": "95df01b4-ee98-5cb9-9903-4c221d41eb5e",
                "Message": "",
                "Type": "Notification",
                "UnsubscribeUrl": "https://sns.us-east-2.amazonaws.com/?Action=Unsubscribe",
                "TopicArn": "arn:aws:sns:us-east-2:123456789012:sns-lambda",
            },
        }
    ]
}

item_list: List[Item] = parse(model=Item, event=payload, envelope=envelopes.SnsEnvelope)
no_item_list: List[NoItem] = parse(model=NoItem, event=payload, envelope=envelopes.SnsEnvelope)

assert item_list[0].message == "Hello from SNS!"

# reveal_type(item_list)
# reveal_type(no_item_list)

print(f"Item list type: {type(item_list)}")
print(f"No item list type: {type(no_item_list)}")

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 10, 2021
@heitorlessa heitorlessa added mypy bug Something isn't working and removed area/utilities labels Dec 10, 2021
@heitorlessa heitorlessa changed the title [WIP] fix(parser): overload parse when using envelope fix(parser): overload parse when using envelope Dec 10, 2021
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2021

Codecov Report

Merging #885 (eefb9e1) into develop (8de3729) will decrease coverage by 0.03%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #885      +/-   ##
===========================================
- Coverage    99.88%   99.84%   -0.04%     
===========================================
  Files          118      118              
  Lines         5133     5138       +5     
  Branches       573      573              
===========================================
+ Hits          5127     5130       +3     
- Misses           2        4       +2     
  Partials         4        4              
Impacted Files Coverage Δ
aws_lambda_powertools/utilities/parser/parser.py 93.93% <80.00%> (-6.07%) ⬇️
...mbda_powertools/utilities/parser/envelopes/base.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8de3729...eefb9e1. Read the comment docs.

@heitorlessa
Copy link
Contributor Author

will work on addressing test coverage once @DanLipsitt confirms the solution

@heitorlessa heitorlessa merged commit 5362a16 into aws-powertools:develop Dec 17, 2021
heitorlessa added a commit to ran-isenberg/aws-lambda-powertools-python that referenced this pull request Dec 31, 2021
…tools-python into complex

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python: (24 commits)
  docs: consistency around admonitions and snippets (aws-powertools#919)
  chore(deps-dev): bump mypy from 0.920 to 0.930 (aws-powertools#925)
  fix(event-sources): handle dynamodb null type as none, not bool (aws-powertools#929)
  fix(apigateway): support @app.not_found() syntax & housekeeping (aws-powertools#926)
  docs: Added GraphQL Sample API to Examples section of README.md (aws-powertools#930)
  feat(idempotency): support dataclasses & pydantic models payloads (aws-powertools#908)
  feat(tracer): ignore tracing for certain hostname(s) or url(s) (aws-powertools#910)
  feat(event-sources): cache parsed json in data class (aws-powertools#909)
  fix(warning): future distutils deprecation (aws-powertools#921)
  docs(batch): remove leftover from legacy
  docs(layer): bump Lambda Layer to version 6
  chore: bump to 1.23.0
  docs(apigateway): add new not_found feature (aws-powertools#915)
  docs: external reference to cloudformation custom resource helper (aws-powertools#914)
  feat(logger): allow handler with custom kwargs signature (aws-powertools#913)
  chore: minor housekeeping before release (aws-powertools#912)
  chore(deps-dev): bump mypy from 0.910 to 0.920 (aws-powertools#903)
  feat(batch): new BatchProcessor for SQS, DynamoDB, Kinesis (aws-powertools#886)
  fix(parser): overload parse when using envelope (aws-powertools#885)
  fix(parser): kinesis sequence number is str, not int (aws-powertools#907)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants