close
Skip to content

Reverting the changes related to RESP2 and RESP3 unification of the returned response objects #4048

Open
petyaslavova wants to merge 4 commits intomasterfrom
ps_resp2_resp3_unification_revert
Open

Reverting the changes related to RESP2 and RESP3 unification of the returned response objects #4048
petyaslavova wants to merge 4 commits intomasterfrom
ps_resp2_resp3_unification_revert

Conversation

@petyaslavova
Copy link
Copy Markdown
Collaborator

@petyaslavova petyaslavova commented Apr 23, 2026

Reverting these changes because they will be done following a different design


Note

High Risk
High risk because it flips default protocol/parsing behavior back to RESP2 and removes many response-normalization callbacks, which can change return types and pipeline post-processing across core and module commands.

Overview
Reverts the prior RESP2/RESP3 response unification effort: removes the migration/unification docs, restores RESP2 as the default for sync/async clients (including default async parser), and simplifies protocol detection logic.

Rolls back many unified response callbacks/parsers (e.g., sorted-set score pairing, XREAD parsing, Sentinel/COMMAND/ACL/MEMORY/STRALGO handling, cluster shards key normalization), and removes several module-level RESP3 normalizations (Bloom *INFO dict support, JSON RESP/type/numop handling and JSON pipeline callback wiring). Type hints/docs are updated accordingly to reflect the reintroduced protocol-specific response shapes.

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

Comment thread redis/_parsers/helpers.py
state = parse_sentinel_state_resp3(master)
result[state["name"]] = state
return result
return [parse_sentinel_state_resp3(master) for master in response]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESP3 sentinel masters returns list, breaking discover_master

High Severity

parse_sentinel_masters_resp3 now returns a list instead of a dict, but discover_master in sentinel.py calls masters.get(service_name) on the result. Lists don't have a .get() method, so any RESP3 Sentinel user calling discover_master will crash with AttributeError.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 664e285. Configure here.

Comment thread redis/_parsers/helpers.py
result[str_if_bytes(key)] = value
except Exception:
result[str_if_bytes(key)] = str_if_bytes(response[key])
result[str_if_bytes(key)] = response[str_if_bytes(key)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESP3 sentinel state parser crashes on bytes keys

High Severity

parse_sentinel_state_resp3 looks up SENTINEL_STATE_TYPES[key] where key is bytes in the default decode_responses=False mode. This always raises KeyError (the dict has string keys). The except handler then does response[str_if_bytes(key)], using a string key on a bytes-keyed dict, causing another KeyError that propagates uncaught.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 664e285. Configure here.

Comment thread redis/_parsers/helpers.py
result[str_if_bytes(key)] = value
except Exception:
result[str_if_bytes(key)] = str_if_bytes(response[key])
result[str_if_bytes(key)] = response[str_if_bytes(key)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESP3 sentinel state missing boolean flag fields

High Severity

parse_sentinel_state_resp3 no longer sets is_master, is_slave, is_sdown, is_odown, or is_sentinel boolean fields. These are required by check_master_state and filter_slaves in sentinel.py, which will raise KeyError when accessing them on RESP3 connections.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 664e285. Configure here.

Comment thread redis/_parsers/helpers.py
return {key: parse_stream_list(value, **options) for key, value in response.items()}
return {
key: [parse_stream_list(value, **options)] for key, value in response.items()
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RESP3 XREAD response wraps value in extra list

Medium Severity

parse_xread_resp3 wraps parse_stream_list(value) in an extra list — [parse_stream_list(value, **options)] — adding a spurious nesting level. RESP3 already returns stream entries as a list, so the parsed result gets double-wrapped, producing {key: [[entries...]]} instead of {key: [entries...]}.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 664e285. Configure here.

p = Pipeline(
connection_pool=self.client.connection_pool,
response_callbacks=self.client.response_callbacks,
response_callbacks=self._MODULE_CALLBACKS,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON pipeline loses core Redis response callbacks

Medium Severity

The JSON pipeline() method reverted to passing response_callbacks=self._MODULE_CALLBACKS instead of self.client.response_callbacks. This means JSON pipelines only have module-specific callbacks and miss all core Redis command callbacks, so core commands executed through a JSON pipeline won't be properly post-processed.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 664e285. Configure here.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented Apr 23, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

There are 6 total unresolved issues (including 5 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 29cacd0. Configure here.

raise ConnectionError("protocol must be either 2 or 3")
self.protocol = p
# Reconcile parser ↔ protocol mismatches.
# Hiredis handles both RESP2 and RESP3 natively, so only
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async parser set before protocol, reconciliation removed

High Severity

set_parser(parser_class) is called before the protocol version is determined, and the parser-protocol reconciliation logic was removed. When hiredis is unavailable, DefaultParser is _AsyncRESP2Parser. If a user passes protocol=3, the connection will use a RESP2 parser for a RESP3 connection, which will fail. The removed reconciliation code was responsible for swapping the parser to match the protocol.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 29cacd0. Configure here.

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.

1 participant