Skip to content

Commit e390965

Browse files
uglideuglide
authored
CommandObjects refactoring Part 3: Enforce protocol value and use RESP2 instead of the null value (#4515)
* Part 3: Enforce setting protocol in CommandObjects constructor * Fix rebase issue by removing redundant constructor * Add RedisProtocol.orServerDefault method to encapsulate the logic around protocol default for CommandObjects * Fix tests: use commandObjectsRespVersions for CommandObjects tests
1 parent 49ac82c commit e390965

10 files changed

Lines changed: 47 additions & 28 deletions

File tree

‎src/main/java/redis/clients/jedis/ClusterPipeline.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -120,10 +120,10 @@ private static Supplier<ClusterCommandObjects> createClusterCommandObjects(
120120
effective = resolved;
121121
}
122122
} catch (JedisException je) {
123-
// Protocol negotiation failed, leave protocol=null on the command objects.
123+
// Protocol negotiation failed; fall back to RESP2 below.
124124
}
125125
}
126-
return new ClusterCommandObjects(effective);
126+
return new ClusterCommandObjects(RedisProtocol.orServerDefault(effective));
127127
};
128128
}
129129

‎src/main/java/redis/clients/jedis/CommandObjects.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ public class CommandObjects {
4141
private final RedisProtocol protocol;
4242

4343
public CommandObjects(RedisProtocol protocol) {
44+
if (protocol == null) {
45+
throw new IllegalArgumentException("protocol must not be null");
46+
}
4447
this.protocol = protocol;
4548
}
4649

@@ -1203,7 +1206,7 @@ public final CommandObject<List<String>> hrandfield(String key, long count) {
12031206

12041207
public final CommandObject<List<Map.Entry<String, String>>> hrandfieldWithValues(String key, long count) {
12051208
return new CommandObject<>(commandArguments(HRANDFIELD).key(key).add(count).add(WITHVALUES),
1206-
protocol != RedisProtocol.RESP3 ? BuilderFactory.STRING_PAIR_LIST : BuilderFactory.STRING_PAIR_LIST_FROM_PAIRS);
1209+
protocol == RedisProtocol.RESP2 ? BuilderFactory.STRING_PAIR_LIST : BuilderFactory.STRING_PAIR_LIST_FROM_PAIRS);
12071210
}
12081211

12091212
public final CommandObject<Map<byte[], byte[]>> hgetAll(byte[] key) {
@@ -1220,7 +1223,7 @@ public final CommandObject<List<byte[]>> hrandfield(byte[] key, long count) {
12201223

12211224
public final CommandObject<List<Map.Entry<byte[], byte[]>>> hrandfieldWithValues(byte[] key, long count) {
12221225
return new CommandObject<>(commandArguments(HRANDFIELD).key(key).add(count).add(WITHVALUES),
1223-
protocol != RedisProtocol.RESP3 ? BuilderFactory.BINARY_PAIR_LIST : BuilderFactory.BINARY_PAIR_LIST_FROM_PAIRS);
1226+
protocol == RedisProtocol.RESP2 ? BuilderFactory.BINARY_PAIR_LIST : BuilderFactory.BINARY_PAIR_LIST_FROM_PAIRS);
12241227
}
12251228

12261229
public final CommandObject<ScanResult<Map.Entry<String, String>>> hscan(String key, String cursor, ScanParams params) {
@@ -3989,7 +3992,7 @@ public final CommandObject<Class<?>> jsonType(String key) {
39893992

39903993
public final CommandObject<List<Class<?>>> jsonType(String key, Path2 path) {
39913994
return new CommandObject<>(commandArguments(JsonCommand.TYPE).key(key).add(path),
3992-
protocol != RedisProtocol.RESP3 ? JsonBuilderFactory.JSON_TYPE_LIST : JsonBuilderFactory.JSON_TYPE_RESPONSE_RESP3_COMPATIBLE);
3995+
protocol == RedisProtocol.RESP2 ? JsonBuilderFactory.JSON_TYPE_LIST : JsonBuilderFactory.JSON_TYPE_RESPONSE_RESP3_COMPATIBLE);
39933996
}
39943997

39953998
@Deprecated

‎src/main/java/redis/clients/jedis/Jedis.java‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,6 @@ public class Jedis implements ServerCommands, DatabaseCommands, JedisCommands, J
9898

9999
private static final Logger logger = LoggerFactory.getLogger(Jedis.class);
100100

101-
private static final RedisProtocol REDIS_SERVER_DEFAULT_PROTO = RedisProtocol.RESP2;
102-
103101
protected final Connection connection;
104102
private final CommandObjects commandObjects;
105103
private int db = 0;
@@ -132,7 +130,7 @@ private static JedisClientConfig sanitize(JedisClientConfig config) {
132130

133131
public Jedis() {
134132
connection = new Connection();
135-
commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
133+
commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
136134
}
137135

138136
/**
@@ -146,12 +144,12 @@ public Jedis(final String url) {
146144

147145
public Jedis(final HostAndPort hp) {
148146
connection = new Connection(hp);
149-
commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
147+
commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
150148
}
151149

152150
public Jedis(final String host, final int port) {
153151
connection = new Connection(host, port);
154-
commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
152+
commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
155153
}
156154

157155
public Jedis(final String host, final int port, final JedisClientConfig config) {
@@ -161,8 +159,7 @@ public Jedis(final String host, final int port, final JedisClientConfig config)
161159
public Jedis(final HostAndPort hostPort, final JedisClientConfig config) {
162160
JedisClientConfig effective = sanitize(config);
163161
connection = new Connection(hostPort, effective);
164-
RedisProtocol proto = effective.getRedisProtocol();
165-
commandObjects = new CommandObjects(proto != null ? proto : REDIS_SERVER_DEFAULT_PROTO);
162+
commandObjects = new CommandObjects(RedisProtocol.orServerDefault(effective.getRedisProtocol()));
166163
}
167164

168165
public Jedis(final String host, final int port, final boolean ssl) {
@@ -242,7 +239,7 @@ public Jedis(URI uri) {
242239
.password(JedisURIHelper.getPassword(uri)).database(JedisURIHelper.getDBIndex(uri))
243240
.protocol(JedisURIHelper.getRedisProtocol(uri))
244241
.ssl(JedisURIHelper.isRedisSSLScheme(uri)).build());
245-
commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
242+
commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
246243
}
247244

248245
public Jedis(URI uri, final SSLSocketFactory sslSocketFactory,
@@ -311,25 +308,23 @@ public Jedis(final URI uri, JedisClientConfig config) {
311308
.ssl(JedisURIHelper.isRedisSSLScheme(uri)).sslSocketFactory(effective.getSslSocketFactory())
312309
.sslParameters(effective.getSslParameters()).hostnameVerifier(effective.getHostnameVerifier())
313310
.build());
314-
RedisProtocol proto = effective.getRedisProtocol();
315-
commandObjects = new CommandObjects(proto != null ? proto : REDIS_SERVER_DEFAULT_PROTO);
311+
commandObjects = new CommandObjects(RedisProtocol.orServerDefault(effective.getRedisProtocol()));
316312
}
317313

318314
public Jedis(final JedisSocketFactory jedisSocketFactory) {
319315
connection = new Connection(jedisSocketFactory);
320-
commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
316+
commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
321317
}
322318

323319
public Jedis(final JedisSocketFactory jedisSocketFactory, final JedisClientConfig clientConfig) {
324320
JedisClientConfig effective = sanitize(clientConfig);
325321
connection = new Connection(jedisSocketFactory, effective);
326-
RedisProtocol proto = effective.getRedisProtocol();
327-
commandObjects = new CommandObjects(proto != null ? proto : REDIS_SERVER_DEFAULT_PROTO);
322+
commandObjects = new CommandObjects(RedisProtocol.orServerDefault(effective.getRedisProtocol()));
328323
}
329324

330325
public Jedis(final Connection connection) {
331326
this.connection = connection;
332-
this.commandObjects = new CommandObjects(REDIS_SERVER_DEFAULT_PROTO);
327+
this.commandObjects = new CommandObjects(RedisProtocol.REDIS_SERVER_DEFAULT_PROTO);
333328
}
334329

335330
@Override

‎src/main/java/redis/clients/jedis/Pipeline.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public Pipeline(Connection connection, boolean closeConnection) {
3232
}
3333

3434
private static CommandObjects createCommandObjects(Connection connection) {
35-
return new CommandObjects(connection.getRedisProtocol());
35+
return new CommandObjects(RedisProtocol.orServerDefault(connection.getRedisProtocol()));
3636
}
3737

3838
Pipeline(Connection connection, boolean closeConnection, CommandObjects commandObjects) {

‎src/main/java/redis/clients/jedis/RedisProtocol.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ public enum RedisProtocol {
2323
RESP2("2"),
2424
RESP3("3");
2525

26+
public static final RedisProtocol REDIS_SERVER_DEFAULT_PROTO = RESP2;
27+
2628
private final String version;
2729

2830
private RedisProtocol(String ver) {
@@ -45,4 +47,8 @@ public static RedisProtocol from(Long proto) {
4547
if (proto == 3) return RESP3;
4648
throw new IllegalArgumentException("Unknown protocol version: " + proto);
4749
}
50+
51+
public static RedisProtocol orServerDefault(RedisProtocol proto) {
52+
return (proto == null) ? REDIS_SERVER_DEFAULT_PROTO : proto;
53+
}
4854
}

‎src/main/java/redis/clients/jedis/ReliableTransaction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public ReliableTransaction(Connection connection, boolean doMulti, boolean close
8585
}
8686

8787
private static CommandObjects createCommandObjects(Connection connection) {
88-
return new CommandObjects(connection.getRedisProtocol());
88+
return new CommandObjects(RedisProtocol.orServerDefault(connection.getRedisProtocol()));
8989
}
9090

9191
@Override

‎src/main/java/redis/clients/jedis/Transaction.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ public Transaction(Connection connection, boolean doMulti, boolean closeConnecti
8484
}
8585

8686
private static CommandObjects createCommandObjects(Connection connection) {
87-
return new CommandObjects(connection.getRedisProtocol());
87+
return new CommandObjects(RedisProtocol.orServerDefault(connection.getRedisProtocol()));
8888
}
8989

9090
@Override

‎src/main/java/redis/clients/jedis/UnifiedJedis.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ protected UnifiedJedis(ConnectionProvider provider, RedisProtocol protocol, Cach
7474
protected UnifiedJedis(Connection connection) {
7575
this.provider = null;
7676
this.executor = new SimpleCommandExecutor(connection);
77-
this.commandObjects = newCommandObjects(connection.getRedisProtocol());
77+
this.commandObjects = newCommandObjects(RedisProtocol.orServerDefault(connection.getRedisProtocol()));
7878

7979
if (connection instanceof CacheConnection) {
8080
this.cache = ((CacheConnection) connection).getCache();
@@ -124,7 +124,9 @@ private UnifiedJedis(CommandExecutor executor, ConnectionProvider provider,
124124

125125
// When the protocol is left unspecified, auto-negotiation may have resolved it to either
126126
// RESP2 or RESP3 on the wire. Probe a connection to learn the actual value so the command
127-
// objects parse replies with the correct decoder.
127+
// objects parse replies with the correct decoder. If the probe fails or the connection has
128+
// not yet negotiated, fall back to RESP2 — matching the wire default and preserving lazy
129+
// construction semantics.
128130
RedisProtocol resolvedProtocol = protocol;
129131
if (resolvedProtocol == null && provider != null) {
130132
try (Connection conn = provider.getConnection()) {
@@ -134,6 +136,9 @@ private UnifiedJedis(CommandExecutor executor, ConnectionProvider provider,
134136
} catch (JedisException ignored) {
135137
}
136138
}
139+
if (resolvedProtocol == null) {
140+
resolvedProtocol = RedisProtocol.RESP2;
141+
}
137142

138143
if (cache != null && resolvedProtocol != RedisProtocol.RESP3) {
139144
throw new IllegalArgumentException("Client-side caching is only supported with RESP3.");

‎src/test/java/redis/clients/jedis/commands/CommandsTestsParameters.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,14 @@ public static Collection<Object[]> jedisRespVersions() {
3636
new Object[] { RedisProtocol.RESP2 }, new Object[] { RedisProtocol.RESP3 });
3737
}
3838

39+
/**
40+
* RESP protocol versions for {@link redis.clients.jedis.CommandObjects} tests. Only the strict
41+
* (non-{@code null}) protocols are valid here — {@code CommandObjects} requires a concrete
42+
* protocol at construction time, as it is meant to be used after auto-negotiation has already
43+
* resolved a specific version.
44+
*/
45+
public static Collection<Object[]> commandObjectsRespVersions() {
46+
return Arrays.asList(new Object[] { RedisProtocol.RESP2 }, new Object[] { RedisProtocol.RESP3 });
47+
}
48+
3949
}

‎src/test/java/redis/clients/jedis/commands/commandobjects/CommandObjectsTestBase.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@
2727
* on if a Redis Stack server is needed, or a standalone suffices.
2828
* <p>
2929
* In principle all subclasses of this class should be parameterized tests,
30-
* to run with several versions of RESP. {@link CommandsTestsParameters#jedisRespVersions}
31-
* The strict (non-{@code null}) RESP versions are the right fit here: CommandObjects receive a
32-
* specific protocol after auto-negotiation has already happened.
30+
* to run with several versions of RESP. {@link CommandsTestsParameters#commandObjectsRespVersions}
31+
* provides the strict (non-{@code null}) RESP versions, which are the right fit here:
32+
* CommandObjects receive a specific protocol after auto-negotiation has already happened.
3333
*/
3434
@Tag("integration")
3535
@ParameterizedClass
36-
@MethodSource("redis.clients.jedis.commands.CommandsTestsParameters#jedisRespVersions")
36+
@MethodSource("redis.clients.jedis.commands.CommandsTestsParameters#commandObjectsRespVersions")
3737
public abstract class CommandObjectsTestBase {
3838

3939
/**

0 commit comments

Comments
 (0)