ZOOKEEPER-1388. Client side 'PathValidation' is missing for the multi…

…-transaction api. (Rakesh R via marshallm, phunt)

git-svn-id: https://svn.apache.org/repos/asf/zookeeper/trunk@1551624 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -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,

src/java/main/org/apache/zookeeper/CreateMode.java

@@ -83,8 +83,10 @@ static public CreateMode fromFlag(int flag) throws KeeperException {
         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);
         }
     }
 }

src/java/main/org/apache/zookeeper/Op.java

@@ -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 String getPath() {
      */
     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 Record toRequestRecord() {
         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 {

src/java/main/org/apache/zookeeper/ZooKeeper.java

@@ -1108,10 +1108,14 @@ public void delete(final String path, int version)
      * 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 List<OpResult> multi(Iterable<Op> ops) throws InterruptedException, Keepe
      * @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>();

src/java/test/org/apache/zookeeper/test/MultiTransactionTest.java

@@ -115,6 +115,50 @@ public void processResult(int rc, String path, Object ctx,
         }
     }
 
+    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 void processResult(int rc, String path, Object ctx,
             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.