diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index a5cea4f958..f92e96a28e 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4292,22 +4292,37 @@ def _load_catalog_config(self, config_path: Path) -> Optional[List[PresetCatalog if not config_path.exists(): return None try: - data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {} + data = yaml.safe_load(config_path.read_text(encoding="utf-8")) except (yaml.YAMLError, OSError, UnicodeError) as e: raise PresetValidationError( f"Failed to read catalog config {config_path}: {e}" ) + # Do NOT coerce with ``or {}`` here: that also turns a FALSY + # non-mapping top level (``[]``, ``false``, ``0``, ``''``) into ``{}`` + # and silently swallows it, while a TRUTHY non-mapping (``5``, a bare + # list) correctly raises below. Only an empty document/explicit + # ``null`` means "no document". + if data is None: + return None if not isinstance(data, dict): raise PresetValidationError( f"Invalid catalog config {config_path}: expected a mapping at root, got {type(data).__name__}" ) - catalogs_data = data.get("catalogs", []) - if not catalogs_data: + # Same asymmetry one nesting level down: the shape check has to run + # BEFORE the emptiness check, or a FALSY non-list ``catalogs`` value + # (``{}``, ``''``, ``0``, ``false``) is silently swallowed as "no + # catalogs" while a TRUTHY non-list (``catalogs: "not-a-list"``) + # correctly raises. An absent key or an explicit ``catalogs: null`` + # both keep their existing "nothing configured here" behavior. + catalogs_data = data.get("catalogs") + if catalogs_data is None: return None if not isinstance(catalogs_data, list): raise PresetValidationError( f"Invalid catalog config: 'catalogs' must be a list, got {type(catalogs_data).__name__}" ) + if not catalogs_data: + return None entries: List[PresetCatalogEntry] = [] for idx, item in enumerate(catalogs_data): if not isinstance(item, dict): diff --git a/tests/test_presets.py b/tests/test_presets.py index f30ab4909e..285cb3de6f 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3412,6 +3412,31 @@ def test_load_catalog_config_not_a_list(self, project_dir): with pytest.raises(PresetValidationError, match="must be a list"): catalog._load_catalog_config(config_path) + @pytest.mark.parametrize("body", ["[]\n", "false\n", "0\n", "''\n"]) + def test_load_catalog_config_rejects_falsy_non_mapping_root(self, project_dir, body): + """A FALSY non-mapping top-level config ([], false, 0, '') must raise, + like a truthy non-mapping (a bare string) already does. The previous + ``yaml.safe_load(...) or {}`` coerced these to {} and silently + swallowed them, diverging from the truthy case.""" + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + + catalog = PresetCatalog(project_dir) + with pytest.raises(PresetValidationError, match="expected a mapping"): + catalog._load_catalog_config(config_path) + + @pytest.mark.parametrize("body", ["catalogs: {}\n", "catalogs: ''\n", "catalogs: 0\n", "catalogs: false\n"]) + def test_load_catalog_config_rejects_falsy_non_list_catalogs(self, project_dir, body): + """A FALSY non-list ``catalogs:`` value must raise, like a truthy one + (``catalogs: "not-a-list"``) already does. The shape check sat behind + the emptiness check, so these were silently swallowed as "no catalogs".""" + config_path = project_dir / ".specify" / "preset-catalogs.yml" + config_path.write_text(body, encoding="utf-8") + + catalog = PresetCatalog(project_dir) + with pytest.raises(PresetValidationError, match="must be a list"): + catalog._load_catalog_config(config_path) + def test_load_catalog_config_invalid_entry(self, project_dir): """Test that non-dict entry raises error.""" config_path = project_dir / ".specify" / "preset-catalogs.yml"