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 ?

[dimo414]

Apologies if I'm miss-reading the proposal, but I really don't think Blob should be overloaded to represent both actual blobs and path components. That just doesn't make sense, regardless of how easy to implement or rarely used it would be. I also don't agree with your premise that it's rarely used, it seems perfectly reasonable and common to want to inspect the hierarchy of your files.

More practically, it's also possible for there to be collisions between prefixes and actual blobs. For example when you hit "Create Folder" in the Storage Console it creates a 0-byte placeholder object. Providing a fake-Blob representing prefixes could easily collide with user's files.

[mziccard]

More practically, it's also possible for there to be collisions between prefixes and actual blobs.

I think this can never happen. Prefixes always finish with the delimiter string while blob names do not. For instance consider the case where you have two blobs test- and test-file. Listing the blobs using - as a delimiter will return the following result:

{
 "kind": "storage#objects",
 "prefixes": [
  "test-"
 ]
}

Which respects the documentation of the delimiter option:

Returns results in a directory-like mode. items will contain only objects whose names, aside from the prefix, do not contain delimiter. Objects whose names, aside from the prefix, contain delimiter will have their name, truncated after the delimiter, returned in prefixes.

@dimo414 To better understand, your preference here would be adding a getPrefixes method to Page?

[dimo414]

It wouldn't collide with other results from the request, but it would confusingly match both a valid Blob and a prefix. There's also nothing stopping a caller from storing a fake-Blob in a variable or collection intended for actual Blob instances and then triggering the wrong behavior. These just seem like totally separate concepts, and treating them as the same type is asking for trouble, or at least user confusion.

I think adding a getPrefixes() (I assume that's what you meant?) method to Page is the least invasive option, but I'd be fine with other alternatives like a new method on Bucket and Storage. Practically, I just need some way to get the prefixes the backing API reports. Philosophically I don't think re-purposing Blob is a good way to do that.

[aozarov]

@dimo414 The "premise that it's rarely used" comes from @BrandonY which I hope could back it up via server-side stats/monitoring.

We are talking about a service that does not have a notion of a folder (as a resource) and therefore I don't think that in the gcloud-java level we should deviate from it and create one.

Using Blob indeed feels like a "workaround" but on the other-hand it does make it clear that there
are no true folders. If this behavior (using a blob to represent a "folder") is only triggered by an explicit
setting that is well documentated then the "element of surprise" should not be an issue (in my opinion)
and people should know what to expect when they are dealing with the results.

Also, I think, the fact that the gcs tool is also using Blob with zero length to represent a "folder" should make it even less surprising.

I would be happy to hear also @BrandonY opinion about it.

I am also fine with the other suggestion of adding getPrefixes() to Page though this one also has 2 issues as mentioned above by @mziccard and myself (with iterateAll and fetchSize).

I don't think we should create a special method for it and/or special data structure to represent a folder.

Votes?

[mziccard]

I don't like the idea of adding getPrefixes to (a subclass of) Page.
Adding a special method to list prefixes is not a good idea either (this makes also complex handling pagination and setting maximum number of results as both currentPage and maxResults refer to the combination of prefixes and actual blobs).

That said I am +1 to the compromise proposed by @aozarov of listing prefixes as Blob objects where only bucket and name are set (and name always ends with the delimiter). I agree this should be well documented.

[Capstan]

First, I think the idea of pagination is an implementation detail that should be entirely hidden away from the users. Unless they are extremely interested in exactly having the data from a single server response, I would hide everything behind Iterators, and hide pages entirely.

I think you would want to have an option that offers iterators that are either backed by:

• a single outstanding request, and if next() is used off the end of a page, a new request to get more data happens
• repeated requests happening on another thread, so that the entire server list is eventually cached locally. Maybe this latter is simply a convenience Iterator backed by a Queue that reads from the above iterator until end.

I personally like the idea of offering a Collection at the bucket level, and offer some method that returns Collection at a prefix, so that you can emit storage.objects.list calls as well as do crazy things like bucket.objects().removeAll(bucket.objects("path/to/foo")), but perhaps that's for a different thread.

I could imagine offering both an objects and prefixes iterator, and letting either drive the pulling of additional pages from the server, at the cost if you do all iterators first, you will have a giant cache of objects. @BrandonY suggested we use a single iterator for both and have a union that is either a Blob or a prefix, which seems a little less fun (I like strong typing) at the cost of less weird caching.

[thobrla]

+1 for pagination being opaque and using iterators.

Supporting custom delimiters is not useful based on service usage. Everyone uses "/".

Supporting collections at a given prefix seems reasonable to me.

Intermingling Blobs and prefixes leads to pain.

[thobrla]

After some offline discussion with @aozarov , I'd clarify by saying that while supporting collections at a prefix seems reasonable to me, I think that "simulating directories" tends to lead to a lot of pain because it's fundamentally a broken abstraction.

Thus, it seems like it should be handled in a layer above gcloud-java, if at all.

[aozarov]

@mziccard lets remove the delimiter and recursive options for now (which are broken).

Though we didn't get a consensus I think we got a green light for either of these options:

• Add currentDirectory option (hard-coded to '/') that returns Blob for prefixes with only bucket, name and a 0 lengths fields populated. Also, consider adding to Blob an isDirectory method that returns true only as a result of listing with currentDirectory option enabled and when the underlying blob does not exists.
• Add listPrefixes method that accepts the same arguments as blob listing and returns Page<String> with only the prefixes.

If you don't feel comfortable with any of these 2 options we can leave the bug open (or create a new one with more specific details/title) but we should at least fix the current broken state.