Support the delimiter parameter and prefixes result field for bucket listing

[dimo414]

See delimiter - this is necessary to be able to list "directories".

In addition to BlobListOption.delimiter(String) it'd be nice if there were an argument-less method that uses / as the delimiter, since that's what ~99% of usages will want. I'd suggest directories() as that method name, so users can write bucket.list("...", BlobListOption.directories()).

[dimo414]

Actually, I just realized this is what BlobListOption.recursive() (which is, imo, not a very clear name) is supposed to do. However that method treats the delimiter argument as a boolean; instead it should be passing in /. As is, I don't think it does anything meaningful. Given that the method is broken, I'd vote to rename it directories() which I think more clearly conveys its intent.

[mziccard]

Hi @dimo414, thanks again for your feedback.
You can set the delimiter through Storage's global options:

StorageOptions options = StorageOptions.builder().pathDelimiter("/").build();

By default "/" is used.
When you use BlobListOption.recursive(true) we take care of setting the delimiter option to the pathDelimiter you specified when you created the StorageOptions.

I agree that the option's name is counter-intuitive. In fact when set to BlobListOption.recursive(true) it does NOT list the content of subdirectories. We should probably make it such that BlobListOption.recursive(false) sets the delimiter option, or change the name. /cc @aozarov

[dimo414]

Playing around with the API Explorer on the list doc page, I see my issue. I am trying to list "directories" which I now see are returned by the API in the prefixes field, not as part of the items field. I don't see a way to get the prefixes (unless, of course, I'm missing something):

Without a delimiter:

GET https://www.googleapis.com/storage/v1/b/my-bucket/o?prefix=dir%2F&fields=items%2Fname%2Ckind%2Cprefixes&key={YOUR_API_KEY}
{
 "kind": "storage#objects",
 "items": [
  {
   "name": "dir/1/foo"
  },
  {
   "name": "dir/2/bar"
  }
 ]
}

With delimiter /:

GET https://www.googleapis.com/storage/v1/b/my-bucket/o?delimiter=%2F&prefix=dir%2F&fields=items%2Fname%2Ckind%2Cprefixes&key={YOUR_API_KEY}
{
 "kind": "storage#objects",
 "prefixes": [
  "dir/1/",
  "dir/2/"
 ]
}

Is there a way to get dir/1/ and dir/2/ back via the gcloud-java API?

I'd agree changing the name of BlobListOption.recursive() would be helpful; it's totally unclear what it does presently.

[aozarov]

Given that the method is broken, I'd vote to rename it directories() which I think more clearly conveys its intent.

Yes, I think we got it wrong here, and instead it should set the DELIMITER when "recursive" is false.

I am fine with renaming (the name now is consistent with the appengine_gcs_client),
however I don't think directories is a better name (especially because, as @dimo414 mentioned,
we only return blobs and don't have a notion of "directories").

Honestly, I am not sure we should add a notion of a directory in this layer (as such notion does not really exists in the underlying service) and string prefixes is a strange beast. Emulating directories is included, ,for example, in the nio contrib. I am open for suggestions though.

[dimo414]

You're right, directories() doesn't make sense now that I understand what it's actually doing. But there's an abstraction issue with calling it recursive() either - it's not actually "recursing" anything unless you pretend it's recursing over a directory structure, and if you pretend there's a directory structure it makes sense to want to have a notion of a directory :)

I do think exposing the prefixes field the API returns in some way would be helpful (I, at least, need it in order to display what sets of files are available to the user). Could it just be another method on Page?

[aozarov]

Yes, we could change blob listing to return a subclass of Page that also has an accessor for prefixes.
However, that is not going to play very great with iterateAll (maybe we can live with that).

Another, more "extreme" option, would be to create a Blob instance also for prefixes with no other
values than their name (suffixed with the delimiter) and their bucket.

Thoughts? @dimo414 @mziccard @BrandonY

[dimo414]

Here's my workaround until there's a way to access the prefixes field; hopefully this demonstrates why it would be beneficial - without that field it's an O(objects) operation to compute the "subdirectories" in a "directory".

public static List<String> directories(Bucket bucket, String prefix) {
    checkArgument(prefix.endsWith("/"), "Prefix must end with a '/', was '%s'", prefix);
    ImmutableSet.Builder<String> builder = ImmutableSet.builder();
    Iterator<Blob> iter = bucket.list(BlobListOption.prefix(prefix)).iterateAll();
    while (iter.hasNext()) {
        String path = iter.next().name();
        String tail = path.replaceFirst("^" + prefix, "");
        int nextSlash = tail.indexOf('/');
        builder.add(nextSlash == -1 ? tail : tail.substring(0, nextSlash));
    }
    return builder.build().asList();
}

[mziccard]

Yes, we could change blob listing to return a subclass of Page that also has an accessor for prefixes. However, that is not going to play very great with iterateAll (maybe we can live with that).

This could be a way of doing that but the behavior of iterateAll becomes a bit unexpected. Also I did a check with both prefix and maxResults options set, what happens is that the sizes of prefixes and items JSON arrays always sum up to maxResults (except for the last page) which makes me think they should be iterated as a whole.

Another, more "extreme" option, would be to create a Blob instance also for prefixes with no other
values than their name (suffixed with the delimiter) and their bucket.

This would be a good option but what happens if we then call delete, update or some other method on the prefix blob?

[BrandonY]

I lean towards leaving the List() call ignorant of delimiters, since they're a fairly uncommon use case outside of building user interfaces and otherwise exploring bucket structure. It does seem reasonable to expose the functionality, though, so perhaps introduce a ListDirectory-type method that assumes a '/' delimiter and returns either a Page structure or an Iterator (where ListEntry contains either a Directory or a Blob).

[dimo414]

But the list() call is already aware of delimiters via the .recursive() option. Unfortunately it's only half-aware, you can list objects but not directories.

[aozarov]

This would be a good option but what happens if we then call delete, update or some other method on the prefix blob?

API will treat it as any other blob, so delete is likely to return false, get would return null,...
If using a delimiter is a non common option, maybe a doc on its settings (see bellow) would be sufficient.

If we think that it is uncommon then I suggest to remove delimiter from the storage options, remove "recursive" (which would be the default mode) and replace it with an explicit option for using the "non-recursive" option, I would actually call it delimiter (like the underlying API) or something more explicit such as simulateDirectories (and document that in this case directories are simulated as a Blob with only name, suffixed with the delimiter, and buckets are populated). Because of being explicit and because with think that this is not going to be a common setting I think that should be enough.

[mziccard]

API will treat it as any other blob, so delete is likely to return false, get would return null,...

Yes my doubt was in fact that this might be confusing as we have no way to distinguish between blobs and directories other than the delimiter suffix. If you think that documenting this is enough (I agree since it seems to be an uncommon usage) then we can go with the solution you propose. I'll take care of this change.

[aozarov]

Yes, that would be my vote. @mziccard I take it that is your vote too?

Summary of my reasoning:

1. Not a common setting.
2. Explicit and therefore could be well documented setting to enable this mode/behavior.

Do you think otherwise or have a different proposal @dimo414 @BrandonY ?