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

feat(parser): enhance API Gateway schemas #2665

Merged
merged 8 commits into from
Jun 26, 2024
Merged

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

As discussed in the linked issue, the schemas for API Gateway in the Parser utility had some inconsistencies and missing schemas.

In the issue I tested all the integration types and auth that I could think of for API Gateway and observed the payloads for each. I have used these payloads (that are in the issue) to inform the changes in this PR.

This PR fixes this by making the following changes:

  • Add APIGatewayRequestAuthorizerEventSchema for Lambda Authorizer Request type in API Gateway v1 (REST)
  • Add APIGatewayTokenAuthorizerEventSchema for Lambda Authorizer Token type in API Gateway v1 (REST)
  • Add APIGatewayRequestAuthorizerEventV2Schema for Lambda Authorizer in API Gateway v2 (HTTP)
  • Reviewed APIGatewayProxyEventSchema to conform with the actual event payload (see linked issue)
  • Reviewed APIGatewayProxyEventV2Schema to conform with the actual event payload (see linked issue)

Additionally, while working on this I have also made three other changes that are not strictly related to the issue at hand but still interest the code I touched

  • Extracted some shared schemas between v1 and v2 into a separate file that is now imported by both - before the v2 schemas were importing (and including in the final bundle) all the schemas from v1 only because they were importing a single shared schema
  • Added docstrings with description, examples (where relevant), and links to all public schemas related to API Gateway
  • Added subpath exports to target import only schemas from API Gateway v1 or v2 - this allows customers to avoid including all the schemas present in Parser when importing one (we'll roll out this change across all schemas)

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: #2624


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.

@dreamorosi dreamorosi self-assigned this Jun 17, 2024
@pull-request-size pull-request-size bot added the size/XL PRs between 500-999 LOC, often PRs that grown with feedback label Jun 17, 2024
@boring-cyborg boring-cyborg bot added dependencies Changes that touch dependencies, e.g. Dependabot, etc. documentation Improvements or additions to documentation parser This item relates to the Parser Utility labels Jun 17, 2024
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Jun 17, 2024
@dreamorosi
Copy link
Contributor Author

@am29d - for when you're back.

Many of the test events used in the unit tests are not in line with what I observed and reported in the API Gateway tests discussed in the linked issue.

If possible, I'd ask you to please review my comment here, and if you agree replace the API Gateway event payloads with the ones I logged from API Gateway.

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for going through all the requests and adding the docs!

One minor ask: can we change to kebab-case apigw-proxy.ts for consistency?

I agree to change the test events with the ones you have generated. The previous events were afaik from the docs, which we imported from PT python.

@dreamorosi dreamorosi linked an issue Jun 25, 2024 that may be closed by this pull request
2 tasks
@boring-cyborg boring-cyborg bot added the tests PRs that add or change tests label Jun 25, 2024
@pull-request-size pull-request-size bot added size/XXL PRs with 1K+ LOC, largely documentation related and removed size/XL PRs between 500-999 LOC, often PRs that grown with feedback labels Jun 25, 2024
@dreamorosi
Copy link
Contributor Author

I have refactored the unit tests for API Gateway to use the events I recorded in the linked issue.

I have put the new test events in dedicated folders and used a slightly different way of reading the files. This new method only reads the files needed for that specific test suite instead of performing i/o to read all the events for each test.

Moving forward we should probably roll out this change across all other schemas/envelopes to improve test runtime.

Finally, I couldn't test the WebSocket API but I suspect there might be an opportunity to split the APIGatewayProxyEventSchema further and create a dedicated APIGatewayProxyWebsocketEventSchema.

I say this because some of the fields in the requestContext, namely: messageId, connectionId, routeKey, and eventType only seem to appear in these events. I'll address this in a future issue since I need to first figure out how to deploy a WS API (never done it) and I don't want to block your other PR.

@dreamorosi dreamorosi marked this pull request as ready for review June 25, 2024 21:07
@dreamorosi dreamorosi requested review from a team as code owners June 25, 2024 21:07
@dreamorosi dreamorosi requested a review from am29d June 25, 2024 21:07
Copy link

Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on refactoring the test event read operation. I remember having a mental todo but forgot about that. Great improvement!

@am29d am29d merged commit b3bc1f0 into main Jun 26, 2024
11 checks passed
@am29d am29d deleted the feat/parser_apigw_enhance branch June 26, 2024 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Changes that touch dependencies, e.g. Dependabot, etc. documentation Improvements or additions to documentation feature PRs that introduce new features or minor changes parser This item relates to the Parser Utility size/XXL PRs with 1K+ LOC, largely documentation related tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: add APIGatewayRequestAuthorizerEventV2 schemas/envelopes
2 participants