[SOLR-9444] Fix path usage for cloud backup/restore

- Refactored code using URI.getPath() API to use URI instance
  uniformly.
- During serialization of URI instance, used toASCIIString() API
  to generate the appropriate String representation.
- Updated the unit test to use whitespaces in the backup directory path

solr/core/src/java/org/apache/solr/cloud/BackupCmd.java

@@ -62,7 +62,6 @@ public class BackupCmd implements OverseerCollectionMessageHandler.Cmd {
     ShardHandler shardHandler = ocmh.shardHandlerFactory.getShardHandler();
     String asyncId = message.getStr(ASYNC);
     String repo = message.getStr(CoreAdminParams.BACKUP_REPOSITORY);
-    String location = message.getStr(CoreAdminParams.BACKUP_LOCATION);
 
     Map<String, String> requestMap = new HashMap<>();
     Instant startTime = Instant.now();
@@ -72,7 +71,8 @@ public class BackupCmd implements OverseerCollectionMessageHandler.Cmd {
     BackupManager backupMgr = new BackupManager(repository, ocmh.zkStateReader, collectionName);
 
     // Backup location
-    URI backupPath = repository.createURI(location, backupName);
+    URI location = repository.createURI(message.getStr(CoreAdminParams.BACKUP_LOCATION));
+    URI backupPath = repository.resolve(location, backupName);
 
     //Validating if the directory already exists.
     if (repository.exists(backupPath)) {
@@ -94,7 +94,7 @@ public class BackupCmd implements OverseerCollectionMessageHandler.Cmd {
       params.set(CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.BACKUPCORE.toString());
       params.set(NAME, slice.getName());
       params.set(CoreAdminParams.BACKUP_REPOSITORY, repo);
-      params.set(CoreAdminParams.BACKUP_LOCATION, backupPath.getPath()); // note: index dir will be here then the "snapshot." + slice name
+      params.set(CoreAdminParams.BACKUP_LOCATION, backupPath.toASCIIString()); // note: index dir will be here then the "snapshot." + slice name
       params.set(CORE_NAME_PROP, coreName);
 
       ocmh.sendShardRequest(replica.getNodeName(), params, shardHandler, asyncId, requestMap);

solr/core/src/java/org/apache/solr/cloud/RestoreCmd.java

@@ -79,13 +79,13 @@ public class RestoreCmd implements OverseerCollectionMessageHandler.Cmd {
     ShardHandler shardHandler = ocmh.shardHandlerFactory.getShardHandler();
     String asyncId = message.getStr(ASYNC);
     String repo = message.getStr(CoreAdminParams.BACKUP_REPOSITORY);
-    String location = message.getStr(CoreAdminParams.BACKUP_LOCATION);
     Map<String, String> requestMap = new HashMap<>();
 
     CoreContainer cc = ocmh.overseer.getZkController().getCoreContainer();
     BackupRepository repository = cc.newBackupRepository(Optional.ofNullable(repo));
 
-    URI backupPath = repository.createURI(location, backupName);
+    URI location = repository.createURI(message.getStr(CoreAdminParams.BACKUP_LOCATION));
+    URI backupPath = repository.resolve(location, backupName);
     ZkStateReader zkStateReader = ocmh.zkStateReader;
     BackupManager backupMgr = new BackupManager(repository, zkStateReader, restoreCollectionName);
 
@@ -195,7 +195,7 @@ public class RestoreCmd implements OverseerCollectionMessageHandler.Cmd {
       ModifiableSolrParams params = new ModifiableSolrParams();
       params.set(CoreAdminParams.ACTION, CoreAdminParams.CoreAdminAction.RESTORECORE.toString());
       params.set(NAME, "snapshot." + slice.getName());
-      params.set(CoreAdminParams.BACKUP_LOCATION, backupPath.getPath());
+      params.set(CoreAdminParams.BACKUP_LOCATION, backupPath.toASCIIString());
       params.set(CoreAdminParams.BACKUP_REPOSITORY, repo);
 
       ocmh.sliceCmd(clusterState, params, null, slice, shardHandler, asyncId, requestMap);

solr/core/src/java/org/apache/solr/core/backup/BackupManager.java

@@ -87,12 +87,12 @@ public class BackupManager {
    * @return the configuration parameters for the specified backup.
    * @throws IOException In case of errors.
    */
-  public Properties readBackupProperties(String backupLoc, String backupId) throws IOException {
+  public Properties readBackupProperties(URI backupLoc, String backupId) throws IOException {
     Preconditions.checkNotNull(backupLoc);
     Preconditions.checkNotNull(backupId);
 
     // Backup location
-    URI backupPath = repository.createURI(backupLoc, backupId);
+    URI backupPath = repository.resolve(backupLoc, backupId);
     if (!repository.exists(backupPath)) {
       throw new SolrException(ErrorCode.SERVER_ERROR, "Couldn't restore since doesn't exist: " + backupPath);
     }
@@ -113,8 +113,8 @@ public class BackupManager {
    * @param props The backup properties
    * @throws IOException in case of I/O error
    */
-  public void writeBackupProperties(String backupLoc, String backupId, Properties props) throws IOException {
+  public void writeBackupProperties(URI backupLoc, String backupId, Properties props) throws IOException {
-    URI dest = repository.createURI(backupLoc, backupId, BACKUP_PROPS_FILE);
+    URI dest = repository.resolve(backupLoc, backupId, BACKUP_PROPS_FILE);
     try (Writer propsWriter = new OutputStreamWriter(repository.createOutput(dest), StandardCharsets.UTF_8)) {
       props.store(propsWriter, "Backup properties file");
     }
@@ -128,10 +128,10 @@ public class BackupManager {
    * @return the meta-data information for the backed-up collection.
    * @throws IOException in case of errors.
    */
-  public DocCollection readCollectionState(String backupLoc, String backupId, String collectionName) throws IOException {
+  public DocCollection readCollectionState(URI backupLoc, String backupId, String collectionName) throws IOException {
     Preconditions.checkNotNull(collectionName);
 
-    URI zkStateDir = repository.createURI(backupLoc, backupId, ZK_STATE_DIR);
+    URI zkStateDir = repository.resolve(backupLoc, backupId, ZK_STATE_DIR);
     try (IndexInput is = repository.openInput(zkStateDir, COLLECTION_PROPS_FILE, IOContext.DEFAULT)) {
       byte[] arr = new byte[(int) is.length()]; // probably ok since the json file should be small.
       is.readBytes(arr, 0, (int) is.length());
@@ -149,9 +149,9 @@ public class BackupManager {
    * @param collectionState The collection meta-data to be stored.
    * @throws IOException in case of I/O errors.
    */
-  public void writeCollectionState(String backupLoc, String backupId, String collectionName,
+  public void writeCollectionState(URI backupLoc, String backupId, String collectionName,
                                    DocCollection collectionState) throws IOException {
-    URI dest = repository.createURI(backupLoc, backupId, ZK_STATE_DIR, COLLECTION_PROPS_FILE);
+    URI dest = repository.resolve(backupLoc, backupId, ZK_STATE_DIR, COLLECTION_PROPS_FILE);
     try (OutputStream collectionStateOs = repository.createOutput(dest)) {
       collectionStateOs.write(Utils.toJSON(Collections.singletonMap(collectionName, collectionState)));
     }
@@ -166,9 +166,9 @@ public class BackupManager {
    * @param targetConfigName  The name of the config to be created.
    * @throws IOException in case of I/O errors.
    */
-  public void uploadConfigDir(String backupLoc, String backupId, String sourceConfigName, String targetConfigName)
+  public void uploadConfigDir(URI backupLoc, String backupId, String sourceConfigName, String targetConfigName)
       throws IOException {
-    URI source = repository.createURI(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR, sourceConfigName);
+    URI source = repository.resolve(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR, sourceConfigName);
     String zkPath = ZkConfigManager.CONFIGS_ZKNODE + "/" + targetConfigName;
     uploadToZk(zkStateReader.getZkClient(), source, zkPath);
   }
@@ -181,10 +181,10 @@ public class BackupManager {
    * @param configName The name of the config to be saved.
    * @throws IOException in case of I/O errors.
    */
-  public void downloadConfigDir(String backupLoc, String backupId, String configName) throws IOException {
+  public void downloadConfigDir(URI backupLoc, String backupId, String configName) throws IOException {
-    URI dest = repository.createURI(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR, configName);
+    URI dest = repository.resolve(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR, configName);
-    repository.createDirectory(repository.createURI(backupLoc, backupId, ZK_STATE_DIR));
+    repository.createDirectory(repository.resolve(backupLoc, backupId, ZK_STATE_DIR));
-    repository.createDirectory(repository.createURI(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR));
+    repository.createDirectory(repository.resolve(backupLoc, backupId, ZK_STATE_DIR, CONFIG_STATE_DIR));
     repository.createDirectory(dest);
 
     downloadFromZK(zkStateReader.getZkClient(), ZkConfigManager.CONFIGS_ZKNODE + "/" + configName, dest);
@@ -201,11 +201,11 @@ public class BackupManager {
         if (children.size() == 0) {
           log.info("Writing file {}", file);
           byte[] data = zkClient.getData(zkPath + "/" + file, null, null, true);
-          try (OutputStream os = repository.createOutput(repository.createURI(dir.getPath(), file))) {
+          try (OutputStream os = repository.createOutput(repository.resolve(dir, file))) {
             os.write(data);
           }
         } else {
-          downloadFromZK(zkClient, zkPath + "/" + file, repository.createURI(dir.getPath(), file));
+          downloadFromZK(zkClient, zkPath + "/" + file, repository.resolve(dir, file));
         }
       }
     } catch (KeeperException | InterruptedException e) {
@@ -221,7 +221,7 @@ public class BackupManager {
 
     for (String file : repository.listAll(sourceDir)) {
       String zkNodePath = destZkPath + "/" + file;
-      URI path = repository.createURI(sourceDir.getPath(), file);
+      URI path = repository.resolve(sourceDir, file);
       PathType t = repository.getPathType(path);
       switch (t) {
         case FILE: {

solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepository.java

@@ -57,13 +57,23 @@ public interface BackupRepository extends NamedListInitializedPlugin, Closeable
   <T> T getConfigProperty(String name);
 
   /**
-   * This method creates a URI using the specified path components (as method arguments).
+   * This method returns the URI representation for the specified path.
+   * Note - the specified path could be a fully qualified URI OR a relative path for a file-system.
    *
+   * @param path The path specified by the user.
+   * @return the URI representation of the user supplied value
+   */
+   URI createURI(String path);
+
+  /**
+   * This method resolves a URI using the specified path components (as method arguments).
+   *
+   * @param baseUri The base URI to use for creating the path
    * @param pathComponents
    *          The directory (or file-name) to be included in the URI.
    * @return A URI containing absolute path
    */
-  URI createURI(String... pathComponents);
+  URI resolve(URI baseUri, String... pathComponents);
 
   /**
    * This method checks if the specified path exists in this repository.

solr/core/src/java/org/apache/solr/core/backup/repository/HdfsBackupRepository.java

@@ -20,6 +20,7 @@ package org.apache.solr.core.backup.repository;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.net.URI;
+import java.net.URISyntaxException;
 
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.FileStatus;
@@ -88,11 +89,31 @@ public class HdfsBackupRepository implements BackupRepository {
   }
 
   @Override
-  public URI createURI(String... pathComponents) {
+  public URI createURI(String location) {
-    Path result = baseHdfsPath;
+    Preconditions.checkNotNull(location);
-    for (String p : pathComponents) {
+
-      result = new Path(result, p);
+    URI result = null;
+    try {
+      result = new URI(location);
+      if (!result.isAbsolute()) {
+        result = resolve(this.baseHdfsPath.toUri(), location);
       }
+    } catch (URISyntaxException ex) {
+      result = resolve(this.baseHdfsPath.toUri(), location);
+    }
+
+    return result;
+  }
+
+  @Override
+  public URI resolve(URI baseUri, String... pathComponents) {
+    Preconditions.checkArgument(baseUri.isAbsolute());
+
+    Path result = new Path(baseUri);
+    for (String path : pathComponents) {
+      result = new Path(result, path);
+    }
+
     return result.toUri();
   }
 

solr/core/src/java/org/apache/solr/core/backup/repository/LocalFileSystemRepository.java

@@ -20,19 +20,20 @@ package org.apache.solr.core.backup.repository;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
+
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.store.FSDirectory;
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexInput;
 import org.apache.lucene.store.NoLockFactory;
 import org.apache.lucene.store.SimpleFSDirectory;
-import org.apache.lucene.util.Constants;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.core.DirectoryFactory;
 
@@ -58,21 +59,28 @@ public class LocalFileSystemRepository implements BackupRepository {
   }
 
   @Override
-  public URI createURI(String... pathComponents) {
+  public URI createURI(String location) {
-    Preconditions.checkArgument(pathComponents.length > 0);
+    Preconditions.checkNotNull(location);
 
-    String basePath = Preconditions.checkNotNull(pathComponents[0]);
+    URI result = null;
-    // Note the URI.getPath() invocation on Windows platform generates an invalid URI.
+    try {
-    // Refer to http://stackoverflow.com/questions/9834776/java-nio-file-path-issue
+      result = new URI(location);
-    // Since the caller may have used this method to generate the string representation
+      if (!result.isAbsolute()) {
-    // for the pathComponents, we implement a work-around specifically for Windows platform
+        result = Paths.get(location).toUri();
-    // to remove the leading '/' character.
+      }
-    if (Constants.WINDOWS) {
+    } catch (URISyntaxException ex) {
-      basePath = basePath.replaceFirst("^/(.:/)", "$1");
+      result = Paths.get(location).toUri();
     }
 
-    Path result = Paths.get(basePath);
+    return result;
-    for (int i = 1; i < pathComponents.length; i++) {
+  }
+
+  @Override
+  public URI resolve(URI baseUri, String... pathComponents) {
+    Preconditions.checkArgument(pathComponents.length > 0);
+
+    Path result = Paths.get(baseUri);
+    for (int i = 0; i < pathComponents.length; i++) {
       result = result.resolve(pathComponents[i]);
     }
 

solr/core/src/java/org/apache/solr/handler/ReplicationHandler.java

@@ -443,14 +443,15 @@ public class ReplicationHandler extends RequestHandlerBase implements SolrCoreAw
       location = core.getDataDir();
     }
 
+    URI locationUri = repo.createURI(location);
+
     //If name is not provided then look for the last unnamed( the ones with the snapshot.timestamp format)
     //snapshot folder since we allow snapshots to be taken without providing a name. Pick the latest timestamp.
     if (name == null) {
-      URI basePath = repo.createURI(location);
+      String[] filePaths = repo.listAll(locationUri);
-      String[] filePaths = repo.listAll(basePath);
       List<OldBackupDirectory> dirs = new ArrayList<>();
       for (String f : filePaths) {
-        OldBackupDirectory obd = new OldBackupDirectory(basePath, f);
+        OldBackupDirectory obd = new OldBackupDirectory(locationUri, f);
         if (obd.getTimestamp().isPresent()) {
           dirs.add(obd);
         }
@@ -465,7 +466,7 @@ public class ReplicationHandler extends RequestHandlerBase implements SolrCoreAw
       name = "snapshot." + name;
     }
 
-    RestoreCore restoreCore = new RestoreCore(repo, core, location, name);
+    RestoreCore restoreCore = new RestoreCore(repo, core, locationUri, name);
     try {
       MDC.put("RestoreCore.core", core.getName());
       MDC.put("RestoreCore.backupLocation", location);
@@ -561,7 +562,8 @@ public class ReplicationHandler extends RequestHandlerBase implements SolrCoreAw
       }
 
       // small race here before the commit point is saved
-      SnapShooter snapShooter = new SnapShooter(repo, core, location, params.get(NAME), commitName);
+      URI locationUri = repo.createURI(location);
+      SnapShooter snapShooter = new SnapShooter(repo, core, locationUri, params.get(NAME), commitName);
       snapShooter.validateCreateSnapshot();
       snapShooter.createSnapAsync(indexCommit, numberToKeep, (nl) -> snapShootDetails = nl);
 

solr/core/src/java/org/apache/solr/handler/RestoreCore.java

@@ -44,11 +44,11 @@ public class RestoreCore implements Callable<Boolean> {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   private final String backupName;
-  private final String backupLocation;
+  private final URI backupLocation;
   private final SolrCore core;
   private final BackupRepository backupRepo;
 
-  public RestoreCore(BackupRepository backupRepo, SolrCore core, String location, String name) {
+  public RestoreCore(BackupRepository backupRepo, SolrCore core, URI location, String name) {
     this.backupRepo = backupRepo;
     this.core = core;
     this.backupLocation = location;
@@ -62,7 +62,7 @@ public class RestoreCore implements Callable<Boolean> {
 
   public boolean doRestore() throws Exception {
 
-    URI backupPath = backupRepo.createURI(backupLocation, backupName);
+    URI backupPath = backupRepo.resolve(backupLocation, backupName);
     SimpleDateFormat dateFormat = new SimpleDateFormat(SnapShooter.DATE_FMT, Locale.ROOT);
     String restoreIndexName = "restore." + dateFormat.format(new Date());
     String restoreIndexPath = core.getDataDir() + restoreIndexName;

solr/core/src/java/org/apache/solr/handler/SnapShooter.java

@@ -19,6 +19,7 @@ package org.apache.solr.handler;
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
 import java.net.URI;
+import java.nio.file.Paths;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -75,17 +76,17 @@ public class SnapShooter {
     } else {
       snapDirStr = core.getCoreDescriptor().getInstanceDir().resolve(location).normalize().toString();
     }
-    initialize(new LocalFileSystemRepository(), core, snapDirStr, snapshotName, null);
+    initialize(new LocalFileSystemRepository(), core, Paths.get(snapDirStr).toUri(), snapshotName, null);
   }
 
-  public SnapShooter(BackupRepository backupRepo, SolrCore core, String location, String snapshotName, String commitName) {
+  public SnapShooter(BackupRepository backupRepo, SolrCore core, URI location, String snapshotName, String commitName) {
     initialize(backupRepo, core, location, snapshotName, commitName);
   }
 
-  private void initialize(BackupRepository backupRepo, SolrCore core, String location, String snapshotName, String commitName) {
+  private void initialize(BackupRepository backupRepo, SolrCore core, URI location, String snapshotName, String commitName) {
     this.solrCore = Preconditions.checkNotNull(core);
     this.backupRepo = Preconditions.checkNotNull(backupRepo);
-    this.baseSnapDirPath = backupRepo.createURI(Preconditions.checkNotNull(location)).normalize();
+    this.baseSnapDirPath = location;
     this.snapshotName = snapshotName;
     if (snapshotName != null) {
       directoryName = "snapshot." + snapshotName;
@@ -93,7 +94,7 @@ public class SnapShooter {
       SimpleDateFormat fmt = new SimpleDateFormat(DATE_FMT, Locale.ROOT);
       directoryName = "snapshot." + fmt.format(new Date());
     }
-    this.snapshotDirPath = backupRepo.createURI(location, directoryName);
+    this.snapshotDirPath = backupRepo.resolve(location, directoryName);
     this.commitName = commitName;
   }
 

solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java

@@ -18,6 +18,7 @@ package org.apache.solr.handler.admin;
 
 import java.io.IOException;
 import java.lang.invoke.MethodHandles;
+import java.net.URI;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -803,8 +804,9 @@ enum CoreAdminOperation implements CoreAdminOp {
       // parameter is not supplied, the latest index commit is backed-up.
       String commitName = params.get(CoreAdminParams.COMMIT_NAME);
 
+      URI locationUri = repository.createURI(location);
       try (SolrCore core = it.handler.coreContainer.getCore(cname)) {
-        SnapShooter snapShooter = new SnapShooter(repository, core, location, name, commitName);
+        SnapShooter snapShooter = new SnapShooter(repository, core, locationUri, name, commitName);
         // validateCreateSnapshot will create parent dirs instead of throw; that choice is dubious.
         //  But we want to throw. One reason is that
         //  this dir really should, in fact must, already exist here if triggered via a collection backup on a shared
@@ -847,8 +849,9 @@ enum CoreAdminOperation implements CoreAdminOp {
           + " parameter or as a default repository property");
     }
 
+    URI locationUri = repository.createURI(location);
     try (SolrCore core = it.handler.coreContainer.getCore(cname)) {
-      RestoreCore restoreCore = new RestoreCore(repository, core, location, name);
+      RestoreCore restoreCore = new RestoreCore(repository, core, locationUri, name);
       boolean success = restoreCore.doRestore();
       if (!success) {
         throw new SolrException(ErrorCode.SERVER_ERROR, "Failed to restore core=" + core.getName());

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

@@ -24,12 +24,20 @@ import org.junit.BeforeClass;
  * such file-system would be exposed via local file-system API.
  */
 public class TestLocalFSCloudBackupRestore extends AbstractCloudBackupRestoreTestCase {
+  private static String backupLocation;
 
   @BeforeClass
   public static void setupClass() throws Exception {
     configureCluster(NUM_SHARDS)// nodes
         .addConfig("conf1", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
         .configure();
+
+    boolean whitespacesInPath = random().nextBoolean();
+    if (whitespacesInPath) {
+      backupLocation = createTempDir("my backup").toFile().getAbsolutePath();
+    } else {
+      backupLocation = createTempDir("mybackup").toFile().getAbsolutePath();
+    }
   }
 
   @Override
@@ -44,6 +52,6 @@ public class TestLocalFSCloudBackupRestore extends AbstractCloudBackupRestoreTes
 
   @Override
   public String getBackupLocation() {
-    return createTempDir().toFile().getAbsolutePath();
+    return backupLocation;
   }
 }