Ensure that mulitpart bodies are always bytes. #1779

tseaver · 2016-05-06T16:11:11Z

tseaver · 2016-05-06T16:17:31Z

Note that opening the file passed to Table.upload_from_file in text mode now always fails on Py3k (for both small and large files). We should likely document that requirement.

gcloud/streaming/test_transfer.py

+def _email_chunk_parser():
+    import six
+    if six.PY3:  # pragma: NO COVER  Python3
+        from email.parser import BytesParser  # pylint: disable=E0611


dhermes · 2016-05-06T16:24:16Z

RE: Always failing tables, couldn't we just convert to bytes? Then instead of documenting "this will fail", we'd document "if you don't give us bytes, we're going to guess your encoding and we might get it wrong"

tseaver · 2016-05-06T17:31:00Z

Do we want to be in the business of reading potentially huge files in memory and guessing their encoding? If we want to try checking for 'stream.mode != 'rb'` (assuming they opened it, rather than passing us some other file-like object) we could raise an exception right away, rather than failing with a much uglier traceback later.

As discovered in googleapis/google-cloud-python#1760, we were mangling bytes when encoding them as part of a multipart upload request. The fix is to switch from using `six.StringIO` to `six.BytesIO` in `transfer.py`. The patch here is closely based on googleapis/google-cloud-python#1779.

tseaver · 2016-05-10T14:10:24Z

@dhermes I'm reluctant to get into the mess of transcoding a large input stream opened in text mode.

@jgeewax WDYT?

jgeewax · 2016-05-11T12:26:42Z

I agree that guessing the format is probably a bad idea (because while it might make some people happy, it might make some other people really really mad).

Can we have a super clear exception saying ... "You gave us a file opened in text mode, which means we don't know the encoding... Can you open in binary mode with the right encoding?"

dhermes · 2016-05-11T16:59:05Z

Sorry for the delay. @tseaver I agree with you, sniffing the entire file was ill-conceived.

gcloud/streaming/test_transfer.py

+    if six.PY3:  # pragma: NO COVER  Python3
+        # pylint: disable=no-name-in-module
+        from email.parser import BytesParser
+        # pylint: enable=no-name-in-module


Fixes #1760.

Addresses: #1779 (comment)

On 'master', it is now disabled globally.

tseaver · 2016-05-11T18:29:25Z

@jgeewax

Can we have a super clear exception saying ... "You gave us a file opened in text mode, which means we don't know the encoding... Can you open in binary mode with the right encoding?"

IFF the user gives us an actual file object (created via open), we can sniff its mode attribute. If they hand us a file-ish object (e.g., an instance of io.StreamIO) it likely won't have such an attribute.

jgeewax · 2016-05-11T20:10:38Z

@dhermes And in that case, we just do our best here, right?

Addresses: #1779 (comment)

tseaver · 2016-05-11T20:47:30Z

@jgeewax 4a8e952 adds a test for a text-mode file, where the exception message says "use open(filename, mode='rb')".

tseaver · 2016-05-12T18:30:51Z

@dhermes, @jgeewax PTAL: I think I've addressed your concerns.

dhermes · 2016-05-12T22:50:54Z

LGTM

…#1779) * Fixed name of model * update model ids

tseaver assigned dhermes May 6, 2016

tseaver added api: storage Issues related to the Cloud Storage API. api: bigquery Issues related to the BigQuery API. labels May 6, 2016

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 6, 2016

tseaver mentioned this pull request May 6, 2016

BigQuery upload_from_file unicode file-like must be opened in binary-mode if it's more than RESUMABLE_UPLOAD_THRESHOLD, otherwise str-mode. #1760

Closed

dhermes reviewed May 6, 2016
View reviewed changes

gcloud/streaming/test_transfer.py

def _email_chunk_parser():

import six

if six.PY3: # pragma: NO COVER Python3

from email.parser import BytesParser # pylint: disable=E0611

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes mentioned this pull request May 6, 2016

Small pylint clean-up. #1780

Merged

craigcitro mentioned this pull request May 9, 2016

Fix an issue with Python3 multipart encodings. google/apitools#105

Merged

dhermes reviewed May 11, 2016
View reviewed changes

gcloud/streaming/test_transfer.py

if six.PY3: # pragma: NO COVER Python3

# pylint: disable=no-name-in-module

from email.parser import BytesParser

# pylint: enable=no-name-in-module

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

tseaver added 5 commits May 11, 2016 14:25

Ensure that mulitpart bodies are always bytes.

6856273

Fixes #1760.

Note that the file to be uploaded must be opened in binary mode.

5cb6e09

Use pylint checker name rather than code.

9ef3548

Addresses: #1779 (comment)

Typo fix.

f798af5

Addresses: #1779 (comment)

Remove inline disabling of 'no-name-in-module'

ceb1c07

On 'master', it is now disabled globally.

Raise ValueError if text-mode file is passed to 'upload_from_file'.

4a8e952

Addresses: #1779 (comment)

tseaver merged commit 6064270 into googleapis:master May 12, 2016

tseaver deleted the 1760-streaming-multipart_upload_py3k branch May 12, 2016 23:01

tseaver mentioned this pull request May 16, 2016

Cut 0.14.0 release. #1799

Merged

theacodes unassigned dhermes Sep 28, 2018

parthea pushed a commit that referenced this pull request Jul 6, 2023

Fixed name of model [(#1779)](GoogleCloudPlatform/python-docs-samples…

e561f44

…#1779) * Fixed name of model * update model ids

parthea pushed a commit that referenced this pull request Oct 21, 2023

Fixed name of model [(#1779)](GoogleCloudPlatform/python-docs-samples…

bf63f4b

…#1779) * Fixed name of model * update model ids

parthea pushed a commit that referenced this pull request Oct 21, 2023

Fixed name of model [(#1779)](GoogleCloudPlatform/python-docs-samples…

0996cea

…#1779) * Fixed name of model * update model ids

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that mulitpart bodies are always bytes. #1779

Ensure that mulitpart bodies are always bytes. #1779

tseaver commented May 6, 2016

tseaver commented May 6, 2016

This comment was marked as spam.

This comment was marked as spam.

dhermes commented May 6, 2016

tseaver commented May 6, 2016

tseaver commented May 10, 2016

jgeewax commented May 11, 2016 •

edited

Loading

dhermes commented May 11, 2016

This comment was marked as spam.

This comment was marked as spam.

tseaver commented May 11, 2016

jgeewax commented May 11, 2016

tseaver commented May 11, 2016

tseaver commented May 12, 2016

dhermes commented May 12, 2016

Ensure that mulitpart bodies are always bytes. #1779

Ensure that mulitpart bodies are always bytes. #1779

Conversation

tseaver commented May 6, 2016

tseaver commented May 6, 2016

This comment was marked as spam.

This comment was marked as spam.

dhermes commented May 6, 2016

tseaver commented May 6, 2016

tseaver commented May 10, 2016

jgeewax commented May 11, 2016 • edited Loading

dhermes commented May 11, 2016

This comment was marked as spam.

This comment was marked as spam.

tseaver commented May 11, 2016

jgeewax commented May 11, 2016

tseaver commented May 11, 2016

tseaver commented May 12, 2016

dhermes commented May 12, 2016

jgeewax commented May 11, 2016 •

edited

Loading