Skip to content

Fix an exception with 0-size test cases in the viewer handler.#666

Merged
mbarbella-chromium merged 3 commits intomasterfrom
viewer-exception
Jul 9, 2019
Merged

Fix an exception with 0-size test cases in the viewer handler.#666
mbarbella-chromium merged 3 commits intomasterfrom
viewer-exception

Conversation

@mbarbella-chromium
Copy link
Contributor

PTAL. We could do a more generic fix in storage.read_data but it would require us to check the size in all cases, which seems like it could hurt performance in cases where we don't expect issues. Since we already know the size in the handler this seems like the best quick fix to me, but I'm open to either option.

@googlebot googlebot added the cla: yes CLA signed. label Jul 9, 2019
@mbarbella-chromium
Copy link
Contributor Author

/gcbrun

# https://github.com/googleapis/google-cloud-python/issues/6572
if blob_size:
try:
content = unicode(blobs.read_key(key), errors='replace')
Copy link
Collaborator

@inferno-chromium inferno-chromium Jul 9, 2019

Choose a reason for hiding this comment

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

maybe remove this unicode replacement hack along the way and fix https://bugs.chromium.org/p/chromium/issues/detail?id=920160 :) stage to make sure it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems to cause issues later in template rendering, which I guess explains why we had the hack initially. I'm going to leave it out of this change.

@mbarbella-chromium
Copy link
Contributor Author

/gcbrun

@mbarbella-chromium mbarbella-chromium merged commit c7c0492 into master Jul 9, 2019
@mbarbella-chromium mbarbella-chromium deleted the viewer-exception branch July 9, 2019 22:11
@Sovann87 Sovann87 mentioned this pull request Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes CLA signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants