Skip to content

Operations on Buckets should use Storage.*Option types #702

Description

@dimo414

Some of the static classes in Storage, such as BlobTargetOption which is used by Storage.create(), are also being erroneously used in Bucket. This is problematic because Bucket keys of of path Strings, not BlobInfo instances, and therefore isn't able to extract information like the blob's generation, causing a line like this:

bucket.create(file, data, null, BlobTargetOption.generationMatch());

to fail:

 java.lang.IllegalArgumentException: Option ifGenerationMatch is missing a value
     at com.google.common.base.Preconditions.checkArgument(Preconditions.java:122)
     at com.google.gcloud.storage.StorageImpl.addToOptionMap(StorageImpl.java:651)
     at com.google.gcloud.storage.StorageImpl.addToOptionMap(StorageImpl.java:643)
     at com.google.gcloud.storage.StorageImpl.optionMap(StorageImpl.java:681)
     at com.google.gcloud.storage.StorageImpl.optionMap(StorageImpl.java:660)
     at com.google.gcloud.storage.StorageImpl.optionMap(StorageImpl.java:695)
     at com.google.gcloud.storage.StorageImpl.optionMap(StorageImpl.java:703)
     at com.google.gcloud.storage.StorageImpl.create(StorageImpl.java:140)
     at com.google.gcloud.storage.StorageImpl.create(StorageImpl.java:129)
     at com.google.gcloud.storage.Bucket.create(Bucket.java:360)

And there is no way in Storage.BlobTargetOption to actually specify a generation.

I don't think Bucket should have any dependency on the Storage.*Option types. There's clearly a good amount of overlap, but I don't think that's a good enough reason for them to share these types. Even where they behave the same it's confusing for the caller.

They could extend from a shared parent if we wanted, but that should be an implementation detail.

I can put together a patch to remove the dependencies on Storage.*Option from Bucket if that's desired.

Activity

  1. mziccard commented on Mar 2, 2016

    @mziccard
    Contributor

    Nice catch!
    We should provide dedicated BlobTargetOption and BlobWriteOption in Bucket. I am assigning this to myself and I'll work on it ASAP.

  2. added
    type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.
    api: storageIssues related to the Cloud Storage API.
    on Mar 2, 2016
  3. self-assigned this
    on Mar 2, 2016
  4. mziccard commented on Mar 2, 2016

    @mziccard
    Contributor

    #705 adds BlobTargetOption and BlobWriteOption to Bucket and fixes the bug reported by this issue. Storage.BlobListOption is still used in Bucket as it makes little sense to me to copy-paste the class given that it applies as-is.

  5. dimo414 commented on Mar 3, 2016

    @dimo414
    Author

    You could have Storage.BlobListOption and Bucket.BlobListOption extend a shared parent. That saves too much typing but makes it much clearer to callers that they're doing the right thing. When I was getting set up the Storage/Bucket overlap was one of the more confusing parts of the API.

  6. 15 remaining items

  7. added a commit that references this issue on Feb 1, 2023
  8. added a commit that references this issue on Mar 30, 2026
  9. added a commit that references this issue on Apr 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

api: storageIssues related to the Cloud Storage API.type: bugError or flaw in code with unintended results or allowing sub-optimal usage patterns.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

    Sponsor
    SponsoredKunjungi sekarang
    Promo