diff --git a/src/specify_cli/bundler/models/catalog.py b/src/specify_cli/bundler/models/catalog.py index 2ef882d576..9514bad9ec 100644 --- a/src/specify_cli/bundler/models/catalog.py +++ b/src/specify_cli/bundler/models/catalog.py @@ -13,6 +13,7 @@ from .. import BundlerError from ..lib.yamlio import ensure_within, load_yaml +from .manifest import _text CONFIG_FILENAME = "bundle-catalogs.yml" # Supported bundle-catalogs.yml schema (major version). Both readers of the @@ -70,8 +71,14 @@ def install_allowed(self) -> bool: def from_dict(cls, data: Any, scope: Scope) -> "CatalogSource": if not isinstance(data, dict): raise BundlerError("Each catalog source must be a mapping.") - source_id = str(data.get("id", "")).strip() - url = str(data.get("url", "")).strip() + # ``_text`` rather than ``str(...get(k, ""))``: the default only covers a + # *missing* key. A key present but null -- how YAML spells an empty field + # (``id:`` with nothing after it) -- yields ``None``, and ``str(None)`` + # is the literal ``"None"``, which is truthy and so sailed straight past + # the required-field guards below: a source with ``id: null`` was + # accepted and registered under the name ``"None"``. + source_id = _text(data.get("id")) + url = _text(data.get("url")) if not source_id: raise BundlerError("A catalog source is missing its 'id'.") if not url: @@ -163,7 +170,11 @@ class CatalogEntry: def from_dict(cls, data: Any) -> "CatalogEntry": if not isinstance(data, dict): raise BundlerError("Each catalog entry must be a mapping.") - entry_id = str(data.get("id", "")).strip() + # ``_text`` here too: an ``id: null`` otherwise became the literal + # "None", which is truthy, so ``load_catalog_payload`` reported it as an + # id MISMATCH against the mapping key rather than the accurate + # missing-id error. + entry_id = _text(data.get("id")) # `or {}` would coerce a FALSY non-mapping (0, '', False, []) to {} before # the isinstance guard, silently accepting a corrupt catalog entry; only # an absent/None value means "not present". @@ -185,14 +196,16 @@ def from_dict(cls, data: Any) -> "CatalogEntry": ) return cls( id=entry_id, - name=str(data.get("name", "")).strip(), - version=str(data.get("version", "")).strip(), - role=str(data.get("role", "")).strip(), - description=str(data.get("description", "")).strip(), - author=str(data.get("author", "")).strip(), - license=str(data.get("license", "")).strip(), - download_url=str(data.get("download_url", "")).strip(), - requires_speckit_version=str(requires.get("speckit_version", "")).strip(), + # See the note in ``CatalogSource.from_dict``: an explicitly null + # field must read as empty, not as the literal string "None". + name=_text(data.get("name")), + version=_text(data.get("version")), + role=_text(data.get("role")), + description=_text(data.get("description")), + author=_text(data.get("author")), + license=_text(data.get("license")), + download_url=_text(data.get("download_url")), + requires_speckit_version=_text(requires.get("speckit_version")), sha256=( None if data.get("sha256") is None diff --git a/tests/contract/test_catalog_schema.py b/tests/contract/test_catalog_schema.py index 3fd3a5c53d..72c7f2ff19 100644 --- a/tests/contract/test_catalog_schema.py +++ b/tests/contract/test_catalog_schema.py @@ -256,6 +256,72 @@ def test_catalog_entry_rejects_non_boolean_verified(): CatalogEntry.from_dict(data) +@pytest.mark.parametrize( + "field", + [ + "name", + "version", + "role", + "description", + "author", + "license", + "download_url", + ], +) +def test_catalog_entry_explicit_null_field_reads_as_empty(field: str): + """An explicitly null field must read as "", not the literal "None". + + `str(data.get(key, ""))` only defaults for a *missing* key. A key present + but null — how YAML spells an empty field (`author:` with nothing after + it) — yields `None`, and `str(None)` is the truthy string `"None"`. The + same constructor already guards `sha256` and `repository` against exactly + this. + """ + from specify_cli.bundler.models.catalog import CatalogEntry + + data = catalog_entry_dict("demo") + data[field] = None + + entry = CatalogEntry.from_dict(data) + + assert getattr(entry, field) == "" + + +def test_catalog_source_rejects_an_explicitly_null_id(): + """`id: null` must be refused, not registered as a source named "None". + + The `if not source_id` guard was defeated by the truthy literal, so the + source was accepted and carried the name `"None"` into the stack. + """ + from specify_cli.bundler.models.catalog import CatalogSource, Scope + + with pytest.raises(BundlerError, match="missing its 'id'"): + CatalogSource.from_dict( + { + "id": None, + "url": "https://example.test/catalog.json", + "priority": 5, + "install_policy": "install-allowed", + }, + Scope.PROJECT, + ) + + +def test_catalog_source_rejects_an_explicitly_null_url(): + from specify_cli.bundler.models.catalog import CatalogSource, Scope + + with pytest.raises(BundlerError, match="missing its 'url'"): + CatalogSource.from_dict( + { + "id": "demo", + "url": None, + "priority": 5, + "install_policy": "install-allowed", + }, + Scope.PROJECT, + ) + + def test_catalog_entry_preserves_sha256_through_provenance(): digest = "a" * 64 payload = catalog_payload( @@ -338,3 +404,37 @@ def test_catalog_entry_rejects_falsy_non_mapping(field, bad): data[field] = bad with pytest.raises(BundlerError, match=f"'{field}' must be a mapping"): CatalogEntry.from_dict(data) + + +def test_catalog_entry_explicit_null_id_reports_missing_id(): + """`id: null` must surface as the missing-id error, not an id mismatch. + + `entry_id` was still computed with `str(data.get("id", ""))`, so an explicit + null became the literal "None". That is truthy, so `load_catalog_payload` + compared it against the mapping key and reported + "id mismatch: key 'demo' != entry id 'None'" instead of the accurate + missing-id error. + """ + from specify_cli.bundler.models.catalog import CatalogEntry + + data = catalog_entry_dict("demo") + data["id"] = None + assert CatalogEntry.from_dict(data).id == "" + + with pytest.raises(BundlerError, match="missing its 'id' field"): + load_catalog_payload(catalog_payload({"demo": data})) + + +def test_catalog_entry_explicit_null_requires_speckit_version_reads_as_empty(): + """The nested `requires.speckit_version` branch is covered too. + + It was switched to `_text` alongside the top-level fields, but only the + top-level attributes were exercised — so this branch could regress while + the suite still passed. + """ + from specify_cli.bundler.models.catalog import CatalogEntry + + data = catalog_entry_dict("demo") + data["requires"] = {"speckit_version": None} + + assert CatalogEntry.from_dict(data).requires_speckit_version == ""