diff --git a/datadog_sync/utils/configuration.py b/datadog_sync/utils/configuration.py index 9934458b..ef7c8ffc 100644 --- a/datadog_sync/utils/configuration.py +++ b/datadog_sync/utils/configuration.py @@ -112,12 +112,10 @@ class Configuration(object): allow_partial_permissions_roles: List[str] = field(default_factory=list) resources: Dict[str, BaseResource] = field(default_factory=dict) resources_arg: List[str] = field(default_factory=list) - # --id-file: id-targeted import via stdin or file payload. - # Supported resource types are explicitly allowlisted in - # _ID_FILE_SUPPORTED_TYPES (currently monitors, authn_mappings, - # team_memberships). Other types use the legacy list-everything path. - # Future expansion (e.g. SLOs) is mechanical via - # BaseResource.get_resources_by_ids inheritance. + # --id-file: id-targeted import and sync state-load scoping via stdin or + # file payload. Supported resource types are explicitly allowlisted in + # _ID_FILE_SUPPORTED_TYPES; runtime paths narrow that union to the + # command-specific import and state-load sets. id_payload: Optional[Dict[str, List[str]]] = None max_concurrent_reads: int = 30 transient_failure_threshold_pct: int = 5 @@ -253,7 +251,7 @@ def _unwrap_exact_match_pattern(pattern: str) -> str: return _regex_literal_from_exact_match_body(pattern[1:-1]) -_ID_FILE_IMPORT_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships"}) +_ID_FILE_IMPORT_SUPPORTED_TYPES = frozenset({"monitors", "authn_mappings", "team_memberships", "dashboards"}) """Resource types eligible for --id-file on the import command. The import path fans out to per-ID GETs via BaseResource.get_resources_by_ids. @@ -264,6 +262,14 @@ def _unwrap_exact_match_pattern(pattern: str) -> str: Widening this set requires per-model verification of the fan-out path. Do NOT widen by config — code-level allowlist forces explicit review. + +dashboards: Dashboards.import_resource(_id=...) performs a real GET to +/api/v1/dashboard/{id} for the full body (widgets are omitted by the LIST +endpoint). The model also short-circuits when the caller passes a body +already carrying widgets, so the id-file path's get_resources_by_ids -> +import_resource(_id=...) -> queue-handler _import_resource(resource=body) +sequence does exactly one GET per dashboard (no double-fetch). Verified via +tests/unit/test_dashboards_id_file.py. """ @@ -273,6 +279,7 @@ def _unwrap_exact_match_pattern(pattern: str) -> str: "team_memberships", "host_tags", "metrics_metadata", + "dashboards", }) """Resource types eligible for --id-file on the sync command with --minimize-reads. @@ -288,6 +295,11 @@ def _unwrap_exact_match_pattern(pattern: str) -> str: - metrics_metadata: state key is metric name; import_resource returns (metric_name, resource). Storage layout: resources/source/metrics_metadata. .json. +- dashboards: state key is the dashboard id (e.g. abc-def-ghi). Storage + layout: resources/source/dashboards..json. ID-derivable, so + State.get_by_ids constructs the correct key. Added alongside the import + allowlist entry so the union (_ID_FILE_SUPPORTED_TYPES) accepts dashboards + on both paths by design rather than incidentally via the import set. Do NOT widen by config — code-level allowlist forces explicit review. """ diff --git a/tests/unit/test_dashboards_id_file.py b/tests/unit/test_dashboards_id_file.py new file mode 100644 index 00000000..ab3a4582 --- /dev/null +++ b/tests/unit/test_dashboards_id_file.py @@ -0,0 +1,149 @@ +# Unless explicitly stated otherwise all files in this repository are licensed +# under the 3-clause BSD style license (see LICENSE). +# This product includes software developed at Datadog (https://www.datadoghq.com/). +# Copyright 2019 Datadog, Inc. + +"""Tests for dashboards support in _ID_FILE_IMPORT_SUPPORTED_TYPES. + +Pins that 'dashboards' is accepted by --id-file and that the per-ID GET +path used by get_resources_by_ids (import --id-file) does exactly one GET +per dashboard, with the prefetched-body short-circuit preventing a +double-fetch on queue-handler re-entry. +""" + +import asyncio +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from datadog_sync.model.dashboards import Dashboards +from datadog_sync.utils.configuration import ( + _ID_FILE_IMPORT_SUPPORTED_TYPES, + _ID_FILE_SUPPORTED_TYPES, +) +from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource + + +class TestDashboardsIDFileSupport: + """Tests for dashboards support in the --id-file allowlist.""" + + def test_dashboards_in_id_file_import_supported_types(self): + """'dashboards' must be present in _ID_FILE_IMPORT_SUPPORTED_TYPES so + that `import --id-file=- < {"dashboards": [...]}` is accepted.""" + assert "dashboards" in _ID_FILE_IMPORT_SUPPORTED_TYPES, ( + "dashboards must be in _ID_FILE_IMPORT_SUPPORTED_TYPES. " + "If this fails, id-file import support for dashboards is missing." + ) + + def test_dashboards_in_id_file_supported_types_union(self): + """'dashboards' must also be in the union allowlist consulted by + _parse_id_file (rejects unknown types up-front).""" + assert "dashboards" in _ID_FILE_SUPPORTED_TYPES + + def test_dashboards_in_state_load_supported_types(self): + """Adding dashboards to the import allowlist also adds it to the + union (_ID_FILE_SUPPORTED_TYPES), which _parse_id_file consults for + *both* import and sync --minimize-reads. Since the state-load path + scopes by --resources intersection rather than by + _ID_FILE_STATE_LOAD_SUPPORTED_TYPES, dashboards must be explicitly + in the state-load set too — otherwise it's accepted incidentally via + the union without the ID-derivability verification the set exists to + enforce. Dashboards' state key is dashboards..json (ID-derivable), + so it qualifies.""" + from datadog_sync.utils.configuration import _ID_FILE_STATE_LOAD_SUPPORTED_TYPES + + assert "dashboards" in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES, ( + "dashboards is in _ID_FILE_IMPORT_SUPPORTED_TYPES, so the union " + "accepts it on the sync state-load path too; it must be explicitly " + "in _ID_FILE_STATE_LOAD_SUPPORTED_TYPES (state key is ID-derivable)." + ) + + def test_import_resource_id_does_real_get(self): + """import_resource(_id=...) performs a GET to /api/v1/dashboard/{id} + and returns the body — the per-ID fan-out path used by + get_resources_by_ids on id-file import runs.""" + mock_config = MagicMock() + mock_client = AsyncMock() + body = { + "id": "abc-def-ghi", + "title": "test-dashboard", + "widgets": [{"definition": {"type": "timeseries"}}], + } + mock_client.get.return_value = body + mock_config.source_client = mock_client + dashboards = Dashboards(mock_config) + + _id, resource = asyncio.run(dashboards.import_resource(_id="abc-def-ghi")) + + mock_client.get.assert_awaited_once() + call_path = mock_client.get.call_args[0][0] + assert call_path == "/api/v1/dashboard/abc-def-ghi", ( + f"import_resource(_id=...) must GET /api/v1/dashboard/{{id}}; got {call_path!r}" + ) + assert _id == "abc-def-ghi" + assert resource == body + + def test_import_resource_id_short_circuits_when_caller_supplies_full_body(self): + """When the queue handler re-enters import_resource with the body + already fetched by get_resources_by_ids (detected by 'widgets' + presence), the model must NOT issue a second GET — otherwise id-file + import doubles rate-limit pressure on /api/v1/dashboard/{id}.""" + mock_config = MagicMock() + mock_client = AsyncMock() + mock_config.source_client = mock_client + dashboards = Dashboards(mock_config) + full_body = { + "id": "abc-def-ghi", + "title": "prefetched", + "widgets": [{"definition": {"type": "timeseries"}}], + } + + _id, resource = asyncio.run(dashboards.import_resource(resource=full_body)) + + mock_client.get.assert_not_awaited() + assert _id == "abc-def-ghi" + assert resource == full_body + + def test_import_resource_id_403_raises_skip_resource(self): + """A 403 on the per-ID GET raises SkipResource so the id-file path + buckets it as 'skipped' (matching get_resources_by_ids' handling) + rather than aborting the whole import.""" + mock_config = MagicMock() + mock_client = AsyncMock() + resp = MagicMock() + resp.status = 403 + mock_client.get.side_effect = CustomClientHTTPError(resp, message="Forbidden") + mock_config.source_client = mock_client + dashboards = Dashboards(mock_config) + + with pytest.raises(SkipResource): + asyncio.run(dashboards.import_resource(_id="abc-def-ghi")) + + def test_import_resource_id_propagates_non_403_errors(self): + """A 500 on the per-ID GET propagates (get_resources_by_ids classifies + 5xx as transient and retries; the model must not swallow it).""" + mock_config = MagicMock() + mock_client = AsyncMock() + resp = MagicMock() + resp.status = 500 + mock_client.get.side_effect = CustomClientHTTPError(resp, message="boom") + mock_config.source_client = mock_client + dashboards = Dashboards(mock_config) + + with pytest.raises(CustomClientHTTPError): + asyncio.run(dashboards.import_resource(_id="abc-def-ghi")) + + def test_import_resource_id_without_explicit_id_uses_resource_id(self): + """import_resource() with only a resource dict derives the id from + resource['id'] (the legacy list-path shape) — pins that both call + shapes produce a consistent _id.""" + mock_config = MagicMock() + mock_client = AsyncMock() + body = {"id": "abc-def-ghi", "title": "t", "widgets": []} + mock_client.get.return_value = body + mock_config.source_client = mock_client + dashboards = Dashboards(mock_config) + + _id, _ = asyncio.run(dashboards.import_resource(_id=None, resource={"id": "abc-def-ghi"})) + + assert _id == "abc-def-ghi" diff --git a/tests/unit/test_get_resources_by_ids_experiment.py b/tests/unit/test_get_resources_by_ids_experiment.py index e669c4bc..cc47b8b5 100644 --- a/tests/unit/test_get_resources_by_ids_experiment.py +++ b/tests/unit/test_get_resources_by_ids_experiment.py @@ -318,9 +318,10 @@ def test_id_file_allowlist_rejects_unsupported_type(tmp_path, monkeypatch): from datadog_sync.utils.configuration import _parse_id_file from unittest.mock import MagicMock - # Write a payload with an unsupported type + # Write a payload with an unsupported type ('notebooks' is not in + # _ID_FILE_SUPPORTED_TYPES; 'dashboards' is now supported). payload_path = tmp_path / "ids.json" - payload_path.write_text(json.dumps({"dashboards": ["abc-def-ghi"]})) + payload_path.write_text(json.dumps({"notebooks": ["abc-def-ghi"]})) logger = MagicMock() # _parse_id_file calls sys.exit(1) on validation failure. Patch to raise instead. @@ -329,7 +330,7 @@ def test_id_file_allowlist_rejects_unsupported_type(tmp_path, monkeypatch): assert excinfo.value.code == 1 # logger.error should have been called with a message naming the unsupported type error_calls = [str(call) for call in logger.error.call_args_list] - assert any("dashboards" in c for c in error_calls), error_calls + assert any("notebooks" in c for c in error_calls), error_calls def test_id_file_allowlist_accepts_monitors(tmp_path): diff --git a/tests/unit/test_id_file_state_load_scoping.py b/tests/unit/test_id_file_state_load_scoping.py index 09b1df62..1ae70cad 100644 --- a/tests/unit/test_id_file_state_load_scoping.py +++ b/tests/unit/test_id_file_state_load_scoping.py @@ -134,7 +134,7 @@ def test_unsupported_type_still_rejected(self, tmp_path): from datadog_sync.utils.configuration import _parse_id_file import logging - payload_path = _write_payload(tmp_path, {"dashboards": ["dash-1"]}) + payload_path = _write_payload(tmp_path, {"notebooks": ["nb-1"]}) with pytest.raises(SystemExit): _parse_id_file(str(payload_path), logging.getLogger("test")) diff --git a/tests/unit/test_id_file_subprocess_experiment.py b/tests/unit/test_id_file_subprocess_experiment.py index e8ecb629..0c836adf 100644 --- a/tests/unit/test_id_file_subprocess_experiment.py +++ b/tests/unit/test_id_file_subprocess_experiment.py @@ -390,7 +390,9 @@ def test_subprocess_stdin_payload_parsed(tmp_path): @pytest.mark.experiment_subprocess def test_subprocess_unsupported_type_in_id_file(tmp_path): """--id-file with unsupported type errors at config-build.""" - payload = json.dumps({"dashboards": ["abc-def-ghi"]}) + # 'notebooks' is not in _ID_FILE_IMPORT_SUPPORTED_TYPES; 'dashboards' is + # now supported, so it can no longer serve as the unsupported-type example. + payload = json.dumps({"notebooks": ["abc-def-ghi"]}) source_dir = tmp_path / "source" # Use any URL — the subprocess shouldn't even reach the network. rc, stdout, stderr = _run_sync_cli( @@ -402,8 +404,8 @@ def test_subprocess_unsupported_type_in_id_file(tmp_path): assert rc == 1, f"expected exit 1, got {rc}\nSTDERR:\n{stderr.decode(errors='replace')}" combined = (stdout + stderr).decode(errors="replace") assert ( - "dashboards" in combined and "not supported" in combined.lower() - ), f"expected error message mentioning 'dashboards' and 'not supported', got:\n{combined}" + "notebooks" in combined and "not supported" in combined.lower() + ), f"expected error message mentioning 'notebooks' and 'not supported', got:\n{combined}" # --- --id-file requires --resources ---