Skip to content

Commit 09a5fd0

Browse files
jawwad-aliclaude
andcommitted
fix(bundler): correct load_yaml so only empty documents become {} (not falsy non-mappings)
Address review (Copilot re-review of #3659): the previous fix duplicated YAML parsing + exception wrapping inline in two readers, bypassing the centralized yamlio helper. Instead, correct the root cause in load_yaml. load_yaml did `yaml.safe_load(...) or {}`, which coerced ANY falsy result (None empty-doc, but also [], false, 0, '') to {} — contradicting its own docstring ("{} for an empty document") and hiding malformed non-mapping configs from callers' shape guards. Change to `{} if data is None else data` so only an empty document becomes {}; a non-mapping top level is returned as-parsed. Revert the inline raw-parse in models/catalog._merge_config and commands_impl/catalog_config._read back to the centralized load_yaml; their existing `isinstance(data, dict)` guards now correctly reject falsy non-mappings too. All three load_yaml callers (these two + manifest.from_dict) already guard the top-level shape, so none regresses. Falsy-case tests for both readers retained. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 7285066 commit 09a5fd0

3 files changed

Lines changed: 22 additions & 33 deletions

File tree

src/specify_cli/bundler/commands_impl/catalog_config.py

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,10 +10,8 @@
1010
from urllib.parse import urlparse
1111
import re
1212

13-
import yaml
14-
1513
from .. import BundlerError
16-
from ..lib.yamlio import dump_yaml, ensure_within
14+
from ..lib.yamlio import dump_yaml, ensure_within, load_yaml
1715
from ..models.catalog import (
1816
CONFIG_FILENAME,
1917
BUILTIN_DEFAULT_STACK,
@@ -42,17 +40,11 @@ def _read(project_root: Path) -> list[dict]:
4240
path = ensure_within(project_root, _config_path(project_root))
4341
if not path.exists():
4442
return []
45-
# Parse the RAW document, not the shared ``load_yaml`` (whose ``… or {}``
46-
# coerces a falsy top-level value to ``{}``): an empty document is a no-op,
47-
# but a falsy non-mapping (``[]``/``false``/``0``/``''``) is malformed and
48-
# must raise like a truthy one, staying consistent with the other reader of
49-
# this file (models/catalog._merge_config).
50-
try:
51-
data = yaml.safe_load(path.read_text(encoding="utf-8"))
52-
except yaml.YAMLError as exc:
53-
raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc
54-
except OSError as exc:
55-
raise BundlerError(f"Could not read {path}: {exc}") from exc
43+
# ``load_yaml`` returns ``{}`` only for an empty document and the raw parse
44+
# otherwise, so a falsy non-mapping (``[]``/``false``/``0``/``''``) is caught
45+
# by the isinstance guard below and raised like a truthy one, staying
46+
# consistent with the other reader of this file (models/catalog._merge_config).
47+
data = load_yaml(path)
5648
if data is None:
5749
return []
5850
if not isinstance(data, dict):

src/specify_cli/bundler/lib/yamlio.py

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,17 +39,25 @@ def ensure_within(root: Path, candidate: Path) -> Path:
3939

4040

4141
def load_yaml(path: Path) -> Any:
42-
"""Parse a YAML file, returning ``{}`` for an empty document."""
42+
"""Parse a YAML file, returning ``{}`` for an empty document.
43+
44+
Only an *empty* document (``yaml.safe_load`` -> ``None``) becomes ``{}``.
45+
A non-empty document is returned exactly as parsed — including a falsy
46+
non-mapping such as ``[]``, ``false``, ``0``, or ``''`` — so callers can
47+
validate the top-level shape (e.g. reject a non-mapping config) instead of
48+
having it silently coerced to an empty mapping.
49+
"""
4350
path = Path(path)
4451
if not path.exists():
4552
raise BundlerError(f"File not found: {path}")
4653
try:
4754
with path.open("r", encoding="utf-8") as handle:
48-
return yaml.safe_load(handle) or {}
55+
data = yaml.safe_load(handle)
4956
except yaml.YAMLError as exc:
5057
raise BundlerError(f"Invalid YAML in {path}: {exc}") from exc
5158
except OSError as exc:
5259
raise BundlerError(f"Could not read {path}: {exc}") from exc
60+
return {} if data is None else data
5361

5462

5563
def dump_yaml(path: Path, data: Any, *, within: Path | None = None) -> Path:

src/specify_cli/bundler/models/catalog.py

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,8 @@
1111
from pathlib import Path
1212
from typing import Any
1313

14-
import yaml
15-
1614
from .. import BundlerError
17-
from ..lib.yamlio import ensure_within
15+
from ..lib.yamlio import ensure_within, load_yaml
1816

1917
CONFIG_FILENAME = "bundle-catalogs.yml"
2018

@@ -251,21 +249,12 @@ def load_source_stack(project_root: Path, user_config_dir: Path | None = None) -
251249
def _merge_config(by_id: dict[str, CatalogSource], config_path: Path, scope: Scope) -> None:
252250
if not config_path.exists():
253251
return
254-
# Parse the RAW document rather than the shared ``load_yaml`` helper, whose
255-
# ``… or {}`` coerces a falsy top-level value to ``{}``: an empty document
256-
# (``None``) is a no-op, but every non-mapping top-level — including the
257-
# falsy ones ``[]``/``false``/``0``/``''`` that ``load_yaml`` would hide —
258-
# is malformed and must raise, matching the sibling reader
259-
# commands_impl/catalog_config._read (which does the same). #3623 already
252+
# ``load_yaml`` returns ``{}`` only for an empty document and the raw parse
253+
# otherwise, so a non-mapping top level (a YAML list or scalar, including
254+
# the falsy ``[]``/``false``/``0``/``''``) is caught here and raised —
255+
# matching the sibling reader commands_impl/catalog_config._read. #3623
260256
# aligned the inner non-list ``catalogs`` value between the two readers.
261-
try:
262-
data = yaml.safe_load(config_path.read_text(encoding="utf-8"))
263-
except yaml.YAMLError as exc:
264-
raise BundlerError(f"Invalid YAML in {config_path}: {exc}") from exc
265-
except OSError as exc:
266-
raise BundlerError(f"Could not read {config_path}: {exc}") from exc
267-
if data is None:
268-
return
257+
data = load_yaml(config_path)
269258
if not isinstance(data, dict):
270259
raise BundlerError(
271260
f"Malformed catalog config at {config_path}: expected a mapping at "

0 commit comments

Comments
 (0)