Merge pull request #2497 from opencb/TASK-6737

TASK-6737 - Force parameter is not recognised in the variant delete operation

opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/manager/operations/VariantDeleteOperationManager.java

@@ -27,13 +27,17 @@
 import org.opencb.opencga.storage.core.metadata.models.TaskMetadata;
 import org.opencb.opencga.storage.core.variant.VariantStorageEngine;
 import org.opencb.opencga.storage.core.variant.VariantStorageOptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.List;
 
 public class VariantDeleteOperationManager extends OperationManager {
 
+    private final Logger logger = LoggerFactory.getLogger(VariantDeleteOperationManager.class);
+
     public VariantDeleteOperationManager(VariantStorageManager variantStorageManager, VariantStorageEngine engine) {
         super(variantStorageManager, engine);
     }
@@ -63,7 +67,14 @@ public void removeFile(String study, List<String> inputFiles, URI outdir, String
                 String catalogIndexStatus = file.getInternal().getVariant().getIndex().getStatus().getId();
                 if (!catalogIndexStatus.equals(VariantIndexStatus.READY)) {
                     // Might be partially loaded in VariantStorage. Check FileMetadata
-                    FileMetadata fileMetadata = variantStorageEngine.getMetadataManager().getFileMetadata(studyMetadata.getId(), fileStr);
+                    FileMetadata fileMetadata = variantStorageEngine.getMetadataManager()
+                            .getFileMetadata(studyMetadata.getId(), file.getName());
+                    if (fileMetadata != null && !fileMetadata.getPath().equals(file.getUri().getPath())) {
+                        // FileMetadata path does not match the catalog path. This file is not registered in the storage.
+                        throw new CatalogException("Unable to remove variants from file '" + file.getPath() + "'. "
+                                + "File is not registered in the storage. "
+                                + "Instead, found file with same name but different path '" + fileMetadata.getPath() + "'");
+                    }
                     boolean canBeRemoved;
                     if (force) {
                         // When forcing remove, just require the file to be registered in the storage
@@ -73,17 +84,18 @@ public void removeFile(String study, List<String> inputFiles, URI outdir, String
                         canBeRemoved = fileMetadata != null && fileMetadata.getIndexStatus() != TaskMetadata.Status.NONE;
                     }
                     if (!canBeRemoved) {
-                        throw new CatalogException("Unable to remove variants from file " + file.getName() + ". "
-                                + "IndexStatus = " + catalogIndexStatus);
+                        throw new CatalogException("Unable to remove variants from file '" + file.getPath() + "'. "
+                                + "IndexStatus = " + catalogIndexStatus + "."
+                                + (fileMetadata == null ? " File not found in storage." : ""));
                     }
                 }
                 fileNames.add(file.getName());
 //                        filePaths.add(file.getPath());
             }
 
-            if (fileNames.isEmpty()) {
-                throw new CatalogException("Nothing to do!");
-            }
+        }
+        if (fileNames.isEmpty()) {
+            throw new CatalogException("Nothing to do!");
         }
 
         variantStorageEngine.removeFiles(study, fileNames, outdir);

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

@@ -197,7 +197,11 @@ protected File getSmallFile() throws IOException, CatalogException {
     }
 
     protected File create(String resourceName) throws IOException, CatalogException {
-        return create(studyId, getResourceUri(resourceName));
+        return create(resourceName, "data/vcfs/");
+    }
+
+    protected File create(String resourceName, String path) throws IOException, CatalogException {
+        return create(studyId, getResourceUri(resourceName), path);
     }
 
     protected File create(String studyId, URI uri) throws IOException, CatalogException {

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

@@ -32,7 +32,9 @@
 import org.opencb.opencga.core.models.sample.Sample;
 import org.opencb.opencga.core.models.study.Study;
 import org.opencb.opencga.core.testclassification.duration.MediumTests;
+import org.opencb.opencga.storage.core.variant.VariantStorageOptions;
 
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -44,6 +46,7 @@
 import static org.hamcrest.CoreMatchers.anyOf;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.*;
+import static org.opencb.opencga.storage.core.variant.VariantStorageBaseTest.getResourceUri;
 
 /**
  * Created on 10/07/17.
@@ -83,6 +86,42 @@ public void testLoadAndRemoveOneWithOtherLoaded() throws Exception {
         testLoadAndRemoveOne();
     }
 
+    @Test
+    public void testLoadAndRemoveForce() throws Exception {
+        File file77 = create("platinum/1K.end.platinum-genomes-vcf-NA12877_S1.genome.vcf.gz");
+        File file78 = create("platinum/1K.end.platinum-genomes-vcf-NA12878_S1.genome.vcf.gz");
+        File file79 = create("platinum/1K.end.platinum-genomes-vcf-NA12879_S1.genome.vcf.gz");
+        indexFile(file77, new QueryOptions(), outputId);
+        indexFile(file78, new QueryOptions(), outputId);
+
+        removeFile(file77, new QueryOptions());
+
+        try {
+            removeFile(file77, new QueryOptions());
+        } catch (Exception e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("Unable to remove variants from file"));
+        }
+        removeFile(file77, new QueryOptions(VariantStorageOptions.FORCE.key(), true));
+
+        try {
+            removeFile(file79, new QueryOptions(VariantStorageOptions.FORCE.key(), true));
+        } catch (Exception e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("File not found in storage."));
+        }
+        Path file77Path = Paths.get(file77.getUri());
+        Path otherDir = file77Path.getParent().resolve("other_dir");
+        Files.createDirectory(otherDir);
+        Path otherFile = Files.copy(file77Path, otherDir.resolve(file77Path.getFileName()));
+        File file77_2 = create(studyFqn, otherFile.toUri(), "other_dir");
+
+        try {
+            removeFile(studyFqn, Collections.singletonList(file77_2.getPath()), new QueryOptions(VariantStorageOptions.FORCE.key(), true));
+        } catch (Exception e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("Unable to remove variants from file"));
+            assertTrue(e.getMessage(), e.getMessage().contains("Instead, found file with same name but different path"));
+        }
+    }
+
 
     @Test
     public void testLoadAndRemoveMany() throws Exception {
@@ -127,8 +166,13 @@ private void removeFile(List<File> files, QueryOptions options) throws Exception
         Study study = catalogManager.getFileManager().getStudy(ORGANIZATION, files.get(0), sessionId);
         String studyId = study.getFqn();
 
+        removeFile(studyId, fileIds, options);
+    }
+
+    private void removeFile(String studyId, List<String> fileIds, QueryOptions options) throws Exception {
+
         Path outdir = Paths.get(opencga.createTmpOutdir(studyId, "_REMOVE_", sessionId));
-        variantManager.removeFile(studyId, fileIds, new QueryOptions(), outdir.toUri(), sessionId);
+        variantManager.removeFile(studyId, fileIds, new QueryOptions(options), outdir.toUri(), sessionId);
 //        assertEquals(files.size(), removedFiles.size());
 
         Cohort all = catalogManager.getCohortManager().search(studyId, new Query(CohortDBAdaptor.QueryParams.ID.key(),

opencga-app/src/main/java/org/opencb/opencga/app/cli/main/options/OperationsVariantStorageCommandOptions.java

@@ -380,13 +380,13 @@ public class DeleteVariantCommandOptions {
         @Parameter(names = {"--study", "-s"}, description = "Study [[organization@]project:]study where study and project can be either the ID or UUID", required = false, arity = 1)
         public String study; 
 
-        @Parameter(names = {"--file"}, description = "The body web service file parameter", required = false, arity = 1)
+        @Parameter(names = {"--file"}, description = "List of file ids to delete. Use 'all' to remove the whole study", required = false, arity = 1)
         public String file;
 
-        @Parameter(names = {"--resume"}, description = "The body web service resume parameter", required = false, help = true, arity = 0)
+        @Parameter(names = {"--resume"}, description = "Resume failed delete operation.", required = false, help = true, arity = 0)
         public boolean resume = false;
 
-        @Parameter(names = {"--force"}, description = "The body web service force parameter", required = false, help = true, arity = 0)
+        @Parameter(names = {"--force"}, description = "Force delete operation. This would allow deleting partially loaded files.", required = false, help = true, arity = 0)
         public boolean force = false;
 
     }

opencga-core/src/main/java/org/opencb/opencga/core/cellbase/CellBaseValidator.java

@@ -23,6 +23,7 @@
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
+import java.util.concurrent.Callable;
 import java.util.stream.Collectors;
 
 
@@ -149,7 +150,7 @@ private CellBaseConfiguration validate(boolean autoComplete) throws IOException
         String inputVersion = getVersion();
         CellBaseDataResponse<SpeciesProperties> species;
         try {
-            species = cellBaseClient.getMetaClient().species();
+            species = retryMetaSpecies();
         } catch (RuntimeException e) {
             throw new IllegalArgumentException("Unable to access cellbase url '" + getURL() + "', version '" + inputVersion + "'", e);
         }
@@ -158,7 +159,7 @@ private CellBaseConfiguration validate(boolean autoComplete) throws IOException
                 // Version might be missing the starting "v"
                 cellBaseConfiguration.setVersion("v" + cellBaseConfiguration.getVersion());
                 cellBaseClient = newCellBaseClient(cellBaseConfiguration, getSpecies(), getAssembly());
-                species = cellBaseClient.getMetaClient().species();
+                species = retryMetaSpecies();
             }
         }
         if (species == null || species.firstResult() == null) {
@@ -308,7 +309,7 @@ private static String majorMinor(String version) {
     public String getVersionFromServer() throws IOException {
         if (serverVersion == null) {
             synchronized (this) {
-                ObjectMap result = retryMetaAbout(3);
+                ObjectMap result = retryMetaAbout();
                 if (result == null) {
                     throw new IOException("Unable to get version from server for cellbase " + toString());
                 }
@@ -322,12 +323,43 @@ public String getVersionFromServer() throws IOException {
         return serverVersion;
     }
 
-    private ObjectMap retryMetaAbout(int retries) throws IOException {
-        ObjectMap result = cellBaseClient.getMetaClient().about().firstResult();
-        if (result == null && retries > 0) {
-            // Retry
-            logger.warn("Unable to get version from server for cellbase " + toString() + ". Retrying...");
-            result = retryMetaAbout(retries - 1);
+    private ObjectMap retryMetaAbout() throws IOException {
+        return retry(3, () -> cellBaseClient.getMetaClient().about().firstResult());
+    }
+
+    private CellBaseDataResponse<SpeciesProperties> retryMetaSpecies() throws IOException {
+        return retry(3, () -> cellBaseClient.getMetaClient().species());
+    }
+
+    private <T> T retry(int retries, Callable<T> function) throws IOException {
+        if (retries <= 0) {
+            return null;
+        }
+        T result = null;
+        Exception e = null;
+        try {
+            result = function.call();
+        } catch (Exception e1) {
+            e = e1;
+        }
+        if (result == null) {
+            try {
+                // Retry
+                logger.warn("Unable to get reach cellbase " + toString() + ". Retrying...");
+                result = retry(retries - 1, function);
+            } catch (Exception e1) {
+                if (e == null) {
+                    e = e1;
+                } else {
+                    e.addSuppressed(e1);
+                }
+                if (e instanceof IOException) {
+                    throw (IOException) e;
+                } else {
+                    throw new IOException("Error reading from cellbase " + toString(), e);
+                }
+            }
+
         }
         return result;
     }

opencga-core/src/main/java/org/opencb/opencga/core/models/operations/variant/VariantFileDeleteParams.java

@@ -16,6 +16,7 @@
 
 package org.opencb.opencga.core.models.operations.variant;
 
+import org.opencb.commons.annotations.DataField;
 import org.opencb.opencga.core.tools.ToolParams;
 
 import java.util.List;
@@ -32,8 +33,13 @@ public VariantFileDeleteParams(List<String> file, boolean resume) {
         this.resume = resume;
     }
 
+    @DataField(description = "List of file ids to delete. Use 'all' to remove the whole study", required = true)
     private List<String> file;
+
+    @DataField(description = "Resume failed delete operation.", defaultValue = "false")
     private boolean resume;
+
+    @DataField(description = "Force delete operation. This would allow deleting partially loaded files.", defaultValue = "false")
     private boolean force;
 
     public List<String> getFile() {

opencga-master/src/main/java/org/opencb/opencga/master/monitor/daemons/ExecutionDaemon.java

@@ -205,7 +205,6 @@ public class ExecutionDaemon extends MonitorParentDaemon implements Closeable {
             put(RvtestsWrapperAnalysis.ID, "variant " + RvtestsWrapperAnalysis.ID + "-run");
             put(GatkWrapperAnalysis.ID, "variant " + GatkWrapperAnalysis.ID + "-run");
             put(ExomiserWrapperAnalysis.ID, "variant " + ExomiserWrapperAnalysis.ID + "-run");
-            put(VariantFileDeleteOperationTool.ID, "variant file-delete");
             put(VariantSecondaryAnnotationIndexOperationTool.ID, "variant secondary-index");
             put(VariantSecondaryIndexSamplesDeleteOperationTool.ID, "variant secondary-index-delete");
             put(VariantScoreDeleteOperationTool.ID, "variant score-delete");

opencga-storage/opencga-storage-app/src/main/java/org/opencb/opencga/storage/app/cli/client/options/StorageVariantCommandOptions.java

@@ -218,7 +218,10 @@ public static class GenericVariantDeleteOptions {
                 splitter = CommaParameterSplitter.class, required = true)
         public List<String> file = null;
 
-        @Parameter(names = {"--resume"}, description = "Resume a previously failed indexation")
+        @Parameter(names = {"--force"}, description = "Force delete operation. This would allow deleting partially loaded files.")
+        public boolean force;
+
+        @Parameter(names = {"--resume"}, description = "Resume failed delete operation.")
         public boolean resume;
     }