fix APIGW path converter with prepended multiple slashes #11268
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 32s ⏱️ Results for commit 767c095. ♻️ This comment has been updated with latest results. |
| StrictMethodRule, | ||
| path_param_regex, | ||
| post_process_arg_name, | ||
| transform_path_params_to_rule_vars, | ||
| ) | ||
| from localstack.http import Request | ||
| from localstack.http.request import get_raw_path | ||
| from localstack.services.edge import GreedyPathConverter |
There was a problem hiding this comment.
i know i said "just add the greedy match router + converter to edge.py?", but really meant either copy&pasting it here, or move it to a more generic place and import it from there :-)
i don't like the localstack.services.edge import here.
i'd suggest moving the GreedyPathConverter to localstack.http.router for now, and perhaps also introduce it in rolo.
There was a problem hiding this comment.
This is much nicer, thanks 👌 I didn't like it either, it felt very out of place. This looks better!
|
|
||
|
|
||
| ROUTER: Router[Handler] = Router( | ||
| dispatcher=handler_dispatcher(), converters={"greedy_path": GreedyPathConverter} |
There was a problem hiding this comment.
Nice drill down, and a clean and well-tested change in API GW! 🏅
I agree with @thrau that the GreedyPathConverter shouldn't move into localstack.services.edge but rather into localstack.http.router.
Besides that I only had a question on a small change in a helper (see comment).
So as soon as the question is answered, the converter is moved, and most importantly all tests are green (this is a tricky change which can break something totally unexpected), this is looking good to get merged! 🚀
f7e9117 to
767c095
Compare
@alexrashed I'll re-run this repo tests after rebasing, which should fix the flakes, but the upstream pipeline is already fully green, that should be good! |
Motivation
As reported in #11253, our Edge Router had issue with
pathparameter starting with multiple slashes, as it would return a redirect which we shouldn't do.This PR introduces a new converter in the default
ROUTER, which is the one we already use internally for the OP Router for AWS, and also internally for the new generation APIGW.This PR introduces the change and uses the new converter for APIGW routes, and fixes the behavior around it, with added integration test cases.
Changes
GreedyPathConvertertoedge.pyas we need to use it in our default router, and refactor its usagesNote: failing tests are unrelated, and I'm running upstream full test suite to validate the changes which is green. Failing tests will be addressed with #11261
fixes #11253