ZOOKEEPER-355. make validatePath non public in Zookeeper client api. (phunt via mahadev)

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

CHANGES.txt

@@ -45,6 +45,9 @@ BUGFIXES:
 
   ZOOKEEPER-347. zkfuse uses non-standard String. (patrick hunt via mahadev)
 
+  ZOOKEEPER-355. make validatePath non public in Zookeeper client api. (phunt
+via mahadev)
+
 IMPROVEMENTS:
   ZOOKEEPER-308. improve the atomic broadcast performance 3x.
   (breed via mahadev)

src/c/src/zookeeper.c

@@ -1902,7 +1902,7 @@ finish:
     return rc;
 }
 
-static int isValidPath(const char* path) {
+static int isValidPath(const char* path, const int flags) {
   if (path == 0)
     return 0;
   const int len = strlen(path);
@@ -1912,7 +1912,7 @@ static int isValidPath(const char* path) {
     return 0;
   if (len == 1) // done checking - it's the root
     return 1;
-  if (path[len - 1] == '/')
+  if (path[len - 1] == '/' && !(flags & ZOO_SEQUENCE))
     return 0;
 
   char lastc = '/';
@@ -1926,11 +1926,13 @@ static int isValidPath(const char* path) {
     } else if (c == '/' && lastc == '/') {
       return 0;
     } else if (c == '.' && lastc == '.') {
-      if (path[i-2] == '/' && ((i + 1 == len) || path[i+1] == '/')) {
+      if (path[i-2] == '/' && (((i + 1 == len) && !(flags & ZOO_SEQUENCE))
+                               || path[i+1] == '/')) {
         return 0;
       }
     } else if (c == '.') {
-      if ((path[i-1] == '/') && ((i + 1 == len) || path[i+1] == '/')) {
+      if ((path[i-1] == '/') && (((i + 1 == len) && !(flags & ZOO_SEQUENCE))
+                                 || path[i+1] == '/')) {
         return 0;
       }
     } else if (c > 0x00 && c < 0x1f) {
@@ -1956,7 +1958,7 @@ int zoo_awget(zhandle_t *zh, const char *path,
     struct GetDataRequest req = { (char*)path, watcher!=0 };
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -1988,7 +1990,7 @@ int zoo_aset(zhandle_t *zh, const char *path, const char *buffer, int buflen,
     struct SetDataRequest req;
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2015,7 +2017,7 @@ int zoo_aset(zhandle_t *zh, const char *path, const char *buffer, int buflen,
 }
 
 int zoo_acreate(zhandle_t *zh, const char *path, const char *value,
-        int valuelen, const struct ACL_vector *acl_entries, int ephemeral,
+        int valuelen, const struct ACL_vector *acl_entries, int flags,
         string_completion_t completion, const void *data)
 {
     struct oarchive *oa;
@@ -2023,13 +2025,13 @@ int zoo_acreate(zhandle_t *zh, const char *path, const char *value,
     struct CreateRequest req;
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, flags))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
     oa = create_buffer_oarchive();
     req.path = (char*)path;
-    req.flags = ephemeral;
+    req.flags = flags;
     req.data.buff = (char*)value;
     req.data.len = valuelen;
     if (acl_entries == 0) {
@@ -2063,7 +2065,7 @@ int zoo_adelete(zhandle_t *zh, const char *path, int version,
     struct DeleteRequest req;
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2102,7 +2104,7 @@ int zoo_awexists(zhandle_t *zh, const char *path,
     struct ExistsRequest req = {(char*)path, watcher!=0 };
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2141,7 +2143,7 @@ int zoo_awget_children(zhandle_t *zh, const char *path,
     struct GetChildrenRequest req={(char*)path, watcher!=0 };
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2172,7 +2174,7 @@ int zoo_async(zhandle_t *zh, const char *path,
     struct SyncRequest req;
     int rc;
 
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2204,7 +2206,7 @@ int zoo_aget_acl(zhandle_t *zh, const char *path, acl_completion_t completion,
     struct GetACLRequest req;
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;
@@ -2235,7 +2237,7 @@ int zoo_aset_acl(zhandle_t *zh, const char *path, int version,
     struct SetACLRequest req;
     int rc;
     
-    if (zh==0 || !isValidPath(path))
+    if (zh==0 || !isValidPath(path, 0))
         return ZBADARGUMENTS;
     if (is_unrecoverable(zh))
         return ZINVALIDSTATE;

src/c/tests/TestClient.cc

@@ -296,6 +296,16 @@ public:
           path, "", 0, &ZOO_OPEN_ACL_UNSAFE, 0, 0, 0));
     }
     
+    static void verifyCreateFailsSeq(const char *path, zhandle_t *zk) {
+      CPPUNIT_ASSERT_EQUAL((int)ZBADARGUMENTS, zoo_create(zk,
+          path, "", 0, &ZOO_OPEN_ACL_UNSAFE, ZOO_SEQUENCE, 0, 0));
+    }
+    
+    static void verifyCreateOkSeq(const char *path, zhandle_t *zk) {
+      CPPUNIT_ASSERT_EQUAL((int)ZOK, zoo_create(zk,
+          path, "", 0, &ZOO_OPEN_ACL_UNSAFE, ZOO_SEQUENCE, 0, 0));
+    }
+    
             
     /**
        returns false if the vectors dont match
@@ -370,6 +380,16 @@ public:
         verifyCreateFails("foo", zk);
         verifyCreateFails("a", zk);
 
+        // verify that trailing fails, except for seq which adds suffix
+        verifyCreateOk("/createseq", zk);
+        verifyCreateFails("/createseq/", zk);
+        verifyCreateOkSeq("/createseq/", zk);
+        verifyCreateOkSeq("/createseq/.", zk);
+        verifyCreateOkSeq("/createseq/..", zk);
+        verifyCreateFailsSeq("/createseq//", zk);
+        verifyCreateFailsSeq("/createseq/./", zk);
+        verifyCreateFailsSeq("/createseq/../", zk);
+
         verifyCreateOk("/.foo", zk);
         verifyCreateOk("/.f.", zk);
         verifyCreateOk("/..f", zk);

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

@@ -33,6 +33,7 @@ import org.apache.zookeeper.AsyncCallback.DataCallback;
 import org.apache.zookeeper.AsyncCallback.StatCallback;
 import org.apache.zookeeper.AsyncCallback.StringCallback;
 import org.apache.zookeeper.AsyncCallback.VoidCallback;
+import org.apache.zookeeper.common.PathUtils;
 import org.apache.zookeeper.data.ACL;
 import org.apache.zookeeper.data.Stat;
 import org.apache.zookeeper.proto.CreateRequest;
@@ -127,7 +128,7 @@ public class ZooKeeper {
      * the public methods will not be exposed as part of the ZooKeeper client
      * API.
      */
-    private class ZKWatchManager implements ClientWatchManager {
+    private static class ZKWatchManager implements ClientWatchManager {
         private final Map<String, Set<Watcher>> dataWatches =
             new HashMap<String, Set<Watcher>>();
         private final Map<String, Set<Watcher>> existWatches =
@@ -500,7 +501,8 @@ public class ZooKeeper {
             CreateMode createMode)
         throws KeeperException, InterruptedException
     {
-        validatePath(path);
+        // also handle the case where server will append name suffix
+        PathUtils.validatePath(path, createMode.isSequential());
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.create);
@@ -521,72 +523,6 @@ public class ZooKeeper {
         return response.getPath();
     }
 
-    /**
-     * Validate the provided znode path string
-     * @param path znode path string
-     * @throws IllegalArgumentException if the path is invalid
-     */
-    public static void validatePath(String path) throws IllegalArgumentException {
-        if (path == null) {
-            throw new IllegalArgumentException("Path cannot be null");
-        }
-        if (path.length() == 0) {
-            throw new IllegalArgumentException("Path length must be > 0");
-        }
-        if (path.charAt(0) != '/') {
-            throw new IllegalArgumentException(
-                         "Path must start with / character");
-        }
-        if (path.length() == 1) { // done checking - it's the root
-            return;
-        }
-        if (path.charAt(path.length() - 1) == '/') {
-            throw new IllegalArgumentException(
-                         "Path must not end with / character");
-        }
-
-        String reason = null;
-        char lastc = '/';
-        char chars[] = path.toCharArray();
-        char c;
-        for (int i = 1; i < chars.length; lastc = chars[i], i++) {
-            c = chars[i];
-
-            if (c == 0) {
-                reason = "null character not allowed @" + i;
-                break;
-            } else if (c == '/' && lastc == '/') {
-                reason = "empty node name specified @" + i;
-                break;
-            } else if (c == '.' && lastc == '.') {
-                if (chars[i-2] == '/' &&
-                        ((i + 1 == chars.length)
-                                || chars[i+1] == '/')) {
-                    reason = "relative paths not allowed @" + i;
-                    break;
-                }
-            } else if (c == '.') {
-                if (chars[i-1] == '/' &&
-                        ((i + 1 == chars.length)
-                                || chars[i+1] == '/')) {
-                    reason = "relative paths not allowed @" + i;
-                    break;
-                }
-            } else if (c > '\u0000' && c < '\u001f'
-                    || c > '\u007f' && c < '\u009F'
-                    || c > '\ud800' && c < '\uf8ff'
-                    || c > '\ufff0' && c < '\uffff') {
-                reason = "invalid charater @" + i;
-                break;
-            }
-        }
-
-        if (reason != null) {
-            throw new IllegalArgumentException(
-                    "Invalid path string \"" + path + "\" caused by " + reason);
-        }
-    }
-
     /**
      * The Asynchronous version of create. The request doesn't actually until
      * the asynchronous callback is called.
@@ -597,7 +533,8 @@ public class ZooKeeper {
     public void create(String path, byte data[], List<ACL> acl,
             CreateMode createMode,  StringCallback cb, Object ctx)
     {
-        validatePath(path);
+        // also handle the case where server will append name suffix
+        PathUtils.validatePath(path, createMode.isSequential());
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.create);
@@ -639,7 +576,7 @@ public class ZooKeeper {
      */
     public void delete(String path, int version) throws
             InterruptedException, KeeperException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.delete);
@@ -659,7 +596,7 @@ public class ZooKeeper {
      * @see #delete(String, int)
      */
     public void delete(String path, int version, VoidCallback cb, Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.delete);
@@ -689,7 +626,7 @@ public class ZooKeeper {
     public Stat exists(String path, Watcher watcher) throws KeeperException,
         InterruptedException
     {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.exists);
@@ -745,7 +682,7 @@ public class ZooKeeper {
     public void exists(String path, Watcher watcher, StatCallback cb,
             Object ctx)
     {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.exists);
@@ -792,7 +729,7 @@ public class ZooKeeper {
      */
     public byte[] getData(String path, Watcher watcher, Stat stat)
             throws KeeperException, InterruptedException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getData);
@@ -844,7 +781,7 @@ public class ZooKeeper {
      * @see #getData(String, Watcher, Stat)
      */
     public void getData(String path, Watcher watcher, DataCallback cb, Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getData);
@@ -900,7 +837,7 @@ public class ZooKeeper {
      */
     public Stat setData(String path, byte data[], int version)
             throws KeeperException, InterruptedException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.setData);
@@ -924,7 +861,7 @@ public class ZooKeeper {
      */
     public void setData(String path, byte data[], int version, StatCallback cb,
             Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.setData);
@@ -955,7 +892,7 @@ public class ZooKeeper {
      */
     public List<ACL> getACL(String path, Stat stat)
             throws KeeperException, InterruptedException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getACL);
@@ -977,7 +914,7 @@ public class ZooKeeper {
      * @see #getACL(String, Stat)
      */
     public void getACL(String path, Stat stat, ACLCallback cb, Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getACL);
@@ -1011,7 +948,7 @@ public class ZooKeeper {
      */
     public Stat setACL(String path, List<ACL> acl, int version)
             throws KeeperException, InterruptedException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.setACL);
@@ -1038,7 +975,7 @@ public class ZooKeeper {
      */
     public void setACL(String path, List<ACL> acl, int version,
             StatCallback cb, Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.setACL);
@@ -1075,7 +1012,7 @@ public class ZooKeeper {
      */
     public List<String> getChildren(String path, Watcher watcher)
             throws KeeperException, InterruptedException {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getChildren);
@@ -1127,7 +1064,7 @@ public class ZooKeeper {
      */
     public void getChildren(String path, Watcher watcher, ChildrenCallback cb,
             Object ctx) {
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.getChildren);
@@ -1162,7 +1099,7 @@ public class ZooKeeper {
      * @throws IllegalArgumentException if an invalid path is specified
      */
     public void sync(String path, VoidCallback cb, Object ctx){
-        validatePath(path);
+        PathUtils.validatePath(path);
 
         RequestHeader h = new RequestHeader();
         h.setType(ZooDefs.OpCode.sync);

src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java

@@ -30,9 +30,9 @@ import org.apache.log4j.Logger;
 import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.ZooDefs;
-import org.apache.zookeeper.ZooKeeper;
 import org.apache.zookeeper.KeeperException.Code;
 import org.apache.zookeeper.ZooDefs.OpCode;
+import org.apache.zookeeper.common.PathUtils;
 import org.apache.zookeeper.data.ACL;
 import org.apache.zookeeper.data.Id;
 import org.apache.zookeeper.data.StatPersisted;
@@ -205,13 +205,6 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                             Long.toHexString(request.sessionId));
                     throw new KeeperException.BadArgumentsException();
                 }
-                try {
-                    ZooKeeper.validatePath(path);
-                } catch(IllegalArgumentException ie) {
-                    LOG.warn("Invalid path " + path + " with session " +
-                            Long.toHexString(request.sessionId));
-                    throw new KeeperException.BadArgumentsException();
-                }
                 if (!fixupACL(request.authInfo, createRequest.getAcl())) {
                     throw new KeeperException.InvalidACLException();
                 }
@@ -221,10 +214,18 @@ public class PrepRequestProcessor extends Thread implements RequestProcessor {
                 checkACL(zks, parentRecord.acl, ZooDefs.Perms.CREATE,
                         request.authInfo);
                 int parentCVersion = parentRecord.stat.getCversion();
-                CreateMode createMode = CreateMode.fromFlag(createRequest.getFlags());
+                CreateMode createMode =
+                    CreateMode.fromFlag(createRequest.getFlags());
                 if (createMode.isSequential()) {
                     path = path + String.format("%010d", parentCVersion);
                 }
+                try {
+                    PathUtils.validatePath(path);
+                } catch(IllegalArgumentException ie) {
+                    LOG.warn("Invalid path " + path + " with session " +
+                            Long.toHexString(request.sessionId));
+                    throw new KeeperException.BadArgumentsException();
+                }
                 try {
                     if (getRecordForPath(path) != null) {
                         throw new KeeperException.NodeExistsException();

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

@@ -487,6 +487,49 @@ public class ClientTest extends ClientBase {
 
         verifyCreateFails("foo", zk);
         verifyCreateFails("a", zk);
+
+        zk.create("/createseqpar", null, Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT);
+        // next two steps - related to sequential processing
+        // 1) verify that empty child name fails if not sequential
+        try {
+            zk.create("/createseqpar/", null, Ids.OPEN_ACL_UNSAFE,
+                    CreateMode.PERSISTENT);
+            assertTrue(false);
+        } catch(IllegalArgumentException be) {
+            // catch this.
+        }
+
+        // 2) verify that empty child name success if sequential 
+        zk.create("/createseqpar/", null, Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT_SEQUENTIAL);
+        zk.create("/createseqpar/.", null, Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT_SEQUENTIAL);
+        zk.create("/createseqpar/..", null, Ids.OPEN_ACL_UNSAFE,
+                CreateMode.PERSISTENT_SEQUENTIAL);
+        try {
+            zk.create("/createseqpar//", null, Ids.OPEN_ACL_UNSAFE,
+                    CreateMode.PERSISTENT_SEQUENTIAL);
+            assertTrue(false);
+        } catch(IllegalArgumentException be) {
+            // catch this.
+        }
+        try {
+            zk.create("/createseqpar/./", null, Ids.OPEN_ACL_UNSAFE,
+                    CreateMode.PERSISTENT_SEQUENTIAL);
+            assertTrue(false);
+        } catch(IllegalArgumentException be) {
+            // catch this.
+        }
+        try {
+            zk.create("/createseqpar/../", null, Ids.OPEN_ACL_UNSAFE,
+                    CreateMode.PERSISTENT_SEQUENTIAL);
+            assertTrue(false);
+        } catch(IllegalArgumentException be) {
+            // catch this.
+        }
+
+        
         //check for the code path that throws at server
         PrepRequestProcessor.failCreate = true;
         try {