Author: phunt
Date: Tue Dec 17 16:55:29 2013
New Revision: 1551624
URL: http://svn.apache.org/r1551624
Log:
ZOOKEEPER-1388. Client side 'PathValidation' is missing for the multi-transaction api. (Rakesh
R via marshallm, phunt)
Modified:
zookeeper/trunk/CHANGES.txt
zookeeper/trunk/src/java/main/org/apache/zookeeper/CreateMode.java
zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java
zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
Modified: zookeeper/trunk/CHANGES.txt
URL: http://svn.apache.org/viewvc/zookeeper/trunk/CHANGES.txt?rev=1551624&r1=1551623&r2=1551624&view=diff
==============================================================================
--- zookeeper/trunk/CHANGES.txt (original)
+++ zookeeper/trunk/CHANGES.txt Tue Dec 17 16:55:29 2013
@@ -504,6 +504,9 @@ BUGFIXES:
ZOOKEEPER-1756. zookeeper_interest() in C client can return a timeval of 0
(Eric Lindvall via michim)
+ ZOOKEEPER-1388. Client side 'PathValidation' is missing for the
+ multi-transaction api. (Rakesh R via marshallm, phunt)
+
IMPROVEMENTS:
ZOOKEEPER-1170. Fix compiler (eclipse) warnings: unused imports,
Modified: zookeeper/trunk/src/java/main/org/apache/zookeeper/CreateMode.java
URL: http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/CreateMode.java?rev=1551624&r1=1551623&r2=1551624&view=diff
==============================================================================
--- zookeeper/trunk/src/java/main/org/apache/zookeeper/CreateMode.java (original)
+++ zookeeper/trunk/src/java/main/org/apache/zookeeper/CreateMode.java Tue Dec 17 16:55:29
2013
@@ -83,8 +83,10 @@ public enum CreateMode {
case 3: return CreateMode.EPHEMERAL_SEQUENTIAL ;
default:
- LOG.error("Received an invalid flag value to convert to a CreateMode");
- throw new KeeperException.BadArgumentsException();
+ String errMsg = "Received an invalid flag value: " + flag
+ + " to convert to a CreateMode";
+ LOG.error(errMsg);
+ throw new KeeperException.BadArgumentsException(errMsg);
}
}
}
Modified: zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java
URL: http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java?rev=1551624&r1=1551623&r2=1551624&view=diff
==============================================================================
--- zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java (original)
+++ zookeeper/trunk/src/java/main/org/apache/zookeeper/Op.java Tue Dec 17 16:55:29 2013
@@ -18,6 +18,7 @@
package org.apache.zookeeper;
import org.apache.jute.Record;
+import org.apache.zookeeper.common.PathUtils;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.proto.CheckVersionRequest;
import org.apache.zookeeper.proto.CreateRequest;
@@ -161,6 +162,18 @@ public abstract class Op {
*/
abstract Op withChroot(String addRootPrefix);
+ /**
+ * Performs client path validations.
+ *
+ * @throws IllegalArgumentException
+ * if an invalid path is specified
+ * @throws KeeperException.BadArgumentsException
+ * if an invalid create mode flag is specified
+ */
+ void validate() throws KeeperException {
+ PathUtils.validatePath(path);
+ }
+
//////////////////
// these internal classes are public, but should not generally be referenced.
//
@@ -221,6 +234,12 @@ public abstract class Op {
Op withChroot(String path) {
return new Create(path, data, acl, flags);
}
+
+ @Override
+ void validate() throws KeeperException {
+ CreateMode createMode = CreateMode.fromFlag(flags);
+ PathUtils.validatePath(getPath(), createMode.isSequential());
+ }
}
public static class Delete extends Op {
Modified: zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java
URL: http://svn.apache.org/viewvc/zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java?rev=1551624&r1=1551623&r2=1551624&view=diff
==============================================================================
--- zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java (original)
+++ zookeeper/trunk/src/java/main/org/apache/zookeeper/ZooKeeper.java Tue Dec 17 16:55:29
2013
@@ -1108,10 +1108,14 @@ public class ZooKeeper {
* partially succeeded if this exception is thrown.
* @throws KeeperException If the operation could not be completed
* due to some error in doing one of the specified ops.
+ * @throws IllegalArgumentException if an invalid path is specified
*
* @since 3.4.0
*/
public List<OpResult> multi(Iterable<Op> ops) throws InterruptedException,
KeeperException {
+ for (Op op : ops) {
+ op.validate();
+ }
return multiInternal(generateMultiTransaction(ops));
}
@@ -1121,9 +1125,45 @@ public class ZooKeeper {
* @see #multi(Iterable)
*/
public void multi(Iterable<Op> ops, MultiCallback cb, Object ctx) {
+ List<OpResult> results = validatePath(ops);
+ if (results.size() > 0) {
+ cb.processResult(KeeperException.Code.BADARGUMENTS.intValue(),
+ null, ctx, results);
+ return;
+ }
multiInternal(generateMultiTransaction(ops), cb, ctx);
}
+ private List<OpResult> validatePath(Iterable<Op> ops) {
+ List<OpResult> results = new ArrayList<OpResult>();
+ boolean error = false;
+ for (Op op : ops) {
+ try {
+ op.validate();
+ } catch (IllegalArgumentException iae) {
+ LOG.error("IllegalArgumentException: " + iae.getMessage());
+ ErrorResult err = new ErrorResult(
+ KeeperException.Code.BADARGUMENTS.intValue());
+ results.add(err);
+ error = true;
+ continue;
+ } catch (KeeperException ke) {
+ LOG.error("KeeperException: " + ke.getMessage());
+ ErrorResult err = new ErrorResult(ke.code().intValue());
+ results.add(err);
+ error = true;
+ continue;
+ }
+ ErrorResult err = new ErrorResult(
+ KeeperException.Code.RUNTIMEINCONSISTENCY.intValue());
+ results.add(err);
+ }
+ if (false == error) {
+ results.clear();
+ }
+ return results;
+ }
+
private MultiTransactionRecord generateMultiTransaction(Iterable<Op> ops) {
// reconstructing transaction with the chroot prefix
List<Op> transaction = new ArrayList<Op>();
Modified: zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java
URL: http://svn.apache.org/viewvc/zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java?rev=1551624&r1=1551623&r2=1551624&view=diff
==============================================================================
--- zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java (original)
+++ zookeeper/trunk/src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java Tue
Dec 17 16:55:29 2013
@@ -115,6 +115,50 @@ public class MultiTransactionTest extend
}
}
+ private void multiHavingErrors(ZooKeeper zk, Iterable<Op> ops,
+ List<Integer> expectedResultCodes, String expectedErr)
+ throws KeeperException, InterruptedException {
+ if (useAsync) {
+ final MultiResult res = new MultiResult();
+ zk.multi(ops, new MultiCallback() {
+ @Override
+ public void processResult(int rc, String path, Object ctx,
+ List<OpResult> opResults) {
+ synchronized (res) {
+ res.rc = rc;
+ res.results = opResults;
+ res.finished = true;
+ res.notifyAll();
+ }
+ }
+ }, null);
+ synchronized (res) {
+ while (!res.finished) {
+ res.wait();
+ }
+ }
+ for (int i = 0; i < res.results.size(); i++) {
+ OpResult opResult = res.results.get(i);
+ Assert.assertTrue("Did't recieve proper error response",
+ opResult instanceof ErrorResult);
+ ErrorResult errRes = (ErrorResult) opResult;
+ Assert.assertEquals("Did't recieve proper error code",
+ expectedResultCodes.get(i).intValue(), errRes.getErr());
+ }
+ } else {
+ try {
+ zk.multi(ops);
+ Assert.fail("Shouldn't have validated in ZooKeeper client!");
+ } catch (KeeperException e) {
+ Assert.assertEquals("Wrong exception", expectedErr, e.code()
+ .name());
+ } catch (IllegalArgumentException e) {
+ Assert.assertEquals("Wrong exception", expectedErr,
+ e.getMessage());
+ }
+ }
+ }
+
private List<OpResult> commit(Transaction txn)
throws KeeperException, InterruptedException {
if (useAsync) {
@@ -145,7 +189,101 @@ public class MultiTransactionTest extend
return txn.commit();
}
}
-
+
+ /**
+ * Test verifies the multi calls with invalid znode path
+ */
+ @Test(timeout = 90000)
+ public void testInvalidPath() throws Exception {
+ List<Integer> expectedResultCodes = new ArrayList<Integer>();
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+ expectedResultCodes.add(KeeperException.Code.BADARGUMENTS.intValue());
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+ // create with CreateMode
+ List<Op> opList = Arrays.asList(Op.create("/multi0", new byte[0],
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT), Op.create(
+ "/multi1/", new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT), Op.create("/multi2", new byte[0],
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT));
+ String expectedErr = "Path must not end with / character";
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+
+ // create with valid sequential flag
+ opList = Arrays.asList(Op.create("/multi0", new byte[0],
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT), Op.create(
+ "multi1/", new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.EPHEMERAL_SEQUENTIAL.toFlag()), Op.create("/multi2",
+ new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT));
+ expectedErr = "Path must start with / character";
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+
+ // check
+ opList = Arrays.asList(Op.check("/multi0", -1),
+ Op.check("/multi1/", 100), Op.check("/multi2", 5));
+ expectedErr = "Path must not end with / character";
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+
+ // delete
+ opList = Arrays.asList(Op.delete("/multi0", -1),
+ Op.delete("/multi1/", 100), Op.delete("/multi2", 5));
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+
+ // Multiple bad arguments
+ expectedResultCodes.add(KeeperException.Code.BADARGUMENTS.intValue());
+
+ // setdata
+ opList = Arrays.asList(Op.setData("/multi0", new byte[0], -1),
+ Op.setData("/multi1/", new byte[0], -1),
+ Op.setData("/multi2", new byte[0], -1),
+ Op.setData("multi3", new byte[0], -1));
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+ }
+
+ /**
+ * Test verifies the multi calls with blank znode path
+ */
+ @Test(timeout = 90000)
+ public void testBlankPath() throws Exception {
+ List<Integer> expectedResultCodes = new ArrayList<Integer>();
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+ expectedResultCodes.add(KeeperException.Code.BADARGUMENTS.intValue());
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+ expectedResultCodes.add(KeeperException.Code.BADARGUMENTS.intValue());
+
+ // delete
+ String expectedErr = "Path cannot be null";
+ List<Op> opList = Arrays.asList(Op.delete("/multi0", -1),
+ Op.delete(null, 100), Op.delete("/multi2", 5),
+ Op.delete("", -1));
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+ }
+
+ /**
+ * Test verifies the multi.create with invalid createModeFlag
+ */
+ @Test(timeout = 90000)
+ public void testInvalidCreateModeFlag() throws Exception {
+ List<Integer> expectedResultCodes = new ArrayList<Integer>();
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+ expectedResultCodes.add(KeeperException.Code.BADARGUMENTS.intValue());
+ expectedResultCodes.add(KeeperException.Code.RUNTIMEINCONSISTENCY
+ .intValue());
+
+ int createModeFlag = 6789;
+ List<Op> opList = Arrays.asList(Op.create("/multi0", new byte[0],
+ Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT), Op.create(
+ "/multi1", new byte[0], Ids.OPEN_ACL_UNSAFE, createModeFlag),
+ Op.create("/multi2", new byte[0], Ids.OPEN_ACL_UNSAFE,
+ CreateMode.PERSISTENT));
+ String expectedErr = KeeperException.Code.BADARGUMENTS.name();
+ multiHavingErrors(zk, opList, expectedResultCodes, expectedErr);
+ }
+
@Test
public void testChRootCreateDelete() throws Exception {
// creating the subtree for chRoot clients.
|