diff --git a/datadog_sync/utils/configuration.py b/datadog_sync/utils/configuration.py index bfd37ca8..e274a46f 100644 --- a/datadog_sync/utils/configuration.py +++ b/datadog_sync/utils/configuration.py @@ -587,8 +587,10 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: raise click.UsageError("--minimize-reads requires --resource-per-file") if not kwargs.get("resources", None): raise click.UsageError("--minimize-reads requires --resources") - if kwargs.get("cleanup") and kwargs["cleanup"].lower() in ("true", "force"): - raise click.UsageError("--minimize-reads cannot be combined with --cleanup") + # The --cleanup compatibility check moved after the ID-targeting decision + # block below. Rationale: --minimize-reads has two sub-modes with + # different safety properties under --cleanup — see the sub-mode-aware + # guard after the state-strategy decision. # Validate --skip-state-load constraints. # The Click decorator lives on `_import.py` only, so the flag is never @@ -649,6 +651,43 @@ def build_config(cmd: Command, **kwargs: Optional[Any]) -> Configuration: ) _state_resource_types = raw_types + # Sub-mode-aware --cleanup compatibility check. + # + # --minimize-reads has two sub-modes selected above: + # * ID-targeted: state.destination[type] loads only the IDs listed by + # --filter exact-IDs or --id-file. A cleanup diff + # (destination[type] - source[type]) over a scoped-load is unsafe — + # legitimate destination records outside the scope are absent from + # the diff, and an ID that dropped from source between prior dispatch + # and now (e.g. a rename, temporary API 404, race) surfaces as an + # orphan and gets DELETE'd from the destination org. + # * Type-scoped: state.destination[type] loads all IDs of the requested + # types via the storage backend's prefix listing — see + # ``BaseStorage.get(origin, resource_types=[...])``. The diff is + # well-defined per type, functionally identical to full-load-then- + # cleanup for the types in scope. + # + # Sub-mode selection completed above. ID-targeted is signalled by + # ``_state_exact_ids is not None``. Note: ``id_payload`` alone is NOT the + # trigger — an --id-file whose types are disjoint from --resources leaves + # ``_state_exact_ids`` at None (intersection empty at line 636-643), the + # path falls through to type-scoped, and cleanup is safe. Gating on + # ``_state_exact_ids`` alone tracks the actual scoping decision. + if ( + minimize_reads + and _state_exact_ids is not None + and kwargs.get("cleanup") + and kwargs["cleanup"].lower() in ("true", "force") + ): + raise click.UsageError( + "--minimize-reads with --id-file or exact-id --filter cannot be " + "combined with --cleanup: state.destination is scoped to the " + "listed IDs, so the cleanup diff would delete legitimate " + "destination records outside the scope. Use type-scoped " + "--minimize-reads (--resources with no exact-id --filter, and " + "no --id-file whose types overlap --resources) with --cleanup." + ) + # Initialize state. --skip-state-load constructs an ImportState (write-only, # no source/destination accessors, no boot-time load); otherwise the regular # State class loads from storage on construction. diff --git a/tests/unit/test_minimize_reads_type_scoped.py b/tests/unit/test_minimize_reads_type_scoped.py index bd7c3ccd..2053e145 100644 --- a/tests/unit/test_minimize_reads_type_scoped.py +++ b/tests/unit/test_minimize_reads_type_scoped.py @@ -143,8 +143,26 @@ def test_minimize_reads_not_available_on_import_command(self, runner): assert result.exit_code != 0 assert "no such option" in result.output.lower() - def test_minimize_reads_cannot_be_combined_with_cleanup(self, runner): - """--minimize-reads + --cleanup must be rejected before any I/O.""" + def test_minimize_reads_type_scoped_with_cleanup_is_accepted(self, runner, tmp_path): + """--minimize-reads without --filter/--id-file selects the type-scoped + sub-mode, which loads full state.destination[type] via prefix listing. + The cleanup diff destination[type] - source[type] is well-defined in + this sub-mode, so the guardrail must accept the combination. + + Regression guard for the sub-mode-aware refinement: prior blanket + rejection incorrectly blocked this safe case, forcing wrapper-side + strips that reintroduced the O(N)→O(K) state-load regression for + types where --id-file is otherwise the fast path. + """ + # Use tmp local storage so the command actually initializes State + # instead of erroring on missing storage config. We assert exit + # against the guard specifically — the run will fail later on + # missing API creds, that's fine; the guard is validated + # separately below. + src = tmp_path / "src" + dst = tmp_path / "dst" + src.mkdir() + dst.mkdir() result = runner.invoke( cli, [ @@ -154,11 +172,144 @@ def test_minimize_reads_cannot_be_combined_with_cleanup(self, runner): "--resources=roles", "--cleanup=Force", "--source-api-key=x", + "--source-app-key=x", "--destination-api-key=y", + "--destination-app-key=y", + "--source-resources-path", + str(src), + "--destination-resources-path", + str(dst), + ], + ) + # The sub-mode-aware guard must NOT fire on type-scoped + cleanup. + # Assert BOTH the negative (guard message absent) AND the positive + # (invocation progressed past validation into runtime — evidenced by + # a downstream failure surface, not by exit_code == 0 which would + # require live API creds). This dual assertion prevents a future + # guard-message rename from silently satisfying only the negative + # substring check. + combined_output = (result.output + str(result.exception)).lower() + assert "cannot be combined with --cleanup" not in combined_output, ( + "sub-mode-aware guard incorrectly rejected type-scoped + cleanup: " + combined_output + ) + # Positive marker: some downstream indication that build_config + # returned and the sync command began running. UsageError from + # earlier validation stages surfaces as "usage:" in the click output; + # its absence combined with an alternative failure (network, auth, + # or successful exit) confirms we reached runtime. + assert "usage:" not in combined_output, ( + "unexpected earlier UsageError blocked the type-scoped path: " + combined_output + ) + + def test_minimize_reads_id_file_with_cleanup_is_rejected(self, runner, tmp_path): + """--minimize-reads + --id-file (or exact-id --filter) selects the + ID-targeted sub-mode, which loads only listed IDs into + state.destination[type]. Cleanup's diff would delete legitimate + destination records outside the ID scope — the guardrail must fire. + """ + id_file = tmp_path / "ids.json" + id_file.write_text('{"monitors": ["12345"]}') + result = runner.invoke( + cli, + [ + "sync", + "--minimize-reads", + "--resource-per-file", + "--resources=monitors", + "--id-file", + str(id_file), + "--cleanup=Force", + "--source-api-key=x", + "--source-app-key=x", + "--destination-api-key=y", + "--destination-app-key=y", ], ) assert result.exit_code != 0 - assert "cleanup" in (result.output + str(result.exception)).lower() + combined_output = (result.output + str(result.exception)).lower() + assert "id-file" in combined_output or "exact-id" in combined_output, ( + "guard message must name the actual trigger (id-file or exact-id filter): " + combined_output + ) + assert "cleanup" in combined_output + + def test_minimize_reads_exact_id_filter_with_cleanup_is_rejected(self, runner): + """Exact-id --filter also selects ID-targeted sub-mode; same guard + must fire with the sub-mode-aware message. + """ + result = runner.invoke( + cli, + [ + "sync", + "--minimize-reads", + "--resource-per-file", + "--resources=roles", + "--filter", + "Type=roles;Name=id;Value=role-1;Operator=exactmatch", + "--cleanup=Force", + "--source-api-key=x", + "--source-app-key=x", + "--destination-api-key=y", + "--destination-app-key=y", + ], + ) + assert result.exit_code != 0 + combined_output = (result.output + str(result.exception)).lower() + # Assert on the specific sub-mode-aware guard message, not a generic + # "cleanup or filter" substring — otherwise an unrelated failure that + # happens to mention "filter" would pass this test. + assert "cannot be combined with --cleanup" in combined_output, combined_output + assert "id-file or exact-id" in combined_output, combined_output + + def test_minimize_reads_id_file_disjoint_types_with_cleanup_is_accepted(self, runner, tmp_path): + """Regression guard for the disjoint-types case: --id-file supplies + IDs for types that are NOT in --resources. Sub-mode selection at + configuration.py:636-643 intersects id_payload with --resources; the + empty intersection leaves _state_exact_ids at None and falls through + to type-scoped loading. Guard must NOT fire — the effective sub-mode + is type-scoped, which is safe with --cleanup. + + Prior draft of the guard gated on ``bool(id_payload)`` and would + incorrectly fire here. + """ + id_file = tmp_path / "ids.json" + # id-file lists monitors, but --resources requests roles only. + # Intersection is empty; _state_exact_ids stays None; sub-mode is + # type-scoped. + id_file.write_text('{"monitors": ["12345"]}') + src = tmp_path / "src" + dst = tmp_path / "dst" + src.mkdir() + dst.mkdir() + result = runner.invoke( + cli, + [ + "sync", + "--minimize-reads", + "--resource-per-file", + "--resources=roles", + "--id-file", + str(id_file), + "--cleanup=Force", + "--source-api-key=x", + "--source-app-key=x", + "--destination-api-key=y", + "--destination-app-key=y", + "--source-resources-path", + str(src), + "--destination-resources-path", + str(dst), + ], + ) + combined_output = (result.output + str(result.exception)).lower() + assert "cannot be combined with --cleanup" not in combined_output, ( + "sub-mode-aware guard incorrectly rejected disjoint-id-file + type-scoped + cleanup: " + + combined_output + ) + # Positive marker: no earlier UsageError, so build_config progressed + # past all validation blocks (see companion test for rationale). + assert "usage:" not in combined_output, ( + "unexpected earlier UsageError blocked the disjoint-id-file path: " + combined_output + ) class TestS3TypeScopedGet: