fix(json): scope JSON callbacks to sub-client, not parent Redis client (#3937)

[me-saurabhkohli]

What this PR does

Fixes a stateful side-effect where calling r.json() permanently mutates the parent Redis client's response_callbacks dict, causing subsequent bare execute_command('JSON.GET', ...) calls on the same client to return decoded dicts instead of raw bytes (even with decode_responses=False).

Fixes

Fixes #3937

Root Cause

_JSONBase.__init__ called self.client.set_response_callback(key, value) for every entry in _MODULE_CALLBACKS:

# Before — permanently pollutes the parent client
for key, value in self._MODULE_CALLBACKS.items():
    self.client.set_response_callback(key, value)

After the first r.json() call, the parent client has 13 JSON.* callbacks registered. Any subsequent execute_command('JSON.GET', ...) on the parent returns a decoded dict regardless of decode_responses.

Fix

• Removed the set_response_callback loop from _JSONBase.__init__
• Removed the self.execute_command = client.execute_command instance attribute (it shadowed the class method)
• Added execute_command() on JSON (sync) and async execute_command() on AsyncJSON — each calls the parent and applies _MODULE_CALLBACKS locally to the raw response, without touching the parent's callback table
• Updated pipeline() to merge _MODULE_CALLBACKS into the pipeline's private response_callbacks snapshot so pipeline JSON calls continue to decode correctly

Tests

New file: tests/test_json_callback_isolation.py — 4 tests, all @pytest.mark.fixed_client (no live Redis required):

Test	Asserts
test_json_method_does_not_mutate_parent_response_callbacks	No JSON.* keys in parent after r.json()
test_json_method_called_multiple_times_does_not_accumulate_callbacks	Idempotent — repeated calls stay clean
test_two_independent_clients_do_not_share_json_callbacks	Client isolation
test_json_subclient_callbacks_are_registered_on_json_object_not_parent	_MODULE_CALLBACKS on sub-client, not parent

tests/test_json_callback_isolation.py::test_json_method_does_not_mutate_parent_response_callbacks PASSED
tests/test_json_callback_isolation.py::test_json_method_called_multiple_times_does_not_accumulate_callbacks PASSED
tests/test_json_callback_isolation.py::test_two_independent_clients_do_not_share_json_callbacks PASSED
tests/test_json_callback_isolation.py::test_json_subclient_callbacks_are_registered_on_json_object_not_parent PASSED

Checklist

• Bug reproduced and root cause confirmed
• Fix scoped to redis/commands/json/__init__.py only — no other modules changed
• Sync (JSON) and async (AsyncJSON) paths both fixed
• Pipeline path fixed (callbacks injected into pipeline's local snapshot)
• 4 regression tests added, all passing, no live Redis required
• No public API changes — r.json().get() behaviour unchanged
• Pre-existing test failures in test_retry.py confirmed unrelated (reproduce on master without this patch)

---

Note

Medium Risk
Touches core command/response handling for the JSON module across sync, async, and cluster pipelines; behavior change is intentional but any missed entry path could leave raw or double-decoded responses.

Overview
Fixes #3937 by stopping r.json() from registering JSON module decoders on the parent Redis client’s shared response_callbacks, which had caused bare execute_command("JSON.*", …) on that client to return parsed objects even when decode_responses=False.

JSON / AsyncJSON now apply _MODULE_CALLBACKS in overridden execute_command (case-insensitive command names, pipeline return-value guard, and only _json_path forwarded to callbacks—not internal kwargs like keys). For pipelines (r.pipeline().json(), r.json().pipeline(), sync/async/cluster), JSON callbacks are merged into a private copy of the pipeline’s callback dict so execute() still decodes JSON responses without touching the parent.

Adds tests/test_json_callback_isolation.py with unit and integration coverage for parent isolation, pipeline/cluster/async paths, and end-to-end pipeline decoding.

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

[jit-ci]

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes issue #3937 by ensuring RedisJSON response callbacks remain isolated to the JSON sub-client, preventing pollution of the parent Redis client’s response_callbacks and incorrect decoding on subsequent raw execute_command("JSON.*") calls.

Changes:

• Stop registering JSON module callbacks on the parent client in _JSONBase.__init__.
• Add local execute_command implementations on JSON / AsyncJSON to apply JSON callbacks without mutating the parent client.
• Update JSON pipeline creation to include JSON callbacks via a merged, pipeline-local callback map.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
tests/test_json_callback_isolation.py	Adds regression/unit tests asserting r.json() does not mutate parent callback tables and keeps JSON callbacks scoped.
redis/commands/json/init.py	Removes parent callback registration, adds local callback application in execute_command, and merges callbacks into JSON pipelines.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[petyaslavova]

Hey @me-saurabhkohli, thank you for your contribution! The over all approach looks good! Can you please check the cursor bot's comments and test failures?

[me-saurabhkohli]

Thanks for flagging those — both issues fixed in the latest commit:

1. TypeError from **kwargs forwarding (high severity)
execute_command was passing all **kwargs straight to the module callback, but most callbacks (_decode, bulk_of_jsons, int, simple lambdas) do not accept keyword arguments. Methods like JSON.GET pass redis-internal kwargs like keys= and options= which caused a TypeError on nearly every operation — silently swallowed by the try/except TypeError in get(), returning None instead of real data.

Fix: only _json_path (the one JSON-module kwarg) is forwarded; everything else is stripped before the callback is invoked.

2. ClusterPipeline missing JSON callbacks (medium severity)
pipeline() was passing self.client.cluster_response_callbacks directly to ClusterPipeline, but those no longer contain JSON callbacks since we stopped registering them on the parent client in __init__. The regular Pipeline path already merged _MODULE_CALLBACKS into a copy — the cluster path now does the same.

Both fixes covered by new tests in test_json_callback_isolation.py.

[me-saurabhkohli]

Hi @petyaslavova, Thanks for the review and kind words!

Just to close the loop on both items:

Cursor Bugbot issues
Both fixed in the latest commit (7b8c4ed):

1. TypeError from **kwargs forwarding — only _json_path is now forwarded to callbacks; all redis-internal kwargs (keys=, options=, etc.) are stripped before invoking the callback.
2. ClusterPipeline missing JSON callbacks — pipeline() now merges _MODULE_CALLBACKS into a private copy for both regular and cluster pipelines, matching the existing Pipeline path.

Test failures in test_retry.py
These are pre-existing on master and reproducable without this patch applied. They are unrelated to the JSON callback changes in this PR. Let me know if you want me to address them as part of this PR as well.

Happy to make any further adjustments if needed. Thanks!

[petyaslavova]

JSON.execute_command() now always assumes self.client.execute_command() returned a Redis response and immediately applies the JSON callback. That breaks when the JSON sub-client is created from a pipeline, because pipeline execute_command() returns the queued pipeline object, not a server response.

Search handles this distinction explicitly. For example, SearchCommands.search() calls self.execute_command(...), then checks:

if isinstance(res, Pipeline):
    return res

That guard is important because module callbacks must run at pipeline execution time, not when the command is queued.

Could you please add pipelines handling similar to what Search does: return the queued pipeline object unchanged?

[me-saurabhkohli]

Hi @petyaslavova , Thanks for the detailed feedback!

I have fixed both the concerns, added a check using isInstance(response, (Pipeline, ClusterPIpeline)) similar to Search module's guard pattern.

[petyaslavova]

With the original implementation the callbacks, have been check in CaseInsensitiveDict and calls like j.execute_command("json.get", "k") have been successfully post-processed.
self._MODULE_CALLBACKS is a regular dict and we are loosing this old behaviour.

[me-saurabhkohli]

Fixed by upper-casing the command name before the lookup, also added test coverage for lower and mixed case inputs.

[petyaslavova]

There is a second pipeline-specific piece here beyond returning the queued pipeline object: pipeline decoding happens later, inside Pipeline.execute(), by looking up each queued command in the pipeline’s own callback table.

For example, the sync pipeline execution path eventually does roughly:

for r, cmd in zip(response, commands):
    args, options = cmd
    command_name = args[0]
    if command_name in self.response_callbacks:
        r = self.response_callbacks[command_name](r, **options)

So if JSON callbacks only live on the JSON sub-client’s _MODULE_CALLBACKS, p.execute() cannot apply them to the individual JSON.* responses. This matters for the existing pipeline.json() access path:

p = redis.Redis().pipeline()
p.json().set("k", ".", {"a": 1})
p.json().get("k")
res = p.execute()

Even if JSON.execute_command() is changed to return the pipeline object unchanged when queueing, the pipeline still needs JSON.SET, JSON.GET, etc. registered in its private response_callbacks map before execute() runs. Otherwise the queued JSON commands will return raw Redis wire responses instead of the module-level Python shapes.

Search handles this by constructing module-specific pipeline classes that copy the parent callbacks and register module callbacks on the pipeline-local map. For JSON, we should make sure both supported pipeline entry points get execution-time callbacks:

r.json().pipeline()   # already intended to merge JSON callbacks into the new pipeline
r.pipeline().json()   # should also ensure the existing pipeline gets JSON callbacks

Without that, the fix only avoids parent-client pollution, but leaves pipeline execution without the callbacks it needs to decode each queued JSON response.

[me-saurabhkohli]

This was the trickiest one, thanks for the detailed explanation.

For the r.pipeline().json() path, I have added check in __init__() at line 136, when the client is a Pipeline, we merge _MODULE_CALLBACKS into the pipeline's own response_callbacks.

One critical use case I ran into - Pipeline.response_callbacks is a shared reference to the parent client's dict (passed by reference in Redis.pipeline()). So we have to replace it with a private copy via dict(client.response_callbacks) before calling .update() otherwise we'd re-introduce the exact parent pollution that #3937 was about.

Tests added for both entry points, callback isolation, idempotent p.json() calls, and a cluster negative assertion confirming the parent's cluster_response_callbacks stays clean.

[petyaslavova]

There are a few test gaps that would make this regression much easier to catch.

The current isolation tests cover “r.json() does not mutate the parent callback table”, but they do not fully cover the pipeline execution paths where callbacks must move to a pipeline-local table instead.

I think we need tests for at least these cases:

p = redis.Redis().pipeline()
result = p.json().set("k", ".", {"a": 1})

assert result is p
assert "JSON.SET" in p.response_callbacks
assert "JSON.SET" not in p.connection_pool.redis_client.response_callbacks  # or equivalent parent assertion

And, more importantly, a test that verifies callbacks are applied when execute() processes each queued JSON response:

p = r.pipeline()
p.json().set("k", ".", {"a": 1})
p.json().get("k")

assert p.execute() == [True, {"a": 1}]

That should be covered for both supported JSON pipeline entry points:

r.json().pipeline()
r.pipeline().json()

The async path should have the same coverage too:

p = r.pipeline()
await p.json().set("k", ".", {"a": 1})
await p.json().get("k")

assert await p.execute() == [True, {"a": 1}]

There should also be a regression test proving callback isolation and execution-time decoding happen together:

r = redis.Redis()
p = r.pipeline()

p.json().get("k")

assert "JSON.GET" not in r.response_callbacks
assert "JSON.GET" in p.response_callbacks

For cluster, the new test currently uses a mocked RedisCluster but does not populate instance attributes such as nodes_manager, so it fails before it reaches the callback assertion. It would be useful to replace/fix that test so it verifies the actual contract:

json_pipeline = r.json().pipeline()

assert "JSON.GET" in json_pipeline.cluster_response_callbacks
assert "JSON.GET" not in r.cluster_response_callbacks

Without these tests, the PR can pass the parent-client isolation checks while still leaving queued JSON commands undecoded at Pipeline.execute() time.

[me-saurabhkohli]

Agreed. Added more test coverage, please review and let me know if few extra corner use cases would need to be removed from this PR. Thanks!

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit 2650cea. Configure here.}

[cursor]

Pipeline loses case-insensitive callbacks

Medium Severity

When p.json() (or cluster p.json()) runs, response_callbacks / cluster_response_callbacks is replaced with a plain dict copy instead of CaseInsensitiveDict. Pipeline.execute() then looks up callbacks with the queued command string as-is, so mixed- or lower-case commands (e.g. json.get) no longer decode after this path.

Additional Locations (1)

• redis/commands/json/__init__.py#L142-L146

 

^{Reviewed by Cursor Bugbot for commit 2650cea. Configure here.}

[me-saurabhkohli]

Intentionally kept dict() as followed in Search / TimeSeries module.

CC: @petyaslavova