diff --git a/src/specify_cli/bundler/commands_impl/catalog_config.py b/src/specify_cli/bundler/commands_impl/catalog_config.py index f763a21c65..50a13ae5dc 100644 --- a/src/specify_cli/bundler/commands_impl/catalog_config.py +++ b/src/specify_cli/bundler/commands_impl/catalog_config.py @@ -186,18 +186,20 @@ 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 = { "id": resolved_id, "url": url, "priority": int(priority), "install_policy": install_policy.value, } + for existing in catalogs: + if existing.get("id") == resolved_id or existing.get("url") == url: + if existing == entry: + return CatalogSource.from_dict(existing, Scope.PROJECT) + raise BundlerError( + f"Catalog source '{resolved_id}' (or url) already exists in this project." + ) + catalogs.append(entry) _write(project_root, catalogs) return CatalogSource.from_dict(entry, Scope.PROJECT) diff --git a/src/specify_cli/extensions/_commands.py b/src/specify_cli/extensions/_commands.py index 454482d054..97a9f56761 100644 --- a/src/specify_cli/extensions/_commands.py +++ b/src/specify_cli/extensions/_commands.py @@ -640,20 +640,24 @@ def catalog_add( safe_name = _escape_markup(name) safe_url = _escape_markup(url) + entry = { + "name": name, + "url": url, + "priority": priority, + "install_allowed": install_allowed, + "description": description, + } + # Check for duplicate name for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: + if existing == entry: + return console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") console.print("Use 'specify extension catalog remove' first, or choose a different name.") raise typer.Exit(1) - catalogs.append({ - "name": name, - "url": url, - "priority": priority, - "install_allowed": install_allowed, - "description": description, - }) + catalogs.append(entry) config["catalogs"] = catalogs config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8") diff --git a/src/specify_cli/integrations/catalog.py b/src/specify_cli/integrations/catalog.py index b8d76cb9c6..6a49409f56 100644 --- a/src/specify_cli/integrations/catalog.py +++ b/src/specify_cli/integrations/catalog.py @@ -432,6 +432,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. + normalized_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,6 +453,12 @@ 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: + generated_name = f"catalog-{valid_catalog_count + 1}" + requested_name = normalized_name or generated_name + existing_name = str(cat.get("name", generated_name)).strip() + if existing_name == requested_name: + self._load_catalog_config(config_path) + return raise IntegrationValidationError( f"Catalog URL already configured: {url}" ) @@ -478,9 +485,8 @@ def add_catalog(self, url: str, name: Optional[str] = None) -> None: # Match `_load_catalog_config()`'s defaulting rule so the new # entry still sorts after implicit-priority siblings. existing_priorities.append(idx + 1) - max_priority = max(existing_priorities, default=0) - normalized_name = str(name).strip() if name is not None else "" + max_priority = max(existing_priorities, default=0) generated_name = f"catalog-{valid_catalog_count + 1}" catalogs.append( { diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index c5dbf0ddca..beeed45000 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -928,20 +928,24 @@ def preset_catalog_add( safe_name = _escape_markup(str(name)) safe_url = _escape_markup(str(url)) + entry = { + "name": name, + "url": url, + "priority": priority, + "install_allowed": install_allowed, + "description": description, + } + # Check for duplicate name for existing in catalogs: if isinstance(existing, dict) and existing.get("name") == name: + if existing == entry: + return console.print(f"[yellow]Warning:[/yellow] A catalog named '{safe_name}' already exists.") console.print("Use 'specify preset catalog remove' first, or choose a different name.") raise typer.Exit(1) - catalogs.append({ - "name": name, - "url": url, - "priority": priority, - "install_allowed": install_allowed, - "description": description, - }) + catalogs.append(entry) config["catalogs"] = catalogs config_path.write_text(yaml.safe_dump(config, default_flow_style=False, sort_keys=False, allow_unicode=True), encoding="utf-8") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 5fffa4b45f..ca56d93e8e 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -732,8 +732,13 @@ def add_catalog(self, url: str, name: str | None = None) -> None: "Catalog config 'catalogs' must be a list." ) # Check for duplicate URL (guard against non-dict entries) - for cat in catalogs: + for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and cat.get("url") == url: + generated_name = f"catalog-{idx + 1}" + requested_name = name or generated_name + if cat.get("name", generated_name) == requested_name: + self._load_catalog_config(config_path) + return raise WorkflowValidationError( f"Catalog URL already configured: {url}" ) @@ -1414,8 +1419,13 @@ def add_catalog(self, url: str, name: str | None = None) -> None: raise StepValidationError( "Catalog config 'catalogs' must be a list." ) - for cat in catalogs: + for idx, cat in enumerate(catalogs): if isinstance(cat, dict) and cat.get("url") == url: + generated_name = f"catalog-{idx + 1}" + requested_name = name or generated_name + if cat.get("name", generated_name) == requested_name: + self._load_catalog_config(config_path) + return raise StepValidationError( f"Catalog URL already configured: {url}" ) diff --git a/tests/integrations/test_cli.py b/tests/integrations/test_cli.py index 2beb411a2a..5b1754a015 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_accepts_identical_duplicate(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) url = "https://dup.example.com/catalog.json" first = self._invoke( @@ -2549,8 +2549,14 @@ def test_catalog_add_rejects_duplicate(self, tmp_path, monkeypatch): second = self._invoke( ["integration", "catalog", "add", url], project ) - assert second.exit_code == 1 - assert "already configured" in second.output + assert second.exit_code == 0, second.output + + conflict = self._invoke( + ["integration", "catalog", "add", url, "--name", "different"], + project, + ) + assert conflict.exit_code == 1 + assert "already configured" in conflict.output def test_catalog_remove_out_of_range(self, tmp_path, monkeypatch): project = self._make_project(tmp_path) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index fb6da1803e..7d44c11d61 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7568,6 +7568,32 @@ def test_extensionignore_negation_pattern(self, temp_dir, valid_manifest_data): class TestExtensionAddCLI: """CLI integration tests for extension add command.""" + def test_catalog_add_is_idempotent_for_identical_entry(self, tmp_path): + 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): + assert runner.invoke(app, args).exit_code == 0 + config_path = project_dir / ".specify" / "extension-catalogs.yml" + original = config_path.read_bytes() + assert runner.invoke(app, args).exit_code == 0 + assert config_path.read_bytes() == original + assert runner.invoke(app, [*args, "--priority", "11"]).exit_code == 1 + def test_catalog_add_escapes_url_markup(self, tmp_path): """Catalog add should render user-supplied URLs literally.""" from typer.testing import CliRunner diff --git a/tests/test_presets.py b/tests/test_presets.py index 17aef20dce..b154c24986 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -3850,6 +3850,28 @@ def test_default_description(self): class TestPresetCatalogMultiCatalog: """Test multi-catalog support in PresetCatalog.""" + def test_catalog_add_is_idempotent_for_identical_entry(self, project_dir): + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + args = [ + "preset", + "catalog", + "add", + "https://example.com/catalog.json", + "--name", + "community", + ] + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + assert runner.invoke(app, args).exit_code == 0 + config_path = project_dir / ".specify" / "preset-catalogs.yml" + original = config_path.read_bytes() + assert runner.invoke(app, args).exit_code == 0 + assert config_path.read_bytes() == original + assert runner.invoke(app, [*args, "--priority", "11"]).exit_code == 1 + def test_default_active_catalogs(self, project_dir): """Test that default catalogs are returned when no config exists.""" catalog = PresetCatalog(project_dir) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index ac9f47b405..c7df1bbc3c 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8786,14 +8786,20 @@ 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_is_idempotent_for_identical_url_and_name( + self, project_dir + ): from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowValidationError catalog = WorkflowCatalog(project_dir) catalog.add_catalog("https://example.com/catalog.json") + config_path = project_dir / ".specify" / "workflow-catalogs.yml" + original = config_path.read_bytes() + catalog.add_catalog("https://example.com/catalog.json") + assert config_path.read_bytes() == original with pytest.raises(WorkflowValidationError, match="already configured"): - catalog.add_catalog("https://example.com/catalog.json") + catalog.add_catalog("https://example.com/catalog.json", "different") def test_remove_catalog(self, project_dir): from specify_cli.workflows.catalog import WorkflowCatalog @@ -9528,14 +9534,20 @@ 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_is_idempotent_for_identical_url_and_name( + self, project_dir + ): from specify_cli.workflows.catalog import StepCatalog, StepValidationError catalog = StepCatalog(project_dir) catalog.add_catalog("https://example.com/steps.json") + config_path = project_dir / ".specify" / "step-catalogs.yml" + original = config_path.read_bytes() + catalog.add_catalog("https://example.com/steps.json") + assert config_path.read_bytes() == original with pytest.raises(StepValidationError, match="already configured"): - catalog.add_catalog("https://example.com/steps.json") + catalog.add_catalog("https://example.com/steps.json", "different") 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..62be668b5b 100644 --- a/tests/unit/test_bundler_catalog_config.py +++ b/tests/unit/test_bundler_catalog_config.py @@ -69,6 +69,31 @@ def test_add_source_persists_absolute_local_path(tmp_path: Path, monkeypatch): assert Path(source.url) == catalog.resolve() +def test_add_source_is_idempotent_for_identical_entry(tmp_path: Path): + project = tmp_path / "proj" + (project / ".specify").mkdir(parents=True) + args = { + "policy": "install-allowed", + "priority": 50, + "source_id": "example", + } + + first = cc.add_source(project, "https://example.com/catalog.json", **args) + original = cc._config_path(project).read_bytes() + second = cc.add_source(project, "https://example.com/catalog.json", **args) + + assert second == first + assert cc._config_path(project).read_bytes() == original + with pytest.raises(BundlerError, match="already exists"): + cc.add_source( + project, + "https://example.com/catalog.json", + policy="install-allowed", + priority=51, + source_id="example", + ) + + def test_remove_source_accepts_relative_local_path(tmp_path: Path, monkeypatch): """add_source stores a local path as an absolute url, so remove_source must accept the same relative path the caller added; otherwise `remove ./cat.json`