Fix JSON.set_path key derivation to strip only the file extension

[SWAPI03]

Summary

JSONCommands.set_path and its async mirror AsyncJSON.set_path build the Redis key for each file with file_path.rsplit(".")[0], which splits on every dot. Any path with a dot before the extension gets truncated at the first dot and produces the wrong key. This is the fix the existing TODO in both files asked for.

Examples of the key that gets created:

File on disk	Before (buggy)	After (fixed)
/data/file.json	/data/file	/data/file
/data/v1.2/file.json	/data/v1	/data/v1.2/file
/data/config.dev.json	/data/config	/data/config.dev

So anyone using a versioned directory (like v1.2) or a multi-dot filename silently ends up with truncated keys.

Fix

Use rsplit(".", 1) in both the sync (redis/commands/json/commands.py) and async (redis/commands/json/__init__.py) implementations, so only the final extension is stripped. This matches the intent noted in the existing TODO comments, which are now resolved.

Tests

• Added sync and async regression tests (test_set_path_key_strips_only_extension) that stub set_file and assert the derived key. They need no server, so they run in normal CI. Verified they fail on the old code and pass on the fixed code.
• Updated the existing redismod test_set_path to place the file under a dotted directory (v1.2), so it also guards this against a live server.

ruff check and ruff format are clean.

---

Note

Low Risk
Small, localized bugfix in key derivation with matching tests; behavior change only affects previously wrong keys for dotted paths.

Overview
set_path (sync and async) now derives each file’s Redis key with rsplit(".", 1) instead of rsplit("."), so only the final extension is removed. Paths with dots in directory or basename segments (e.g. v1.2 or config.dev.json) no longer get truncated at the first dot.

Regression coverage adds test_set_path_key_strips_only_extension (stubbed set_file, no server) and updates integration test_set_path to use a v1.2 subdirectory.

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

[eeshsaxena]

Superseded by my follow-up review below, which has the full analysis. Short version: this is a real improvement, but rsplit(".", 1) still truncates a no-extension file under a dotted directory (/data/v1.2/README -> /data/v1), so the stated goal is not fully met; os.path.splitext(file_path)[0] closes that case (and dotfiles) with no new import. See the detailed review for specifics.

[eeshsaxena]

Nice catch, and thanks for handling both the sync (commands.py) and async (__init__.py) call sites plus adding server-less regression tests for each. The common case is correct now: "/data/v1.2/file.json".rsplit(".", 1)[0] gives /data/v1.2/file as intended.

One gap remains, though: rsplit(".", 1) splits on the last dot anywhere in the full path, not just within the filename. So the stated goal ("avoid truncating paths that contain dots in a directory") isn't fully met when the file itself has no extension, because the split then falls back onto a dot in the directory:

>>> "/data/v1.2/README".rsplit(".", 1)[0]
'/data/v1'          # directory truncated at its dot
>>> "/data/.env".rsplit(".", 1)[0]
'/data/'            # dotfile basename swallowed as an "extension"

os.path.splitext scopes the extension split to the basename, so it handles both of these (and multi-dot names) correctly:

>>> os.path.splitext("/data/v1.2/README")[0]
'/data/v1.2/README'
>>> os.path.splitext("/data/v1.2/file.json")[0]
'/data/v1.2/file'
>>> os.path.splitext("/data/.env")[0]
'/data/.env'

Since os is already imported in both modules (__init__.py:2, and commands.py already uses os.walk/os.path.join), file_name = os.path.splitext(file_path)[0] is a drop-in that closes the remaining cases with no new import. It might also be worth adding a v1.2/README-style case (no-extension file under a dotted directory) to the new regression tests, since that is the one rsplit(".", 1) still gets wrong.

Everything else looks good to me: the two call sites stay consistent, and stubbing set_file to run the regression without a server is a nice touch.

[petyaslavova]

Hey @SWAPI03, thank you for your contribution! I'll take a look at it next week.

[petyaslavova]

Hey @SWAPI03, thank you for your contribution!

This is the right direction and it resolves the common case the TODO described — thanks for covering both the sync and async call sites and for adding server-less regression tests for each. One gap remains, as @eeshsaxena also noted: rsplit(".", 1) splits on the last dot in the whole path, not just in the file name. So a file without an extension under a dotted directory is still truncated (/data/v1.2/README -> /data/v1), and a dotfile collapses to its directory (/data/.env -> /data/). Since set_path walks every file, not only *.json, both cases are reachable.

Before we can merge, please address the following. First, switch both call sites to file_name = os.path.splitext(file_path)[0] (os is already imported in both modules), and extend the new regression tests with a no-extension file under a dotted directory and a dotfile case. Second, move the json / os / tempfile imports in the new and updated tests to the module top — per our contributor guidelines imports belong at the top of the file and function-level imports should be avoided. Third, please have the test_set_path assertion state the expected key directly rather than repeating the implementation's split, so it cannot pass by construction if the derivation changes again.

One optional nit, not blocking: the new tests use tempfile.mkdtemp() without cleanup, where pytest's tmp_path fixture would be simpler and cleaned up automatically.

Thanks again — once those points are covered this should be ready for another review.

[petyaslavova]

Please move those imports to the top level in the import section.

[petyaslavova]

Same note for the imports.

[Mukller]

Verified locally on the PR branch:

Tests: the new server-independent regression test passes (pytest tests/test_asyncio/test_json.py -k strips_only — 1 passed), and I confirmed a matching sync-side regression was added too (tests/test_json.py::test_set_path_key_strips_only_extension). Nice that both sides are covered.

Both call sites fixed: rsplit(".", 1) in redis/commands/json/commands.py:711 (sync set_path) and redis/commands/json/__init__.py:353 (async variant) — no remaining bare rsplit(".") in the JSON module.

This executes exactly what the codebase's own TODO comment prescribed ("Should be rsplit(".", 1) — fix in a separate PR"), with the comment now replaced by an explanation of why. The stubbed-set_file test design is good: it pins key derivation without needing a RedisJSON server, and asserts /data/v1.2/data.json → key /data/v1.2/data rather than /data/v1.

No issues found.