feat(parser): Support for API GW v1 proxy schema & envelope #403

ran-isenberg · 2021-04-18T12:00:19Z

This schema exists in the validator but not in the parser. It was requested in the webinar audience.
This feature will also provide an envelope class which will parse the string body parameter as a pydantic Model.

Also added new identity parameters as defined at https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html

…nvelope #402

codecov-commenter · 2021-04-18T12:42:03Z

Codecov Report

Merging #403 (ee58a5c) into develop (0edec8e) will decrease coverage by 0.05%.
The diff coverage is 97.82%.

@@             Coverage Diff             @@
##           develop     #403      +/-   ##
===========================================
- Coverage    99.94%   99.89%   -0.06%     
===========================================
  Files           98      100       +2     
  Lines         3688     3780      +92     
  Branches       174      175       +1     
===========================================
+ Hits          3686     3776      +90     
- Misses           0        1       +1     
- Partials         2        3       +1

Impacted Files	Coverage Δ
...lambda_powertools/utilities/parser/models/apigw.py	`97.43% <97.43%> (ø)`
..._powertools/utilities/parser/envelopes/__init__.py	`100.00% <100.00%> (ø)`
...bda_powertools/utilities/parser/envelopes/apigw.py	`100.00% <100.00%> (ø)`
...bda_powertools/utilities/parser/models/__init__.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 9be19b2...ee58a5c. Read the comment docs.

heitorlessa

Awesome, thank you @risenberg-cyberark. A few minor questions on the additional checks, and an explicit comment to tag whether this is REST API (v1) or HTTP API (v2), or both, so it's easier to add and use HTTP API later judging by the import, naming, etc.

aws_lambda_powertools/utilities/parser/envelopes/apigw.py

heitorlessa · 2021-04-19T19:46:16Z

aws_lambda_powertools/utilities/parser/models/apigw.py

+        return values
+
+    @root_validator(pre=True)
+    def check_both_http_methods(cls, values):


could you add a note why this check is needed?

what would you like to write here? basically what i saw that in the v1 version there's a duplication 3 fields, so i made sure that all values are actually the same. V2 removes this duplication

heitorlessa · 2021-04-19T19:46:21Z

aws_lambda_powertools/utilities/parser/models/apigw.py

+        return values
+
+    @root_validator(pre=True)
+    def check_both_paths(cls, values):


could you add a note why this check is needed?

what would you like to write here? basically what i saw that in the v1 version there's a duplication 3 fields, so i made sure that all values are actually the same. V2 removes this duplication

heitorlessa · 2021-04-19T19:46:50Z

aws_lambda_powertools/utilities/parser/models/apigw.py

+    body: str
+
+    @root_validator()
+    def check_message_id(cls, values):


could you add a note why this check is needed? e.g. spoofing?

what would you like to write here? basically what i saw that in the v1 version there's a duplication 3 fields, so i made sure that all values are actually the same. V2 removes this duplication

heitorlessa · 2021-04-19T19:47:33Z

docs/utilities/parser.md

@@ -161,6 +161,7 @@ Parser comes with the following built-in models:
 | **KinesisDataStreamModel** | Lambda Event Source payload for Amazon Kinesis Data Streams        |
 | **SesModel**               | Lambda Event Source payload for Amazon Simple Email Service        |
 | **SnsModel**               | Lambda Event Source payload for Amazon Simple Notification Service |
+| **APIGatewayProxyEvent**   | Lambda Event Source payload for Amazon API Gateway                 |


API Gateway v1, v2, or both? e.g. REST vs HTTP API. By my first look I'd guess REST API

heitorlessa · 2021-04-19T19:47:59Z

docs/utilities/parser.md

+| **KinesisDataStreamEnvelope**                                                                                                                | 1. Parses data using `KinesisDataStreamModel` which will base64 decode it. <br/> 2. Parses records in in `Records` key using your model and returns them in a list.                                         | `List[Model]`                      |
+| **SnsEnvelope**                                                                                                                              | 1. Parses data using `SnsModel`. <br/> 2. Parses records in `body` key using your model and return them in a list.                                                                                          | `List[Model]`                      |
+| **SnsSqsEnvelope**                                                                                                                           | 1. Parses data using `SqsModel`. <br/> 2. Parses SNS records in `body` key using `SnsNotificationModel`. <br/> 3. Parses data in `Message` key using your model and return them in a list.                  | `List[Model]`                      |
+| **ApiGatewayEnvelope**         1. Parses data using `APIGatewayProxyEventModel`. <br/> 2. Parses `body` key using your model and returns it. | `Model`                                                                                                                                                                                                     |


API Gateway v1, v2, or both? e.g. REST vs HTTP API. By my first look I'd guess REST API

Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>

heitorlessa · 2021-04-20T19:39:48Z

Thanks for the clarifications @risenberg-cyberark - I'm happy to merge after we remove those root validators.

I don't see a need a benefit in keeping them if we, the provider, are the ones duplicating these possibly for backward compatible reasons (until HTTP API when they're gone).

Alternatively, I can merge and fiz that later too - up to you.

Thanks a lot for adding this support!

ran-isenberg · 2021-04-21T05:08:35Z

Thanks for the clarifications @risenberg-cyberark - I'm happy to merge after we remove those root validators.

I don't see a need a benefit in keeping them if we, the provider, are the ones duplicating these possibly for backward compatible reasons (until HTTP API when they're gone).

Alternatively, I can merge and fiz that later too - up to you.

Thanks a lot for adding this support!

@heitorlessa
I'll explain. They are duplicated in the payload. I didnt duplicate them, i just made sure that the values are the same since they clearly are supposed to be the same. Those 2 items reside on the main event and are also found in the request context part.

You remove them but I do think they add a value because if there's a discrepancy in between them, you should not use the entire payload at all ,

heitorlessa · 2021-04-21T05:15:36Z

Thanks for the clarifications @risenberg-cyberark - I'm happy to merge after we remove those root validators.

I don't see a need a benefit in keeping them if we, the provider, are the ones duplicating these possibly for backward compatible reasons (until HTTP API when they're gone).

Alternatively, I can merge and fiz that later too - up to you.

Thanks a lot for adding this support!

@heitorlessa

I'll explain. They are duplicated in the payload. I didnt duplicate them, i just made sure that the values are the same since they clearly are supposed to be the same. Those 2 items reside on the main event and are also found in the request context part.

You remove them but I do think they add a value because if there's a discrepancy in between them, you should not use the entire payload at all ,

That's what I understood too @risenberg-cyberark ;)

Putting in another way, I wouldn't like to break customers applications because this behaviour isn't documented. For this reason, I don't trust that API Gateway Lambda integration to follow through these checks consistently.

ran-isenberg · 2021-04-21T05:18:11Z

Thanks for the clarifications @risenberg-cyberark - I'm happy to merge after we remove those root validators.

I don't see a need a benefit in keeping them if we, the provider, are the ones duplicating these possibly for backward compatible reasons (until HTTP API when they're gone).

Alternatively, I can merge and fiz that later too - up to you.

Thanks a lot for adding this support!

@heitorlessa
I'll explain. They are duplicated in the payload. I didnt duplicate them, i just made sure that the values are the same since they clearly are supposed to be the same. Those 2 items reside on the main event and are also found in the request context part.
You remove them but I do think they add a value because if there's a discrepancy in between them, you should not use the entire payload at all ,

That's what I understood too @risenberg-cyberark ;)

Putting in another way, I wouldn't like to break customers applications because this behaviour isn't documented. For this reason, I don't trust that API Gateway Lambda integration to follow through these checks consistently.

np, removed the relevant root validator. is the last root validator ok?

heitorlessa

All good on the validators except I think there's an accidental duplicates field (eventType).

Mind checking before I merge?

heitorlessa · 2021-04-21T16:31:26Z

aws_lambda_powertools/utilities/parser/models/apigw.py

+    connectedAt: Optional[datetime]
+    connectionId: Optional[str]
+    eventType: Optional[Literal["CONNECT", "MESSAGE", "DISCONNECT"]]
+    eventType: Optional[str]


Is this perhaps a typo as it's defined twice?

That's for WebSockets and the line 65 is correct

seems like a bad copy paste :)
removed

heitorlessa · 2021-04-21T17:13:10Z

I bet :D Glad I checked w/ you before merging :) Merging now!

…

On Wed, 21 Apr 2021 at 18:41, Ran Isenberg ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In aws_lambda_powertools/utilities/parser/models/apigw.py <#403 (comment)> : > + protocol: str + identity: APIGatewayEventIdentity + requestId: str + requestTime: str + requestTimeEpoch: datetime + resourceId: Optional[str] + resourcePath: str + domainName: Optional[str] + domainPrefix: Optional[str] + extendedRequestId: Optional[str] + httpMethod: Literal["DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT"] + path: str + connectedAt: Optional[datetime] + connectionId: Optional[str] + eventType: Optional[Literal["CONNECT", "MESSAGE", "DISCONNECT"]] + eventType: Optional[str] seems like a bad copy paste :) removed — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#403 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBHENA66LKXNJP3EKZLTJ354JANCNFSM43EEWPWA> .

…tools-python into fix/docs-latest-api-ref * 'develop' of https://github.com/awslabs/aws-lambda-powertools-python: feat(parser): Support for API GW v1 proxy schema & envelope (#403) fix(validator): event type annotation as any in validate fn (#405) docs(tracer): Fix line highlighting (#395) fix(workflow): github actions depends on for release chore: bump to 1.14.0 docs(logger): add example on how to set UTC timestamp (#392) docs(idempotency): add default configuration for those not using CFN (#391)

feat (parser): Feature request: Add parser API GW v1 proxy schema & e…

462a0be

…nvelope #402

ran-isenberg changed the title ~~feat (parser): Feature request: Add parser API GW v1 proxy schema & envelope~~ feat(parser): Feature request: Add parser API GW v1 proxy schema & envelope Apr 18, 2021

Ran Isenberg added 2 commits April 18, 2021 15:06

fix renamed model

efd429c

fix literal import

1eba236

heitorlessa added area/utilities feature New feature or functionality labels Apr 19, 2021

heitorlessa self-assigned this Apr 19, 2021

heitorlessa added this to the 1.15.0 milestone Apr 19, 2021

heitorlessa suggested changes Apr 19, 2021

View reviewed changes

Update aws_lambda_powertools/utilities/parser/envelopes/apigw.py

a88fb89

Co-authored-by: Heitor Lessa <heitor.lessa@hotmail.com>

cr fixes

ee58a5c

heitorlessa suggested changes Apr 21, 2021

View reviewed changes

remove duplicate

5c67f40

heitorlessa approved these changes Apr 21, 2021

View reviewed changes

heitorlessa changed the title ~~feat(parser): Feature request: Add parser API GW v1 proxy schema & envelope~~ feat(parser): Support for API GW v1 proxy schema & envelope Apr 21, 2021

heitorlessa merged commit 68d4110 into aws-powertools:develop Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(parser): Support for API GW v1 proxy schema & envelope #403

feat(parser): Support for API GW v1 proxy schema & envelope #403

ran-isenberg commented Apr 18, 2021

codecov-commenter commented Apr 18, 2021 •

edited

Loading

heitorlessa left a comment

heitorlessa Apr 19, 2021

ran-isenberg Apr 19, 2021 •

edited

Loading

heitorlessa Apr 19, 2021

ran-isenberg Apr 19, 2021

heitorlessa Apr 19, 2021

ran-isenberg Apr 19, 2021

heitorlessa Apr 19, 2021

ran-isenberg Apr 19, 2021

heitorlessa Apr 19, 2021

ran-isenberg Apr 19, 2021

heitorlessa commented Apr 20, 2021

ran-isenberg commented Apr 21, 2021 •

edited

Loading

heitorlessa commented Apr 21, 2021

ran-isenberg commented Apr 21, 2021

heitorlessa left a comment

heitorlessa Apr 21, 2021

ran-isenberg Apr 21, 2021

heitorlessa commented Apr 21, 2021 via email

feat(parser): Support for API GW v1 proxy schema & envelope #403

feat(parser): Support for API GW v1 proxy schema & envelope #403

Conversation

ran-isenberg commented Apr 18, 2021

codecov-commenter commented Apr 18, 2021 • edited Loading

Codecov Report

heitorlessa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ran-isenberg Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heitorlessa commented Apr 20, 2021

ran-isenberg commented Apr 21, 2021 • edited Loading

heitorlessa commented Apr 21, 2021

ran-isenberg commented Apr 21, 2021

heitorlessa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heitorlessa commented Apr 21, 2021 via email

codecov-commenter commented Apr 18, 2021 •

edited

Loading

ran-isenberg Apr 19, 2021 •

edited

Loading

ran-isenberg commented Apr 21, 2021 •

edited

Loading