migrating NG APIGW routing to Werkzeug routing #11057
Conversation
There was a problem hiding this comment.
Wow! That simplifies things a lot! and reuses our operation router.
Definitely not great to be importing the private method though 😁
Looks like most of these are used only once. And maybe we could restructure the op_router.py to have a RestOperationRouter class from which RestServiceOperationRouter and RestAPIResourceRouter are subclassed.
We could then have the shared values and methods as part of the parent class 🤔
| try: | ||
| # trailing slashes are ignored in APIGW | ||
| path = context.invocation_request["path"].rstrip("/") | ||
|
|
||
| rule, args = matcher.match(path, method=request.method, return_rule=True) | ||
| except MethodNotAllowed as e: | ||
| # MethodNotAllowed (405) exception is raised if a path is matching, but the method does not. | ||
| # Our router handles this as a 404. | ||
| # TODO: raise proper Gateway exception | ||
| raise Exception("Not found") from e |
There was a problem hiding this comment.
Should we also catch the exception raised if no path are matching in order to raise the proper Gateway Exception also?
There was a problem hiding this comment.
Great catch! Thanks, I'll add this right now 🙏
Really good point, that sounds reasonable! I'd also like to get @alexrashed opinion on this, it feels a bit direct to directly import from |
There was a problem hiding this comment.
This PR really makes my day! Great to see that the custom parsing logic is finally going away, and that you could even re-use stuff we built for the op-router before! 🥳
Concerning the imports from op_router:
I think you can just move forward and refactor the op_router module. It's really great to see that the utility functions / base implementation in the operation router were useful for this, and there's actually that much overlap.
We could just create a separate module localstack.services.aws.protocol.routing with all the stuff that is not directly related to the service-operation-routing. Maybe you can even abstract it in a way such that an inheritance structure makes sense as @cloutierMat suggestion (but this isn't a necessity and shouldn't be forced if it doesn't work / feel well). 🥳
But that could also be done in a later iteration if you would prefer that, which is why I'll just approve this PR right away. 😛
Big kudos for tackling this! 🦸🏽
There was a problem hiding this comment.
-148 LoC of super complex self built parsing logic replaced by +127 LoC in very nicely abstracted route matching configuration handed forward to a bullet proof library (Werkzeug) 😍
|
Awesome, thanks a lot @alexrashed! I will refactor |
Motivation
This PR is currently target #11051.
Follow up from #11051, looking at the
op_routercode, I realized the format of the paths for our specs were the same as API Gateway, so much that we could reuse a simplified version of theRestServiceOperationRouter(we do not need to match on required headers and query string parameters, so that makes it much, much simpler).All the logic to already create the rules were there, so I've re-used most of the code.
Only issue is that some of the code is private, I'm not sure how to re-architecture to make it re-usable, maybe in some
routingpackage? \cc @alexrashedChanges