Repository navigation
CommandObjects refactoring Part 1: Move responsibility to the client class - #4513
Conversation
Test Results 191 files ± 0 191 suites ±0 9m 21s ⏱️ +10s Results for commit d1493e7. ± Comparison against base commit 20c2f3d. This pull request removes 2 and adds 14 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
8fe8c58 to
c793b91
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ggivo
left a comment
ggivo
left a comment
There was a problem hiding this comment.
LGTM in general
A builder for the newly introduced CommandObjectsConfig will avoid more constructors in the future.
And we need to document changes in AbstractClientBuilder in the migration guide.
|
Overall this feels like we are introducing more test surface and (more importantly) public API without much benefit. it would be better to drop those preprocessor, dialect and mapper at all from abstract client and have a abstract supplier method for Not trying to make it a huge deal but i'd push to refine what abstract class promises on the surface; providing defaults, configuring individual sub-objects and having more room for extensibility in concrete classes,, doing all those at one shot signals more refactoring in the future. |
b0171d7 to
3538739
Compare
uglide
added
the
breakingchange
ggivo
left a comment
ggivo
left a comment
There was a problem hiding this comment.
LGTM,
Suggestion to move Moving commandKeyArgumentPreProcessor / jsonObjectMapper / searchDialect onto JedisClientConfig makes sense to me.
Posted a follow-up question on whether CommandObjectsBuilder is needed.
…#4531) CommandObjects and ClusterCommandObjects already expose (RedisProtocol) constructors, so the builder layered nothing on top of two plain setters. Replace the protected createCommandObjects(protocol, clientConfig) + buildCommandObjects(...) seam with a single newCommandObjects(protocol) factory hook and a package-private applyCommandObjectsConfiguration(...) that wires the three JedisClientConfig knobs onto the freshly constructed instance. JedisCluster and RedisClusterClient overrides collapse to one line each. Net effect: -1 public class (CommandObjectsBuilder), -1 static factory (CommandObjects.builder), and a cleaner step toward the "// TODO: Remove together with setProtocol" plan already noted on both no-arg ctors. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
980f47c to
d79428a
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ 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 d1493e7. Configure here.
| } | ||
|
|
||
| this.commandObjects = newCommandObjects(resolvedProtocol); | ||
| applyCommandObjectsConfiguration(commandObjects, clientConfig); |
There was a problem hiding this comment.
Overridable method called from superclass constructor is fragile
Medium Severity
newCommandObjects(resolvedProtocol) is a non-final overridable method invoked from the UnifiedJedis constructor. JedisCluster and RedisClusterClient override it to return ClusterCommandObjects. While currently safe (overrides don't access subclass instance state), this is a classic anti-pattern — any future modification adding subclass field references in the override would silently encounter uninitialized state, since the subclass constructor hasn't run yet. A factory-based or template-based approach that doesn't rely on virtual dispatch during construction would be more robust.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d1493e7. Configure here.
Note
Medium Risk
Refactors core client construction and how
CommandObjectsare created/configured, which could subtly affect protocol negotiation and module command defaults across allUnifiedJedis-based clients.Overview
Moves
CommandObjectsresponsibility from builders to the client layer.UnifiedJedisnow constructsCommandObjectsinternally via anewCommandObjects(RedisProtocol)factory hook (overridden by cluster clients) and applies config-derived knobs in one place.Promotes command-object configuration into
JedisClientConfig. AddscommandKeyArgumentPreProcessor,jsonObjectMapper, andsearchDialecttoJedisClientConfig/DefaultJedisClientConfig, wires builders/clients to pass the full config through, and deprecates the old builder-level setters while still folding them into the final config for compatibility.Reviewed by Cursor Bugbot for commit d1493e7. Bugbot is set up for automated code reviews on this repo. Configure here.