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

chore(logger): overload inject_lambda_context with generics #1583

Conversation

ascopes
Copy link
Contributor

@ascopes ascopes commented Oct 11, 2022

Issue number: #1711

Summary

Changes

This enables static-type checking correctly with mypy when using pydantic models for the lambda event argument rather than a mapping type. Currently using this decorator results in an error and requires a typing ignore directive on the line using the decorator.

User experience

Before - using Pydantic models with lambda handlers produces a static typing error if
the @inject_lambda_context decorator is used.

After - the callable passed in is treated as a generic type and this generic flag is retained as the decorator return value, providing the same semantics that the tracer decorators do. This should enable MyPy to handle these constraints correctly.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

This should not be a breaking change other than places where invalid types or signatures may have been used previously.

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

This enables static-type checking correctly with mypy when using
pydantic models for the lambda event argument rather than a mapping
type. Currently using this decorator results in an error and
requires a typing ignore directive on the line using the decorator.
@ascopes ascopes requested a review from a team as a code owner October 11, 2022 08:21
@ascopes ascopes requested review from mploski and removed request for a team October 11, 2022 08:21
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 11, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Oct 11, 2022

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our AWS Lambda Powertools Discord: Invite link

@github-actions
Copy link
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added do-not-merge need-issue PRs that are missing related issues labels Oct 11, 2022
@heitorlessa
Copy link
Contributor

hey @ascopes, thank you for sending this improvement - would you be able to create an static typing issue to report the Mypy error, etc.?

We'd need an issue before proceeding - happy to do talk next week if you can't. We've also made the decision to bring typing-extensions as a dependency in upcoming releases -- this means we can now use the ParamSpec and more accurate annotations to preserve Callable signatures. Feel free to add them in as part of the PR.

Thanks a lot!

@heitorlessa heitorlessa removed the request for review from mploski October 26, 2022 16:21
@ascopes
Copy link
Contributor Author

ascopes commented Oct 26, 2022

Will try and dig the error out tomorrow if I have time. Thanks for the response!

@heitorlessa heitorlessa removed do-not-merge need-issue PRs that are missing related issues labels Nov 14, 2022
@heitorlessa
Copy link
Contributor

Created issue #1711 - I'll resolve the conflict, and push a change to use the new ParamSpec for more accurate typing now that we can use it 🙌

* develop: (155 commits)
  chore: apigw test event wrongly set with base64
  chore(deps-dev): bump types-requests from 2.28.11.3 to 2.28.11.4 (aws-powertools#1701)
  update changelog with latest changes
  feat(apigateway): multiple exceptions in exception_handler (aws-powertools#1707)
  chore(deps-dev): bump mypy-boto3-logs from 1.25.0 to 1.26.3 (aws-powertools#1702)
  update changelog with latest changes
  chore(ci): revert custom hw for E2E due to lack of hw
  update changelog with latest changes
  docs: project name consistency
  chore(ci): prevent dependabot updates to trigger E2E
  chore(ci): use new custom hw for E2E
  chore(ci): limit to src only to prevent dependabot failures
  update changelog with latest changes
  docs(examples): linting unnecessary whitespace
  chore(deps-dev): bump pytest-xdist from 2.5.0 to 3.0.2 (aws-powertools#1655)
  update changelog with latest changes
  docs(apigateway): add all resolvers in testing your code section for accuracy (aws-powertools#1688)
  chore(deps-dev): bump mkdocs-material from 8.5.7 to 8.5.9 (aws-powertools#1697)
  update changelog with latest changes
  docs(homepage): update default value for `POWERTOOLS_DEV` (aws-powertools#1695)
  ...
@codecov-commenter
Copy link

Codecov Report

Base: 99.27% // Head: 99.27% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (d0156c1) compared to base (9f3d748).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1583   +/-   ##
========================================
  Coverage    99.27%   99.27%           
========================================
  Files          129      129           
  Lines         6054     6055    +1     
  Branches       402      402           
========================================
+ Hits          6010     6011    +1     
  Misses          20       20           
  Partials        24       24           
Impacted Files Coverage Δ
aws_lambda_powertools/logging/logger.py 99.39% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@heitorlessa
Copy link
Contributor

Turns out partial isn't supported in ParamSpec, casting + narrowing return results into deeper issues that I can't dedicate time yet (narrow types for decorator factory for test code below).

Merging as-is. Thanks a lot for the patience @ascopes !


Test code for mypy which overrides event earlier than logger decorator

from aws_lambda_powertools import Logger
from aws_lambda_powertools.utilities.data_classes import event_source
from aws_lambda_powertools.utilities.data_classes.kinesis_stream_event import KinesisStreamEvent


logger = Logger()


@logger.inject_lambda_context
@event_source(data_class=KinesisStreamEvent)
def simple_handler(event: KinesisStreamEvent, context) -> str:
    return "success"


payload = {
    "Records": [
        {
            "kinesis": {
                "kinesisSchemaVersion": "1.0",
                "partitionKey": "da10bf66b1f54bff5d96eae99149ad1f",
                "sequenceNumber": "49635052289529725553291405521504870233219489715332317186",
                "data": "H4sIAAAAAAAAAK2Sa2vbMBSG/4ox+xg3Oror39IlvaztVmJv7WjCUGwl8+ZLZstts5L/vuOsZYUyWGEgJHiP9J7nvOghLF3b2rVLthsXjsLJOBl/uZjG8fh4Gg7C+q5yDcqUAWcSONHEoFzU6+Om7jZYGdq7dljYcpnZ4cZHwLWOJl1Zbs/r9cR6e9RVqc/rKlpXV9eXt+fy27vt8W+L2DfOlr07oXQIMAQyvHlzPk6mcbKgciktF5lQfMU5dZZqzrShLF2uFC60aLtlmzb5prc/ygvvmjYc3YRPFG+LusuurE+/Ikqb1Gd55dq8jV+8isT6+317Rk42J5PTcLFnm966yvd2D2GeISJTYIwCJSQ1BE9OtWZCABWaKMIJAMdDMyU5MYZLhmkxBhQxfY4Re1tiWiAlBsgIVQTE4Cl6tI+T8SwJZu5Hh1dPs1FApOMSDI9WVKmIC+4irTMWQZYpx7QkztrgE06MU4yCx9DmVbgbvABmQJTGtkYAB0NwEwyYQUBpqEFuSbkGrThTRKi/AlP+HHj6fvJa3P9Ap/+Rbja9/PD6POd+0jXW7xM1B8CDsp37w7woXBb8qQDZ6xeurJttEOc/HWpUBxeHKNr74LHwsXXYlsm9flrl/rmFIQeS7m3m1fVs/DlIGpu6nhMiyWQGXNKIMbcCIgkhElKbaZnZpYJUz33s1iV+z/6+StMlR3yphHNcCyxiNEXf2zed6xuEu8XuF2wb6krnAwAA",
                "approximateArrivalTimestamp": 1668093033.744,
            },
            "eventSource": "aws:kinesis",
            "eventVersion": "1.0",
            "eventID": "shardId-000000000000:49635052289529725553291405521504870233219489715332317186",
            "eventName": "aws:kinesis:record",
            "invokeIdentityArn": "arn:aws:iam::231436140809:role/pt-1488-CloudWatchKinesisLogsFunctionRole-1M4G2TIWIE49",
            "awsRegion": "eu-west-1",
            "eventSourceARN": "arn:aws:kinesis:eu-west-1:231436140809:stream/pt-1488-KinesisStreamCloudWatchLogs-D8tHs0im0aJG",
        },
        {
            "kinesis": {
                "kinesisSchemaVersion": "1.0",
                "partitionKey": "cf4c4c2c9a49bdfaf58d7dbbc2b06081",
                "sequenceNumber": "49635052289529725553291405520881064510298312199003701250",
                "data": "H4sIAAAAAAAAAK2SW2/TQBCF/4pl8ViTvc7u5i0laVraQhUbWtREaG1PgsGXYK/bhqr/nXVoBRIgUYnXc2bPfHO092GFXWc3mOy2GI7D6SSZfDyfxfFkPgsPwua2xtbLjFPBgQqiifFy2WzmbdNvvTOyt92otFWa29HWRVRoHU37qtqdNZupdfaorzNXNHW0qS+vLm7O4PPr3fxHROxatNWQThgbUTqiZHT94mySzOJkBUqYLOWY8ZQLbaTRkEvDciUYzWzKfETXp13WFtsh/qgoHbZdOL4OnyhelU2fX1qXffIoXdKcFjV2RRf/9iqSmy933Sk53h5PT8LVnm12g7Ub4u7DIveIXFFjFNGUKUlAaMY0EUJKLjkQbxhKGCWeknMKoAGUkYoJ7TFd4St2tvJtDRYxDAg3VB08Ve/j42SySIIFfu396Ek+DkS+xkwAiYhM00isgUV6jXmEMrM5EmMsh+C9v9hfMQ4eS1vW4cPBH4CZVpoTJkEIAp5RUMo8vGFae3JNCCdUccMVgPw7sP4VePZm+lzc/0AH/0i3mF28fX6fSzftW+v2jZKXRgVVt3SHRVliHvx06F4+x6ppd0FcfEMvMR2cH3rR3gWPxrsO/Vau9vqyvlpMPgRJazMcYGgEHHLKBhLGJaBA0JLxNc0JppoS9Cwxbir/B4d5QDBAQSnfFFGp8aa/vxw2uLbHYUH4sHr4Dj5RJxfMAwAA",
                "approximateArrivalTimestamp": 1668092612.992,
            },
            "eventSource": "aws:kinesis",
            "eventVersion": "1.0",
            "eventID": "shardId-000000000000:49635052289529725553291405520881064510298312199003701250",
            "eventName": "aws:kinesis:record",
            "invokeIdentityArn": "arn:aws:iam::231436140809:role/pt-1488-CloudWatchKinesisLogsFunctionRole-1M4G2TIWIE49",
            "awsRegion": "eu-west-1",
            "eventSourceARN": "arn:aws:kinesis:eu-west-1:231436140809:stream/pt-1488-KinesisStreamCloudWatchLogs-D8tHs0im0aJG",
        },
    ]
}


ret: str = simple_handler(payload, object())

@heitorlessa heitorlessa changed the title improv: Use generics for logger.inject_lambda_context. chore(logger): overload inject_lambda_context with generics Nov 14, 2022
@heitorlessa heitorlessa merged commit 053baef into aws-powertools:develop Nov 14, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 14, 2022

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@github-actions github-actions bot added the internal Maintenance changes label Nov 14, 2022
@ascopes ascopes deleted the task/support-pydantic-models-in-logger branch November 15, 2022 12:21
@ascopes ascopes restored the task/support-pydantic-models-in-logger branch November 15, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Maintenance changes logger size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants