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
7 changes: 5 additions & 2 deletions src/specify_cli/bundler/commands_impl/catalog_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@ def _read(project_root: Path) -> list[dict]:
path = ensure_within(project_root, _config_path(project_root))
if not path.exists():
return []
# ``load_yaml`` returns ``{}`` only for an empty document and the raw parse
# otherwise, so a non-mapping top level — a falsy ``[]``/``false``/``0``/``''``
# or an explicit null (``load_yaml`` -> ``None``) — is caught by the isinstance
# guard below and raised like a truthy one, staying consistent with the other
# reader of this file (models/catalog._merge_config).
data = load_yaml(path)
if data is None:
return []
if not isinstance(data, dict):
raise BundlerError(
f"Malformed catalog config at {path}: expected a mapping at the top "
Expand Down
28 changes: 23 additions & 5 deletions src/specify_cli/bundler/lib/yamlio.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,17 +39,35 @@ def ensure_within(root: Path, candidate: Path) -> Path:


def load_yaml(path: Path) -> Any:
"""Parse a YAML file, returning ``{}`` for an empty document."""
"""Parse a YAML file, returning ``{}`` only for an *empty* document.

A non-empty document is returned exactly as parsed — including a
non-mapping such as ``[]``, ``false``, ``0``, ``''``, or an explicit null
(``null``/``~``) — so callers can validate the top-level shape (e.g. reject
a non-mapping config) instead of having it silently coerced to an empty
mapping.

``yaml.safe_load`` returns ``None`` for *both* an empty document and an
explicit null scalar, so ``yaml.compose`` (which yields no node only for a
truly empty document) is used to tell them apart: an empty document becomes
``{}`` while an explicit ``null``/``~`` is returned as ``None`` for the
caller to reject.
"""
path = Path(path)
if not path.exists():
raise BundlerError(f"File not found: {path}")
try:
with path.open("r", encoding="utf-8") as handle:
return yaml.safe_load(handle) or {}
except yaml.YAMLError as exc:
raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc
text = path.read_text(encoding="utf-8")
except OSError as exc:
raise BundlerError(f"Could not read {path}: {exc}") from exc
try:
has_node = yaml.compose(text) is not None
data = yaml.safe_load(text)
except yaml.YAMLError as exc:
raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc
if data is None and not has_node:
return {}
return data


def dump_yaml(path: Path, data: Any, *, within: Path | None = None) -> Path:
Expand Down
12 changes: 11 additions & 1 deletion src/specify_cli/bundler/models/catalog.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,18 @@ def load_source_stack(project_root: Path, user_config_dir: Path | None = None) -
def _merge_config(by_id: dict[str, CatalogSource], config_path: Path, scope: Scope) -> None:
if not config_path.exists():
return
# ``load_yaml`` returns ``{}`` only for an empty document and the raw parse
# otherwise, so a non-mapping top level (a YAML list or scalar, including
# the falsy ``[]``/``false``/``0``/``''``) is caught here and raised —
# matching the sibling reader commands_impl/catalog_config._read. #3623
# aligned the inner non-list ``catalogs`` value between the two readers.
data = load_yaml(config_path)
catalogs = data.get("catalogs") if isinstance(data, dict) else None
if not isinstance(data, dict):
Comment thread
jawwad-ali marked this conversation as resolved.
raise BundlerError(
f"Malformed catalog config at {config_path}: expected a mapping at "
f"the top level, got {type(data).__name__}."
)
catalogs = data.get("catalogs")
if catalogs is None:
return
if not isinstance(catalogs, list):
Expand Down
40 changes: 37 additions & 3 deletions tests/contract/test_catalog_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,44 @@ def test_falsy_non_list_catalogs_still_raises(tmp_path: Path, value: str):
load_source_stack(tmp_path)


@pytest.mark.parametrize("body", ["catalogs:\n", "catalogs: []\n"])
@pytest.mark.parametrize(
"body",
[
"- a\n- b\n", # truthy list
"42\n", # truthy scalar
"[]\n", # falsy list
"false\n", # falsy bool
"0\n", # falsy int
"''\n", # falsy empty string
"null\n", # explicit null scalar (safe_load -> None, but a real node)
"~\n", # explicit null scalar (alt spelling)
],
)
def test_toplevel_non_mapping_raises(tmp_path: Path, body: str):
"""A top-level non-mapping bundle-catalogs.yml (list/scalar/null) must raise,
matching the sibling reader (catalog_config._read) — not silently fall back
to the built-in default stack. This includes FALSY non-mappings ([], false,
0, '') and an explicit null (null/~); the shared load_yaml would coerce those
to {} and hide them, so it distinguishes them from a truly empty document."""
make_project(tmp_path)
(tmp_path / ".specify" / "bundle-catalogs.yml").write_text(body, encoding="utf-8")
with pytest.raises(BundlerError, match="expected a mapping at the top level"):
load_source_stack(tmp_path)


@pytest.mark.parametrize(
"body",
[
"catalogs:\n", # present key, null value
"catalogs: []\n", # present key, empty list
"", # truly empty document
"# only a comment\n", # comment-only == empty document
],
)
def test_absent_or_empty_catalogs_is_noop(tmp_path: Path, body: str):
"""An absent (``None``) or empty-list ``catalogs:`` is valid: it contributes
no project sources and falls back to the built-in default stack."""
"""An empty document, comment-only file, or absent/empty-list ``catalogs:``
is valid: it contributes no project sources and falls back to the built-in
default stack (must not be confused with an explicit top-level null)."""
make_project(tmp_path)
(tmp_path / ".specify" / "bundle-catalogs.yml").write_text(body, encoding="utf-8")
# Does not raise; still yields the built-in defaults.
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/test_bundler_catalog_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,20 @@ def test_read_rejects_non_mapping_top_level(tmp_path: Path):
cc._read(project)


@pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n", "null\n", "~\n"])
def test_read_rejects_falsy_non_mapping_top_level(tmp_path: Path, body: str):
# A FALSY non-mapping top level ([], false, 0, '') OR an explicit null
# (null/~) must raise like a truthy one. safe_load coerces these to
# None/{}, so load_yaml distinguishes them from a truly empty document —
# staying consistent with models/catalog._merge_config.
project = tmp_path / "proj"
(project / ".specify").mkdir(parents=True)
cc._config_path(project).write_text(body, encoding="utf-8")

with pytest.raises(BundlerError, match="expected a mapping at the top level"):
cc._read(project)


def test_read_rejects_unknown_schema_version(tmp_path: Path):
project = tmp_path / "proj"
(project / ".specify").mkdir(parents=True)
Expand Down