From a36c9dcef06f14d26850c2cf3f3a661aa303914b Mon Sep 17 00:00:00 2001 From: Joe Meiring Date: Fri, 16 Nov 2018 14:33:25 -0600 Subject: [PATCH] Fix for FileSystem blob store clearContainer with options --- .../FilesystemStorageStrategyImpl.java | 83 +++++++++++++++---- .../FilesystemContainerIntegrationTest.java | 4 - .../BaseContainerIntegrationTest.java | 27 ++++-- 3 files changed, 88 insertions(+), 26 deletions(-) diff --git a/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java b/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java index b557845273..a9f0a9cc5c 100644 --- a/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java +++ b/apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java @@ -27,6 +27,7 @@ import static java.nio.file.Files.getPosixFilePermissions; import static java.nio.file.Files.probeContentType; import static java.nio.file.Files.readAttributes; import static java.nio.file.Files.setPosixFilePermissions; +import static java.nio.file.Files.newDirectoryStream; import static org.jclouds.filesystem.util.Utils.delete; import static org.jclouds.filesystem.util.Utils.isPrivate; import static org.jclouds.filesystem.util.Utils.isWindows; @@ -41,6 +42,7 @@ import java.io.InputStream; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.nio.file.AccessDeniedException; +import java.nio.file.DirectoryStream; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributes; @@ -58,6 +60,7 @@ import javax.inject.Inject; import javax.inject.Named; import javax.inject.Provider; +import com.google.common.base.Strings; import org.jclouds.blobstore.ContainerNotFoundException; import org.jclouds.blobstore.KeyNotFoundException; import org.jclouds.blobstore.LocalStorageStrategy; @@ -253,16 +256,45 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { @Override public void clearContainer(String container, ListContainerOptions options) { filesystemContainerNameValidator.validate(container); - // TODO: these require calling removeDirectoriesTreeOfBlobKey - checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix"); + checkArgument(options.getDir() == null || options.getPrefix() == null, "cannot specify both directory and prefix"); + String optsPrefix = Strings.nullToEmpty(options.getDir() == null ? options.getPrefix() : options.getDir()); + String normalizedOptsPath = normalize(optsPrefix); + String basePath = buildPathStartingFromBaseDir(container, normalizedOptsPath); + filesystemBlobKeyValidator.validate(basePath); try { - File containerFile = openFolder(container); - File[] children = containerFile.listFiles(); - if (null != children) { - for (File child : children) - if (options.isRecursive() || child.isFile()) { - Utils.deleteRecursively(child); + File object = new File(basePath); + if (object.isFile()) { + // To mimic the S3 type blobstores, a prefix for an object blob + // should also get deleted + delete(object); + } + else if (object.isDirectory() && (optsPrefix.endsWith(File.separator) || isNullOrEmpty(optsPrefix))) { + // S3 blobstores will only match prefixes that end with a trailing slash/file separator + // For instance, if we have a blob at /path/1/2/a, a prefix of /path/1/2 will not list /path/1/2/a + // but a prefix of /path/1/2/ will + File containerFile = openFolder(container + File.separator + normalizedOptsPath); + File[] children = containerFile.listFiles(); + if (null != children) { + for (File child : children) { + if (options.isRecursive()) { + Utils.deleteRecursively(child); + } else { + if (child.isFile()) { + Utils.delete(child); + } + } } + } + + // Empty dirs in path if they don't have any objects + if (!optsPrefix.isEmpty()) { + if (options.isRecursive()) { + //first, remove the empty dir. It should be totally empty if it was a + // recursive delete + deleteDirectory(container, optsPrefix); + } + removeDirectoriesTreeOfBlobKey(container, optsPrefix); + } } } catch (IOException e) { logger.error(e, "An error occurred while clearing container %s", container); @@ -858,6 +890,18 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { return pathToBeCleaned.substring(beginIndex, endIndex); } + /** + * Checks if a directory is empty using a DirectoryStream iterator + * + * @param directoryPath + */ + private boolean isDirEmpty(String directoryPath) throws IOException { + Path path = new File(directoryPath).toPath(); + try (DirectoryStream dirStream = newDirectoryStream(path)) { + return !dirStream.iterator().hasNext(); + } + } + /** * Removes recursively the directory structure of a complex blob key, only if the directory is * empty @@ -889,16 +933,21 @@ public class FilesystemStorageStrategyImpl implements LocalStorageStrategy { logger.debug("Could not look for attributes from %s: %s", directory, e); } - String[] children = directory.list(); - if (null == children || children.length == 0) { - try { - delete(directory); - } catch (IOException e) { - logger.debug("Could not delete %s: %s", directory, e); - return; + // Don't need to do a listing on the dir, which could be costly. The iterator should be more performant. + try { + if (isDirEmpty(directory.getPath())) { + try { + delete(directory); + } catch (IOException e) { + logger.debug("Could not delete %s: %s", directory, e); + return; + } + // recursively call for removing other path + removeDirectoriesTreeOfBlobKey(container, parentPath); } - // recursively call for removing other path - removeDirectoriesTreeOfBlobKey(container, parentPath); + } catch (IOException e) { + logger.debug("Could not locate directory %s", directory, e); + return; } } } diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java index c9fee7356a..45f88d0193 100644 --- a/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java +++ b/apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java @@ -194,8 +194,4 @@ public class FilesystemContainerIntegrationTest extends BaseContainerIntegration throw new SkipException("filesystem does not support anonymous access"); } - @Override - public void testClearWithOptions() throws InterruptedException { - throw new SkipException("filesystem does not support clear with options"); - } } diff --git a/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java b/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java index 6364c984c8..0c7b50378b 100644 --- a/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java +++ b/blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java @@ -182,6 +182,8 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { try { ListContainerOptions options; + // Should wipe out all objects, as there are empty folders + // above add5NestedBlobsToContainer(containerName); options = new ListContainerOptions(); options.prefix("path/1/"); @@ -195,7 +197,9 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { options.prefix("path/1/2/3"); options.recursive(); view.getBlobStore().clearContainer(containerName, options); - assertConsistencyAwareContainerSize(containerName, 2); + assertConsistencyAwareBlobExists(containerName, "path/1/a"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/b"); + assertConsistencyAwareBlobDoesntExist(containerName, "path/1/2/3"); view.getBlobStore().clearContainer(containerName); add5NestedBlobsToContainer(containerName); @@ -203,7 +207,10 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { options.prefix("path/1/2/3/4/"); options.recursive(); view.getBlobStore().clearContainer(containerName, options); - assertConsistencyAwareContainerSize(containerName, 4); + assertConsistencyAwareBlobExists(containerName, "path/1/a"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/b"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/3/5/e"); + assertConsistencyAwareBlobDoesntExist(containerName, "path/1/2/3/4"); // non-recursive, should not clear anything, as prefix does not match view.getBlobStore().clearContainer(containerName); @@ -211,7 +218,11 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { options = new ListContainerOptions(); options.prefix("path/1/2/3"); view.getBlobStore().clearContainer(containerName, options); - assertConsistencyAwareContainerSize(containerName, 5); + assertConsistencyAwareBlobExists(containerName, "path/1/a"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/b"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/3/c"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/3/5/e"); + // non-recursive, should only clear path/1/2/3/c view.getBlobStore().clearContainer(containerName); @@ -219,7 +230,10 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { options = new ListContainerOptions(); options.prefix("path/1/2/3/"); view.getBlobStore().clearContainer(containerName, options); - assertConsistencyAwareContainerSize(containerName, 4); + assertConsistencyAwareBlobExists(containerName, "path/1/a"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/b"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/3/4/d"); + assertConsistencyAwareBlobDoesntExist(containerName, "path/1/2/3/c"); // non-recursive, should only clear path/1/2/3/c view.getBlobStore().clearContainer(containerName); @@ -227,7 +241,10 @@ public class BaseContainerIntegrationTest extends BaseBlobStoreIntegrationTest { options = new ListContainerOptions(); options.prefix("path/1/2/3/c"); view.getBlobStore().clearContainer(containerName, options); - assertConsistencyAwareContainerSize(containerName, 4); + assertConsistencyAwareBlobExists(containerName, "path/1/a"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/b"); + assertConsistencyAwareBlobExists(containerName, "path/1/2/3/4/d"); + assertConsistencyAwareBlobDoesntExist(containerName, "path/1/2/3/c"); } finally { returnContainer(containerName); }