Repository navigation
fix(event_handler): fix prefix stripping, gzip negotiation, JSON detection and CORS Vary - #8638
Conversation
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
leandrodamascena
left a comment
There was a problem hiding this comment.
Thanks @Oakcube4life, really good work on this, and thanks for flagging item 1 instead of just shipping it. You were right to ask: regex in route rules is a documented feature (the "Catch-all routes" section and dynamic_routes_catch_all.py rely on .+, .* and /users/<user_id>/files/.+), and with the escaping those routes return 404. That one is on me, I listed it in the issue without checking the docs, I'm correcting #8580 now.
Could you drop item 1 and its two tests, keep items 2 to 6 exactly as they are, and, since you're there, add a test for the documented catch-all routes (.+, /files/.+, /users/<user_id>/files/.+)? We have none today, which is how I missed it. Everything else checks out: I ran the suite and probed strip_prefixes, gzip negotiation, bytes bodies, lowercase content-type and Vary, all behave.
|
Thanks for the review! Done: item 1 is reverted (d1b797c), items 2–6 are unchanged, and |
leandrodamascena
left a comment
There was a problem hiding this comment.
Thanks @Oakcube4life, that was fast. I went through items 2 to 6 again, this time behind a real API Gateway REST API: curl --compressed gets gzip, gzip;q=0 does not, a bytes JSON body comes back intact, lowercase content-type serializes, specific origins carry Vary: Origin while * stays clean, and the documented catch-all routes still match with user_id captured. The new catch-all test is exactly what I wanted to see.
One tiny thing, not worth another round: a route that already has Origin in its own Vary ends up with Origin twice. Harmless, we can dedupe later if anyone cares.
Approving. This goes out in 3.38.0 and I'll merge it shortly. Thanks for the patience with the back and forth on item 1.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8638 +/- ##
===========================================
- Coverage 96.95% 96.94% -0.02%
===========================================
Files 318 318
Lines 16102 16121 +19
Branches 1440 1445 +5
===========================================
+ Hits 15612 15628 +16
- Misses 349 352 +3
Partials 141 141 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
45f7a4d
into
aws-powertools:develop
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
|
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |



Issue number: closes #8580
Summary
Changes
One fix plus tests per bug, one commit each, in
event_handler/api_gateway.py. Item 1 (escaping route rules) was dropped after review, because regex in route rules is a documented feature; a test for the documented catch-all routes was added instead.strip_prefixes:re.sub(prefix, "", path)removed every match anywhere in the path, so/v1/users/v2withre.compile(r"/v[1-9]+")became/usersand returned 404. It now usesprefix.match(path)and strips a single leading match.q=0: the check was a substring test, sogzip;q=0was still compressed andGZIPwas not.Accept-Encodingis now split into codings, compared case-insensitively, and a zero q-value is treated as a refusal.bytesbody was passed to the JSON serializer and raisedTypeError. The serializer now skipsbytes, which are base64 encoded as the docstring says.is_json()casing:Response.is_json()only foundContent-Typewith exact casing, soheaders={"content-type": "application/json"}returned the dict unserialized. The header name is now matched in any casing.Vary: Origin:CORSConfig.to_dictnow addsVary: Originwhen a specific origin is echoed. AVaryvalue already set by the route is kept andOriginis appended. Wildcard (*) responses are unchanged.Catch-all routes test:
test_route_rule_supports_documented_catch_all_regexcovers the documented.+,/files/.+and/users/<user_id>/files/.+rules, including thatuser_idis still captured. It fails if route rules are escaped, so the regression from item 1 can't come back unnoticed.User experience
Each repro for items 2 to 6 from the issue snippet now behaves as expected: a regex prefix is stripped once from the start,
gzip;q=0gets an uncompressed response, a JSONbytesbody is returned base64 encoded, a lowercasecontent-typeheader serializes the body, and a response echoing a specific origin carriesVary: Origin. Documented catch-all routes keep working and are now covered by tests.14 test cases added (8 test functions); each fix's tests were verified to fail before the fix. The full test suite passes, and ruff, mypy and the bandit baseline are clean.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.