Skip to content

Commit ab52d2d

Browse files
committed
fix: address third Copilot review
1 parent af12d5a commit ab52d2d

2 files changed

Lines changed: 48 additions & 4 deletions

File tree

src/specify_cli/integrations/catalog.py

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -436,10 +436,13 @@ def get_catalog_configs(self) -> List[Dict[str, Any]]:
436436
def add_catalog(self, url: str, name: Optional[str] = None) -> None:
437437
"""Add a catalog source to the project-level config file.
438438
439-
The URL is validated before being written. Duplicate URLs are rejected.
440-
Priority is derived as ``max(existing) + 1`` so the new entry sorts last
441-
in the resolution order unless the user edits the file manually.
439+
The URL is normalized (whitespace stripped) and validated before being
440+
written. Duplicate URLs are rejected, including near-duplicates that
441+
differ only by surrounding whitespace. Priority is derived as
442+
``max(existing) + 1`` so the new entry sorts last in the resolution
443+
order unless the user edits the file manually.
442444
"""
445+
url = url.strip()
443446
self._validate_catalog_url(url)
444447
config_path = self.project_root / ".specify" / self.CONFIG_FILENAME
445448

@@ -581,7 +584,12 @@ def remove_catalog(self, index: int) -> str:
581584
# Deleting the file lets the project fall back to built-in
582585
# defaults, which matches the behavior before any
583586
# `catalog add` was ever run.
584-
config_path.unlink()
587+
try:
588+
config_path.unlink()
589+
except OSError as exc:
590+
raise IntegrationValidationError(
591+
f"Failed to delete catalog config {config_path}: {exc}"
592+
) from exc
585593

586594
if isinstance(removed, dict):
587595
return removed.get("name", f"catalog-{index + 1}")

tests/integrations/test_integration_catalog.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -881,6 +881,42 @@ def test_add_catalog_defaults_missing_priority_to_index_plus_one(
881881
# max(implicit [1, 2]) + 1 == 3
882882
assert new_entry["priority"] == 3
883883

884+
def test_add_catalog_strips_whitespace_in_url(self, tmp_path, monkeypatch):
885+
"""Whitespace around the incoming URL should be normalized before write."""
886+
self._isolate(tmp_path, monkeypatch)
887+
cat = IntegrationCatalog(tmp_path)
888+
cat.add_catalog(" https://a.example.com/catalog.json\n", name="a")
889+
890+
cfg_path = tmp_path / ".specify" / "integration-catalogs.yml"
891+
data = yaml.safe_load(cfg_path.read_text(encoding="utf-8"))
892+
assert data["catalogs"][0]["url"] == "https://a.example.com/catalog.json"
893+
894+
def test_add_catalog_rejects_whitespace_only_duplicate(self, tmp_path, monkeypatch):
895+
"""A second add with only whitespace differences must be rejected as a duplicate."""
896+
self._isolate(tmp_path, monkeypatch)
897+
cat = IntegrationCatalog(tmp_path)
898+
cat.add_catalog("https://a.example.com/catalog.json", name="a")
899+
with pytest.raises(IntegrationValidationError, match="already configured"):
900+
cat.add_catalog(" https://a.example.com/catalog.json ")
901+
902+
def test_remove_catalog_wraps_unlink_oserror(self, tmp_path, monkeypatch):
903+
"""An OSError from `Path.unlink` surfaces as IntegrationValidationError."""
904+
self._isolate(tmp_path, monkeypatch)
905+
cat = IntegrationCatalog(tmp_path)
906+
cat.add_catalog("https://only.example.com/catalog.json", name="only")
907+
908+
from pathlib import Path as _Path
909+
910+
def boom(self, *args, **kwargs):
911+
raise OSError("simulated unlink failure")
912+
913+
monkeypatch.setattr(_Path, "unlink", boom)
914+
915+
with pytest.raises(
916+
IntegrationValidationError, match="Failed to delete catalog config"
917+
):
918+
cat.remove_catalog(0)
919+
884920
def test_remove_catalog_empty_list_gives_clear_error(self, tmp_path, monkeypatch):
885921
"""Hand-edited empty `catalogs:` produces a clear error, not '0--1'."""
886922
self._isolate(tmp_path, monkeypatch)

0 commit comments

Comments
 (0)