diff --git a/doc/release-notes/harden-clean-storage.md b/doc/release-notes/harden-clean-storage.md new file mode 100644 index 00000000000..6681dd50203 --- /dev/null +++ b/doc/release-notes/harden-clean-storage.md @@ -0,0 +1,8 @@ +The dataset storage cleanup call is now `PUT /api/datasets/{id}/cleanStorage`. It was a `GET`, which meant anything that follows links (browser prefetch, link previews in chat and mail clients, crawlers, or revisiting the URL from history) could trigger a deletion. + +Two further changes make it safer: + +- `dryrun` now defaults to `true`. Omitting it reports what would be removed instead of removing it. Pass `dryrun=false` to actually delete. +- Storage objects modified more recently than `dataverse.files.clean-storage-min-age-days` (7 by default) are never removed. An upload is written to the dataset's storage location before Dataverse registers it as a file, so without a grace period a completed upload still waiting to be saved was indistinguishable from an abandoned one and could be deleted. Raise the setting if uploads in your installation stay unregistered for longer than a week. + +Scripts calling this endpoint need to switch to `PUT` and, if they relied on the old default to delete, to pass `dryrun=false` explicitly. diff --git a/doc/sphinx-guides/source/api/native-api.rst b/doc/sphinx-guides/source/api/native-api.rst index e08777c9cf2..8db4ec4ec98 100644 --- a/doc/sphinx-guides/source/api/native-api.rst +++ b/doc/sphinx-guides/source/api/native-api.rst @@ -3462,8 +3462,14 @@ Cleanup Storage of a Dataset This is an experimental feature and should be tested on your system before using it in production. Also, make sure that your backups are up-to-date before using this on production servers. -It is advised to first call this method with the ``dryrun`` parameter set to ``true`` before actually deleting the files. -This will allow you to manually inspect the files that would be deleted if that parameter is set to ``false`` or is omitted (a list of the files that would be deleted is provided in the response). +This call only reports what it would remove unless ``dryrun`` is explicitly set to ``false``. Omitting the parameter is +the same as setting it to ``true``, so inspecting the reported list is always the default (a list of the files that would +be deleted is provided in the response). + +Storage objects that were modified more recently than ``dataverse.files.clean-storage-min-age-days`` (7 by default) are +never removed. An upload writes its object to the dataset's storage location before Dataverse registers it as a file, so +without that grace period a completed upload waiting to be saved would look the same as an abandoned one. Raise the +setting if your workflows leave uploads unregistered for longer than a week. If your Dataverse installation has been configured to support direct uploads, or in some other situations, you could end up with some files in the storage of a dataset that are not linked to that dataset directly. Most commonly, this could @@ -3479,13 +3485,13 @@ All the files stored in the Dataset storage location that are not in the file li export PERSISTENT_ID=doi:10.5072/FK2/J8SJZB export DRYRUN=true - curl -H "X-Dataverse-key: $API_TOKEN" -X GET "$SERVER_URL/api/datasets/:persistentId/cleanStorage?persistentId=$PERSISTENT_ID&dryrun=$DRYRUN" + curl -H "X-Dataverse-key: $API_TOKEN" -X PUT "$SERVER_URL/api/datasets/:persistentId/cleanStorage?persistentId=$PERSISTENT_ID&dryrun=$DRYRUN" The fully expanded example above (without environment variables) looks like this: .. code-block:: bash - curl -H "X-Dataverse-key: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" -X GET "https://demo.dataverse.org/api/datasets/:persistentId/cleanStorage?persistentId=doi:10.5072/FK2/J8SJZB&dryrun=true" + curl -H "X-Dataverse-key: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx" -X PUT "https://demo.dataverse.org/api/datasets/:persistentId/cleanStorage?persistentId=doi:10.5072/FK2/J8SJZB&dryrun=true" Adding Files To a Dataset via Other Tools ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java index e30be924903..d63024583e9 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Admin.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Admin.java @@ -68,6 +68,7 @@ import jakarta.ws.rs.core.Response; import static edu.harvard.iq.dataverse.util.json.NullSafeJsonBuilder.jsonObjectBuilder; +import java.time.Duration; import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.util.Collections; @@ -2957,7 +2958,9 @@ public Response getAuditFiles(@Context ContainerRequestContext crc, try { Predicate filter = s -> true; StorageIO datasetIO = DataAccess.getStorageIO(dataset); - final List result = datasetIO.cleanUp(filter, true); + // Auditing lists everything, so no minimum age applies here. The + // listing is not used; the call is kept for its existence checks. + datasetIO.cleanUp(filter, Duration.ZERO, true); // add files that are in dataset files but not in cleanup result or DataFiles with missing FileMetadata dataset.getFiles().forEach(df -> { try { diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java index 1b549b10a6b..c00ce3eaaef 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Datasets.java @@ -97,6 +97,7 @@ import java.sql.Timestamp; import java.text.MessageFormat; import java.text.SimpleDateFormat; +import java.time.Duration; import java.time.LocalDate; import java.time.LocalDateTime; import java.time.ZoneId; @@ -3410,11 +3411,14 @@ public Response addFileToDataset(@Context ContainerRequestContext crc, * @param idSupplied * @return */ - @GET + @PUT @AuthRequired @Path("{id}/cleanStorage") @Operation(summary = "Cleans dataset storage", - description = "Finds and optionally deletes storage objects that are no longer referenced by files in a dataset.") + description = "Finds and optionally deletes storage objects that are no longer referenced by files in a dataset. " + + "Reports without deleting unless dryrun is explicitly set to false. Objects modified more recently than " + + "dataverse.files.clean-storage-min-age-days are never removed, so that an upload which has completed but " + + "has not been registered yet is not mistaken for an abandoned one.") public Response cleanStorage(@Context ContainerRequestContext crc, @Parameter(description = "Resource id or persistent identifier.") @PathParam("id") String idSupplied, @Parameter(description = "Whether to validate the request without applying changes.") @QueryParam("dryrun") Boolean dryrun) { // get user and dataset User authUser = getRequestUser(crc); @@ -3428,10 +3432,11 @@ public Response cleanStorage(@Context ContainerRequestContext crc, @Parameter(de // check permissions if (!permissionSvc.permissionsFor(req, dataset).contains(Permission.EditDataset)) { - return error(Response.Status.INTERNAL_SERVER_ERROR, "Access denied!"); + return error(Response.Status.FORBIDDEN, "Access denied!"); } - boolean doDryRun = dryrun != null && dryrun.booleanValue(); + // Reporting is the default: deleting requires asking for it explicitly. + boolean doDryRun = dryrun == null || dryrun.booleanValue(); // check if no legacy files are present Set datasetFilenames = getDatasetFilenames(dataset); @@ -3443,7 +3448,7 @@ public Response cleanStorage(@Context ContainerRequestContext crc, @Parameter(de List deleted; try { StorageIO datasetIO = DataAccess.getStorageIO(dataset); - deleted = datasetIO.cleanUp(filter, doDryRun); + deleted = datasetIO.cleanUp(filter, getCleanStorageMinimumAge(), doDryRun); } catch (IOException ex) { logger.log(Level.SEVERE, null, ex); return error(Response.Status.INTERNAL_SERVER_ERROR, "IOException! Serious Error! See administrator!"); @@ -3499,6 +3504,10 @@ public Response getCompareVersionsSummary(@Context ContainerRequestContext crc, }, getRequestUser(crc)); } + private static Duration getCleanStorageMinimumAge() { + return Duration.ofDays(JvmSettings.CLEAN_STORAGE_MIN_AGE_DAYS.lookup(Integer.class)); + } + private static Set getDatasetFilenames(Dataset dataset) { Set files = new HashSet<>(); for (DataFile dataFile: dataset.getFiles()) { diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/AbstractRemoteOverlayAccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/AbstractRemoteOverlayAccessIO.java index 18269f6970e..dcfd4c9b59a 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/AbstractRemoteOverlayAccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/AbstractRemoteOverlayAccessIO.java @@ -9,6 +9,7 @@ import java.security.KeyManagementException; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; +import java.time.Duration; import java.util.List; import java.util.function.Predicate; import java.util.logging.Logger; @@ -213,8 +214,8 @@ public boolean exists() { } @Override - public List cleanUp(Predicate filter, boolean dryRun) throws IOException { - return baseStore.cleanUp(filter, dryRun); + public List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException { + return baseStore.cleanUp(filter, minimumAge, dryRun); } @Override diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java index e5006645b7f..ac86025caef 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIO.java @@ -32,10 +32,13 @@ import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; +import java.time.Duration; +import java.time.Instant; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.function.Predicate; import java.util.logging.Logger; -import java.util.stream.Collectors; // Dataverse imports: import edu.harvard.iq.dataverse.DataFile; @@ -676,7 +679,7 @@ protected static boolean isValidIdentifier(String driverId, String storageId) { return true; } - private List listAllFiles() throws IOException { + private Map listAllFiles() throws IOException { Dataset dataset = this.getDataset(); if (dataset == null) { throw new IOException("This FileAccessIO object hasn't been properly initialized."); @@ -689,10 +692,17 @@ private List listAllFiles() throws IOException { DirectoryStream dirStream = Files.newDirectoryStream(Paths.get(this.getFilesRootDirectory(), datasetDirectoryPath.toString())); - List res = new ArrayList<>(); + Map res = new HashMap<>(); if (dirStream != null) { for (Path filePath : dirStream) { - res.add(filePath.getFileName().toString()); + Instant lastModified; + try { + lastModified = Files.getLastModifiedTime(filePath).toInstant(); + } catch (IOException ex) { + // Unknown age is treated as too recent to remove. + lastModified = null; + } + res.put(filePath.getFileName().toString(), lastModified); } dirStream.close(); } @@ -716,8 +726,8 @@ private void deleteFile(String fileName) throws IOException { } @Override - public List cleanUp(Predicate filter, boolean dryRun) throws IOException { - List toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList()); + public List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException { + List toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge); if (dryRun) { return toDelete; } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/InputStreamIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/InputStreamIO.java index de392b74cca..87fcdb1cf3d 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/InputStreamIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/InputStreamIO.java @@ -13,6 +13,7 @@ import java.nio.channels.Channels; import java.nio.channels.WritableByteChannel; import java.nio.file.Path; +import java.time.Duration; import java.util.List; import java.util.function.Predicate; import java.util.logging.Logger; @@ -161,7 +162,7 @@ public void revertBackupAsAux(String auxItemTag) throws IOException { } @Override - public List cleanUp(Predicate filter, boolean dryRun) throws IOException { + public List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException { throw new UnsupportedDataAccessOperationException("InputStreamIO: tthis method is not supported in this DataAccess driver."); } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java index 4aa81284eea..bc5991c2b8f 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIO.java @@ -56,6 +56,8 @@ import java.nio.file.Paths; import java.time.Duration; import java.time.Instant; +import java.util.HashMap; +import java.util.Map; import java.util.ArrayList; import java.util.Collections; import java.util.Date; @@ -1517,7 +1519,7 @@ protected static boolean isValidIdentifier(String driverId, String storageId) { return true; } - private List listAllFiles() throws IOException { + private Map listAllFiles() throws IOException { if (!this.canWrite()) { open(); } @@ -1527,7 +1529,7 @@ private List listAllFiles() throws IOException { } String prefix = dataset.getAuthorityForFileStorage() + "/" + dataset.getIdentifierForFileStorage() + "/"; - List ret = new ArrayList<>(); + Map ret = new HashMap<>(); ListObjectsV2Request listObjectsReqManual = ListObjectsV2Request.builder().bucket(bucketName).prefix(prefix) .build(); @@ -1571,7 +1573,7 @@ private List listAllFiles() throws IOException { for (S3Object item : storedFilesSummary) { String fileName = item.key().substring(prefix.length()); - ret.add(fileName); + ret.put(fileName, item.lastModified()); } return ret; } @@ -1623,8 +1625,8 @@ public void closeInputStream() { } @Override - public List cleanUp(Predicate filter, boolean dryRun) throws IOException { - List toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList()); + public List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException { + List toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge); if (dryRun) { return toDelete; } diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java index b009c4e86ec..d91e9662770 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/StorageIO.java @@ -36,6 +36,8 @@ import java.nio.channels.WritableByteChannel; import java.nio.file.Path; import java.util.HashMap; +import java.time.Duration; +import java.time.Instant; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -641,7 +643,42 @@ protected static boolean usesStandardNamePattern(String identifier) { return m.find(); } - public abstract List cleanUp(Predicate filter, boolean dryRun) throws IOException; + /** + * Deletes stored objects that {@code filter} selects and that have not been modified + * for at least {@code minimumAge}. + * + * @param filter selects objects by name, typically those no longer referenced by the dataset + * @param minimumAge how long an object must have been untouched before it can be removed + * @param dryRun when true, report what would be removed without removing anything + */ + public abstract List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException; + + /** + * The stored objects that {@code filter} selects and that are old enough to + * remove. Shared by the storage backends so the selection rule has one + * definition. + * + * @param stored object names mapped to their last modification time + */ + protected static List selectForCleanUp(Map stored, Predicate filter, + Duration minimumAge) { + return stored.entrySet().stream() + .filter(e -> filter.test(e.getKey()) && isOlderThan(e.getValue(), minimumAge)) + .map(Map.Entry::getKey) + .toList(); + } + + /** + * Whether an object last modified at {@code lastModified} is old enough to remove. + * An unknown timestamp is treated as too recent, so that a storage backend which + * cannot report one never causes a deletion. + */ + protected static boolean isOlderThan(Instant lastModified, Duration minimumAge) { + if (lastModified == null) { + return false; + } + return lastModified.isBefore(Instant.now().minus(minimumAge)); + } /** * A storage-type-specific mechanism for retrieving the size of a file. Intended diff --git a/src/main/java/edu/harvard/iq/dataverse/dataaccess/SwiftAccessIO.java b/src/main/java/edu/harvard/iq/dataverse/dataaccess/SwiftAccessIO.java index 717f46ffd60..6f634b949d6 100644 --- a/src/main/java/edu/harvard/iq/dataverse/dataaccess/SwiftAccessIO.java +++ b/src/main/java/edu/harvard/iq/dataverse/dataaccess/SwiftAccessIO.java @@ -19,12 +19,15 @@ import java.security.SignatureException; import java.util.ArrayList; import java.util.Collection; +import java.time.Duration; +import java.time.Instant; +import java.util.HashMap; +import java.util.Map; import java.util.Formatter; import java.util.List; import java.util.Properties; import java.util.function.Predicate; import java.util.logging.Logger; -import java.util.stream.Collectors; import javax.crypto.Mac; import javax.crypto.spec.SecretKeySpec; @@ -912,7 +915,7 @@ public static String calculateRFC2104HMAC(String data, String key) return toHexString(mac.doFinal(data.getBytes())); } - private List listAllFiles() throws IOException { + private Map listAllFiles() throws IOException { if (!this.canWrite()) { open(DataAccessOption.WRITE_ACCESS); } @@ -924,12 +927,19 @@ private List listAllFiles() throws IOException { Collection items; String lastItemName = null; - List ret = new ArrayList<>(); + Map ret = new HashMap<>(); while ((items = this.swiftContainer.list(prefix, lastItemName, LIST_PAGE_LIMIT)) != null && items.size() > 0) { for (StoredObject item : items) { lastItemName = item.getName().substring(prefix.length()); - ret.add(lastItemName); + Instant lastModified; + try { + lastModified = item.getLastModifiedAsDate().toInstant(); + } catch (RuntimeException ex) { + // Unknown age is treated as too recent to remove. + lastModified = null; + } + ret.put(lastItemName, lastModified); } } @@ -956,8 +966,8 @@ private void deleteFile(String fileName) throws IOException { } @Override - public List cleanUp(Predicate filter, boolean dryRun) throws IOException { - List toDelete = this.listAllFiles().stream().filter(filter).collect(Collectors.toList()); + public List cleanUp(Predicate filter, Duration minimumAge, boolean dryRun) throws IOException { + List toDelete = selectForCleanUp(this.listAllFiles(), filter, minimumAge); if (dryRun) { return toDelete; } diff --git a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java index 8ed18b4c63f..7aaab1eebeb 100644 --- a/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java +++ b/src/main/java/edu/harvard/iq/dataverse/settings/JvmSettings.java @@ -57,6 +57,7 @@ public enum JvmSettings { FEATURED_ITEMS_IMAGE_UPLOADS_DIRECTORY(SCOPE_FEATURED_ITEMS, "image-uploads"), HIDE_SCHEMA_DOT_ORG_DOWNLOAD_URLS(SCOPE_FILES, "hide-schema-dot-org-download-urls"), DEFAULT_DATASET_FILE_COUNT_LIMIT(SCOPE_FILES, "default-dataset-file-count-limit"), + CLEAN_STORAGE_MIN_AGE_DAYS(SCOPE_FILES, "clean-storage-min-age-days"), //STORAGE DRIVER SETTINGS SCOPE_DRIVER(SCOPE_FILES), diff --git a/src/main/resources/META-INF/microprofile-config.properties b/src/main/resources/META-INF/microprofile-config.properties index b2ad2e176fc..816898937d9 100644 --- a/src/main/resources/META-INF/microprofile-config.properties +++ b/src/main/resources/META-INF/microprofile-config.properties @@ -19,6 +19,9 @@ dataverse.files.directory=${STORAGE_DIR:/tmp/dataverse} dataverse.files.uploads=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/uploads dataverse.files.docroot=${STORAGE_DIR:${com.sun.aas.instanceRoot}}/docroot dataverse.files.globus-cache-maxage=5 +# Storage objects younger than this are left alone by cleanStorage, so that an upload +# that has finished but has not been registered yet is not mistaken for junk. +dataverse.files.clean-storage-min-age-days=7 dataverse.files.featured-items.image-maxsize=1000000 dataverse.files.featured-items.image-uploads=featuredItems diff --git a/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java b/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java index ea5cc4b66a8..454a12857cb 100644 --- a/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/dataaccess/FileAccessIOTest.java @@ -20,6 +20,8 @@ import java.io.InputStream; import java.io.InputStreamReader; import java.nio.file.Path; +import java.time.Duration; +import java.util.function.Predicate; import java.util.ArrayList; import java.util.List; import org.apache.commons.io.FileUtils; @@ -317,4 +319,45 @@ public void testFileIdentifierFormats() throws IOException { System.clearProperty("dataverse.files.filetest.label"); System.clearProperty("dataverse.files.filetest.directory"); } + + private File stageOrphan(String name, Duration age) throws IOException { + File f = new File("/tmp/files/tmp/dataset/" + name); + f.createNewFile(); + assertTrue(f.setLastModified(System.currentTimeMillis() - age.toMillis())); + return f; + } + + private static final Predicate ORPHANS = name -> name.startsWith("orphan"); + + @Test + public void testCleanUp_leavesRecentlyModifiedFilesAlone() throws IOException { + File old = stageOrphan("orphan-old", Duration.ofDays(30)); + File fresh = stageOrphan("orphan-fresh", Duration.ofHours(2)); + + List deleted = datasetAccess.cleanUp(ORPHANS, Duration.ofDays(7), false); + + assertEquals(List.of("orphan-old"), deleted); + assertFalse(old.exists()); + assertTrue(fresh.exists(), "an upload that has not been registered yet must survive"); + } + + @Test + public void testCleanUp_dryRunReportsWithoutDeleting() throws IOException { + File old = stageOrphan("orphan-old", Duration.ofDays(30)); + + List reported = datasetAccess.cleanUp(ORPHANS, Duration.ofDays(7), true); + + assertEquals(List.of("orphan-old"), reported); + assertTrue(old.exists(), "a dry run must not delete anything"); + } + + @Test + public void testCleanUp_filterKeepsReferencedFiles() throws IOException { + File referenced = stageOrphan("kept-old", Duration.ofDays(30)); + + List deleted = datasetAccess.cleanUp(ORPHANS, Duration.ofDays(7), false); + + assertTrue(deleted.isEmpty()); + assertTrue(referenced.exists()); + } } diff --git a/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java b/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java index 5095c12024c..89b95c72452 100644 --- a/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/dataaccess/S3AccessIOTest.java @@ -25,6 +25,15 @@ import java.io.FileNotFoundException; import java.io.IOException; +import java.time.Duration; +import java.time.Instant; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.function.Predicate; +import software.amazon.awssdk.services.s3.model.DeleteObjectRequest; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Request; +import software.amazon.awssdk.services.s3.model.ListObjectsV2Response; +import software.amazon.awssdk.services.s3.model.S3Object; @ExtendWith(MockitoExtension.class) @MockitoSettings(strictness = Strictness.STRICT_STUBS) @@ -129,4 +138,32 @@ void testS3IdentifierFormats() throws IOException { //bad bucket assertFalse(DataAccess.isValidDirectStorageIdentifier("s3test://bucket:" + FileUtil.generateStorageIdentifier())); } -} \ No newline at end of file + + private static final Predicate ORPHANS = name -> name.startsWith("orphan"); + + private S3Object storedObject(String name, Duration age) { + String prefix = dataSet.getAuthorityForFileStorage() + "/" + dataSet.getIdentifierForFileStorage() + "/"; + return S3Object.builder().key(prefix + name).lastModified(Instant.now().minus(age)).build(); + } + + private void givenBucketContains(S3Object... objects) { + given(s3client.listObjectsV2(any(ListObjectsV2Request.class))) + .willReturn(CompletableFuture.completedFuture( + ListObjectsV2Response.builder().contents(List.of(objects)).build())); + } + + @Test + public void testCleanUp_dryRunSkipsRecentlyModifiedObjects() throws IOException { + // Skip open(): the injected client is already the one under test. + dataSetAccess.isWriteAccess = true; + givenBucketContains( + storedObject("orphan-old", Duration.ofDays(30)), + storedObject("orphan-fresh", Duration.ofHours(2)), + storedObject("referenced-old", Duration.ofDays(30))); + + List reported = dataSetAccess.cleanUp(ORPHANS, Duration.ofDays(7), true); + + assertEquals(List.of("orphan-old"), reported); + verify(s3client, never()).deleteObject(any(DeleteObjectRequest.class)); + } +} diff --git a/src/test/java/edu/harvard/iq/dataverse/dataaccess/StorageIOCleanUpAgeTest.java b/src/test/java/edu/harvard/iq/dataverse/dataaccess/StorageIOCleanUpAgeTest.java new file mode 100644 index 00000000000..7469950a1e5 --- /dev/null +++ b/src/test/java/edu/harvard/iq/dataverse/dataaccess/StorageIOCleanUpAgeTest.java @@ -0,0 +1,112 @@ +package edu.harvard.iq.dataverse.dataaccess; + +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.time.Instant; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.function.Predicate; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The age guard that keeps cleanUp from removing an upload that has finished but has + * not been registered as a DataFile yet. + */ +class StorageIOCleanUpAgeTest { + + private static final Duration ONE_WEEK = Duration.ofDays(7); + + @Test + void testIsOlderThan_unknownTimestampIsNeverRemoved() { + assertFalse(StorageIO.isOlderThan(null, ONE_WEEK)); + } + + @Test + void testIsOlderThan_justUploadedIsKept() { + assertFalse(StorageIO.isOlderThan(Instant.now(), ONE_WEEK)); + } + + @Test + void testIsOlderThan_withinGracePeriodIsKept() { + assertFalse(StorageIO.isOlderThan(Instant.now().minus(Duration.ofDays(6)), ONE_WEEK)); + } + + @Test + void testIsOlderThan_justInsideGracePeriodIsKept() { + assertFalse(StorageIO.isOlderThan(Instant.now().minus(Duration.ofDays(7)).plusSeconds(30), ONE_WEEK)); + } + + @Test + void testIsOlderThan_pastGracePeriodIsRemovable() { + assertTrue(StorageIO.isOlderThan(Instant.now().minus(Duration.ofDays(8)), ONE_WEEK)); + } + + @Test + void testIsOlderThan_longAbandonedIsRemovable() { + assertTrue(StorageIO.isOlderThan(Instant.now().minus(Duration.ofDays(365)), ONE_WEEK)); + } + + @Test + void testIsOlderThan_zeroGracePeriodRemovesAnythingWithATimestamp() { + assertTrue(StorageIO.isOlderThan(Instant.now().minusSeconds(1), Duration.ZERO)); + } + + private static Map stored(Object... nameThenAge) { + Map m = new HashMap<>(); + for (int i = 0; i < nameThenAge.length; i += 2) { + Duration age = (Duration) nameThenAge[i + 1]; + m.put((String) nameThenAge[i], age == null ? null : Instant.now().minus(age)); + } + return m; + } + + private static final Predicate ORPHANS = name -> name.startsWith("orphan"); + + @Test + void testSelectForCleanUp_takesOnlyOldOrphans() { + Map stored = stored( + "orphan-old", Duration.ofDays(30), + "orphan-fresh", Duration.ofHours(1), + "referenced-old", Duration.ofDays(30)); + + List selected = StorageIO.selectForCleanUp(stored, ORPHANS, ONE_WEEK); + + assertEquals(List.of("orphan-old"), selected); + } + + @Test + void testSelectForCleanUp_skipsUnknownTimestamps() { + List selected = StorageIO.selectForCleanUp(stored("orphan-a", null), ORPHANS, ONE_WEEK); + + assertTrue(selected.isEmpty()); + } + + @Test + void testSelectForCleanUp_emptyStoreSelectsNothing() { + assertTrue(StorageIO.selectForCleanUp(Map.of(), ORPHANS, ONE_WEEK).isEmpty()); + } + + @Test + void testSelectForCleanUp_zeroGraceStillHonoursTheFilter() { + Map stored = stored( + "orphan-a", Duration.ofSeconds(5), + "referenced-b", Duration.ofSeconds(5)); + + List selected = StorageIO.selectForCleanUp(stored, ORPHANS, Duration.ZERO); + + assertEquals(List.of("orphan-a"), selected); + } + + @Test + void testSelectForCleanUp_resultIsNotMeantToBeModified() { + List selected = StorageIO.selectForCleanUp(stored("orphan-a", Duration.ofDays(30)), ORPHANS, ONE_WEEK); + + assertThrows(UnsupportedOperationException.class, () -> selected.add("x")); + } +}