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
Localized bugfix to JSON bulk-import key derivation; behavior changes only for paths that were previously truncated at the first dot, with no security or persistence-layer impact beyond different Redis keys for those cases.

Overview
JSON.set_path (sync and async) no longer builds each file’s Redis key with file_path.rsplit(".")[0], which treated every dot as a separator and broke paths like v1.2/data.json. Both implementations now use os.path.splitext(file_path)[0] so only the final path component’s extension is removed; related TODO comments are replaced with notes on dotted directories, extensionless names, dotfiles, and multi-segment suffixes (e.g. .tar.gz → .tar).

Tests add test_set_path_key_strips_only_extension (sync/async, set_file stubbed, no server) and refresh test_set_path to load JSON from a v1.2 folder and assert retrieval by the full path-based key.

^{Reviewed by Cursor Bugbot for commit ee9ac53. 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.

[SWAPI03]

Thanks for the thorough review @petyaslavova, and good catch on the remaining gap. You're right that rsplit(".", 1) splits on the last dot in the whole path, so /data/v1.2/README and /data/.env were still truncated. Pushed a follow-up commit addressing all points:

1. Both call sites now use file_name = os.path.splitext(file_path)[0] (sync commands.py and async __init__.py; os was already imported in both). This strips only the extension of the final path component, so a dotted directory, an extension-less file, a dotfile, and a multi-part extension are all handled correctly.
2. The server-less regression tests now cover a no-extension file (README) and a dotfile (.env) under the dotted directory, plus a .tar.gz to show only the last extension is stripped. Verified they fail on the old derivation and pass now.
3. Moved the json/os/tempfile imports out of the test bodies. The unit tests no longer need them (they use tmp_path + Path.write_text); test_set_path uses a single module-level import json.
4. test_set_path now asserts the expected key directly (str(dotted_dir / "data")) instead of repeating the implementation's split.
5. Took the optional nit too: both the updated and new tests use the tmp_path fixture, so there's no manual tempfile cleanup.

Ready for another look when you have a moment.