From f253d4128d7dcf64fb70a6c1b0ed295b2bfca324 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Thu, 6 Dec 2018 13:07:34 +0100 Subject: [PATCH] DATAREDIS-749 - Polishing. Upgrade to Jedis 3.0. Remove reflective sendCommand calls in favor of public Jedis sendCommand API. Invocations with null arguments by introducing overloads with argument defaulting. Fix exception translation because of a changed exception hierarchy. Fix tests and adapt tests to new transactional results that return null values instead if empty collections for nested array responses. Reformat code. Add author tags. Add note to reference docs. Original pull request: #374. --- pom.xml | 2 +- src/main/asciidoc/new-features.adoc | 1 + .../connection/jedis/JedisClientUtils.java | 36 +------ .../jedis/JedisClusterServerCommands.java | 13 +-- .../jedis/JedisClusterStringCommands.java | 8 +- .../jedis/JedisClusterZSetCommands.java | 7 +- .../connection/jedis/JedisConnection.java | 2 +- .../connection/jedis/JedisConverters.java | 102 +++++++++++++----- .../jedis/JedisExceptionConverter.java | 37 +++---- .../connection/jedis/JedisKeyCommands.java | 4 +- .../connection/jedis/JedisServerCommands.java | 11 +- .../connection/jedis/JedisStringCommands.java | 2 +- .../connection/jedis/JedisZSetCommands.java | 10 +- .../AbstractConnectionIntegrationTests.java | 18 ++-- .../jedis/JedisConvertersUnitTests.java | 44 ++++---- ...> TransactionalJedisIntegrationTests.java} | 5 +- 16 files changed, 154 insertions(+), 148 deletions(-) rename src/test/java/org/springframework/data/redis/connection/jedis/{TransactionalJedisItegrationTests.java => TransactionalJedisIntegrationTests.java} (83%) diff --git a/pom.xml b/pom.xml index c202db66e..594d0d7fd 100644 --- a/pom.xml +++ b/pom.xml @@ -22,7 +22,7 @@ 1.4.8 2.6.0 5.1.3.RELEASE - 3.0.0-m1 + 3.0.0 1.01 4.1.22.Final spring.data.redis diff --git a/src/main/asciidoc/new-features.adoc b/src/main/asciidoc/new-features.adoc index 52c16b1f0..6cd6d460d 100644 --- a/src/main/asciidoc/new-features.adoc +++ b/src/main/asciidoc/new-features.adoc @@ -8,6 +8,7 @@ This section briefly covers items that are new and noteworthy in the latest rele * <> * Refined `union`/`diff`/`intersect` set-operation methods accepting a single collection of keys. +* Upgrade to Jedis 3. [[new-in-2.1.0]] == New in Spring Data Redis 2.1 diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java index 12d3d1ba5..aac0f92bd 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java @@ -18,7 +18,6 @@ package org.springframework.data.redis.connection.jedis; import redis.clients.jedis.BinaryJedis; import redis.clients.jedis.Builder; import redis.clients.jedis.Client; -import redis.clients.jedis.Connection; import redis.clients.jedis.Jedis; import redis.clients.jedis.Protocol; import redis.clients.jedis.Protocol.Command; @@ -35,8 +34,6 @@ import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.springframework.beans.DirectFieldAccessor; -import org.springframework.util.ClassUtils; import org.springframework.util.ReflectionUtils; /** @@ -50,7 +47,6 @@ import org.springframework.util.ReflectionUtils; class JedisClientUtils { private static final Field CLIENT_FIELD; - private static final Method SEND_COMMAND; private static final Method GET_RESPONSE; private static final Method PROTOCOL_SEND_COMMAND; private static final Set KNOWN_COMMANDS; @@ -65,20 +61,6 @@ class JedisClientUtils { byte[].class, byte[][].class); ReflectionUtils.makeAccessible(PROTOCOL_SEND_COMMAND); - try { - - Class commandType = ClassUtils.isPresent("redis.clients.jedis.ProtocolCommand", null) - ? ClassUtils.forName("redis.clients.jedis.ProtocolCommand", null) - : ClassUtils.forName("redis.clients.jedis.Protocol$Command", null); - - SEND_COMMAND = ReflectionUtils.findMethod(Connection.class, "sendCommand", commandType, byte[][].class); - } catch (Exception e) { - throw new NoClassDefFoundError( - "Could not find required flavor of command required by 'redis.clients.jedis.Connection#sendCommand'."); - } - - ReflectionUtils.makeAccessible(SEND_COMMAND); - GET_RESPONSE = ReflectionUtils.findMethod(Queable.class, "getResponse", Builder.class); ReflectionUtils.makeAccessible(GET_RESPONSE); @@ -149,26 +131,12 @@ class JedisClientUtils { private static void sendCommand(Client client, String command, byte[][] args) { if (isKnownCommand(command)) { - ReflectionUtils.invokeMethod(SEND_COMMAND, client, Command.valueOf(command.trim().toUpperCase()), args); + client.sendCommand(Command.valueOf(command.trim().toUpperCase()), args); } else { - sendProtocolCommand(client, command, args); + client.sendCommand(() -> SafeEncoder.encode(command.trim().toUpperCase()), args); } } - private static void sendProtocolCommand(Client client, String command, byte[][] args) { - - // quite expensive to construct for each command invocation - DirectFieldAccessor dfa = new DirectFieldAccessor(client); - - client.connect(); - - RedisOutputStream os = (RedisOutputStream) dfa.getPropertyValue("outputStream"); - ReflectionUtils.invokeMethod(PROTOCOL_SEND_COMMAND, null, os, SafeEncoder.encode(command), args); - - Integer pipelinedCommands = (Integer) dfa.getPropertyValue("pipelinedCommands"); - dfa.setPropertyValue("pipelinedCommands", pipelinedCommands + 1); - } - private static boolean isKnownCommand(String command) { return KNOWN_COMMANDS.contains(command); } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java index 898d972f1..fe60edd7d 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterServerCommands.java @@ -335,8 +335,10 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { Assert.notNull(pattern, "Pattern must not be null!"); - return connection.getClusterCommandExecutor().executeCommandOnSingleNode( - (JedisClusterCommandCallback) client -> Converters.toProperties(client.configGet(pattern)), node) + return connection.getClusterCommandExecutor() + .executeCommandOnSingleNode( + (JedisClusterCommandCallback) client -> Converters.toProperties(client.configGet(pattern)), + node) .getValue(); } @@ -502,8 +504,7 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { * @see org.springframework.data.redis.connection.RedisServerCommands#migrate(byte[], org.springframework.data.redis.connection.RedisNode, int, org.springframework.data.redis.connection.RedisServerCommands.MigrateOption, long) */ @Override - public void migrate(byte[] key, RedisNode target, int dbIndex, @Nullable MigrateOption option, - long timeout) { + public void migrate(byte[] key, RedisNode target, int dbIndex, @Nullable MigrateOption option, long timeout) { Assert.notNull(key, "Key must not be null!"); Assert.notNull(target, "Target node must not be null!"); @@ -511,8 +512,8 @@ class JedisClusterServerCommands implements RedisClusterServerCommands { RedisClusterNode node = connection.getTopologyProvider().getTopology().lookup(target.getHost(), target.getPort()); - executeCommandOnSingleNode(client -> client.migrate(target.getHost(), target.getPort(), - key, dbIndex, timeoutToUse), node); + executeCommandOnSingleNode(client -> client.migrate(target.getHost(), target.getPort(), key, dbIndex, timeoutToUse), + node); } private Long convertListOfStringToTime(List serverTimeInformation) { diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java index ba19d3fe3..fba3f39b2 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterStringCommands.java @@ -158,12 +158,11 @@ class JedisClusterStringCommands implements RedisStringCommands { } } else { - SetParams params = JedisConverters.toSetCommandNxXxArgument(option, null); - JedisConverters.toSetCommandExPxArgument(expiration, params); + SetParams params = JedisConverters.toSetCommandNxXxArgument(option); + JedisConverters.toSetCommandExPxArgument(expiration, params); try { - return Converters - .stringToBoolean(connection.getCluster().set(key, value, params)); + return Converters.stringToBoolean(connection.getCluster().set(key, value, params)); } catch (Exception ex) { throw convertJedisAccessException(ex); } @@ -491,7 +490,6 @@ class JedisClusterStringCommands implements RedisStringCommands { } } - /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisStringCommands#bitOp(org.springframework.data.redis.connection.RedisStringCommands.BitOperation, byte[], byte[][]) diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterZSetCommands.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterZSetCommands.java index 4645d7650..ad9918753 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterZSetCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterZSetCommands.java @@ -642,8 +642,7 @@ class JedisClusterZSetCommands implements RedisZSetCommands { if (ClusterSlotHashUtil.isSameSlotForAllKeys(allKeys)) { - ZParams zparams = new ZParams().weights(weights.toArray()) - .aggregate(ZParams.Aggregate.valueOf(aggregate.name())); + ZParams zparams = new ZParams().weights(weights.toArray()).aggregate(ZParams.Aggregate.valueOf(aggregate.name())); try { return connection.getCluster().zunionstore(destKey, zparams, sets); @@ -680,7 +679,6 @@ class JedisClusterZSetCommands implements RedisZSetCommands { throw new InvalidDataAccessApiUsageException("ZINTERSTORE can only be executed when all keys map to the same slot"); } - /* * (non-Javadoc) * @see org.springframework.data.redis.connection.RedisZSetCommands#zInterStore(byte[], org.springframework.data.redis.connection.RedisZSetCommands.Aggregate, org.springframework.data.redis.connection.RedisZSetCommands.Weights, byte[][]) @@ -698,8 +696,7 @@ class JedisClusterZSetCommands implements RedisZSetCommands { if (ClusterSlotHashUtil.isSameSlotForAllKeys(allKeys)) { - ZParams zparams = new ZParams().weights(weights.toArray()) - .aggregate(ZParams.Aggregate.valueOf(aggregate.name())); + ZParams zparams = new ZParams().weights(weights.toArray()).aggregate(ZParams.Aggregate.valueOf(aggregate.name())); try { return connection.getCluster().zinterstore(destKey, zparams, sets); diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index 68949a754..7b97f5e2d 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -38,7 +38,6 @@ import org.springframework.dao.DataAccessException; import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.redis.ExceptionTranslationStrategy; import org.springframework.data.redis.FallbackExceptionTranslationStrategy; -import org.springframework.data.redis.RedisConnectionFailureException; import org.springframework.data.redis.RedisSystemException; import org.springframework.data.redis.connection.*; import org.springframework.data.redis.connection.convert.TransactionResultConverter; @@ -62,6 +61,7 @@ import org.springframework.util.StringUtils; * @author Milan Agatonovic * @author Mark Paluch * @author Ninad Divadkar + * @author Guy Korland */ public class JedisConnection extends AbstractRedisConnection { diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java index 50cbd5970..1966c7465 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConverters.java @@ -88,6 +88,7 @@ import org.springframework.util.StringUtils; * @author Jungtaek Lim * @author Mark Paluch * @author Ninad Divadkar + * @author Guy Korland */ abstract public class JedisConverters extends Converters { @@ -196,7 +197,8 @@ abstract public class JedisConverters extends Converters { }; GEO_COORDINATE_TO_POINT_CONVERTER = geoCoordinate -> geoCoordinate != null - ? new Point(geoCoordinate.getLongitude(), geoCoordinate.getLatitude()) : null; + ? new Point(geoCoordinate.getLongitude(), geoCoordinate.getLatitude()) + : null; LIST_GEO_COORDINATE_TO_POINT_CONVERTER = new ListConverter<>(GEO_COORDINATE_TO_POINT_CONVERTER); BYTES_LIST_TO_LONG_LIST_CONVERTER = new ListConverter(new Converter() { @@ -220,8 +222,7 @@ abstract public class JedisConverters extends Converters { if (command instanceof BitFieldIncrBy) { - BitFieldIncrBy.Overflow overflow = ((BitFieldIncrBy) command) - .getOverflow(); + BitFieldIncrBy.Overflow overflow = ((BitFieldIncrBy) command).getOverflow(); if (overflow != null) { args.add(JedisConverters.toBytes("OVERFLOW")); args.add(JedisConverters.toBytes(overflow.name())); @@ -498,51 +499,94 @@ abstract public class JedisConverters extends Converters { } /** - * Converts a given {@link Expiration} to the according {@code SET} command argument.
+ * Converts a given {@link Expiration} to the according {@code SET} command argument. *
- *
{@link TimeUnit#SECONDS}
- *
{@code EX}
*
{@link TimeUnit#MILLISECONDS}
*
{@code PX}
+ *
{@link TimeUnit#SECONDS}
+ *
{@code EX}
*
* - * @param expiration + * @param expiration must not be {@literal null}. * @return - * @since 1.7 + * @since 2.2 + */ + public static SetParams toSetCommandExPxArgument(Expiration expiration) { + return toSetCommandExPxArgument(expiration, SetParams.setParams()); + } + + /** + * Converts a given {@link Expiration} to the according {@code SET} command argument. + *
+ *
{@link TimeUnit#MILLISECONDS}
+ *
{@code PX}
+ *
{@link TimeUnit#SECONDS}
+ *
{@code EX}
+ *
+ * + * @param expiration must not be {@literal null}. + * @param params + * @return + * @since 2.2 */ public static SetParams toSetCommandExPxArgument(Expiration expiration, SetParams params) { - params = params==null? SetParams.setParams() : params; - if(expiration.getTimeUnit() == TimeUnit.MILLISECONDS) { - return params.px(expiration.getExpirationTime()); - } - return params.ex((int)expiration.getExpirationTime()); - } + + SetParams paramsToUse = params == null ? SetParams.setParams() : params; + + if (expiration.getTimeUnit() == TimeUnit.MILLISECONDS) { + return paramsToUse.px(expiration.getExpirationTime()); + } + + return paramsToUse.ex((int) expiration.getExpirationTime()); + } /** * Converts a given {@link SetOption} to the according {@code SET} command argument.
*
- *
{@link SetOption#UPSERT}
- *
{@code byte[0]}
- *
{@link SetOption#SET_IF_ABSENT}
- *
{@code NX}
*
{@link SetOption#SET_IF_PRESENT}
*
{@code XX}
+ *
{@link SetOption#SET_IF_ABSENT}
+ *
{@code NX}
+ *
{@link SetOption#UPSERT}
+ *
{@code byte[0]}
*
* - * @param option + * @param option must not be {@literal null}. * @return - * @since 1.7 + * @since 2.2 + */ + public static SetParams toSetCommandNxXxArgument(SetOption option) { + return toSetCommandNxXxArgument(option, SetParams.setParams()); + } + + /** + * Converts a given {@link SetOption} to the according {@code SET} command argument.
+ *
+ *
{@link SetOption#SET_IF_PRESENT}
+ *
{@code XX}
+ *
{@link SetOption#SET_IF_ABSENT}
+ *
{@code NX}
+ *
{@link SetOption#UPSERT}
+ *
{@code byte[0]}
+ *
+ * + * @param option must not be {@literal null}. + * @param params + * @return + * @since 2.2 */ public static SetParams toSetCommandNxXxArgument(SetOption option, SetParams params) { - params = params==null? SetParams.setParams() : params; - switch(option) { - case SET_IF_PRESENT: - return params.xx(); - case SET_IF_ABSENT: - return params.nx(); - default: - return params; - } + + SetParams paramsToUse = params == null ? SetParams.setParams() : params; + + switch (option) { + case SET_IF_PRESENT: + return paramsToUse.xx(); + case SET_IF_ABSENT: + return paramsToUse.nx(); + default: + return paramsToUse; + } } private static byte[] boundaryToBytes(Boundary boundary, byte[] inclPrefix, byte[] exclPrefix) { diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisExceptionConverter.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisExceptionConverter.java index 43b286814..9ecd3f1fa 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisExceptionConverter.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisExceptionConverter.java @@ -15,6 +15,11 @@ */ package org.springframework.data.redis.connection.jedis; +import redis.clients.jedis.exceptions.JedisClusterMaxAttemptsException; +import redis.clients.jedis.exceptions.JedisConnectionException; +import redis.clients.jedis.exceptions.JedisException; +import redis.clients.jedis.exceptions.JedisRedirectionException; + import java.io.IOException; import java.net.UnknownHostException; @@ -25,18 +30,14 @@ import org.springframework.data.redis.ClusterRedirectException; import org.springframework.data.redis.RedisConnectionFailureException; import org.springframework.data.redis.TooManyClusterRedirectionsException; -import redis.clients.jedis.exceptions.JedisClusterMaxAttemptsException; -import redis.clients.jedis.exceptions.JedisConnectionException; -import redis.clients.jedis.exceptions.JedisDataException; -import redis.clients.jedis.exceptions.JedisException; -import redis.clients.jedis.exceptions.JedisRedirectionException; - /** * Converts Exceptions thrown from Jedis to {@link DataAccessException}s * * @author Jennifer Hickey * @author Thomas Darimont * @author Christoph Strobl + * @author Guy Korland + * @author Mark Paluch */ public class JedisExceptionConverter implements Converter { @@ -49,29 +50,29 @@ public class JedisExceptionConverter implements Converter values) { @@ -241,6 +233,22 @@ public class JedisConvertersUnitTests { } } + private static String toString(SetParams setParams) { + + StringBuilder builder = new StringBuilder(); + + for (byte[] parameter : setParams.getByteParams()) { + + if (builder.length() != 0) { + builder.append(' '); + } + + builder.append(new String(parameter)); + } + + return builder.toString(); + } + private Map getRedisServerInfoMap(String name, int port) { Map map = new HashMap<>(); map.put("name", name); diff --git a/src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisItegrationTests.java b/src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisIntegrationTests.java similarity index 83% rename from src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisItegrationTests.java rename to src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisIntegrationTests.java index 16f248e90..e0c66a25e 100644 --- a/src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisItegrationTests.java +++ b/src/test/java/org/springframework/data/redis/connection/jedis/TransactionalJedisIntegrationTests.java @@ -19,15 +19,14 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.data.redis.SettingsUtils; import org.springframework.data.redis.connection.AbstractTransactionalTestBase; -import org.springframework.data.redis.connection.jedis.TransactionalJedisItegrationTests.JedisContextConfiguration; import org.springframework.test.context.ContextConfiguration; /** * @author Christoph Strobl * @author Mark Paluch */ -@ContextConfiguration(classes = { JedisContextConfiguration.class }) -public class TransactionalJedisItegrationTests extends AbstractTransactionalTestBase { +@ContextConfiguration +public class TransactionalJedisIntegrationTests extends AbstractTransactionalTestBase { @Configuration public static class JedisContextConfiguration extends RedisContextConfiguration {