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

Storage: Make signurl work with objects containing tildes. #8200

Closed
wants to merge 1 commit into from
Closed

Storage: Make signurl work with objects containing tildes. #8200

wants to merge 1 commit into from

Conversation

MewX
Copy link

@MewX MewX commented May 30, 2019

@MewX MewX requested a review from busunkim96 as a code owner May 30, 2019 23:52
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 30, 2019
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 30, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 31, 2019
@busunkim96
Copy link
Contributor

Passing this along to the storage team. Would it be worth adding a unit test to test this case? @crwilcox @frankyn

@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 31, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 31, 2019
@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Jun 3, 2019
@tseaver tseaver changed the title Make signurl work with objects containing tildes Storage: Make signurl work with objects containing tildes. Jun 3, 2019
@tseaver
Copy link
Contributor

tseaver commented Jun 3, 2019

This PR breaks existing unit tests which assert that a slash in the blob name is not escaped, e.g.:

__________________ Test_Blob.test_public_url_w_slash_in_name ___________________
self = <tests.unit.test_blob.Test_Blob testMethod=test_public_url_w_slash_in_name>
    def test_public_url_w_slash_in_name(self):
        BLOB_NAME = "parent/child"
        bucket = _Bucket()
        blob = self._make_one(BLOB_NAME, bucket=bucket)
        self.assertEqual(
>           blob.public_url, "https://storage.googleapis.com/name/parent/child"
        )
E       AssertionError: 'https://storage.googleapis.com/name/parent%2Fchild' != 'https://storage.googleapis.com/name/parent/child'
E       - https://storage.googleapis.com/name/parent%2Fchild
E       ?                                           ^^^
E       + https://storage.googleapis.com/name/parent/child
E       ?                                           ^
tests/unit/test_blob.py:344: AssertionError

which would re-open #3809 (fixed in #4716).

@frankyn
Copy link
Member

frankyn commented Jun 3, 2019

Hi @MewX, could you add "/" as a safe character as well? gsutil also does this at https://github.com/GoogleCloudPlatform/gsutil/pull/783/files#diff-2203ca5e293c09b69dae32ab51ee067aR543

@busunkim96 busunkim96 removed their request for review June 3, 2019 22:12
@MewX
Copy link
Author

MewX commented Jun 4, 2019

Hi @frankyn, I have reverted my change to 0d4d991 which included / as a safe character.

However, that commit encountered a test failure:

Infrastructure error. Please apply label 'kokoro:force-run' to rerun the build.

@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 4, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 4, 2019
@busunkim96
Copy link
Contributor

Hi @MewX, that's our CI being flaky. I've re-triggered the tests.

@MewX
Copy link
Author

MewX commented Jun 5, 2019

Hi @busunkim96, the test failed again with the same error message. Would you like to take a look at the issue?

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 5, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 5, 2019
@tseaver
Copy link
Contributor

tseaver commented Jun 5, 2019

The Core Kokoro job fails due to a Kokoro-internal glitch.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 6, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 6, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 6, 2019
@MewX
Copy link
Author

MewX commented Jun 18, 2019

Hi @tseaver, could you or anyone please take a look at this bug fix?

A customer might be waiting for this bug fix. Thanks!

@frankyn frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Jun 18, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019
@frankyn
Copy link
Member

frankyn commented Jun 18, 2019

@MewX rerunning tests. Thank you for your patience.

@frankyn
Copy link
Member

frankyn commented Jun 18, 2019

@tseaver @crwilcox the test has ran for 3 hours and hasn't completed. Is this normal for Storage IT?

@busunkim96
Copy link
Contributor

busunkim96 commented Jun 18, 2019

@frankyn No idea if that's normal or not, but I just kicked off the storage job manually.

@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019
@tseaver
Copy link
Contributor

tseaver commented Jun 19, 2019

@MewX I've just pushed PR #8434, which adds explicit unit test coverage for all the cases where _quote is used. In order to make them all pass, I had to add the safe parameter to it: anything where the URL is passed to the JSON API must percent quote /, but anything touching the XML API should not.

@tseaver tseaver closed this Jun 19, 2019
tseaver added a commit that referenced this pull request Jun 20, 2019
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. needs work This is a pull request that needs a little love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants