From f6182d070b1d5b6c2401b84d722b36812cec25cb Mon Sep 17 00:00:00 2001 From: "Dr. Ernie Prabhakar" Date: Thu, 21 Nov 2024 06:50:43 -0800 Subject: [PATCH] fix #4215: More informative error messages when package construction fails (#4216) Co-authored-by: Dr. Ernie Prabhakar <19791+drernie@users.noreply.github.com> Co-authored-by: Sergey Fedoseev --- api/python/quilt3/packages.py | 30 ++++--- api/python/tests/integration/test_packages.py | 87 ++++++++++++++++++- docs/CHANGELOG.md | 1 + 3 files changed, 101 insertions(+), 17 deletions(-) diff --git a/api/python/quilt3/packages.py b/api/python/quilt3/packages.py index ed844bf97e1..09f1391b60c 100644 --- a/api/python/quilt3/packages.py +++ b/api/python/quilt3/packages.py @@ -548,7 +548,7 @@ def install(cls, name, registry=None, top_hash=None, dest=None, dest_registry=No if subpkg_key is not None: if subpkg_key not in pkg: - raise QuiltException(f"Package {name} doesn't contain {subpkg_key!r}.") + raise QuiltException(f"Package {name!r} doesn't contain {subpkg_key!r}.") entry = pkg[subpkg_key] entries = entry.walk() if isinstance(entry, Package) else ((subpkg_key.split('/')[-1], entry),) else: @@ -831,7 +831,7 @@ def _load(cls, readable_file): subpkg.set_meta(obj['meta']) continue if key in subpkg._children: - raise PackageException("Duplicate logical key while loading package") + raise PackageException(f"Duplicate logical key {key!r} while loading package entry: {obj!r}") subpkg._children[key] = PackageEntry( PhysicalKey.from_url(obj['physical_keys'][0]), obj['size'], @@ -869,7 +869,7 @@ def set_dir(self, lkey, path=None, meta=None, update_policy="incoming", unversio ValueError: When `update_policy` is invalid. """ if update_policy not in PACKAGE_UPDATE_POLICY: - raise ValueError(f"Update policy should be one of {PACKAGE_UPDATE_POLICY}, not {update_policy!r}") + raise ValueError(f"Update policy should be one of {PACKAGE_UPDATE_POLICY!r}, not {update_policy!r}") lkey = lkey.strip("/") @@ -890,7 +890,7 @@ def set_dir(self, lkey, path=None, meta=None, update_policy="incoming", unversio if src.is_local(): src_path = pathlib.Path(src.path) if not src_path.is_dir(): - raise PackageException("The specified directory doesn't exist") + raise PackageException(f"The specified directory {src_path!r} doesn't exist") files = src_path.rglob('*') ignore = src_path / '.quiltignore' @@ -951,7 +951,7 @@ def get(self, logical_key): """ obj = self[logical_key] if not isinstance(obj, PackageEntry): - raise ValueError("Key does not point to a PackageEntry") + raise ValueError(f"Key {logical_key!r} does not point to a PackageEntry") return obj.get() def readme(self): @@ -1190,8 +1190,7 @@ def _set( ): if not logical_key or logical_key.endswith('/'): raise QuiltException( - f"Invalid logical key {logical_key!r}. " - f"A package entry logical key cannot be a directory." + f"A package entry logical key {logical_key!r} must be a file." ) validate_key(logical_key) @@ -1238,7 +1237,7 @@ def _set( if len(format_handlers) == 0: error_message = f'Quilt does not know how to serialize a {type(entry)}' if ext is not None: - error_message += f' as a {ext} file.' + error_message += f' as a {ext!r} file.' error_message += '. If you think this should be supported, please open an issue or PR at ' \ 'https://github.com/quiltdata/quilt' raise QuiltException(error_message) @@ -1271,7 +1270,7 @@ def _set( pkg = self._ensure_subpackage(path[:-1], ensure_no_entry=True) if path[-1] in pkg and isinstance(pkg[path[-1]], Package): - raise QuiltException("Cannot overwrite directory with PackageEntry") + raise QuiltException(f"Cannot overwrite directory {path[-1]!r} with PackageEntry") pkg._children[path[-1]] = entry return self @@ -1292,7 +1291,10 @@ def _ensure_subpackage(self, path, ensure_no_entry=False): for key_fragment in path: if ensure_no_entry and key_fragment in pkg \ and isinstance(pkg[key_fragment], PackageEntry): - raise QuiltException("Already a PackageEntry along the path.") + raise QuiltException( + f"Already a PackageEntry for {key_fragment!r} " + f"along the path {path!r}: {pkg[key_fragment].physical_key!r}", + ) pkg = pkg._children.setdefault(key_fragment, Package()) return pkg @@ -1342,7 +1344,7 @@ def _get_top_hash_parts(cls, meta, entries): for logical_key, entry in entries: if entry.hash is None or entry.size is None: raise QuiltException( - "PackageEntry missing hash and/or size: %s" % entry.physical_key + "PackageEntry missing hash and/or size: %r" % entry.physical_key ) yield { 'hash': entry.hash, @@ -1453,7 +1455,7 @@ def dest_fn(*args, **kwargs): raise TypeError(f'{dest!r} returned {url!r}, but str is expected') pk = PhysicalKey.from_url(url) if pk.is_local(): - raise util.URLParseError("Unexpected scheme: 'file'") + raise util.URLParseError(f"Unexpected scheme: 'file' for {pk!r}") if pk.version_id: raise ValueError(f'{dest!r} returned {url!r}, but URI must not include versionId') return pk @@ -1488,8 +1490,8 @@ def check_hash_conficts(latest_hash): if self._origin is None or latest_hash != self._origin.top_hash: raise QuiltConflictException( - f"Package with hash {latest_hash} already exists at the destination; " - f"expected {None if self._origin is None else self._origin.top_hash}. " + f"Package with hash {latest_hash!r} already exists at the destination; " + f"expected {None if self._origin is None else self._origin.top_hash!r}. " "Use force=True (Python) or --force (CLI) to overwrite." ) diff --git a/api/python/tests/integration/test_packages.py b/api/python/tests/integration/test_packages.py index dc6bf844efd..12701ca71dd 100644 --- a/api/python/tests/integration/test_packages.py +++ b/api/python/tests/integration/test_packages.py @@ -25,10 +25,13 @@ LocalPackageRegistryV2, ) from quilt3.backends.s3 import S3PackageRegistryV1, S3PackageRegistryV2 +from quilt3.exceptions import PackageException +from quilt3.packages import PackageEntry from quilt3.util import ( PhysicalKey, QuiltConflictException, QuiltException, + URLParseError, validate_package_name, ) @@ -855,7 +858,9 @@ def test_iter(self): def test_invalid_set_key(self): """Verify an exception when setting a key with a path object.""" pkg = Package() - with pytest.raises(TypeError): + with pytest.raises(TypeError, + match="Expected a string for entry, but got an instance of " + r"\."): pkg.set('asdf/jkl', Package()) def test_brackets(self): @@ -1264,7 +1269,8 @@ def test_commit_message_on_push(self, mocked_workflow_validate): ) def test_overwrite_dir_fails(self): - with pytest.raises(QuiltException): + with pytest.raises(QuiltException, + match="Cannot overwrite directory 'asdf' with PackageEntry"): pkg = Package() pkg.set('asdf/jkl', LOCAL_MANIFEST) pkg.set('asdf', LOCAL_MANIFEST) @@ -2015,7 +2021,7 @@ def test_max_manifest_record_size(self): with mock.patch('quilt3.packages.MANIFEST_MAX_RECORD_SIZE', 1): with pytest.raises(QuiltException) as excinfo: Package().dump(buf) - assert 'Size of manifest record for package metadata' in str(excinfo.value) + assert "Size of manifest record for package metadata" in str(excinfo.value) with mock.patch('quilt3.packages.MANIFEST_MAX_RECORD_SIZE', 10_000): with pytest.raises(QuiltException) as excinfo: @@ -2244,3 +2250,78 @@ def test_set_dir_update_policy_s3(update_policy, expected_a_url, expected_xy_url assert pkg['z.txt'].get() == 's3://bucket/bar/z.txt?versionId=123' assert list_object_versions_mock.call_count == 2 list_object_versions_mock.assert_has_calls([call('bucket', 'foo/'), call('bucket', 'bar/')]) + + +def create_test_file(filename): + file_path = Path(filename) + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text("test") + return filename + + +def test_set_meta_error(): + with pytest.raises(PackageException, match="Must specify either path or meta"): + entry = PackageEntry( + PhysicalKey("test-bucket", "without-hash", "without-hash"), + 42, + None, + {}, + ) + entry.set() + + +def test_loading_duplicate_logical_key_error(): + # Create a manifest with duplicate logical keys + KEY = "duplicate_key" + ROW = {"logical_key": KEY, "physical_keys": [f"s3://bucket/{KEY}"], "size": 123, "hash": None, "meta": {}} + buf = io.BytesIO() + jsonlines.Writer(buf).write_all([{"version": "v0"}, ROW, ROW]) + buf.seek(0) + + # Attempt to load the package, which should raise the error + with pytest.raises(PackageException, match=f"Duplicate logical key {KEY!r} while loading package entry: .*"): + Package.load(buf) + + +def test_directory_not_exist_error(): + pkg = Package() + with pytest.raises(PackageException, match="The specified directory .*non_existent_directory'. doesn't exist"): + pkg.set_dir("foo", "non_existent_directory") + + +def test_key_not_point_to_package_entry_error(): + DIR = "foo" + KEY = create_test_file(f"{DIR}/foo.txt") + pkg = Package().set(KEY) + + with pytest.raises(ValueError, match=f"Key {DIR!r} does not point to a PackageEntry"): + pkg.get(DIR) + + +def test_commit_message_type_error(): + pkg = Package() + with pytest.raises( + ValueError, + match="The package commit message must be a string, " + "but the message provided is an instance of .", + ): + pkg.build("test/pkg", message=123) + + +def test_already_package_entry_error(): + DIR = "foo" + KEY = create_test_file(f"{DIR}/foo.txt") + KEY2 = create_test_file(f"{DIR}/bar.txt") + pkg = Package().set(DIR, KEY) + with pytest.raises( + QuiltException, match=f"Already a PackageEntry for {DIR!r} along the path " rf"\['{DIR}'\]: .*/{KEY}" + ): + pkg.set(KEY2) + + +@patch("quilt3.workflows.validate", return_value=None) +def test_unexpected_scheme_error(workflow_validate_mock): + KEY = create_test_file("foo.txt") + pkg = Package().set(KEY) + with pytest.raises(URLParseError, match="Unexpected scheme: 'file' for .*"): + pkg.push("foo/bar", registry="s3://test-bucket", dest=lambda lk, entry: "file:///foo.txt", force=True) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 33e4594a1eb..c1fa0729e6d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -17,6 +17,7 @@ Entries inside each section should be ordered by type: ## Python API +* [Changed] More informative error messages when package construction fails ([#4216](https://github.com/quiltdata/quilt/pull/4216)) * [Fixed] Allow S3 paths starting with `/` in `Package.set_dir()` and `Package.set()` ([#4207](https://github.com/quiltdata/quilt/pull/4207)) # 6.1.0 - 2024-10-14