Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing projection=full from Blob/Bucket PATCH requests. #758

Closed

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Mar 25, 2015

There was a previous comment

Pass ?projection=full here because PATCH documented not to work properly w/ noAcl.

I tested a little with this and it doesn't seem to be an issue.

@thobrla Can you confirm or deny any issues with projection=full?

@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Mar 25, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 25, 2015
@thobrla
Copy link

thobrla commented Mar 25, 2015

I'm not aware of any issues, but is there a reason to specify noAcl? What if you're patching the ACL, don't you want to see the new one?

Projection=full won't return the acl to you unless you have OWNER permission on the bucket/object anyway.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 25, 2015

RE: "is there a reason to specify noAcl" That is a discussion in flux. Currently ACL behavior is managed via dedicated objects instead of directly with the Blob or Bucket. (UPDATE: I don't think this is necessarily the right abstraction, but am in the process of making "interface drive design" document to discuss.)

RE: "won't return the acl to you unless you have OWNER permission" I would wager that was the original author's concern. Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 853d240 on dhermes:ditch-full-projection-in-patch into d34a15e on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Mar 26, 2015

The API docs say:

projection  string  Set of properties to return. Defaults to no_acl.

Note: Currently only works if you override the default and specify full.

Acceptable values are:

    "full": Include all properties.
    "noAcl": Omit acl and defaultObjectAcl properties.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 26, 2015

@tseaver Thanks for pointing this out. @thobrla is that still true? When was it true? "only works" is somewhat vague as it is.

@Capstan
Copy link

Capstan commented Mar 27, 2015

@dhermes It has to do with the way that patch currently is implemented. I suspect that if you had custom ACLs on your bucket that if you patch with no_acl, you inadvertently reset your bucket ACL to project-private.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 28, 2015

I see.

  • Do you mean sending a patch with an empty list in the acl field or none at all? (ISTM you're describing a process where a GET is performed internally with noAcl and then the contents are updated based on the PATCH contents, and then this just gets inserted directly.)
  • Are there plans to fix / address / document this behavior?
  • Is there a corresponding issue with objects or is it limited to buckets?

@dhermes dhermes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 30, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Mar 30, 2015

Adding the don't merge label. It seems from @Capstan feedback (and just from designing the optimal interface in #761) that projection=noAcl isn't something we need. Splitting apart ACLs from other properties isn't worth the payload size savings (1KB to 2KB).

@lukesneeringer
Copy link
Contributor

Adding the don't merge label. It seems from @Capstan feedback (and just from designing the optimal interface in #761) that projection=noAcl isn't something we need. Splitting apart ACLs from other properties isn't worth the payload size savings (1KB to 2KB).

Closing, based on "we do not need this" and also there is no possible way we would successfully wrangle a PR this old in even if we did.

@dhermes dhermes deleted the ditch-full-projection-in-patch branch March 16, 2017 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants