Skip to content

Commit

Permalink
Feat: Add "auto" checksum option and make default
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewsg committed Nov 16, 2024
1 parent ffe8135 commit 7c76ff3
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 102 deletions.
6 changes: 3 additions & 3 deletions google/cloud/storage/_media/_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ def __init__(self, upload_url, headers=None, checksum="auto"):
super(MultipartUpload, self).__init__(upload_url, headers=headers)
self._checksum_type = checksum
if self._checksum_type == "auto":
self.checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"
self._checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"

def _prepare_request(self, data, metadata, content_type):
"""Prepare the contents of an HTTP request.
Expand Down Expand Up @@ -390,7 +390,7 @@ def __init__(self, upload_url, chunk_size, checksum="auto", headers=None):
self._bytes_checksummed = 0
self._checksum_type = checksum
if self._checksum_type == "auto":
self.checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"
self._checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"
self._checksum_object = None
self._total_bytes = None
self._resumable_url = None
Expand Down Expand Up @@ -1228,7 +1228,7 @@ def __init__(
self._etag = None
self._checksum_type = checksum
if self._checksum_type == "auto":
self.checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"
self._checksum_type = "crc32c" if _helpers._is_crc32c_available_and_fast() else "md5"
self._checksum_object = None

@property
Expand Down
2 changes: 1 addition & 1 deletion tests/resumable_media/unit/requests/test_upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ def test_mpu_container_cancel():


def test_mpu_part(filename):
part = upload_mod.XMLMPUPart(EXAMPLE_XML_UPLOAD_URL, UPLOAD_ID, filename, 0, 128, 1)
part = upload_mod.XMLMPUPart(EXAMPLE_XML_UPLOAD_URL, UPLOAD_ID, filename, 0, 128, 1, checksum=None)

transport = mock.Mock(spec=["request"])
transport.request.return_value = _make_response(headers={"etag": PARTS[1]})
Expand Down
83 changes: 12 additions & 71 deletions tests/resumable_media/unit/test__helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -209,80 +209,21 @@ def test__get_checksum_object_invalid():
_helpers._get_checksum_object("invalid")


# @mock.patch("builtins.__import__")
# def test__get_crc32_object_wo_google_crc32c_wo_crcmod(mock_import):
# mock_import.side_effect = ImportError("testing")
def test__is_crc32c_available_and_fast():
import sys

# with pytest.raises(ImportError):
# _helpers._get_crc32c_object()
assert _helpers._is_crc32c_available_and_fast() is True

# expected_calls = [
# mock.call("google_crc32c", mock.ANY, None, None, 0),
# mock.call("crcmod", mock.ANY, None, None, 0),
# ]
# mock_import.assert_has_calls(expected_calls)
del sys.modules["google_crc32c"]
with mock.patch('builtins.__import__', side_effect=ImportError):
assert _helpers._is_crc32c_available_and_fast() is False

import google_crc32c
with mock.patch('google_crc32c.implementation', new="python"):
assert _helpers._is_crc32c_available_and_fast() is False

# @mock.patch("builtins.__import__")
# def test__get_crc32_object_w_google_crc32c(mock_import):
# google_crc32c = mock.Mock(spec=["Checksum"])
# mock_import.return_value = google_crc32c

# found = _helpers._get_crc32c_object()

# assert found is google_crc32c.Checksum.return_value
# google_crc32c.Checksum.assert_called_once_with()

# mock_import.assert_called_once_with("google_crc32c", mock.ANY, None, None, 0)


# @mock.patch("builtins.__import__")
# def test__get_crc32_object_wo_google_crc32c_w_crcmod(mock_import):
# crcmod = mock.Mock(spec=["predefined", "crcmod"])
# crcmod.predefined = mock.Mock(spec=["Crc"])
# crcmod.crcmod = mock.Mock(spec=["_usingExtension"])
# mock_import.side_effect = [ImportError("testing"), crcmod, crcmod.crcmod]

# found = _helpers._get_crc32c_object()

# assert found is crcmod.predefined.Crc.return_value
# crcmod.predefined.Crc.assert_called_once_with("crc-32c")

# expected_calls = [
# mock.call("google_crc32c", mock.ANY, None, None, 0),
# mock.call("crcmod", mock.ANY, None, None, 0),
# mock.call("crcmod.crcmod", mock.ANY, {}, ["_usingExtension"], 0),
# ]
# mock_import.assert_has_calls(expected_calls)


@pytest.mark.filterwarnings("ignore::RuntimeWarning")
@mock.patch("builtins.__import__")
def test__is_fast_crcmod_wo_extension_warning(mock_import):
crcmod = mock.Mock(spec=["crcmod"])
crcmod.crcmod = mock.Mock(spec=["_usingExtension"])
crcmod.crcmod._usingExtension = False
mock_import.return_value = crcmod.crcmod

assert not _helpers._is_fast_crcmod()

mock_import.assert_called_once_with(
"crcmod.crcmod",
mock.ANY,
{},
["_usingExtension"],
0,
)


@mock.patch("builtins.__import__")
def test__is_fast_crcmod_w_extension(mock_import):
crcmod = mock.Mock(spec=["crcmod"])
crcmod.crcmod = mock.Mock(spec=["_usingExtension"])
crcmod.crcmod._usingExtension = True
mock_import.return_value = crcmod.crcmod

assert _helpers._is_fast_crcmod()
# Run this again to confirm we're back to the initial state.
assert _helpers._is_crc32c_available_and_fast() is True


def test__DoNothingHash():
Expand Down Expand Up @@ -312,7 +253,7 @@ def _get_headers(response):

checksum_types = {
"md5": type(hashlib.md5()),
"crc32c": type(_helpers._get_crc32c_object()),
"crc32c": type(google_crc32c.Checksum()),
}
assert isinstance(checksum_obj, checksum_types[checksum])

Expand Down
16 changes: 13 additions & 3 deletions tests/resumable_media/unit/test__upload.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def test_constructor_defaults(self):
upload = _upload.MultipartUpload(MULTIPART_URL)
assert upload.upload_url == MULTIPART_URL
assert upload._headers == {}
assert upload._checksum_type is None
assert upload._checksum_type is "crc32c" # converted from "auto"
assert not upload._finished
_check_retry_strategy(upload)

Expand All @@ -204,6 +204,15 @@ def test_constructor_explicit(self):
assert not upload._finished
_check_retry_strategy(upload)

def test_constructor_explicit_auto(self):
headers = {"spin": "doctors"}
upload = _upload.MultipartUpload(MULTIPART_URL, headers=headers, checksum="auto")
assert upload.upload_url == MULTIPART_URL
assert upload._headers is headers
assert upload._checksum_type == "crc32c"
assert not upload._finished
_check_retry_strategy(upload)

def test__prepare_request_already_finished(self):
upload = _upload.MultipartUpload(MULTIPART_URL)
upload._finished = True
Expand Down Expand Up @@ -345,7 +354,7 @@ def test_constructor(self):
assert upload._checksum_object is None
assert upload._total_bytes is None
assert upload._resumable_url is None
assert upload._checksum_type is None
assert upload._checksum_type is "crc32c" # converted from "auto"

def test_constructor_bad_chunk_size(self):
with pytest.raises(ValueError):
Expand Down Expand Up @@ -771,7 +780,7 @@ def test__process_resumable_response_bad_status(self):
assert upload.invalid

def test__process_resumable_response_success(self):
upload = _upload.ResumableUpload(RESUMABLE_URL, ONE_MB)
upload = _upload.ResumableUpload(RESUMABLE_URL, ONE_MB, checksum=None)
_fix_up_virtual(upload)

# Check / set status before.
Expand Down Expand Up @@ -1401,6 +1410,7 @@ def test_xml_mpu_part(filename):
END,
PART_NUMBER,
headers=EXAMPLE_HEADERS,
checksum=None,
)
verb, url, payload, headers = part._prepare_upload_request()
assert verb == _upload._PUT
Expand Down
Loading

0 comments on commit 7c76ff3

Please sign in to comment.