-
Notifications
You must be signed in to change notification settings - Fork 162
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. |
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
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.
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
BaseRouter
class added in #3972. Route validation functions were also added.Changes
Route
class for internally representing routesRouteHandlerRegistry
class, following the pattern established in theappsync-events
andappsync-resolver
event handlers.register
function where handlers are registered and stored by route id (HttpMethod:path
) and by HTTP methodBaseRouter
class:route
method is no longer abstract and registration of routes using theRouteHandlerRegistry
class is done in one place hereEnvService
has 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/:param3
validatePathPattern
function ensures that parameters conform to the rules described abovecompilePath
function is responsible for converting paths into regexes that will then be used when implementing Feature request: Implement Route Matching & Resolution System for Event Handler #4140BaseRouter
tests 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.