zookeeper-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ph...@apache.org
Subject svn commit: r1551624 - in /zookeeper/trunk: ./ src/java/main/org/apache/zookeeper/ src/java/test/org/apache/zookeeper/test/
Date Tue, 17 Dec 2013 16:55:29 GMT
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.



Mime
View raw message