From fdf290423f52554b94c0a98d78a17addef3635de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jacobo=20Coll=20Morag=C3=B3n?= Date: Thu, 22 Aug 2024 10:53:47 +0100 Subject: [PATCH] storage: Add tests for variant-file-delete `--force`. #TASK-6737 --- .../VariantDeleteOperationManager.java | 15 +++++- .../AbstractVariantOperationManagerTest.java | 6 ++- .../operations/RemoveVariantsTest.java | 46 ++++++++++++++++- .../core/cellbase/CellBaseValidator.java | 50 +++++++++++++++---- 4 files changed, 104 insertions(+), 13 deletions(-) diff --git a/opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/manager/operations/VariantDeleteOperationManager.java b/opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/manager/operations/VariantDeleteOperationManager.java index bc3ba9cb2ab..c0f57f0b04f 100644 --- a/opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/manager/operations/VariantDeleteOperationManager.java +++ b/opencga-analysis/src/main/java/org/opencb/opencga/analysis/variant/manager/operations/VariantDeleteOperationManager.java @@ -27,6 +27,8 @@ 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; @@ -34,6 +36,8 @@ public class VariantDeleteOperationManager extends OperationManager { + private final Logger logger = LoggerFactory.getLogger(VariantDeleteOperationManager.class); + public VariantDeleteOperationManager(VariantStorageManager variantStorageManager, VariantStorageEngine engine) { super(variantStorageManager, engine); } @@ -65,6 +69,12 @@ public void removeFile(String study, List inputFiles, URI outdir, String // Might be partially loaded in VariantStorage. Check FileMetadata 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 @@ -74,8 +84,9 @@ public void removeFile(String study, List 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()); diff --git a/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/AbstractVariantOperationManagerTest.java b/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/AbstractVariantOperationManagerTest.java index 6063e8f1070..63b6ab090a3 100644 --- a/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/AbstractVariantOperationManagerTest.java +++ b/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 { diff --git a/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/RemoveVariantsTest.java b/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/RemoveVariantsTest.java index 74014489902..e5e190328b5 100644 --- a/opencga-analysis/src/test/java/org/opencb/opencga/analysis/variant/manager/operations/RemoveVariantsTest.java +++ b/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 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 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(), diff --git a/opencga-core/src/main/java/org/opencb/opencga/core/cellbase/CellBaseValidator.java b/opencga-core/src/main/java/org/opencb/opencga/core/cellbase/CellBaseValidator.java index 4dd5543e9b8..e6b7f7353c8 100644 --- a/opencga-core/src/main/java/org/opencb/opencga/core/cellbase/CellBaseValidator.java +++ b/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 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 retryMetaSpecies() throws IOException { + return retry(3, () -> cellBaseClient.getMetaClient().species()); + } + + private T retry(int retries, Callable 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; }