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

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

Merged
merged 3 commits into from
Feb 26, 2018

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jan 8, 2018

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 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
Adding a system test to verify fetching hierarchical blobs directly:  it
fails without the quoting.
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
Member

frankyn commented Feb 22, 2018

@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
Member

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
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
Contributor Author

tseaver commented Feb 26, 2018

@frankyn Thanks for tracking that down!

@tseaver
Copy link
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
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants