Skip to content

fix: stop retries from re-encoding path parameters - #298

Merged
tbarbugli merged 2 commits into
GetStream:mainfrom
RaphaelFakhri:fix/retry-double-encodes-path-params
Oct 1, 2026
Merged

tbarbugli merged 2 commits into
GetStream:mainfrom
RaphaelFakhri:fix/retry-double-encodes-path-params

Conversation

@RaphaelFakhri

@RaphaelFakhri RaphaelFakhri commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Why

build_path percent-encodes each value in the caller's path_params dict in place. The retry loop in _request_sync and _request_async passes the same kwargs, and therefore the same path_params dict, to every attempt. On the first attempt a value such as a b:c becomes a%20b%3Ac. On a retry the encoded value is encoded again and the request goes to a%2520b%253Ac, which addresses a different resource and typically returns a 404 instead of the result of the retried request.

The problem only appears when RetryConfig(enabled=True) is set, the request is a GET or HEAD, and a path parameter contains a character that quote escapes.

Changes

  • build_path quotes the values into a new dict and leaves the caller's mapping unchanged.
  • Add sync and async tests that retry a GET after a 429 and assert that both attempts request the same path.

Both new tests fail without the change (the second request uses a%2520b%253Ac) and pass with it.


Note

Low Risk
Small, localized fix to URL building with regression tests; only affects requests that use path_params and retries.

Overview
Fixes a bug where retried GET/HEAD requests could hit the wrong URL when path_params contain characters that need percent-encoding (e.g. spaces or colons).

build_path previously mutated the caller's path_params dict while quoting values. The retry loop reuses the same kwargs on every attempt, so the second try quoted already-encoded values again (a%20b%3Ac → a%2520b%253Ac), often yielding 404s instead of a successful retry.

The change quotes into a new dict and leaves the original mapping unchanged. Sync and async tests retry after a 429 and assert both attempts use the same raw path.

Reviewed by Cursor Bugbot for commit e250acd. Bugbot is set up for automated code reviews on this repo. Configure here.

@coderabbitai

coderabbitai Bot commented Oct 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 24 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: eb5de60e-7551-4327-91f7-e2a47cfead81

📥 Commits

Reviewing files that changed from the base of the PR and between 5689d4f and e250acd.

📒 Files selected for processing (2)
  • getstream/base.py
  • tests/test_retry.py
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tbarbugli
tbarbugli merged commit a238cb2 into GetStream:main Oct 1, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants