You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Timur Alperovich <no...@github.com> on 2017/07/18 08:01:20 UTC
[jclouds/jclouds] Handle empty delimiter/prefix in FS store. (#1121)
When delimiter/prefix is an empty string, jclouds filesystem blobstore
should treat them as not being set.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1121
-- Commit Summary --
* Handle empty delimiter/prefix in FS store.
-- File Changes --
M apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java (19)
M apis/s3/src/test/java/org/jclouds/s3/S3ClientLiveTest.java (29)
M blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1121.patch
https://github.com/jclouds/jclouds/pull/1121.diff
--
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
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
Pushed to master as d07c4a215e452c99f5ddae7fe006d801339d96e7 and 2.0.x as 8ab58f075f9bd9a77ed7fa27ab55c93f96db0aa0.
--
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#issuecomment-320099302
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> + /** Test that listing with an empty string for prefix and delimiter returns all of the keys. */
+ @Test(groups = {"integration", "live"})
+ public void testListEmptyPrefixDelimiter() throws Exception {
+ final String container = getContainerName();
+ BlobStore blobStore = view.getBlobStore();
+
+ try {
+ blobStore.createContainerInLocation(null, container);
+ ImmutableList<String> blobs = ImmutableList.of("a", "b", "c");
+ for (String blob : blobs) {
+ blobStore.putBlob(container, blobStore.blobBuilder(blob).payload("").build());
+ }
+ ListContainerOptions options = ListContainerOptions.Builder.delimiter("")
+ .prefix("").afterMarker("");
+ PageSet<? extends StorageMetadata> rs = blobStore.list(container, options);
+ assertEquals(rs.size(), 3);
Done
--
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#discussion_r131264676
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
Closed #1121.
--
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#event-1192226533
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
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
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Timur Alperovich <no...@github.com>.
Removed the spurious white space and import ordering changes.
--
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#issuecomment-319578968
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
Thanks for the review @andrewgaul! I reworked the tests to avoid code duplication -- thanks for the suggestions.
> @@ -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
Done
> @@ -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());
I'm not mutating the iterated container, though -- `rs` is the result set. Regardless, will rework to the suggested approach.
> @@ -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);
Sorry about that -- the test surely fails and I must have failed to run it and didn't catch this.
> @@ -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");
Yes -- will combine these. I wasn't sure about other providers' behavior, but I suspect it should either match or ignore the test.
> @@ -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 &&
Done
--
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-53887958
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul approved this pull request.
--
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-54230679
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Ignasi Barrera <no...@github.com>.
Thanks, @timuralp! Conceptually LGTM, but I lack a broad knowledge of the blobstore providers and I'd prefer @andrewgaul to have a look at the change.
--
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#issuecomment-316291066
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> @@ -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);
Further, this assert is wrong for this input.
--
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-53862611
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Timur Alperovich <no...@github.com>.
timuralp commented on this pull request.
> @@ -595,6 +596,32 @@ public void testListMarkerPrefix() throws Exception {
}
}
+ /** Test that listing with an empty string for prefix and delimiter returns all of the keys. */
+ @Test(groups = {"integration", "live"})
+ public void testListEmptyPrefixDelimiter() throws Exception {
+ final String container = getContainerName();
+ BlobStore blobStore = view.getBlobStore();
+
+ try {
+ blobStore.createContainerInLocation(null, container);
Done
--
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#discussion_r131264644
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> + /** Test that listing with an empty string for prefix and delimiter returns all of the keys. */
+ @Test(groups = {"integration", "live"})
+ public void testListEmptyPrefixDelimiter() throws Exception {
+ final String container = getContainerName();
+ BlobStore blobStore = view.getBlobStore();
+
+ try {
+ blobStore.createContainerInLocation(null, container);
+ ImmutableList<String> blobs = ImmutableList.of("a", "b", "c");
+ for (String blob : blobs) {
+ blobStore.putBlob(container, blobStore.blobBuilder(blob).payload("").build());
+ }
+ ListContainerOptions options = ListContainerOptions.Builder.delimiter("")
+ .prefix("").afterMarker("");
+ PageSet<? extends StorageMetadata> rs = blobStore.list(container, options);
+ assertEquals(rs.size(), 3);
This is not necessary since the `assertThat` call below also checks length.
--
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-53940228
Re: [jclouds/jclouds] Handle empty delimiter/prefix in FS store.
(#1121)
Posted by Andrew Gaul <no...@github.com>.
andrewgaul commented on this pull request.
> @@ -595,6 +596,32 @@ public void testListMarkerPrefix() throws Exception {
}
}
+ /** Test that listing with an empty string for prefix and delimiter returns all of the keys. */
+ @Test(groups = {"integration", "live"})
+ public void testListEmptyPrefixDelimiter() throws Exception {
+ final String container = getContainerName();
+ BlobStore blobStore = view.getBlobStore();
+
+ try {
+ blobStore.createContainerInLocation(null, container);
This should be outside the `try` block so that a failed call to `createContainerInLocation` does not return a bogus container in the `finally` block.
--
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-53940390