diff --git a/docs/reference/bundles.md b/docs/reference/bundles.md index 2bd33c960b..7bbda6e948 100644 --- a/docs/reference/bundles.md +++ b/docs/reference/bundles.md @@ -152,6 +152,8 @@ specify bundle catalog add Registers a project-scoped catalog source and persists it. +Adding a source is idempotent (identity is the source **id or url**): re-running `catalog add` with the same id/url and identical `--policy`/`--priority` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding a matching id/url with *different* settings is rejected as a conflict rather than silently overwriting the existing source — remove it first to change it. + ### Remove a Catalog Source ```bash diff --git a/docs/reference/extensions.md b/docs/reference/extensions.md index 0473e72008..affe8f3331 100644 --- a/docs/reference/extensions.md +++ b/docs/reference/extensions.md @@ -140,6 +140,8 @@ specify extension catalog add Adds a catalog to the project's `.specify/extension-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/integrations.md b/docs/reference/integrations.md index 551f73c97e..f125a4aeeb 100644 --- a/docs/reference/integrations.md +++ b/docs/reference/integrations.md @@ -213,6 +213,8 @@ specify integration catalog add Adds a custom catalog URL to the project's `.specify/integration-catalogs.yml`. The URL must use HTTPS (except `http://localhost`, `http://127.0.0.1`, or `http://[::1]` for local testing). +Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 1098abfb42..fac50589ab 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -109,6 +109,8 @@ specify preset catalog add Adds a catalog to the project's `.specify/preset-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **name**): re-running `catalog add` with the same name and identical settings is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same name with *different* settings is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index e2deaf181c..0d06f1df2d 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -405,6 +405,8 @@ specify workflow catalog add Adds a custom catalog URL to the project's `.specify/workflow-catalogs.yml`. +Adding a catalog is idempotent (identity is the catalog **URL**): re-running `catalog add` with the same URL and the same (or no) `--name` is a successful no-op (exit code 0), so it is safe to include in a re-runnable workflow. Re-adding the same URL with a *different* `--name` is rejected as a conflict rather than silently overwriting the existing entry — remove it first to change it. + ### Remove a Catalog ```bash diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index f763a21c65..7bc669d093 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -139,7 +139,7 @@ def add_source( policy: str, priority: int, source_id: str | None = None, -) -> CatalogSource: +) -> tuple[CatalogSource, str]: url = url.strip() if not url: raise BundlerError("A catalog url is required.") @@ -186,21 +186,32 @@ def add_source( resolved_id = (source_id or _derive_id(url)).strip() catalogs = _read(project_root) - for existing in catalogs: - if existing.get("id") == resolved_id or existing.get("url") == url: - raise BundlerError( - f"Catalog source '{resolved_id}' (or url) already exists in this project." - ) - - entry = { + desired = { "id": resolved_id, "url": url, "priority": int(priority), "install_policy": install_policy.value, } - catalogs.append(entry) + for existing in catalogs: + if existing.get("id") == resolved_id or existing.get("url") == url: + # Idempotent add (#4505): identity is the source id or url. A rerun + # requesting the same settings is a successful no-op; differing + # settings are a conflict rather than a silent overwrite. + if ( + existing.get("id") == resolved_id + and existing.get("url") == url + and int(existing.get("priority", 0)) == desired["priority"] + and str(existing.get("install_policy", "")) == desired["install_policy"] + ): + return CatalogSource.from_dict(dict(existing), Scope.PROJECT), "unchanged" + raise BundlerError( + f"Catalog source '{resolved_id}' (or url) already exists in this " + "project with different settings. Remove it first to change it." + ) + + catalogs.append(desired) _write(project_root, catalogs) - return CatalogSource.from_dict(entry, Scope.PROJECT) + return CatalogSource.from_dict(desired, Scope.PROJECT), "added" def remove_source(project_root: Path, id_or_url: str) -> str: diff --git a/src/specify_cli/commands/bundle/__init__.py b/src/specify_cli/commands/bundle/__init__.py index 165f674a36..b298452300 100644 --- a/src/specify_cli/commands/bundle/__init__.py +++ b/src/specify_cli/commands/bundle/__init__.py @@ -662,15 +662,21 @@ def catalog_add( project_root = require_project_root() from ...bundler.commands_impl.catalog_config import add_source - source = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) + source, status = add_source(project_root, url, policy=policy, priority=priority, source_id=source_id) except BundlerError as exc: _fail(str(exc)) return - console.print( - f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " - f"(priority {source.priority}, {source.install_policy.value})." - ) + if status == "unchanged": + console.print( + f"[green]✓[/green] Catalog '{_escape_markup(str(source.id))}' is already " + f"configured (priority {source.priority}, {source.install_policy.value})." + ) + else: + console.print( + f"[green]✓[/green] Added catalog '{_escape_markup(str(source.id))}' " + f"(priority {source.priority}, {source.install_policy.value})." + ) @bundle_catalog_app.command("remove") diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 11ab50385e..d573ef4a72 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -613,10 +613,28 @@ def catalog_add( safe_name = _escape_markup(name) safe_url = _escape_markup(url) - # Check for duplicate name + # Idempotent add (#4505): a rerun that requests an identical entry is a + # successful no-op so the same `catalog add` can live in a re-runnable + # workflow without failing. A same-name entry whose settings differ is + # still a conflict — we refuse to silently change priority/install + # permissions and ask the user to remove it first. for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: - console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + if ( + str(existing.get("url", "")) == url + and existing.get("priority") == priority + and bool(existing.get("install_allowed", False)) == install_allowed + and str(existing.get("description", "")) == description + ): + console.print( + f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " + "configured with these settings; nothing to do." + ) + return + console.print( + f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " + "different settings." + ) console.print("Use 'specify extension catalog remove' first, or choose a different name.") raise typer.Exit(1) diff --git a/src/specify_cli/integrations/_query_commands.py b/src/specify_cli/integrations/_query_commands.py index 0cd254879a..5525822923 100644 --- a/src/specify_cli/integrations/_query_commands.py +++ b/src/specify_cli/integrations/_query_commands.py @@ -543,14 +543,19 @@ def integration_catalog_add( normalized_url = url.strip() try: - catalog.add_catalog(normalized_url, name) + status = catalog.add_catalog(normalized_url, name) except IntegrationCatalogError as exc: # Covers both URL validation (base class) and config-file validation # (IntegrationValidationError subclass). console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") + if status == "unchanged": + console.print( + f"[green]✓[/green] Catalog source already configured: {normalized_url}" + ) + else: + console.print(f"[green]✓[/green] Catalog source added: {normalized_url}") @integration_catalog_app.command("remove") diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index b8d76cb9c6..70dcb9caac 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -390,14 +390,20 @@ def get_project_catalog_configs(self) -> Optional[List[Dict[str, Any]]]: for e in entries ] - def add_catalog(self, url: str, name: Optional[str] = None) -> None: + def add_catalog(self, url: str, name: Optional[str] = None) -> str: """Add a catalog source to the project-level config file. The URL is normalized (whitespace stripped) and validated before being - written. Duplicate URLs are rejected, including near-duplicates that - differ only by surrounding whitespace. Priority is derived as - ``max(existing) + 1`` so the new entry sorts last in the resolution - order unless the user edits the file manually. + written. Identity for an integration catalog is the (normalized) URL. + Adding a URL that is already configured is idempotent (#4505): a rerun + that requests the same name (or no explicit name) is a successful + no-op, while a rerun that requests a *different* name is rejected as a + conflict rather than silently overwriting the stored entry. Priority is + derived as ``max(existing) + 1`` so a newly added entry sorts last in + the resolution order unless the user edits the file manually. + + Returns ``"added"`` when a new entry is written, or ``"unchanged"`` + when an equivalent entry already existed. """ url = url.strip() if not url: @@ -432,6 +438,7 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: # Validate each existing entry before mutating anything. Fail fast so # we don't silently preserve a corrupt sibling entry or derive a new # priority from a bogus value. + requested_name = str(name).strip() if name is not None else "" existing_priorities: List[int] = [] valid_catalog_count = 0 for idx, cat in enumerate(catalogs): @@ -452,8 +459,14 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: f"Invalid catalog entry at index {idx} in {config_path}: {exc}" ) from exc if existing_url == url: + # Idempotent add (#4505): same URL already configured. + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise IntegrationValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) valid_catalog_count += 1 if "priority" in cat: @@ -502,6 +515,7 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: sort_keys=False, allow_unicode=True, ) + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by 0-based index. diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index ab74a8e029..8f8f3c39dc 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -902,10 +902,28 @@ def preset_catalog_add( safe_name = _escape_markup(str(name)) safe_url = _escape_markup(str(url)) - # Check for duplicate name + # Idempotent add (#4505): a rerun that requests an identical entry is a + # successful no-op so the same `catalog add` can live in a re-runnable + # workflow without failing. A same-name entry whose settings differ is + # still a conflict — we refuse to silently change priority/install + # permissions and ask the user to remove it first. for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: - console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") + if ( + str(existing.get("url", "")) == url + and existing.get("priority") == priority + and bool(existing.get("install_allowed", False)) == install_allowed + and str(existing.get("description", "")) == description + ): + console.print( + f"[green]✓[/green] Catalog '[bold]{safe_name}[/bold]' is already " + "configured with these settings; nothing to do." + ) + return + console.print( + f"[red]Error:[/red] A catalog named '{safe_name}' already exists with " + "different settings." + ) console.print("Use 'specify preset catalog remove' first, or choose a different name.") raise typer.Exit(1) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index 716b6c8a19..1cd9cc6d07 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -3023,12 +3023,15 @@ def workflow_catalog_add( project_root = _require_specify_project() catalog = WorkflowCatalog(project_root) try: - catalog.add_catalog(url, name) + status = catalog.add_catalog(url, name) except WorkflowValidationError as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Catalog source added: {url}") + if status == "unchanged": + console.print(f"[green]✓[/green] Catalog source already configured: {url}") + else: + console.print(f"[green]✓[/green] Catalog source added: {url}") @workflow_catalog_app.command("remove") @@ -3750,12 +3753,15 @@ def workflow_step_catalog_add( catalog = StepCatalog(project_root) try: - catalog.add_catalog(url, name) + status = catalog.add_catalog(url, name) except StepValidationError as exc: console.print(f"[red]Error:[/red] {exc}") raise typer.Exit(1) - console.print(f"[green]✓[/green] Step catalog source added: {url}") + if status == "unchanged": + console.print(f"[green]✓[/green] Step catalog source already configured: {url}") + else: + console.print(f"[green]✓[/green] Step catalog source added: {url}") @workflow_step_catalog_app.command("remove") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 5fffa4b45f..72b3431e0d 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -705,8 +705,15 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> None: - """Add a catalog source to the project-level config.""" + def add_catalog(self, url: str, name: str | None = None) -> str: + """Add a catalog source to the project-level config. + + Identity is the URL: adding a URL that is already configured is + idempotent (#4505). A rerun requesting the same name (or no explicit + name) is a no-op that returns ``"unchanged"``; a rerun requesting a + different name is rejected as a conflict. Returns ``"added"`` when a + new entry is written. + """ self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "workflow-catalogs.yml" @@ -731,11 +738,18 @@ def add_catalog(self, url: str, name: str | None = None) -> None: raise WorkflowValidationError( "Catalog config 'catalogs' must be a list." ) - # Check for duplicate URL (guard against non-dict entries) + # Idempotent add (#4505): identity is the URL. A rerun requesting the + # same name (or no explicit name) is a no-op; a different name conflicts. + requested_name = str(name).strip() if name is not None else "" for cat in catalogs: if isinstance(cat, dict) and cat.get("url") == url: + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise WorkflowValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) # Derive priority from the highest existing priority + 1. @@ -776,6 +790,7 @@ def _coerce_priority(value: Any) -> int: raise WorkflowValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" @@ -1388,8 +1403,15 @@ def get_catalog_configs(self) -> list[dict[str, Any]]: for e in entries ] - def add_catalog(self, url: str, name: str | None = None) -> None: - """Add a catalog source to the project-level config.""" + def add_catalog(self, url: str, name: str | None = None) -> str: + """Add a catalog source to the project-level config. + + Identity is the URL: adding a URL that is already configured is + idempotent (#4505). A rerun requesting the same name (or no explicit + name) is a no-op that returns ``"unchanged"``; a rerun requesting a + different name is rejected as a conflict. Returns ``"added"`` when a + new entry is written. + """ self._validate_catalog_url(url) config_path = self.project_root / ".specify" / "step-catalogs.yml" @@ -1414,10 +1436,18 @@ def add_catalog(self, url: str, name: str | None = None) -> None: raise StepValidationError( "Catalog config 'catalogs' must be a list." ) + # Idempotent add (#4505): identity is the URL. A rerun requesting the + # same name (or no explicit name) is a no-op; a different name conflicts. + requested_name = str(name).strip() if name is not None else "" for cat in catalogs: if isinstance(cat, dict) and cat.get("url") == url: + existing_name = str(cat.get("name", "")).strip() + if not requested_name or requested_name == existing_name: + return "unchanged" raise StepValidationError( - f"Catalog URL already configured: {url}" + f"Catalog URL already configured with a different name " + f"('{existing_name}'): {url}. Remove it first or pass " + f"--name '{existing_name}'." ) # Coerce existing priorities to int with a safe fallback so a user-edited @@ -1459,6 +1489,7 @@ def _coerce_priority(value: Any) -> int: raise StepValidationError( f"Failed to write catalog config {config_path}: {exc}" ) from exc + return "added" def remove_catalog(self, index: int) -> str: """Remove a catalog source by index (0-based). Returns the removed name.""" diff --git a/tests/contract/test_bundle_cli.py b/tests/contract/test_bundle_cli.py index 6db4dab769..3029064e2a 100644 --- a/tests/contract/test_bundle_cli.py +++ b/tests/contract/test_bundle_cli.py @@ -211,6 +211,40 @@ def test_catalog_add_and_remove(project: Path): assert removed.exit_code == 0 +def test_catalog_add_duplicate_is_idempotent(project: Path): + catalog = project / "local-catalog.json" + write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) + + first = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert first.exit_code == 0, first.output + second = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert second.exit_code == 0, second.output + assert "already" in second.output + + +def test_catalog_add_duplicate_different_settings_conflicts(project: Path): + catalog = project / "local-catalog.json" + write_catalog_file(catalog, {"demo": catalog_entry_dict("demo")}) + + first = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "10"], + ) + assert first.exit_code == 0, first.output + second = runner.invoke( + app, + ["bundle", "catalog", "add", str(catalog), "--id", "local", "--priority", "20"], + ) + assert second.exit_code == 1 + assert "different settings" in second.output + + def test_catalog_remove_builtin_is_refused(project: Path): result = runner.invoke(app, ["bundle", "catalog", "remove", "default"]) assert result.exit_code == 1 diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index 640d12a5fc..df6658d665 100644 --- a/tests/integrations/test_cli.py +++ b/tests/integrations/test_cli.py @@ -2539,7 +2539,7 @@ def test_catalog_add_rejects_invalid_url(self, tmp_path, monkeypatch): assert result.exit_code == 1 assert "HTTPS" in result.output - def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): + def test_catalog_add_duplicate_is_idempotent(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) url = "https://dup.example.com/catalog.json" first = self._invoke( @@ -2549,9 +2549,22 @@ def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): second = self._invoke( ["integration", "catalog", "add", url], project ) - assert second.exit_code == 1 + assert second.exit_code == 0, second.output assert "already configured" in second.output + def test_catalog_add_duplicate_different_name_conflicts(self, tmp_path, monkeypatch): + project = self._make_project(tmp_path) + url = "https://dup.example.com/catalog.json" + first = self._invoke( + ["integration", "catalog", "add", url, "--name", "first"], project + ) + assert first.exit_code == 0, first.output + second = self._invoke( + ["integration", "catalog", "add", url, "--name", "second"], project + ) + assert second.exit_code == 1 + assert "different name" in second.output + def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) # Need a config file for remove to attempt an index lookup diff --git a/tests/integrations/test_integration_catalog.py b/tests/integrations/test_integration_catalog.py index c414c3d8ea..f413640c37 100644 --- a/tests/integrations/test_integration_catalog.py +++ b/tests/integrations/test_integration_catalog.py @@ -1168,12 +1168,23 @@ def test_add_catalog_normalizes_name(self, tmp_path, monkeypatch): entries = data["catalogs"] assert [e["name"] for e in entries] == ["mine", "catalog-2"] - def test_add_catalog_rejects_duplicate_url(self, tmp_path, monkeypatch): + def test_add_catalog_duplicate_url_is_idempotent_noop(self, tmp_path, monkeypatch): + """Re-adding the same URL (no explicit name) is a successful no-op (#4505).""" self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) - cat.add_catalog("https://dup.example.com/catalog.json") - with pytest.raises(IntegrationValidationError, match="already configured"): - cat.add_catalog("https://dup.example.com/catalog.json") + assert cat.add_catalog("https://dup.example.com/catalog.json") == "added" + assert cat.add_catalog("https://dup.example.com/catalog.json") == "unchanged" + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_url_different_name_conflicts(self, tmp_path, monkeypatch): + """Re-adding the same URL with a different name is rejected as a conflict (#4505).""" + self._isolate(tmp_path, monkeypatch) + cat = IntegrationCatalog(tmp_path) + cat.add_catalog("https://dup.example.com/catalog.json", name="first") + with pytest.raises(IntegrationValidationError, match="different name"): + cat.add_catalog("https://dup.example.com/catalog.json", name="second") def test_add_catalog_rejects_invalid_url(self, tmp_path, monkeypatch): self._isolate(tmp_path, monkeypatch) @@ -1502,13 +1513,15 @@ def test_add_catalog_strips_whitespace_in_url(self, tmp_path, monkeypatch): data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) assert data["catalogs"][0]["url"] == "https://a.example.com/catalog.json" - def test_add_catalog_rejects_whitespace_only_duplicate(self, tmp_path, monkeypatch): - """A second add with only whitespace differences must be rejected as a duplicate.""" + def test_add_catalog_whitespace_only_duplicate_is_noop(self, tmp_path, monkeypatch): + """A second add differing only by whitespace (no new name) is an idempotent no-op.""" self._isolate(tmp_path, monkeypatch) cat = IntegrationCatalog(tmp_path) cat.add_catalog("https://a.example.com/catalog.json", name="a") - with pytest.raises(IntegrationValidationError, match="already configured"): - cat.add_catalog(" https://a.example.com/catalog.json ") + assert cat.add_catalog(" https://a.example.com/catalog.json ") == "unchanged" + cfg_path = tmp_path / ".specify" / "integration-catalogs.yml" + data = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 def test_remove_catalog_wraps_unlink_oserror(self, tmp_path, monkeypatch): """An OSError from `Path.unlink` surfaces as IntegrationValidationError.""" diff --git a/tests/test_extensions.py b/tests/test_extensions.py index aec32dc4ba..8163984263 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7420,6 +7420,56 @@ def test_catalog_add_escapes_url_markup(self, tmp_path): assert result.exit_code == 0, result.output assert f"URL: {url}" in result.output + def test_catalog_add_duplicate_is_idempotent(self, tmp_path): + """Re-adding an identical catalog is a successful no-op (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + args = [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, args, catch_exceptions=True) + second = runner.invoke(app, args, catch_exceptions=True) + + assert first.exit_code == 0, first.output + assert second.exit_code == 0, second.output + assert "nothing to do" in second.output + + def test_catalog_add_duplicate_different_settings_conflicts(self, tmp_path): + """Re-adding a same-named catalog with different settings errors (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + project_dir = tmp_path / "test-project" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + "--priority", "10", + ], catch_exceptions=True) + second = runner.invoke(app, [ + "extension", "catalog", "add", + "https://example.com/catalog.json", "--name", "community", + "--priority", "20", + ], catch_exceptions=True) + + assert first.exit_code == 0, first.output + assert second.exit_code == 1 + assert "different settings" in second.output + def test_catalog_add_escapes_config_saved_path_markup(self, tmp_path): """Catalog add's saved-path label should render literally under Rich.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index 57a70b4192..693f03141e 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3903,6 +3903,44 @@ def test_catalog_add_escapes_rich_markup(self, project_dir): assert config["catalogs"][0]["name"] == name assert config["catalogs"][0]["url"] == url + def test_catalog_add_duplicate_is_idempotent(self, project_dir): + """Re-adding an identical preset catalog is a successful no-op (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + args = [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, args) + second = runner.invoke(app, args) + assert first.exit_code == 0, first.output + assert second.exit_code == 0, second.output + assert "nothing to do" in second.output + + def test_catalog_add_duplicate_different_settings_conflicts(self, project_dir): + """Re-adding a same-named preset catalog with different settings errors (#4505).""" + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + first = runner.invoke(app, [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", "--priority", "10", + ]) + second = runner.invoke(app, [ + "preset", "catalog", "add", + "https://example.com/c.json", "--name", "mine", "--priority", "20", + ]) + assert first.exit_code == 0, first.output + assert second.exit_code == 1 + assert "different settings" in second.output + def test_catalog_remove_escapes_rich_markup(self, project_dir): """`preset catalog remove` must not parse the name as Rich markup.""" from typer.testing import CliRunner diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 9b1c17881e..e28ac385bc 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8556,14 +8556,24 @@ def test_add_catalog_with_existing_inf_priority(self, project_dir): new = next(c for c in data["catalogs"] if c["url"] == "https://b.example.com/c.json") assert new["priority"] == 1 # max(inf coerced to 0) + 1 - def test_add_catalog_duplicate_rejected(self, project_dir): + def test_add_catalog_duplicate_is_idempotent(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError catalog = WorkflowCatalog(project_dir) - catalog.add_catalog("https://example.com/catalog.json") + assert catalog.add_catalog("https://example.com/catalog.json") == "added" + assert catalog.add_catalog("https://example.com/catalog.json") == "unchanged" - with pytest.raises(WorkflowValidationError, match="already configured"): - catalog.add_catalog("https://example.com/catalog.json") + cfg = project_dir / ".specify" / "workflow-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError + + catalog = WorkflowCatalog(project_dir) + catalog.add_catalog("https://example.com/catalog.json", "first") + with pytest.raises(WorkflowValidationError, match="different name"): + catalog.add_catalog("https://example.com/catalog.json", "second") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -9298,14 +9308,24 @@ def test_add_catalog_rejects_falsy_non_mapping_config( assert config_path.read_text(encoding="utf-8") == original - def test_add_catalog_duplicate_rejected(self, project_dir): + def test_add_catalog_duplicate_is_idempotent(self, project_dir): from specify_cli.workflows.catalog import StepCatalog, StepValidationError catalog = StepCatalog(project_dir) - catalog.add_catalog("https://example.com/steps.json") + assert catalog.add_catalog("https://example.com/steps.json") == "added" + assert catalog.add_catalog("https://example.com/steps.json") == "unchanged" - with pytest.raises(StepValidationError, match="already configured"): - catalog.add_catalog("https://example.com/steps.json") + cfg = project_dir / ".specify" / "step-catalogs.yml" + data = yaml.safe_load(cfg.read_text(encoding="utf-8")) + assert len(data["catalogs"]) == 1 + + def test_add_catalog_duplicate_different_name_conflicts(self, project_dir): + from specify_cli.workflows.catalog import StepCatalog, StepValidationError + + catalog = StepCatalog(project_dir) + catalog.add_catalog("https://example.com/steps.json", "first") + with pytest.raises(StepValidationError, match="different name"): + catalog.add_catalog("https://example.com/steps.json", "second") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import StepCatalog diff --git a/tests/unit/test_bundler_catalog_config.py b/tests/unit/test_bundler_catalog_config.py index 46c333700a..26bdcbc95c 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -63,8 +63,9 @@ def test_add_source_persists_absolute_local_path(tmp_path: Path, monkeypatch): catalog.write_text("{}", encoding="utf-8") monkeypatch.chdir(project) - source = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + source, status = cc.add_source(project, "sub/cat.json", policy="install-allowed", priority=50) + assert status == "added" assert Path(source.url).is_absolute() assert Path(source.url) == catalog.resolve() @@ -234,7 +235,7 @@ def test_add_source_allows_local_path_with_colon(tmp_path: Path, monkeypatch): (project / ".specify").mkdir(parents=True) monkeypatch.chdir(project) # A relative path containing ':' but no '://' is still a local path. - source = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) + source, _ = cc.add_source(project, "weird:name.json", policy="install-allowed", priority=50) assert source.url.endswith("weird:name.json") or "weird" in source.url @@ -248,7 +249,7 @@ def test_add_source_rejects_plain_http_for_non_localhost(tmp_path: Path): def test_add_source_allows_http_for_localhost(tmp_path: Path): project = tmp_path / "proj" (project / ".specify").mkdir(parents=True) - source = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) + source, _ = cc.add_source(project, "http://localhost:8080/c.json", policy="install-allowed", priority=50) assert source.url == "http://localhost:8080/c.json"