jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Gaul <notificati...@github.com>
Subject Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store. (#1121)
Date Wed, 02 Aug 2017 16:29:57 GMT
andrewgaul requested changes on this pull request.



> @@ -266,7 +266,8 @@ public StorageMetadata apply(String key) {
       if (options != null) {
          if (options.getDir() != null && !options.getDir().isEmpty()) {
             contents = filterDirectory(contents, options);
-         } else if (options.getPrefix() != null) {
+         } else if (options.getPrefix() != null &&

`Strings.isNullOrEmpty`

> @@ -688,4 +691,26 @@ public void testUpdateObjectCannedACL() throws Exception {
          returnContainer(containerName);
       }
    }
+
+   public void testList_EmptyOptionSingleContainer() throws Exception {
+      String containerName = getContainerName();
+      try {
+         S3Object object = getApi().newS3Object();
+         object.getMetadata().setKey("a");
+         object.setPayload(TEST_STRING);
+         getApi().putObject(containerName, object);
+         // Test listing where we set the prefix and delimiter to empty string
+         ListBucketResponse rs = getApi().listBucket(containerName,
+               ListBucketOptions.Builder.delimiter("").withPrefix("").afterMarker(""));
+         assertEquals(rs.size(), 3);
+         Set<String> expected = Sets.newHashSet("a");

This s3 test is similar to the filesystem test -- should you add one test to `BaseContainerIntegrationTest`
instead of duplicating code?

> @@ -182,6 +182,25 @@ public void testList_NoOptionSingleContainer() throws IOException
{
         checkForContainerContent(CONTAINER_NAME, blobsExpected);
     }
 
+    public void testList_EmptyOptionSingleContainer() throws IOException {
+       blobStore.createContainerInLocation(null, CONTAINER_NAME);
+
+       checkForContainerContent(CONTAINER_NAME, null);
+
+       TestUtils.createBlobsInContainer(CONTAINER_NAME, "a", "b", "c");
+       // Test listing where we set the prefix and delimiter to empty string

Make this a method javadoc instead?

> @@ -182,6 +182,25 @@ public void testList_NoOptionSingleContainer() throws IOException
{
         checkForContainerContent(CONTAINER_NAME, blobsExpected);
     }
 
+    public void testList_EmptyOptionSingleContainer() throws IOException {
+       blobStore.createContainerInLocation(null, CONTAINER_NAME);
+
+       checkForContainerContent(CONTAINER_NAME, null);
+
+       TestUtils.createBlobsInContainer(CONTAINER_NAME, "a", "b", "c");
+       // Test listing where we set the prefix and delimiter to empty string
+       ListContainerOptions options = ListContainerOptions.Builder.delimiter("")
+             .prefix("").afterMarker("");
+       PageSet<? extends StorageMetadata> rs = blobStore.list(CONTAINER_NAME, options);
+       assertEquals(rs.size(), 3);
+       Set<String> expected = Sets.newHashSet("a", "b", "c");
+       for (StorageMetadata sm : rs) {
+          assertTrue(expected.contains(sm.getName()));
+          expected.remove(sm.getName());

Iterating while removing requires use of `Iterator.remove` to avoid invalidating the iterator.
 Instead could you use assertj:

```java
ImmutableList.Builder<String> builder = ImmutableList.builder();
for (StorageMetadata sm : rs) {
    builder.add(sm.getName());
}
assertThat(builder.build()).containsExactly(expected);
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1121#pullrequestreview-53853763
Mime
View raw message