Skip to content

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

Closed
MewX wants to merge 1 commit intogoogleapis:masterfrom
MewX:patch-1
Closed

Storage: Make signurl work with objects containing tildes.#8200
MewX wants to merge 1 commit intogoogleapis:masterfrom
MewX:patch-1

Conversation

@MewX
Copy link
Copy Markdown

@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 requested review from crwilcox and frankyn May 30, 2019 23:56
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

frankyn commented Jun 3, 2019

@busunkim96 busunkim96 removed their request for review June 3, 2019 22:12
@MewX
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@MewX
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

frankyn commented Jun 18, 2019

@MewX rerunning tests. Thank you for your patience.

@frankyn
Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
parthea pushed a commit that referenced this pull request Mar 9, 2026
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