-
Notifications
You must be signed in to change notification settings - Fork 185
feat(event-handler): add route management system for ApiGw event handler #4211
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(event-handler): add route management system for ApiGw event handler #4211
Conversation
|
Thank you Stefano, I'll block some time to review this for tomorrow. |
|
Sounds good. I've hewn fairly close to your sample implementation, aside from the utility stuff, so hopefully it won't be too much work. |
dreamorosi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work on this PR!
Besides some style/organization comments, I left two more interesting comments
- when we log issues, should we do so in separate log entries (current) or in a single one?
- should we store routes in the registry as a set rather than an array?
packages/event-handler/tests/unit/rest/RouteHandlerRegistry.test.ts
Outdated
Show resolved
Hide resolved
packages/event-handler/tests/unit/rest/RouteHandlerRegistry.test.ts
Outdated
Show resolved
Hide resolved
1f3befa to
7c2c347
Compare
dreamorosi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on the PR and for addressing my previous comments.
I've answered your questions about logs & Set usage and I don't feel strongly about either at this stage, so I'm happy for you to make a decision and either resolve the conversations, then merge - or change it + me sending another "Approve" right after that.
leandrodamascena
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @svozza @dreamorosi I just left one comment. Pls let me know if make sense.
|



Summary
Adds the route management layer for the API Gateway event handler. The main focus of this PR is to create the registry that will hold all the handlers registered to various routes and integrate it with the
BaseRouterclass added in #3972. Route validation functions were also added.Changes
Routeclass for internally representing routesRouteHandlerRegistryclass, following the pattern established in theappsync-eventsandappsync-resolverevent handlers.registerfunction where handlers are registered and stored by route id (HttpMethod:path) and by HTTP methodBaseRouterclass:routemethod is no longer abstract and registration of routes using theRouteHandlerRegistryclass is done in one place hereEnvServicehas shown, should we wish to use these elsewhere in the future, it is easier to use plain functions rather than require consumers to instantiate a class with multiple methods when all they might want is one function./user/:userId) can take./:param1/:param2/:param3validatePathPatternfunction ensures that parameters conform to the rules described abovecompilePathfunction is responsible for converting paths into regexes that will then be used when implementing Feature request: Implement Route Matching & Resolution System for Event Handler #4140BaseRoutertests to reflect the changes made there.Issue number: closes #4139
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.