Skip to content

CommandObjects refactoring Part 3: Enforce protocol value and use RESP2 instead of the null value #4515

Merged
uglide merged 4 commits into
masterfrom
im/refactor-command-objects-p3
May 20, 2026
Merged

CommandObjects refactoring Part 3: Enforce protocol value and use RESP2 instead of the null value #4515
uglide merged 4 commits into
masterfrom
im/refactor-command-objects-p3

Conversation

@uglide
Copy link
Copy Markdown
Contributor

@uglide uglide commented May 8, 2026

Note

Medium Risk
Touches protocol selection/negotiation paths across Jedis, pipelines, transactions, and cluster pipeline, which can affect reply decoding and compatibility when protocol is unspecified or negotiation fails. Changes are small but impact core client behavior and are best validated against both RESP2/RESP3 servers.

Overview
Enforces a concrete RESP protocol for CommandObjects and removes reliance on null as a protocol sentinel. CommandObjects now throws if constructed with null, and callers (Jedis, Pipeline, Transaction, ReliableTransaction, UnifiedJedis, ClusterPipeline) consistently use RedisProtocol.orServerDefault(...) to fall back to the server default (RESP2) when no protocol is resolved.

Updates a few command reply parsers (hrandfieldWithValues, jsonType) to branch explicitly on RESP2 vs non-RESP2 rather than non-RESP3, and adjusts tests to stop parameterizing CommandObjects tests with a null protocol.

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

Test Results

  195 files  ±  0    195 suites  ±0   9m 34s ⏱️ -28s
7 307 tests  - 459  7 084 ✅  - 447  223 💤  - 12  0 ❌ ±0 
7 327 runs   - 459  7 104 ✅  - 447  223 💤  - 12  0 ❌ ±0 

Results for commit 9006db1. ± Comparison against base commit 49ac82c.

This pull request removes 459 tests.
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitcount()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitfield()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitfieldBinary()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitop()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitopBinary()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testBitpos()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testSetbitAndGetbit()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBitmapCommandsTest[3] ‑ testSetbitAndGetbitBinary()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBloomFilterCommandsTest[3] ‑ testBfAddAndExists()[3]
redis.clients.jedis.commands.commandobjects.CommandObjectsBloomFilterCommandsTest[3] ‑ testBfInfo()[3]
…
This pull request skips 8 tests.
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonArrLenRoot()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonDebugMemoryRoot()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonGetWithClass()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonObjKeysRoot()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonObjLenRoot()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonStrAppendRootPath()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonStrLenRootPath()[2]
redis.clients.jedis.commands.commandobjects.CommandObjectsJsonCommandsTest[2] ‑ testJsonTypeOldPath()[2]

♻️ This comment has been updated with latest results.

@jit-ci
Copy link
Copy Markdown

jit-ci Bot commented May 8, 2026

🛡️ Jit Security Scan Results

CRITICAL HIGH MEDIUM

✅ No security findings were detected in this PR


Security scan by Jit

@uglide uglide added the breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes. label May 8, 2026
@uglide uglide force-pushed the im/refactor-command-objects-p3 branch from b0de2e4 to 51b646b Compare May 8, 2026 08:43
@uglide uglide added the techdebt Things that can be improved or refactored label May 8, 2026
@uglide uglide requested review from atakavci, ggivo and tishun and removed request for ggivo May 8, 2026 09:00
Copy link
Copy Markdown
Contributor

@atakavci atakavci left a comment

Choose a reason for hiding this comment

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

LGTM
(went through only the changes against part2)

Comment thread src/main/java/redis/clients/jedis/Jedis.java Outdated
@atakavci
Copy link
Copy Markdown
Contributor

many test case impacted;
java.lang.IllegalArgumentException: protocol must not be null

@uglide uglide force-pushed the im/refactor-command-objects-p3 branch from 51b646b to 2f2d293 Compare May 20, 2026 09:48
Copy link
Copy Markdown
Collaborator

@ggivo ggivo left a comment

Choose a reason for hiding this comment

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

LGTM

@uglide uglide merged commit e390965 into master May 20, 2026
18 checks passed
@uglide uglide deleted the im/refactor-command-objects-p3 branch May 20, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breakingchange Pull request that has breaking changes. Must include the breaking behavior in release notes. techdebt Things that can be improved or refactored

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants