Skip to content

Do not quote embedded slashes for public / signed URLs#4716

Merged
tseaver merged 3 commits intomasterfrom
3809-storage-investigate-not-quoting-embedded-slash
Feb 26, 2018
Merged

Do not quote embedded slashes for public / signed URLs#4716
tseaver merged 3 commits intomasterfrom
3809-storage-investigate-not-quoting-embedded-slash

Conversation

@tseaver
Copy link
Copy Markdown
Contributor

@tseaver tseaver commented Jan 8, 2018 •
edited
Loading

Closes #3809.

As noted on that issue, the public_url and generate_signed_url methods use the XML endpoints, which allow for / expect the slash not to be quoted. All other methods use the JSON endpoints, which require quoting the embedded slashes.

@tseaver tseaver added api: datastore Issues related to the Datastore API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Jan 8, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 8, 2018
@chemelnucfin chemelnucfin added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jan 15, 2018
@tseaver tseaver mentioned this pull request Jan 18, 2018
Closed
@tseaver tseaver removed priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Feb 20, 2018
@tseaver
WIP: try not quoting the embedded slash in blob names.
878fba7 
Adding a system test to verify fetching hierarchical blobs directly:  it
fails without the quoting.
@tseaver
Restore quoting '/' for API requests.
5437739 
Leave it unquoted for 'public_url' and 'generate_signed_url'.
@tseaver tseaver force-pushed the 3809-storage-investigate-not-quoting-embedded-slash branch from 93074c3 to 5437739 Compare February 22, 2018 22:46
@frankyn
Copy link
Copy Markdown
Contributor

frankyn commented Feb 22, 2018 •
edited
Loading

@tseaver, it looks like it's still failing. Did you update the PR given the last commit was back in Jan 8?

Required test coverage of 97% reached. Total coverage: 99.97%
================================== FAILURES ===================================
__________________ Test_Blob.test_public_url_with_non_ascii ___________________
Traceback (most recent call last):
  File "C:\projects\google-cloud-python\storage\tests\unit\test_blob.py", line 182, in test_public_url_with_non_ascii
    self.assertEqual(blob.public_url, expected_url)
  File "C:\projects\google-cloud-python\storage\google\cloud\storage\blob.py", line 247, in public_url
    quoted_name=quote(self.name))
  File "c:\python27\Lib\urllib.py", line 1298, in quote
    return ''.join(map(quoter, s))
KeyError: u'\u2603'
1 failed, 422 passed in 3.61 seconds

@frankyn
Copy link
Copy Markdown
Contributor

frankyn commented Feb 22, 2018

@tseaver, I think it's a unicode issue given the following SO question.

Given that, the line could be updated with:

quoted_name=quote(self.name.encode('utf-8')))

@tseaver tseaver changed the title WIP: try not quoting the embedded slash in blob names. Do not quote embedded slashes for public / signed URLs Feb 26, 2018
@tseaver
Encode name before quoting.
84ace6c 
Python 2.7's 'quote' requires bytes.
@tseaver tseaver added api: storage Issues related to the Cloud Storage API. and removed api: datastore Issues related to the Datastore API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Feb 26, 2018
@tseaver
Copy link
Copy Markdown
Contributor Author

tseaver commented Feb 26, 2018

@frankyn Thanks for tracking that down!

@tseaver tseaver requested a review from frankyn February 26, 2018 16:59
@tseaver
Copy link
Copy Markdown
Contributor Author

tseaver commented Feb 26, 2018

@frankyn I opened issue #4935 for the unrelated Bigtable system test flakiness, and restarted the build after cleaning out the orphaned instances. I plan to merge here later today unless you see a problem.

@tseaver tseaver merged commit 5980055 into master Feb 26, 2018
@tseaver tseaver deleted the 3809-storage-investigate-not-quoting-embedded-slash branch February 26, 2018 20:54
@pfk-beta pfk-beta mentioned this pull request Feb 27, 2018
Closed
@tseaver tseaver mentioned this pull request Jun 3, 2019
Closed
@IlyaFaer IlyaFaer mentioned this pull request Apr 16, 2020
Closed
parthea pushed a commit that referenced this pull request Mar 9, 2026
@tseaver
Do not quote embedded slashes for public / signed URLs (#4716)
ce15ebe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

@frankyn frankyn Awaiting requested review from frankyn

1 more reviewer

@theacodes theacodes theacodes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@tseaver @frankyn @theacodes @chemelnucfin @googlebot