Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions google/cloud/storage/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@
DataCorruptionDynamicParent = Exception


class InvalidPathError(Exception):
"""Raised when the provided path string is malformed."""

pass


class InvalidResponse(InvalidResponseDynamicParent):
"""Error class for responses which are not in the correct state.
Expand Down
12 changes: 10 additions & 2 deletions google/cloud/storage/transfer_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@

from google.cloud.storage._media.requests.upload import XMLMPUContainer
from google.cloud.storage._media.requests.upload import XMLMPUPart
from google.cloud.storage.exceptions import DataCorruption
from google.cloud.storage.exceptions import DataCorruption, InvalidPathError

TM_DEFAULT_CHUNK_SIZE = 32 * 1024 * 1024
DEFAULT_MAX_WORKERS = 8
Expand Down Expand Up @@ -263,6 +263,8 @@ def upload_many(


def _resolve_path(target_dir, blob_path):
if os.name == "nt" and ":" in blob_path:
raise InvalidPathError(f"{blob_path} cannot be downloaded into {target_dir}")
target_dir = Path(target_dir)
blob_path = Path(blob_path)
# blob_path.anchor will be '/' if `blob_path` is full path else it'll empty.
Expand Down Expand Up @@ -818,7 +820,13 @@ def download_many_to_path(

for i, blob_name in enumerate(blob_names):
full_blob_name = blob_name_prefix + blob_name
resolved_path = _resolve_path(destination_directory, blob_name)
try:
resolved_path = _resolve_path(destination_directory, blob_name)
except InvalidPathError as e:
msg = f"The blob {blob_name} will **NOT** be downloaded. {e}"
warnings.warn(msg)
results[i] = UserWarning(msg)
continue
if not resolved_path.parent.is_relative_to(
Path(destination_directory).resolve()
):
Expand Down
51 changes: 51 additions & 0 deletions tests/unit/test_transfer_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,57 @@ def test_upload_many_from_filenames_additional_properties():
assert getattr(blob, attrib) == value



def test__resolve_path_raises_invalid_path_error_on_windows():
from google.cloud.storage.transfer_manager import _resolve_path, InvalidPathError

with mock.patch("os.name", "nt"):
with pytest.raises(InvalidPathError) as exc_info:
_resolve_path("C:\\target", "C:\\target\\file.txt")
assert "cannot be downloaded into" in str(exc_info.value)
Comment on lines +517 to +523
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The assertion message cannot be downloaded into is too specific and duplicates the implementation detail. It would be better to assert a more general message indicating that the path is invalid, which is less prone to break if the implementation changes.

        with pytest.raises(InvalidPathError) as exc_info:
            _resolve_path("C:\\target", "C:\\target\\file.txt")
        assert "Invalid path" in str(exc_info.value)


# Test that it DOES NOT raise on non-windows
with mock.patch("os.name", "posix"):
# Should not raise
_resolve_path("/target", "C:\\target\\file.txt")


def test_download_many_to_path_raises_invalid_path_error():
bucket = mock.Mock()

BLOBNAMES = ["C:\\target\\file.txt"]
PATH_ROOT = "mypath/"
BLOB_NAME_PREFIX = "myprefix/"
DOWNLOAD_KWARGS = {"accept-encoding": "fake-gzip"}
MAX_WORKERS = 7
DEADLINE = 10
WORKER_TYPE = transfer_manager.THREAD

with mock.patch("os.name", "nt"):
import warnings

with warnings.catch_warnings(record=True) as w:
warnings.simplefilter("always")
results = transfer_manager.download_many_to_path(
bucket,
BLOBNAMES,
destination_directory=PATH_ROOT,
blob_name_prefix=BLOB_NAME_PREFIX,
download_kwargs=DOWNLOAD_KWARGS,
deadline=DEADLINE,
create_directories=False,
raise_exception=True,
max_workers=MAX_WORKERS,
worker_type=WORKER_TYPE,
skip_if_exists=True,
)

assert len(w) == 1
assert "will **NOT** be downloaded" in str(w[0].message)
assert len(results) == 1
assert isinstance(results[0], UserWarning)


def test_download_many_to_path():
bucket = mock.Mock()

Expand Down
Loading