Skip to content

Commit af12d5a

Browse files
committed
fix: address second Copilot review
1 parent 522acb8 commit af12d5a

2 files changed

Lines changed: 77 additions & 2 deletions

File tree

src/specify_cli/integrations/catalog.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,12 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None:
445445

446446
data: Dict[str, Any] = {"catalogs": []}
447447
if config_path.exists():
448-
raw = yaml.safe_load(config_path.read_text(encoding="utf-8"))
448+
try:
449+
raw = yaml.safe_load(config_path.read_text(encoding="utf-8"))
450+
except (yaml.YAMLError, OSError, UnicodeError) as exc:
451+
raise IntegrationValidationError(
452+
f"Failed to read catalog config {config_path}: {exc}"
453+
) from exc
449454
if not isinstance(raw, dict):
450455
raise IntegrationValidationError(
451456
"Catalog config file is corrupted (expected a mapping)."
@@ -491,6 +496,10 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None:
491496
f"{type(raw_priority).__name__}."
492497
)
493498
existing_priorities.append(raw_priority)
499+
else:
500+
# Match `_load_catalog_config()`'s defaulting rule so the new
501+
# entry still sorts after implicit-priority siblings.
502+
existing_priorities.append(idx + 1)
494503

495504
max_priority = max(existing_priorities, default=0)
496505
catalogs.append(
@@ -520,7 +529,12 @@ def remove_catalog(self, index: int) -> str:
520529
if not config_path.exists():
521530
raise IntegrationValidationError("No catalog config file found.")
522531

523-
data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
532+
try:
533+
data = yaml.safe_load(config_path.read_text(encoding="utf-8")) or {}
534+
except (yaml.YAMLError, OSError, UnicodeError) as exc:
535+
raise IntegrationValidationError(
536+
f"Failed to read catalog config {config_path}: {exc}"
537+
) from exc
524538
if not isinstance(data, dict):
525539
raise IntegrationValidationError(
526540
"Catalog config file is corrupted (expected a mapping)."

tests/integrations/test_integration_catalog.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -820,6 +820,67 @@ def test_add_catalog_rejects_existing_entry_with_bad_url(self, tmp_path, monkeyp
820820
with pytest.raises(IntegrationCatalogError, match="HTTPS"):
821821
cat.add_catalog("https://good.example.com/catalog.json")
822822

823+
def test_add_catalog_wraps_yaml_parse_errors(self, tmp_path, monkeypatch):
824+
"""Invalid YAML on disk surfaces as IntegrationValidationError, not a raw YAMLError."""
825+
self._isolate(tmp_path, monkeypatch)
826+
cfg_path = tmp_path / ".specify" / "integration-catalogs.yml"
827+
cfg_path.write_text(
828+
"catalogs:\n - url: 'https://a.example.com/cat.json'\n - [bad\n",
829+
encoding="utf-8",
830+
)
831+
cat = IntegrationCatalog(tmp_path)
832+
with pytest.raises(
833+
IntegrationValidationError, match="Failed to read catalog config"
834+
):
835+
cat.add_catalog("https://b.example.com/catalog.json")
836+
837+
def test_remove_catalog_wraps_yaml_parse_errors(self, tmp_path, monkeypatch):
838+
"""Invalid YAML on disk surfaces as IntegrationValidationError from remove_catalog too."""
839+
self._isolate(tmp_path, monkeypatch)
840+
cfg_path = tmp_path / ".specify" / "integration-catalogs.yml"
841+
cfg_path.write_text(
842+
"catalogs:\n - url: 'https://a.example.com/cat.json'\n - [bad\n",
843+
encoding="utf-8",
844+
)
845+
cat = IntegrationCatalog(tmp_path)
846+
with pytest.raises(
847+
IntegrationValidationError, match="Failed to read catalog config"
848+
):
849+
cat.remove_catalog(0)
850+
851+
def test_add_catalog_defaults_missing_priority_to_index_plus_one(
852+
self, tmp_path, monkeypatch
853+
):
854+
"""Existing entries without `priority` should be treated as idx + 1.
855+
856+
Matches the rule in `_load_catalog_config()`: a valid catalog entry
857+
without an explicit `priority` sorts at `idx + 1`, so the new entry
858+
should get `max(...) + 1` from those derived values.
859+
"""
860+
self._isolate(tmp_path, monkeypatch)
861+
cfg_path = tmp_path / ".specify" / "integration-catalogs.yml"
862+
cfg_path.write_text(
863+
yaml.dump(
864+
{
865+
"catalogs": [
866+
# No explicit priority → should be treated as 1
867+
{"url": "https://a.example.com/cat.json", "name": "a"},
868+
# No explicit priority → should be treated as 2
869+
{"url": "https://b.example.com/cat.json", "name": "b"},
870+
]
871+
}
872+
),
873+
encoding="utf-8",
874+
)
875+
cat = IntegrationCatalog(tmp_path)
876+
cat.add_catalog("https://c.example.com/cat.json", name="c")
877+
878+
data = yaml.safe_load(cfg_path.read_text(encoding="utf-8"))
879+
new_entry = data["catalogs"][-1]
880+
assert new_entry["name"] == "c"
881+
# max(implicit [1, 2]) + 1 == 3
882+
assert new_entry["priority"] == 3
883+
823884
def test_remove_catalog_empty_list_gives_clear_error(self, tmp_path, monkeypatch):
824885
"""Hand-edited empty `catalogs:` produces a clear error, not '0--1'."""
825886
self._isolate(tmp_path, monkeypatch)

0 commit comments

Comments
 (0)