Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/main/java/redis/clients/jedis/BinaryJedis.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
import redis.clients.jedis.commands.MultiKeyBinaryCommands;
import redis.clients.jedis.commands.ProtocolCommand;
import redis.clients.jedis.exceptions.InvalidURIException;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.exceptions.JedisBatchOperationException;
import redis.clients.jedis.exceptions.JedisException;
import redis.clients.jedis.params.ClientKillParams;
import redis.clients.jedis.params.GeoRadiusParam;
Expand Down Expand Up @@ -2027,11 +2027,11 @@ public Transaction multi() {

protected void checkIsInMultiOrPipeline() {
if (client.isInMulti()) {
throw new JedisDataException(
throw new JedisBatchOperationException(
"Cannot use Jedis when in Multi. Please use Transaction or reset jedis state.");
} else if (pipeline != null && pipeline.hasPipelinedResponse()) {
throw new JedisDataException(
"Cannot use Jedis when in Pipeline. Please use Pipeline or reset jedis state .");
throw new JedisBatchOperationException(
"Cannot use Jedis when in Pipeline. Please use Pipeline or reset jedis state.");
}
}

Expand Down
14 changes: 7 additions & 7 deletions src/main/java/redis/clients/jedis/Pipeline.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.ArrayList;
import java.util.List;

import redis.clients.jedis.exceptions.JedisBatchOperationException;
import redis.clients.jedis.exceptions.JedisDataException;

public class Pipeline extends MultiKeyPipelineBase implements Closeable {
Expand All @@ -20,8 +21,8 @@ public List<Object> build(Object data) {
List<Object> values = new ArrayList<>();

if (list.size() != responses.size()) {
throw new JedisDataException("Expected data size " + responses.size() + " but was "
+ list.size());
throw new JedisBatchOperationException(
"Expected data size " + responses.size() + " but was " + list.size());
}

for (int i = 0; i < list.size(); i++) {
Expand Down Expand Up @@ -126,14 +127,14 @@ public List<Object> syncAndReturnAll() {
}

public Response<String> discard() {
if (currentMulti == null) throw new JedisDataException("DISCARD without MULTI");
if (currentMulti == null) throw new JedisBatchOperationException("DISCARD without MULTI");
client.discard();
currentMulti = null;
return getResponse(BuilderFactory.STRING);
}

public Response<List<Object>> exec() {
if (currentMulti == null) throw new JedisDataException("EXEC without MULTI");
if (currentMulti == null) throw new JedisBatchOperationException("EXEC without MULTI");

client.exec();
Response<List<Object>> response = super.getResponse(currentMulti);
Expand All @@ -143,11 +144,10 @@ public Response<List<Object>> exec() {
}

public Response<String> multi() {
if (currentMulti != null) throw new JedisDataException("MULTI calls can not be nested");
if (currentMulti != null) throw new JedisBatchOperationException("MULTI calls can not be nested");

client.multi();
Response<String> response = getResponse(BuilderFactory.STRING); // Expecting
// OK
Response<String> response = getResponse(BuilderFactory.STRING); // Expecting OK
currentMulti = new MultiResponseBuilder();
return response;
}
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/redis/clients/jedis/Response.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package redis.clients.jedis;

import redis.clients.jedis.exceptions.JedisBatchOperationException;
import redis.clients.jedis.exceptions.JedisDataException;

public class Response<T> {
Expand All @@ -24,13 +25,12 @@ public void set(Object data) {
}

public T get() {
// if response has dependency response and dependency is not built,
// build it first and no more!!
// if response has dependency response and dependency is not built, build it first and no more!!
if (dependency != null && dependency.set && !dependency.built) {
dependency.build();
}
if (!set) {
throw new JedisDataException(
throw new JedisBatchOperationException(
"Please close pipeline or multi block before calling this method.");
}
if (!built) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package redis.clients.jedis.exceptions;

public class JedisBatchOperationException extends JedisException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make it extend "JedisDataException" this way it won't make a backward issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkorland It was done deliberately.

According to marcosnils @marcosnils

I thought about this and I believe the best approach would be to change the type of the exception to JedisException instead of JedisDataException and to improve the logging message. ... Currently JedisDataException is used mainly when Redis returns an error about the operation. The pipeline/transaction validation we're doing it's something from the Jedis side, so it doesn't make sense to return this type of exception. ...

According to HeartSaVioR @HeartSaVioR

... I also think we shouldn't use JedisDataException which is not ERR from Redis side. ...

I agree with both of them. And that's why I crafted this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkorland Please check #2360

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree @gkorland advice, we should be compatible with JedisDataException, because maybe user code has catch JedisDataException.

So, the inheritance path is : JedisBatchOperationException extends JedisDataException extends JedisException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yangbodong22011 WDYT about #2360 ?

private static final long serialVersionUID = -1464241173812705493L;

public JedisBatchOperationException(String message) {
super(message);
}

}
3 changes: 1 addition & 2 deletions src/main/java/redis/clients/jedis/util/SafeEncoder.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import java.util.List;

import redis.clients.jedis.Protocol;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.exceptions.JedisException;

/**
Expand All @@ -27,7 +26,7 @@ public static byte[][] encodeMany(final String... strs) {
public static byte[] encode(final String str) {
try {
if (str == null) {
throw new JedisDataException("value sent to redis cannot be null");
throw new JedisException("null value cannot be sent to redis");
}
return str.getBytes(Protocol.CHARSET);
} catch (UnsupportedEncodingException e) {
Expand Down
5 changes: 2 additions & 3 deletions src/test/java/redis/clients/jedis/tests/JedisTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import redis.clients.jedis.Protocol;
import redis.clients.jedis.exceptions.InvalidURIException;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.exceptions.JedisException;
import redis.clients.jedis.tests.commands.JedisCommandTestBase;
import redis.clients.jedis.util.SafeEncoder;
Expand Down Expand Up @@ -114,7 +113,7 @@ public void infiniteTimeout() throws Exception {
jedis.close();
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisException.class)
public void failWhenSendingNullValues() {
jedis.set("foo", null);
}
Expand Down Expand Up @@ -204,4 +203,4 @@ public void checkDisconnectOnQuit() {
assertFalse(jedis.getClient().isConnected());
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,4 @@ public void allowUrlWithNoDBAndNoPassword() {
}
}

}
}
19 changes: 10 additions & 9 deletions src/test/java/redis/clients/jedis/tests/PipeliningTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import redis.clients.jedis.Pipeline;
import redis.clients.jedis.Response;
import redis.clients.jedis.Tuple;
import redis.clients.jedis.exceptions.JedisBatchOperationException;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.tests.commands.JedisCommandTestBase;
import redis.clients.jedis.util.SafeEncoder;
Expand Down Expand Up @@ -183,7 +184,7 @@ public void pipelineResponseWithoutData() {
assertNull(score.get());
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisBatchOperationException.class)
public void pipelineResponseWithinPipeline() {
jedis.set("string", "foo");

Expand Down Expand Up @@ -351,27 +352,27 @@ public void multiUnwatch() {
assertEquals(expect, pipe.syncAndReturnAll());
}

@Test(expected = JedisDataException.class)
public void pipelineExecShoudThrowJedisDataExceptionWhenNotInMulti() {
@Test(expected = JedisBatchOperationException.class)
public void pipelineExecShoudThrowBatchExceptionWhenNotInMulti() {
Pipeline pipeline = jedis.pipelined();
pipeline.exec();
}

@Test(expected = JedisDataException.class)
public void pipelineDiscardShoudThrowJedisDataExceptionWhenNotInMulti() {
@Test(expected = JedisBatchOperationException.class)
public void pipelineDiscardShoudThrowBatchExceptionWhenNotInMulti() {
Pipeline pipeline = jedis.pipelined();
pipeline.discard();
}

@Test(expected = JedisDataException.class)
public void pipelineMultiShoudThrowJedisDataExceptionWhenAlreadyInMulti() {
@Test(expected = JedisBatchOperationException.class)
public void pipelineMultiShoudThrowBatchExceptionWhenAlreadyInMulti() {
Pipeline pipeline = jedis.pipelined();
pipeline.multi();
pipeline.set("foo", "3");
pipeline.multi();
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisBatchOperationException.class)
public void testJedisThowExceptionWhenInPipeline() {
Pipeline pipeline = jedis.pipelined();
pipeline.set("foo", "3");
Expand Down Expand Up @@ -680,7 +681,7 @@ public void testCloseableWithMulti() throws IOException {
try {
pipeline.exec();
fail("close should discard transaction");
} catch (JedisDataException e) {
} catch (JedisBatchOperationException e) {
assertTrue(e.getMessage().contains("EXEC without MULTI"));
// pass
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import redis.clients.jedis.ShardedJedis;
import redis.clients.jedis.ShardedJedisPipeline;
import redis.clients.jedis.Tuple;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.exceptions.JedisBatchOperationException;

public class ShardedJedisPipelineTest {

Expand Down Expand Up @@ -109,7 +109,7 @@ public void pipelineResponse() {
assertEquals(1, zrangeWithScores.get().size());
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisBatchOperationException.class)
public void pipelineResponseWithinPipeline() {
jedis.set("string", "foo");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import redis.clients.jedis.Protocol.Keyword;
import redis.clients.jedis.Response;
import redis.clients.jedis.Transaction;
import redis.clients.jedis.exceptions.JedisBatchOperationException;
import redis.clients.jedis.exceptions.JedisDataException;
import redis.clients.jedis.util.SafeEncoder;

Expand Down Expand Up @@ -156,7 +157,7 @@ public void unwatch() {
assertEquals("OK", resp.get(0));
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisBatchOperationException.class)
public void validateWhenInMulti() {
jedis.multi();
jedis.ping();
Expand Down Expand Up @@ -215,7 +216,7 @@ public void transactionResponseBinary() {
assertArrayEquals("foo".getBytes(), set.get());
}

@Test(expected = JedisDataException.class)
@Test(expected = JedisBatchOperationException.class)
public void transactionResponseWithinPipeline() {
jedis.set("string", "foo");

Expand Down