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

fix(storage): use OrderedDict while encoding POST policy #95

Merged
merged 3 commits into from Apr 1, 2020

Conversation

IlyaFaer
Copy link

@IlyaFaer IlyaFaer commented Apr 1, 2020

Towards #64 (comment)
Sometimes policy losses it's order while encoding into JSON. Adding OrderedDict() to fix this.

@IlyaFaer IlyaFaer added the api: storage Issues related to the googleapis/python-storage API. label Apr 1, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 1, 2020
@IlyaFaer IlyaFaer changed the title use OrderedDict() while encoding POST policy fix(storage): use OrderedDict() while encoding POST policy Apr 1, 2020
"expiration": policy_expires.isoformat() + "Z",
}.items()
)
),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expiration sometimes encoded before conditions - in such a cases tests are failing

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about imposing order in signing code given it's only used for conformance tests.

However, it doesn't change the outcome of expectation and will work as expected as a user.

out_data = test_data["policyOutput"]

decoded_policy = base64.b64decode(fields["policy"]).decode("unicode_escape")
assert decoded_policy == out_data["expectedDecodedPolicy"]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved decoded_policy assert to the top: if something gone wrong in policy, it'll be easier to see it in decoded state - probably will save some time on debugging

@IlyaFaer IlyaFaer requested a review from frankyn April 1, 2020 20:27
@IlyaFaer IlyaFaer marked this pull request as ready for review April 1, 2020 20:28
@IlyaFaer
Copy link
Author

IlyaFaer commented Apr 1, 2020

@frankyn, kokoro failed in TestStorageCompose.test_compose_create_new_blob_wo_content_type. Not related I assume

_______ TestStorageCompose.test_compose_create_new_blob_wo_content_type ________

self = <test_system.TestStorageCompose testMethod=test_compose_create_new_blob_wo_content_type>

    def test_compose_create_new_blob_wo_content_type(self):
        SOURCE_1 = b"AAA\n"
        source_1 = self.bucket.blob("source-1")
>       source_1.upload_from_string(SOURCE_1)

tests/system/test_system.py:1176:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
google/cloud/storage/blob.py:1435: in upload_from_string
    self.upload_from_file(
google/cloud/storage/blob.py:1344: in upload_from_file
    _raise_from_invalid_response(exc)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

error = InvalidResponse('Request failed with status code', 403, 'Expected one of', <HTTPStatus.OK: 200>)

    def _raise_from_invalid_response(error):
        """Re-wrap and raise an ``InvalidResponse`` exception.

        :type error: :exc:`google.resumable_media.InvalidResponse`
        :param error: A caught exception from the ``google-resumable-media``
                      library.

        :raises: :class:`~google.cloud.exceptions.GoogleCloudError` corresponding
                 to the failed status code
        """
        response = error.response
        error_message = str(error)

        message = u"{method} {url}: {error}".format(
            method=response.request.method, url=response.request.url, error=error_message
        )

>       raise exceptions.from_http_status(response.status_code, message, response=response)
E       google.api_core.exceptions.Forbidden: 403 POST https://storage.googleapis.com/upload/storage/v1/b/new_1585772487875/o?uploadType=multipart: ('Request failed with status code', 403, 'Expected one of', <HTTPStatus.OK: 200>)

@IlyaFaer IlyaFaer changed the title fix(storage): use OrderedDict() while encoding POST policy fix(storage): use OrderedDict while encoding POST policy Apr 1, 2020
@frankyn frankyn added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Apr 1, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 1, 2020
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, one side note. Thanks @IlyaFaer

"expiration": policy_expires.isoformat() + "Z",
}.items()
)
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little concerned about imposing order in signing code given it's only used for conformance tests.

However, it doesn't change the outcome of expectation and will work as expected as a user.

@frankyn frankyn merged commit df560e1 into googleapis:master Apr 1, 2020
@IlyaFaer IlyaFaer deleted the post_policies_flaky_fix branch April 1, 2020 21:40
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* use OrderedDict() while encoding POST policy

* fix(storage): use OrderedDict() while encoding POST policy
cojenco pushed a commit to cojenco/python-storage that referenced this pull request Oct 13, 2021
* use OrderedDict() while encoding POST policy

* fix(storage): use OrderedDict() while encoding POST policy
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 googleapis/python-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.

None yet

4 participants