SOLR-10387: zkTransfer normalizes destination path incorrectly if source is a windows directory

solr/CHANGES.txt

@@ -148,6 +148,9 @@ Bug Fixes
 
 * SOLR-10369: bin\solr.cmd delete and healthcheck now works again; fixed continuation chars ^ (Luis Goes via janhoy)
 
+* SOLR-10387: zkTransfer normalizes destination path incorrectly if source is a windows directory 
+  (gopikannan venugopalsamy, Erick Erickson)
+
 Other Changes
 ----------------------
 

solr/core/src/test/org/apache/solr/cloud/SolrCLIZkUtilsTest.java

@@ -18,6 +18,7 @@
 package org.apache.solr.cloud;
 
 import java.io.ByteArrayOutputStream;
+import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.nio.charset.Charset;
@@ -229,9 +230,9 @@ public class SolrCLIZkUtilsTest extends SolrCloudTestCase {
     // NOTE: really can't test copying to '.' because the test framework doesn't allow altering the source tree
     // and at least IntelliJ's CWD is in the source tree.
 
-    // copy to local ending in '/'
+    // copy to local ending in separator
     //src and cp3 and cp4 are valid
-    String localSlash = tmp.normalize() + "/cpToLocal/";
+    String localSlash = tmp.normalize() +  File.separator +"cpToLocal" + File.separator;
     args = new String[]{
         "-src", "zk:/cp3/schema.xml",
         "-dst", localSlash,
@@ -246,7 +247,7 @@ public class SolrCLIZkUtilsTest extends SolrCloudTestCase {
     // copy to ZK ending in '/'.
     //src and cp3 are valid
     args = new String[]{
-        "-src", "file:" + srcPathCheck.normalize().toAbsolutePath().toString() + "/solrconfig.xml",
+        "-src", "file:" + srcPathCheck.normalize().toAbsolutePath().toString() + File.separator + "solrconfig.xml",
         "-dst", "zk:/powerup/",
         "-recurse", "false",
         "-zkHost", zkAddr,
@@ -259,7 +260,7 @@ public class SolrCLIZkUtilsTest extends SolrCloudTestCase {
     // copy individual file up
     //src and cp3 are valid
     args = new String[]{
-        "-src", "file:" + srcPathCheck.normalize().toAbsolutePath().toString() + "/solrconfig.xml",
+        "-src", "file:" + srcPathCheck.normalize().toAbsolutePath().toString() + File.separator + "solrconfig.xml",
         "-dst", "zk:/copyUpFile.xml",
         "-recurse", "false",
         "-zkHost", zkAddr,
@@ -272,7 +273,7 @@ public class SolrCLIZkUtilsTest extends SolrCloudTestCase {
     // copy individual file down
     //src and cp3 are valid
 
-    String localNamed = tmp.normalize().toString() + "/localnamed/renamed.txt";
+    String localNamed = tmp.normalize().toString() + File.separator + "localnamed" + File.separator +  "renamed.txt";
     args = new String[]{
         "-src", "zk:/cp4/solrconfig.xml",
         "-dst", "file:" + localNamed,
@@ -404,7 +405,7 @@ public class SolrCLIZkUtilsTest extends SolrCloudTestCase {
     // Files are in mv2
     // Now fail if we specify "file:". Everything should still be in /mv2
     args = new String[]{
-        "-src", "file:/mv2",
+        "-src", "file:" + File.separator + "mv2",
         "-dst", "/mv3",
         "-zkHost", zkAddr,
     };

solr/solrj/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java

@@ -119,33 +119,36 @@ public class ZkMaintenanceUtils {
         throw new SolrServerException("Local path " + Paths.get(src).toAbsolutePath() + " is a directory and recurse is false");
       }
     }
-    if (srcIsZk == false && dstIsZk == false) {
-      throw new SolrServerException("At least one of the source and dest parameters must be prefixed with 'zk:' ");
-    }
+
     if (dstIsZk && dst.length() == 0) {
       dst = "/"; // for consistency, one can copy from zk: and send to zk:/
     }
-    dst = normalizeDest(src, dst);
+    dst = normalizeDest(src, dst, srcIsZk, dstIsZk);
 
+    // ZK -> ZK copy.
     if (srcIsZk && dstIsZk) {
       traverseZkTree(zkClient, src, VISIT_ORDER.VISIT_PRE, new ZkCopier(zkClient, src, dst));
       return;
     }
+
+    //local -> ZK copy
     if (dstIsZk) {
       uploadToZK(zkClient, Paths.get(src), dst, null);
       return;
     }
 
-    // Copying individual files from ZK requires special handling since downloadFromZK assumes it's a directory.
+    // Copying individual files from ZK requires special handling since downloadFromZK assumes the node has children.
     // This is kind of a weak test for the notion of "directory" on Zookeeper.
+    // ZK -> local copy where ZK is a parent node
     if (zkClient.getChildren(src, null, true).size() > 0) {
       downloadFromZK(zkClient, src, Paths.get(dst));
       return;
     }
 
+    // Single file ZK -> local copy where ZK is a leaf node
     if (Files.isDirectory(Paths.get(dst))) {
-      if (dst.endsWith("/") == false) dst += "/";
-      dst = normalizeDest(src, dst);
+      if (dst.endsWith(File.separator) == false) dst += File.separator;
+      dst = normalizeDest(src, dst, srcIsZk, dstIsZk);
     }
     byte[] data = zkClient.getData(src, null, null, true);
     Path filename = Paths.get(dst);
@@ -154,18 +157,19 @@ public class ZkMaintenanceUtils {
     Files.write(filename, data);
   }
 
-  
-  private static String normalizeDest(String srcName, String dstName) {
+  // If the dest ends with a separator, it's a directory or non-leaf znode, so return the
+  // last element of the src to appended to the dstName.
+  private static String normalizeDest(String srcName, String dstName, boolean srcIsZk, boolean dstIsZk) {
     // Special handling for "."
     if (dstName.equals(".")) {
       return Paths.get(".").normalize().toAbsolutePath().toString();
     }
-    // Pull the last element of the src path and add it to the dst if the src does NOT end in a slash 
 
-    // If the source ends in a slash, do not append the last segment to the dest
+    String dstSeparator = (dstIsZk) ? "/" : File.separator;
+    String srcSeparator = (srcIsZk) ? "/" : File.separator;
 
-    if (dstName.endsWith("/")) { // Dest is a directory.
-      int pos = srcName.lastIndexOf("/");
+    if (dstName.endsWith(dstSeparator)) { // Dest is a directory or non-leaf znode, append last element of the src path.
+      int pos = srcName.lastIndexOf(srcSeparator);
       if (pos < 0) {
         dstName += srcName;
       } else {
@@ -178,7 +182,7 @@ public class ZkMaintenanceUtils {
   }
 
   public static void moveZnode(SolrZkClient zkClient, String src, String dst) throws SolrServerException, KeeperException, InterruptedException {
-    String destName = normalizeDest(src, dst);
+    String destName = normalizeDest(src, dst, true, true);
 
     // Special handling if the source has no children, i.e. copying just a single file.
     if (zkClient.getChildren(src, null, true).size() == 0) {
@@ -384,12 +388,11 @@ public class ZkMaintenanceUtils {
     }
   }
 
-  // Take into account Windows file separaters when making a Znode's name.
+  // Take into account Windows file separators when making a Znode's name.
   public static String createZkNodeName(String zkRoot, Path root, Path file) {
     String relativePath = root.relativize(file).toString();
     // Windows shenanigans
-    String separator = root.getFileSystem().getSeparator();
-    if ("\\".equals(separator))
+    if ("\\".equals(File.separator))
       relativePath = relativePath.replaceAll("\\\\", "/");
     // It's possible that the relative path and file are the same, in which case
     // adding the bare slash is A Bad Idea unless it's a non-leaf data node