merge with master

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."
                 )
 

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"<class 'quilt3\.packages\.Package'>\."):
             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 <class 'int'>.",
+    ):
+        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)

catalog/CHANGELOG.md

@@ -18,6 +18,7 @@ where verb is one of
 ## Changes
 
 - [Added] Preview Markdown while editing ([#4153](https://github.com/quiltdata/quilt/pull/4153))
+- [Changed] Add `catalog` fragment to Quilt+ URIs (and to documentation) ([#4213](https://github.com/quiltdata/quilt/pull/4213))
 - [Fixed] Athena: fix minor UI bugs ([#4232](https://github.com/quiltdata/quilt/pull/4232))
 - [Fixed] Show Athena query editor when no named queries ([#4230](https://github.com/quiltdata/quilt/pull/4230))
 - [Fixed] Fix some doc URLs in catalog ([#4205](https://github.com/quiltdata/quilt/pull/4205))

catalog/app/containers/Bucket/CodeSamples/Package.spec.tsx

@@ -0,0 +1,33 @@
+import * as React from 'react'
+import renderer from 'react-test-renderer'
+
+import PackageCodeSamples from './Package'
+
+jest.mock(
+  './Code',
+  () =>
+    ({ children }: { children: { label: string; contents: string }[] }) => (
+      <div>
+        {children.map(({ label, contents }) => (
+          <dl key={label}>
+            <dt>{label}:</dt> <dd>{contents}</dd>
+          </dl>
+        ))}
+      </div>
+    ),
+)
+
+describe('containers/Bucket/CodeSamples/Package', () => {
+  it('renders catalog property', () => {
+    const props = {
+      bucket: 'bucket',
+      name: 'name',
+      hash: 'hash',
+      hashOrTag: 'tag',
+      path: 'path',
+      catalog: 'catalog',
+    }
+    const tree = renderer.create(<PackageCodeSamples {...props} />).toJSON()
+    expect(tree).toMatchSnapshot()
+  })
+})

catalog/app/containers/Bucket/CodeSamples/Package.tsx

@@ -54,6 +54,7 @@ interface PackageCodeSamplesProps extends Partial<SectionProps> {
   hash: string
   hashOrTag: string
   path: string
+  catalog: string
 }
 
 export default function PackageCodeSamples({
@@ -62,6 +63,7 @@ export default function PackageCodeSamples({
   hash,
   hashOrTag,
   path,
+  catalog,
   ...props
 }: PackageCodeSamplesProps) {
   const hashDisplay = hashOrTag === 'latest' ? '' : R.take(10, hash)
@@ -85,10 +87,10 @@ export default function PackageCodeSamples({
       {
         label: 'URI',
         hl: 'uri',
-        contents: PackageUri.stringify({ bucket, name, hash, path }),
+        contents: PackageUri.stringify({ bucket, name, hash, path, catalog }),
       },
     ],
-    [bucket, name, hashDisplay, hash, path],
+    [bucket, name, hashDisplay, hash, path, catalog],
   )
   return <Code {...props}>{code}</Code>
 }

catalog/app/containers/Bucket/CodeSamples/__snapshots__/Package.spec.tsx.snap

@@ -0,0 +1,48 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`containers/Bucket/CodeSamples/Package renders catalog property 1`] = `
+<div>
+  <dl>
+    <dt>
+      Python
+      :
+    </dt>
+     
+    <dd>
+      import quilt3 as q3
+# Browse [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/package#package.browse]]
+p = q3.Package.browse("name", top_hash="hash", registry="s3://bucket")
+# make changes to package adding individual files [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/package#package.set]]
+p.set("data.csv", "data.csv")
+# or whole directories [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/package#package.set_dir]]
+p.set_dir("subdir", "subdir")
+# and push changes [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/package#package.push]]
+p.push("name", registry="s3://bucket", message="Hello World")
+
+# Download (be mindful of large packages) [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/package#package.install]]
+q3.Package.install("name", path="path", top_hash="hash", registry="s3://bucket", dest=".")
+    </dd>
+  </dl>
+  <dl>
+    <dt>
+      CLI
+      :
+    </dt>
+     
+    <dd>
+      # Download package [[https://docs.quiltdata.com/quilt-python-sdk-developers/api-reference/cli#install]]
+quilt3 install "name" --path "path" --top-hash hash --registry s3://bucket --dest .
+    </dd>
+  </dl>
+  <dl>
+    <dt>
+      URI
+      :
+    </dt>
+     
+    <dd>
+      quilt+s3://bucket#package=name@hash&path=path&catalog=catalog
+    </dd>
+  </dl>
+</div>
+`;

catalog/app/containers/Bucket/PackageTree/PackageTree.tsx

@@ -468,7 +468,14 @@ function DirDisplay({
                   Ok: ({ ui: { blocks } }) => (
                     <>
                       {blocks.code && (
-                        <PackageCodeSamples {...{ ...packageHandle, hashOrTag, path }} />
+                        <PackageCodeSamples
+                          {...{
+                            ...packageHandle,
+                            hashOrTag,
+                            path,
+                            catalog: window.location.hostname,
+                          }}
+                        />
                       )}
                       {blocks.meta && (
                         <FileView.PackageMetaSection
@@ -821,7 +828,14 @@ function FileDisplay({
                   Ok: ({ ui: { blocks } }) => (
                     <>
                       {blocks.code && (
-                        <PackageCodeSamples {...{ ...packageHandle, hashOrTag, path }} />
+                        <PackageCodeSamples
+                          {...{
+                            ...packageHandle,
+                            hashOrTag,
+                            path,
+                            catalog: window.location.hostname,
+                          }}
+                        />
                       )}
                       {blocks.meta && (
                         <>

catalog/app/containers/Bucket/Selection/Dashboard.tsx

@@ -59,6 +59,7 @@ function FileLink({ className, handle, packageHandle }: FileLinkProps) {
   const packageUri = PackageUri.stringify({
     ...packageHandle,
     path: handle.key,
+    catalog: window.location.hostname,
   })
   const children = decodeURIComponent(
     packageUri.slice(0, packageUri.indexOf('@') + 10) +