Recurse in subdirectories before deleting blobs

This ensures that we have at most one PageSet of Futures awaiting
completion.

blobstore/src/main/java/org/jclouds/blobstore/strategy/internal/DeleteAllKeysInList.java

@@ -93,9 +93,9 @@ public class DeleteAllKeysInList implements ClearListStrategy, ClearContainerStr
          message = message + " recursively";
       Map<StorageMetadata, Exception> exceptions = Maps.newHashMap();
       PageSet<? extends StorageMetadata> listing;
-      Iterable<? extends StorageMetadata> toDelete;
       int maxErrors = 3; // TODO parameterize
       for (int i = 0; i < maxErrors; ) {
+         // fetch partial directory listing
          try {
             listing = connection.list(containerName, options).get();
          } catch (ExecutionException ee) {
@@ -108,36 +108,52 @@ public class DeleteAllKeysInList implements ClearListStrategy, ClearContainerStr
          } catch (InterruptedException ie) {
             throw Throwables.propagate(ie);
          }
-         toDelete = filterListing(listing, options);
 
-         Map<StorageMetadata, Future<?>> responses = Maps.newHashMap();
+         // recurse on subdirectories
-         try {
+         if (options.isRecursive()) {
-            for (final StorageMetadata md : toDelete) {
+            for (StorageMetadata md : listing) {
                String fullPath = parentIsFolder(options, md) ? options.getDir() + "/"
                         + md.getName() : md.getName();
                switch (md.getType()) {
                   case BLOB:
-                     responses.put(md, connection.removeBlob(containerName, fullPath));
                      break;
                   case FOLDER:
-                     if (options.isRecursive() && !fullPath.equals(options.getDir())) {
-                        execute(containerName, options.clone().inDirectory(fullPath));
-                     }
-                     responses.put(md, connection.deleteDirectory(containerName, fullPath));
-                     break;
                   case RELATIVE_PATH:
                      if (options.isRecursive() && !fullPath.equals(options.getDir())) {
                         execute(containerName, options.clone().inDirectory(fullPath));
                      }
-                     responses.put(md, connection.deleteDirectory(containerName, md.getName()));
                      break;
                   case CONTAINER:
                      throw new IllegalArgumentException("Container type not supported");
                }
             }
-         } finally {
-            exceptions = awaitCompletion(responses, userExecutor, maxTime, logger, message);
          }
+
+         // remove blobs and now-empty subdirectories
+         Map<StorageMetadata, Future<?>> responses = Maps.newHashMap();
+         for (StorageMetadata md : listing) {
+            String fullPath = parentIsFolder(options, md) ? options.getDir() + "/"
+                     + md.getName() : md.getName();
+            switch (md.getType()) {
+               case BLOB:
+                  responses.put(md, connection.removeBlob(containerName, fullPath));
+                  break;
+               case FOLDER:
+                  if (options.isRecursive()) {
+                     responses.put(md, connection.deleteDirectory(containerName, fullPath));
+                  }
+                  break;
+               case RELATIVE_PATH:
+                  if (options.isRecursive()) {
+                     responses.put(md, connection.deleteDirectory(containerName, md.getName()));
+                  }
+                  break;
+               case CONTAINER:
+                  throw new IllegalArgumentException("Container type not supported");
+            }
+         }
+
+         exceptions = awaitCompletion(responses, userExecutor, maxTime, logger, message);
          if (!exceptions.isEmpty()) {
             ++i;
             retryHandler.imposeBackoffExponentialDelay(i, message);
@@ -157,29 +173,4 @@ public class DeleteAllKeysInList implements ClearListStrategy, ClearContainerStr
    private boolean parentIsFolder(final ListContainerOptions options, final StorageMetadata md) {
       return (options.getDir() != null && md.getName().indexOf('/') == -1);
    }
-
-   private Iterable<? extends StorageMetadata> filterListing(
-            final PageSet<? extends StorageMetadata> listing,
-            final ListContainerOptions options) {
-      Iterable<? extends StorageMetadata> toDelete = Iterables.filter(listing,
-               new Predicate<StorageMetadata>() {
-
-         @Override
-         public boolean apply(StorageMetadata input) {
-            switch (input.getType()) {
-               case BLOB:
-                  return true;
-               case FOLDER:
-               case RELATIVE_PATH:
-                  if (options.isRecursive())
-                     return true;
-                  break;
-            }
-            return false;
-         }
-
-      });
-      return toDelete;
-   }
-
 }

blobstore/src/test/java/org/jclouds/blobstore/strategy/internal/DeleteAllKeysInListTest.java

@@ -23,8 +23,8 @@ import static org.testng.Assert.assertEquals;
 import org.jclouds.ContextBuilder;
 import org.jclouds.blobstore.BlobStore;
 import org.jclouds.blobstore.options.ListContainerOptions;
-import org.testng.annotations.AfterClass;
+import org.testng.annotations.AfterMethod;
-import org.testng.annotations.BeforeClass;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
 import com.google.common.io.Closeables;
@@ -38,40 +38,60 @@ import com.google.inject.Injector;
 public class DeleteAllKeysInListTest {
    private BlobStore blobstore;
    private DeleteAllKeysInList deleter;
+   private static final String containerName = "container";
+   private static final String directoryName = "directory";
 
-   @BeforeClass
+   @BeforeMethod
    void setupBlobStore() {
       Injector injector = ContextBuilder.newBuilder("transient").buildInjector();
       blobstore = injector.getInstance(BlobStore.class);
       deleter = injector.getInstance(DeleteAllKeysInList.class);
+      createDataSet();
+   }
+
+   @AfterMethod
+   void close() {
+      Closeables.closeQuietly(blobstore.getContext());
    }
 
    public void testExecuteWithoutOptionsClearsRecursively() {
-      blobstore.createContainerInLocation(null, "goodies");
+      deleter.execute(containerName);
-      for (int i = 0; i < 1001; i++) {
+      assertEquals(blobstore.countBlobs(containerName), 0);
-         blobstore.putBlob("goodies", blobstore.blobBuilder(i + "").payload(i + "").build());
+   }
-      }
+
-      assertEquals(blobstore.countBlobs("goodies"), 1001);
+   public void testExecuteRecursive() {
-      deleter.execute("goodies");
+      deleter.execute(containerName, ListContainerOptions.Builder.recursive());
-      assertEquals(blobstore.countBlobs("goodies"), 0);
+      assertEquals(blobstore.countBlobs(containerName), 0);
    }
 
    public void testExecuteNonRecursive() {
-      blobstore.createContainerInLocation(null, "foo");
+      deleter.execute(containerName, ListContainerOptions.NONE);
-      for (int i = 0; i < 1001; i++) {
+      assertEquals(blobstore.countBlobs(containerName), 2222);
-         blobstore.putBlob("foo", blobstore.blobBuilder(i + "").payload(i + "").build());
-      }
-      for (int i = 0; i < 1001; i++) {
-         blobstore.putBlob("foo", blobstore.blobBuilder("dir/" + i + "").payload(i + "").build());
-      }
-      assertEquals(blobstore.countBlobs("foo"), 2002);
-      deleter.execute("foo", ListContainerOptions.Builder.inDirectory("dir"));
-      assertEquals(blobstore.countBlobs("foo"), 1001);
    }
 
-   @AfterClass
+   public void testExecuteInDirectory() {
-   void close() {
+      deleter.execute(containerName, ListContainerOptions.Builder.inDirectory(directoryName));
-      if (blobstore != null)
+      assertEquals(blobstore.countBlobs(containerName), 1111);
-         Closeables.closeQuietly(blobstore.getContext());
+   }
+
+   /**
+    * Create a container "container" with 1111 blobs named "blob-%d".  Create a
+    * subdirectory "directory" which contains 2222 more blobs named
+    * "directory/blob-%d".
+    */
+   private void createDataSet() {
+      String blobNameFmt = "blob-%d";
+      String directoryBlobNameFmt = "%s/blob-%d";
+
+      blobstore.createContainerInLocation(null, containerName);
+      for (int i = 0; i < 1111; i++) {
+         String blobName = String.format(blobNameFmt, i);
+         blobstore.putBlob(containerName, blobstore.blobBuilder(blobName).payload(blobName).build());
+      }
+      for (int i = 0; i < 2222; i++) {
+         String directoryBlobName = String.format(directoryBlobNameFmt, directoryName, i);
+         blobstore.putBlob(containerName, blobstore.blobBuilder(directoryBlobName).payload(directoryBlobName).build());
+      }
+      assertEquals(blobstore.countBlobs(containerName), 3333);
    }
 }