fix(http): prevent cache-key collision from unescaped param values - #116
Conversation
_cache_key hand-joined params with "&" and "=", so a param value
containing those characters could alias a different param set to the
same cache key (e.g. {"a": "1&b=2"} vs {"a": "1", "b": "2"}). Use
urlencode() instead, which percent-escapes reserved characters.
Fixes mldsveda#114
|
please review! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness bug in PyScrappy’s HTTP response caching by making cache keys resilient to special characters in query parameter values, preventing distinct requests from aliasing to the same cached entry (Issue #114).
Changes:
- Use
urllib.parse.urlencode()to build query-string portions of cache keys in both sync and async HTTP clients. - Add unit tests ensuring cache keys differ for
{"a": "1&b=2"}vs{"a": "1", "b": "2"}for both clients.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/pyscrappy/core/http.py | Switch cache-key param serialization to urlencode() to avoid collisions. |
| src/pyscrappy/core/async_http.py | Mirror the same cache-key fix in the async client. |
| tests/test_core/test_http.py | Add regression test for cache-key collision on special characters (sync). |
| tests/test_core/test_async_http.py | Add regression test for cache-key collision on special characters (async). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| except (TypeError, ValueError): | ||
| return url | ||
| return url + "?" + "&".join(f"{k}={v}" for k, v in items) | ||
| return url + "?" + urlencode(items) |
… string _cache_key always appended params with "?", so a URL that already contained a query string produced keys like "http://x?a=1?b=2". That could collide with a different call passing the same string as a literal URL with no params, causing a wrong cached response. Use "&" as the separator when the URL already contains "?", matching the existing scraper_api routing logic in the same file. Addresses review feedback on mldsveda#116.
|
Verified locally: full suite passes (402), ruff clean. urlencode fixes the collision, and the sep guard matches how the request path already handles existing query strings. Tests cover both cases for sync and async. Merging, thanks for the clean first contribution @BhoomikaAP-11. |
_cache_key hand-joined params with "&" and "=", so a param value containing those characters could alias a different param set to the same cache key (e.g. {"a": "1&b=2"} vs {"a": "1", "b": "2"}). Use urlencode() instead, which percent-escapes reserved characters.
Fixes #114