Reject a malformed db in the connection URL path

[chiruu12]

parse_url reads the db from the URL path with int(unquote(url.path).replace("/", ""))
inside except (AttributeError, ValueError): pass. Stripping every slash runs the path
segments together, and the bare except drops whatever is left.

parse_url("redis://h/3/4/5")    # db=345
parse_url("redis://h/1/2")      # db=12
parse_url("redis://h/abc")      # db absent, so the client uses 0
parse_url("redis://h?db=abc")   # ValueError: Invalid value for 'db' in connection URL.

Two problems. A malformed URL picks a database rather than failing, so the client talks to
the wrong one and nothing says so. And the same invalid value raises when it is spelled as
a query argument but is ignored when it is spelled in the path.

Only the leading separator belongs to the URL syntax, so this uses removeprefix("/") and
raises the message the query branch already raises. An empty path still leaves db unset.

redis://h/0, /2, /2?db=3, /, and no path at all   unchanged
redis://h/3/4/5, /1/2, /abc, /1.5, //7            ValueError, same message as ?db=

The async client carries its own copy of parse_url with the same two lines, so it is
fixed alongside; leaving it would have made the sync and async clients disagree about the
same URL.

Tests cover both clients: the concatenated path, the non-numeric path, and a pair asserting
the path and query spellings now raise alike. Five of them fail on the commit before the
fix. test_empty_path_leaves_db_unset passes either way and is there as the control.

I compared the failing-test set for tests/test_connection_pool.py tests/test_connection.py tests/test_asyncio/test_connection_pool.py with and without the change: the only
difference is the new tests, and no existing test changes state. The failures and errors
that remain in those files need a live Redis server and are unrelated. ruff check and
ruff format --check are clean.

One thing I deliberately left alone: redis://h/-1 is still accepted as db -1. That is a
separate question from parsing and I did not want to widen the change.

---

Note

Medium Risk
Changes connection URL parsing at client startup; apps relying on the old concatenation or silent fallback to db 0 will now get errors or must fix URLs.

Overview
Tightens Redis URL path parsing for the logical database (db) in both sync and async parse_url, so malformed paths fail loudly instead of picking an unintended database.

Path segments are no longer concatenated by stripping every / (e.g. redis://host/3/4/5 used to become db 345). The path is stripped of leading/trailing slashes and parsed as a single integer; invalid values raise ValueError with the same message as ?db=, matching query-string behavior instead of silently falling back to db 0.

Valid cases such as /2, /0/, empty or separator-only paths, and query db overriding path are preserved. Tests cover rejection of multi-segment and non-numeric paths, trailing-separator tolerance, and db 0 edge cases for sync and async pools.

^{Reviewed by Cursor Bugbot for commit 4e23f9d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[eeshsaxena]

Good catch on the 3/4/5 → 345 and abc → silent-db-0 cases. One regression to flag: removeprefix("/") only strips the leading separator, so a trailing slash on an otherwise-valid db path now raises where it previously worked:

• redis://localhost/2/ → was db=2, now ValueError
• redis://localhost/0/ → was db=0, now ValueError

("/2/".removeprefix("/") is "2/", and int("2/") raises.)

Using str.strip("/") instead keeps the rejections you want while tolerating a trailing slash:

• "/2/" → "2" (db 2)
• "/3/4/5" → "3/4/5" (still raises)
• "/abc" → "abc" (still raises)
• "/" → "" (db unset)

Might be worth adding a redis://localhost/2/ case to the tests as well.

[petyaslavova]

Hey @youdie006, thank you for your contribution!

The bug is real - I traced _append_filer_by_value and a single bound was indeed dropped, so all six range commands returned unfiltered samples to a caller who asked for a filtered one. Enforcing the group in the shared param builder is the right layer, DataError matches this module, and the sync and async tests pin it where the guard lives. The code looks good to me as it stands.

The one thing to flag is timing. Unlike _append_block (#4170) and _append_insertion_filters (#3228), which shipped their guards together with brand-new parameters, this changes behavior that has been released for a long time. Breaking changes only go into major releases here, so I will label it accordingly and queue it for the next major rather than an 8.x release. Nothing further needed from you - thanks for the thorough write-up and the mutation check.

[chiruu12]

Hey @youdie006, thank you for your contribution!

The bug is real - I traced _append_filer_by_value and a single bound was indeed dropped, so all six range commands returned unfiltered samples to a caller who asked for a filtered one. Enforcing the group in the shared param builder is the right layer, DataError matches this module, and the sync and async tests pin it where the guard lives. The code looks good to me as it stands.

The one thing to flag is timing. Unlike _append_block (#4170) and _append_insertion_filters (#3228), which shipped their guards together with brand-new parameters, this changes behavior that has been released for a long time. Breaking changes only go into major releases here, so I will label it accordingly and queue it for the next major rather than an 8.x release. Nothing further needed from you - thanks for the thorough write-up and the mutation check.

@petyaslavova sorry, I may be misreading this. This PR changes db parsing in the connection URL path in connection.py, and I could not find _append_filer_by_value or the range commands in it. Was this meant for a different PR?

[chiruu12]

Good catch on the 3/4/5 → 345 and abc → silent-db-0 cases. One regression to flag: removeprefix("/") only strips the leading separator, so a trailing slash on an otherwise-valid db path now raises where it previously worked:

• redis://localhost/2/ → was db=2, now ValueError
• redis://localhost/0/ → was db=0, now ValueError

("/2/".removeprefix("/") is "2/", and int("2/") raises.)

Using str.strip("/") instead keeps the rejections you want while tolerating a trailing slash:

• "/2/" → "2" (db 2)
• "/3/4/5" → "3/4/5" (still raises)
• "/abc" → "abc" (still raises)
• "/" → "" (db unset)

Might be worth adding a redis://localhost/2/ case to the tests as well.

Confirmed, and there are three more cases than the two above. removeprefix takes only the leading separator, so a leading double slash and a percent-encoded trailing one break as well, and a path of nothing but separators becomes an error instead of leaving db unset:

url	before	removeprefix	strip("/")
redis://localhost/2/	db 2	raised	db 2
redis://localhost/0/	db 0	raised	db 0
redis://localhost//2	db 2	raised	db 2
redis://localhost/2%2F	db 2	raised	db 2
redis://localhost///	unset	raised	unset