-
Notifications
You must be signed in to change notification settings - Fork 546
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
Encode object name with URI instead of CGI #3081
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Hi @prapicault, thank you for this PR!
I do not believe the bucket name needs to be encoded with either, since it contains only lowercase letters, numbers, dashes (-), underscores (_), and dots (.). (See also Encoding URI path parts.) @frankyn Does this sound correct to you? Also, will this PR introduce a breaking change for users? |
@@ -43,9 +43,9 @@ def self.from_bucket bucket, path | |||
# The external path to the file. | |||
def ext_path | |||
escaped_path = String(@path).split("/").map do |node| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole method can be replaced with:
Addressable::URI.escape "/#{@bucket}/#{@path}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Can you also add a direct dependency for the addressable gem to the |
Done |
Still on working in the CLA :( |
LGTM, found a related issue in the google-cloud-python lib: googleapis/google-cloud-python#3809 Percent encode the object name -- except '/' which is done by the split. Please don't remove that. |
CGI.escape node | ||
end.join("/") | ||
"/#{CGI.escape @bucket}/#{escaped_path}" | ||
Addressable::URI.escape "/#{@bucket}/#{@path}" | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankyn I think we are relying here on the behavior (seen in the unit tests below) of Addressable::URI.encode
to not encode forward slashes at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, okay, I take back my last comment about not removing the split. Thanks for clarifying @quartzmo.
Can you please replace |
I believe |
True, in |
|
Remove trailing slash from path if no file. Update CGI.escape to Addressable::URI.escape. [refs googleapis#3081]
@prapicault How is it going? Are you blocked by the CLA? Or anything else? |
@prapicault We are planning a release of google-cloud-storage, it would be great to include this change. Is there anything I can do to help? |
@prapicault We would like to release this fix in the next few working days, are you making any progress with the CLA? |
@prapicault I'm going to re-do this fix today in my own PR, but I will credit you for the find and the idea. I hope this is acceptable. We need to release the fix. Thank you! |
This has been superseded by #3109. |
Sorry for not being able to follow up. I just got the go to sign the CLA. |
Hi, this is a fix for issue #3064.
I've updated the code and the tests, and I have also tested the change in my setup.
The only thing that I have not been able to test is whether the bucket name needs to be encoded using CGI or URI.