TASK-2530 - Error working with files with URL codes (e.g. %A3) in their name

opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/metadata/CatalogStorageMetadataSynchronizer.java

@@ -680,13 +680,7 @@ private boolean synchronizeIndexedFile(StudyMetadata study, File file, Map<Strin
     }
 
     private String toUri(String path) {
-        String uri;
-        if (path.startsWith("/")) {
-            uri = "file://" + path;
-        } else {
-            uri = Paths.get(path).toUri().toString();
-        }
-        return uri;
+        return Paths.get(path).toUri().toString();
     }
 
     private boolean synchronizeSamples(StudyMetadata study, Collection<Integer> samples, String token) throws CatalogException {

opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/VariantFileIndexerOperationManagerTest.java

@@ -67,6 +67,7 @@
 import static org.junit.Assert.*;
 import static org.opencb.biodata.models.variant.StudyEntry.DEFAULT_COHORT;
 import static org.opencb.opencga.analysis.variant.manager.operations.StatsVariantStorageTest.checkCalculatedStats;
+import static org.opencb.opencga.storage.core.variant.VariantStorageBaseTest.getResourceUri;
 
 /**
  * Created by hpccoll1 on 13/07/15.
@@ -462,6 +463,23 @@ public void testIndexDuplicated() throws Exception {
         assertTrue(Files.exists(outDir.resolve("variant-test-duplicated.vcf.duplicated.tsv")));
     }
 
+    @Test
+    public void testIndexWeirdFileName() throws Exception {
+        ToolRunner toolRunner = new ToolRunner(opencga.getOpencgaHome().toString(), catalogManager, StorageEngineFactory.get(variantManager.getStorageConfiguration()));
+
+        Path outDir = Paths.get(opencga.createTmpOutdir("_escaped_name"));
+
+        Path filePath = Paths.get(getResourceUri("variant-test-file.vcf.gz"));
+        filePath = Files.move(filePath, filePath.getParent().resolve("variant-test%3Afile.vcf.gz"));
+
+        VariantIndexParams params = new VariantIndexParams();
+        params.setFile(create(studyId, filePath.toUri()).getName());
+
+        ExecutionResult er = toolRunner.execute(VariantIndexOperationTool.class, params.toObjectMap()
+                        .append(ParamConstants.STUDY_PARAM, studyId)
+                , outDir, null, sessionId);
+    }
+
     @Override
     protected Aggregation getAggregation() {
         return Aggregation.NONE;

opencga-client/src/main/java/org/opencb/opencga/client/rest/AbstractParentClient.java

@@ -236,7 +236,9 @@ private <T> RestResponse<T> execute(String category1, List<String> id1, String c
              // Select batch. Either by ID or with limit/skip
              if (CollectionUtils.isNotEmpty(id1)) {
                  // Select batch of IDs
-                 path = path.path(String.join(",", id1.subList(skip, skip + batchLimit)));
+                 String ids = String.join(",", id1.subList(skip, skip + batchLimit));
+                 // Use templated variables to ensure correct URL encoding
+                 path = path.path("{id1}").resolveTemplate("id1", ids);
                  // FIXME: This should not be needed!
                  params.put(QueryOptions.LIMIT, batchLimit);
              } else {
@@ -248,7 +250,8 @@ private <T> RestResponse<T> execute(String category1, List<String> id1, String c
                  path = path.path(category2);
              }
              if (StringUtils.isNotEmpty(id2)) {
-                 path = path.path(id2);
+                 // Use templated variables to ensure correct URL encoding
+                 path = path.path("{id2}").resolveTemplate("id2", id2);
              }
              path = path.path(action);
              //privateLogger.info("PATH ::: " + path);

opencga-core/src/main/java/org/opencb/opencga/core/common/UriUtils.java

@@ -22,12 +22,15 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.nio.file.Paths;
+import java.util.regex.Pattern;
 
 /**
  * Created by hpccoll1 on 11/05/15.
  */
 public class UriUtils {
 
+    private static final Pattern HAS_SCHEMA = Pattern.compile("^[^/?#]+:/.*$");
+
     public static void checkUri(URI uri, String uriName, String schema) throws IOException {
         if(uri == null || uri.getScheme() != null && !uri.getScheme().equals(schema)) {
             throw new IOException("Expected " + schema + ":// uri scheme for " + uriName);
@@ -49,8 +52,14 @@ public static URI createUriSafe(String input) {
 
     public static URI createUri(String input, boolean failOnInvalidUri) throws URISyntaxException {
         try {
-            URI sourceUri = new URI(null, input, null);
-            if (sourceUri.getScheme() == null || sourceUri.getScheme().isEmpty()) {
+            URI sourceUri;
+            if (HAS_SCHEMA.matcher(input).matches()) {
+                // Already a URI. Assume it is already escaped.
+                // Avoid double code escaping
+                sourceUri = new URI(input);
+            } else {
+                // Assume direct path name.
+                // Escape if needed.
                 sourceUri = Paths.get(input).toUri();
             }
             return sourceUri;
@@ -97,6 +106,11 @@ public static String dirName(URI uri) {
         return path.substring(idx1 + 1, idx2 + 1);
     }
 
+    public static URI resolve(URI uri, String file) {
+        String newPath = Paths.get(uri.getPath()).resolve(file).toString();
+        return replacePath(uri, newPath);
+    }
+
     public static URI replacePath(URI uri, String path) {
         try {
             return new URIBuilder(uri).setPath(path).build();

opencga-core/src/test/java/org/opencb/opencga/core/common/UriUtilsTest.java

@@ -19,8 +19,9 @@
 import org.junit.Test;
 
 import java.net.URI;
+import java.net.URISyntaxException;
 
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
 
 /**
  * Created on 03/05/19.
@@ -48,9 +49,29 @@ public void testFileName() {
         assertEquals("file", UriUtils.fileName(URI.create("file:///file")));
         assertEquals("file", UriUtils.fileName(URI.create("/file")));
         assertEquals("file", UriUtils.fileName(URI.create("file")));
+        assertEquals("file#2", UriUtils.fileName(UriUtils.createUriSafe("s3:///other/dir/file%232")));
+        assertEquals("file%3A2", UriUtils.fileName(UriUtils.createUriSafe("/other/dir/file%3A2")));
 
         assertEquals("", UriUtils.fileName(URI.create("dir/")));
         assertEquals("", UriUtils.fileName(URI.create("/dir/")));
         assertEquals("", UriUtils.fileName(URI.create("file:///dir/")));
     }
+
+    @Test
+    public void testBuildUri() throws URISyntaxException {
+        // Do not escape anything
+        assertEquals("file:///other/dir/file", UriUtils.createUri("file:///other/dir/file").toString());
+        assertEquals("file:///other/dir/file:2", UriUtils.createUri("file:///other/dir/file:2").toString());
+        assertEquals("http:///other/dir/file%3A2", UriUtils.createUri("http:///other/dir/file%3A2").toString());
+        assertEquals("my.schema:///other/dir/file%3A2", UriUtils.createUri("my.schema:///other/dir/file%3A2").toString());
+        assertEquals("s3:///other/dir/file%232", UriUtils.createUri("s3:///other/dir/file%232").toString());
+
+        // Escape special characters
+        // %
+        assertEquals("file:///other/dir/file%253A2", UriUtils.createUri("/other/dir/file%3A2").toString());
+        // ?
+        assertEquals("file:///other/dir/file%3F2", UriUtils.createUri("/other/dir/file?2").toString());
+        // #
+        assertEquals("file:///other/dir/file%232", UriUtils.createUri("/other/dir/file#2").toString());
+    }
 }

opencga-storage/opencga-storage-app/src/main/java/org/opencb/opencga/storage/app/cli/client/executors/VariantCommandExecutor.java

@@ -591,7 +591,7 @@ private void stats() throws IOException, URISyntaxException, StorageEngineExcept
             DefaultVariantStatisticsManager statisticsManager =
                     (DefaultVariantStatisticsManager) variantStorageEngine.newVariantStatisticsManager();
             statisticsManager.loadStats(
-                    UriUtils.createUri(statsVariantsCommandOptions.load, true),
+                    UriUtils.createUri(statsVariantsCommandOptions.load),
                     statsVariantsCommandOptions.study, options);
         } else {
             /*

opencga-storage/opencga-storage-core/src/main/java/org/opencb/opencga/storage/core/variant/VariantStoragePipeline.java

@@ -244,8 +244,8 @@ public URI transform(URI input, URI pedigree, URI output) throws StorageEngineEx
             throw new IllegalArgumentException("Unknown compression method " + compression);
         }
 
-        URI outputMalformedVariants = output.resolve(fileName + '.' + VariantReaderUtils.MALFORMED_FILE + ".txt");
-        URI outputVariantsFile = output.resolve(fileName + '.' + VariantReaderUtils.VARIANTS_FILE + '.' + format + extension);
+        URI outputMalformedVariants = UriUtils.resolve(output, fileName + '.' + VariantReaderUtils.MALFORMED_FILE + ".txt");
+        URI outputVariantsFile = UriUtils.resolve(output, fileName + '.' + VariantReaderUtils.VARIANTS_FILE + '.' + format + extension);
         URI outputMetaFile = VariantReaderUtils.getMetaFromTransformedFile(outputVariantsFile);
 
         // Close at the end!