fix(dashboards): clone on read-only PUT conflict; skip unmodified suggested presets

datadog_sync/model/dashboards.py

@@ -4,14 +4,24 @@
 # Copyright 2019 Datadog, Inc.
 
 from __future__ import annotations
+from copy import deepcopy
 from typing import TYPE_CHECKING, Optional, List, Dict, Tuple, cast
 
 from datadog_sync.utils.base_resource import BaseResource, ResourceConfig
-from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource
+from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource, check_diff, prep_resource
 
 if TYPE_CHECKING:
     from datadog_sync.utils.custom_client import CustomClient
 
+# Substrings we treat as "destination refuses this update because the sync
+# identity cannot edit this dashboard in place". Match must be specific
+# enough that a generic 403 (missing scope, RBAC failure, edge/WAF)
+# does not fall through to the clone path.
+_READ_ONLY_MARKERS = (
+    "This dashboard is read-only",
+    "user does not have permission to edit",
+)
+
 
 class Dashboards(BaseResource):
     resource_type = "dashboards"
@@ -92,13 +102,70 @@ async def create_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]:
 
     async def update_resource(self, _id: str, resource: Dict) -> Tuple[str, Dict]:
         destination_client = self.config.destination_client
-        resp = await destination_client.put(
-            self.resource_config.base_path + f"/{self.config.state.destination[self.resource_type][_id]['id']}",
-            resource,
-        )
+        destination_id = self.config.state.destination[self.resource_type][_id]["id"]
+        try:
+            resp = await destination_client.put(
+                self.resource_config.base_path + f"/{destination_id}",
+                resource,
+            )
+        except CustomClientHTTPError as e:
+            if e.status_code == 403 and self._is_read_only_conflict(e):
+                return await self._handle_read_only_conflict(_id, resource, destination_id)
+            raise
 
         return _id, resp
 
+    @staticmethod
+    def _is_read_only_conflict(err: CustomClientHTTPError) -> bool:
+        # PUT returns 403 with one of the messages in _READ_ONLY_MARKERS when
+        # the destination copy cannot be edited in place. The markers are
+        # deliberately specific to avoid swallowing unrelated 403s (RBAC,
+        # missing scope, edge/WAF).
+        body = str(err)
+        return any(marker in body for marker in _READ_ONLY_MARKERS)
+
+    async def _handle_read_only_conflict(
+        self, _id: str, resource: Dict, destination_id: str
+    ) -> Tuple[str, Dict]:
+        destination_client = self.config.destination_client
+        destination_state = self.config.state.destination[self.resource_type][_id]
+        description = destination_state.get("description") or ""
+        is_suggested_preset = "[[suggested_dashboards]]" in description
+
+        if is_suggested_preset and self._suggested_preset_matches(resource, destination_state):
+            # Datadog auto-provisions "suggested" dashboards when an integration
+            # is installed. If source content matches the preset byte-for-byte
+            # in the sync-relevant fields, the update is a no-op — skipping is
+            # safe. If the customer forked the preset, fall through to clone.
+            raise SkipResource(
+                _id,
+                self.resource_type,
+                "Destination is an unmodified Datadog-suggested dashboard preset; source content matches.",
+            )
+
+        clone_resp = await destination_client.post(self.resource_config.base_path, resource)
+        new_destination_id = clone_resp.get("id") if isinstance(clone_resp, dict) else None
+        self.config.logger.warning(
+            f"dashboard {_id}: destination copy is read-only for the sync identity; "
+            f"cloned as a new dashboard. old dst id={destination_id} new dst id={new_destination_id}. "
+            f"The old copy is left in place on destination and is no longer referenced by sync state.",
+        )
+        return _id, clone_resp
+
+    @classmethod
+    def _suggested_preset_matches(cls, source: Dict, destination_state: Dict) -> bool:
+        # Use the same predicate the sync handler uses to decide whether an
+        # update is needed at all (resources_handler._apply_resource_cb). A
+        # partial field enumeration would miss dashboard-shape fields like
+        # layout_type / reflow_type / restricted_roles and cause a
+        # false-match skip that drops customer edits on the floor. If the
+        # handler saw a diff (that's why update_resource was invoked), and
+        # check_diff still sees a diff after prep, the customer forked the
+        # preset — fall through to clone.
+        destination_copy = deepcopy(destination_state)
+        prep_resource(cls.resource_config, destination_copy)
+        return not check_diff(cls.resource_config, source, destination_copy)
+
     async def delete_resource(self, _id: str) -> None:
         destination_client = self.config.destination_client
         await destination_client.delete(

tests/unit/test_dashboards.py

@@ -6,14 +6,50 @@
 """
 Unit tests for dashboards resource configuration and short-circuit.
 
-Pins the list_omitted_attr_prefixes opt-in and the prefetched-body
-short-circuit shared with notebooks.
+Pins the list_omitted_attr_prefixes opt-in, the prefetched-body
+short-circuit shared with notebooks, and the read-only-conflict
+clone-on-update fallback.
 """
 
 import asyncio
 from unittest.mock import AsyncMock, MagicMock
 
+import pytest
+
 from datadog_sync.model.dashboards import Dashboards
+from datadog_sync.utils.resource_utils import CustomClientHTTPError, SkipResource
+
+
+def _read_only_error() -> CustomClientHTTPError:
+    resp = MagicMock()
+    resp.status = 403
+    resp.message = "Forbidden"
+    return CustomClientHTTPError(resp, message='{"errors": ["This dashboard is read-only"]}')
+
+
+def _user_no_permission_error() -> CustomClientHTTPError:
+    resp = MagicMock()
+    resp.status = 403
+    resp.message = "Forbidden"
+    return CustomClientHTTPError(
+        resp,
+        message=(
+            '{"errors":[{"status":"403","title":"Forbidden",'
+            '"detail":"user does not have permission to edit this dashboard"}]}'
+        ),
+    )
+
+
+def _unrelated_403_error() -> CustomClientHTTPError:
+    resp = MagicMock()
+    resp.status = 403
+    resp.message = "Forbidden"
+    return CustomClientHTTPError(
+        resp,
+        # Generic "does not have permission" wording that must NOT match — the
+        # marker is "user does not have permission to edit", not just "permission".
+        message='{"errors":["user does not have permission for widget datasource"]}',
+    )
 
 
 def test_resource_config_list_omitted_attr_prefixes():
@@ -53,3 +89,199 @@ def test_import_resource_short_circuits_when_caller_supplies_full_body():
     mock_config.source_client.get.assert_not_awaited()
     assert _id == "abc-123"
     assert resource["widgets"]
+
+
+def _make_dashboards_with_destination(dst_state: dict) -> Dashboards:
+    mock_config = MagicMock()
+    mock_config.state = MagicMock()
+    mock_config.state.destination = {"dashboards": {"abc-123": dst_state}}
+    mock_config.destination_client = AsyncMock()
+    mock_config.logger = MagicMock()
+    dashboards = Dashboards(mock_config)
+    return dashboards
+
+
+class TestDashboardsReadOnlyConflict:
+    """Coverage for the PUT-403-read-only fallback path introduced in T29.
+
+    When the destination refuses the update because the copy on that side is
+    read-only for the sync identity, we fall back to POST (clone) so the
+    customer's source content still lands on destination. If the destination
+    is an unmodified Datadog-suggested preset AND source matches it byte-for-
+    byte on the sync-relevant fields, we SkipResource instead.
+    """
+
+    def test_update_falls_back_to_clone_on_read_only_error(self):
+        """PUT returns 403 'This dashboard is read-only' -> POST is issued
+        with the source body and its response is returned as the new state."""
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error())
+        dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new", "title": "src"})
+        source = {"title": "src", "widgets": [], "tags": []}
+
+        _id, resp = asyncio.run(dashboards.update_resource("abc-123", source))
+
+        dashboards.config.destination_client.put.assert_awaited_once()
+        dashboards.config.destination_client.post.assert_awaited_once_with("/api/v1/dashboard", source)
+        assert _id == "abc-123"
+        assert resp == {"id": "dst-new", "title": "src"}
+
+    def test_update_falls_back_to_clone_on_user_permission_error(self):
+        """The alternate JSON:API-shaped 403 body triggers the same clone path."""
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_user_no_permission_error())
+        dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"})
+        source = {"title": "src", "widgets": []}
+
+        _id, resp = asyncio.run(dashboards.update_resource("abc-123", source))
+
+        dashboards.config.destination_client.post.assert_awaited_once()
+        assert resp["id"] == "dst-new"
+
+    def test_update_reraises_unrelated_403(self):
+        """A 403 that is not the read-only class must not silently clone —
+        the caller upstream needs to see it as a real failure."""
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        err = _unrelated_403_error()
+        dashboards.config.destination_client.put = AsyncMock(side_effect=err)
+        dashboards.config.destination_client.post = AsyncMock()
+
+        with pytest.raises(CustomClientHTTPError):
+            asyncio.run(dashboards.update_resource("abc-123", {"title": "src"}))
+        dashboards.config.destination_client.post.assert_not_awaited()
+
+    def test_update_reraises_generic_permission_403_without_edit_marker(self):
+        """A 403 whose body says 'does not have permission' but WITHOUT the
+        read-only edit marker must propagate. Pins the narrowed marker so a
+        future change back to a broad substring doesn't silently start
+        cloning dashboards for unrelated RBAC / scope failures."""
+        resp = MagicMock()
+        resp.status = 403
+        resp.message = "Forbidden"
+        err = CustomClientHTTPError(
+            resp,
+            message='{"errors":["scope check failed: does not have permission"]}',
+        )
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        dashboards.config.destination_client.put = AsyncMock(side_effect=err)
+        dashboards.config.destination_client.post = AsyncMock()
+
+        with pytest.raises(CustomClientHTTPError):
+            asyncio.run(dashboards.update_resource("abc-123", {"title": "src"}))
+        dashboards.config.destination_client.post.assert_not_awaited()
+
+    def test_suggested_preset_matching_source_is_skipped(self):
+        """Unmodified suggested-preset on destination with matching source
+        content across all diffed fields: skip. Nothing to sync."""
+        dst_state = {
+            "id": "dst-old",
+            "description": "some intro [[suggested_dashboards]]",
+            "title": "preset title",
+            "widgets": [{"definition": {"type": "timeseries"}}],
+            "template_variables": [],
+            "tags": [],
+            "layout_type": "ordered",
+            "reflow_type": "auto",
+        }
+        source = {
+            "title": "preset title",
+            "widgets": [{"definition": {"type": "timeseries"}}],
+            "template_variables": [],
+            "tags": [],
+            "description": "some intro [[suggested_dashboards]]",
+            "layout_type": "ordered",
+            "reflow_type": "auto",
+        }
+        dashboards = _make_dashboards_with_destination(dst_state)
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error())
+        dashboards.config.destination_client.post = AsyncMock()
+
+        with pytest.raises(SkipResource) as exc:
+            asyncio.run(dashboards.update_resource("abc-123", source))
+        assert "suggested" in str(exc.value).lower()
+        dashboards.config.destination_client.post.assert_not_awaited()
+
+    def test_suggested_preset_with_layout_type_divergence_is_cloned(self):
+        """Regression pin: the preset-match predicate must not restrict itself
+        to a hand-picked subset of fields. If source diverges from destination
+        on layout_type (a dashboard-shape field), the customer forked the
+        preset — clone, do not skip."""
+        dst_state = {
+            "id": "dst-old",
+            "description": "[[suggested_dashboards]]",
+            "title": "preset title",
+            "widgets": [],
+            "template_variables": [],
+            "tags": [],
+            "layout_type": "ordered",
+            "reflow_type": "auto",
+        }
+        dashboards = _make_dashboards_with_destination(dst_state)
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error())
+        dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"})
+        source = {
+            "title": "preset title",
+            "widgets": [],
+            "template_variables": [],
+            "tags": [],
+            "description": "[[suggested_dashboards]]",
+            # layout_type changed on source; the old five-field match would
+            # incorrectly declare "unchanged" and skip. check_diff catches it.
+            "layout_type": "free",
+            "reflow_type": "auto",
+        }
+
+        _id, resp = asyncio.run(dashboards.update_resource("abc-123", source))
+        dashboards.config.destination_client.post.assert_awaited_once()
+        assert resp["id"] == "dst-new"
+
+    def test_suggested_preset_with_diverged_source_is_cloned(self):
+        """Source customized on top of a suggested preset: clone, don't skip
+        (would otherwise lose customer edits)."""
+        dst_state = {
+            "id": "dst-old",
+            "description": "[[suggested_dashboards]]",
+            "title": "preset title",
+            "widgets": [],
+            "template_variables": [],
+            "tags": [],
+        }
+        dashboards = _make_dashboards_with_destination(dst_state)
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error())
+        dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"})
+        source = {
+            "title": "preset title",
+            "widgets": [{"definition": {"type": "timeseries"}}],  # customer added a widget
+            "template_variables": [],
+            "tags": [],
+            "description": "[[suggested_dashboards]]",
+        }
+
+        _id, resp = asyncio.run(dashboards.update_resource("abc-123", source))
+        dashboards.config.destination_client.post.assert_awaited_once_with("/api/v1/dashboard", source)
+        assert resp["id"] == "dst-new"
+
+    def test_read_only_conflict_with_no_description_still_clones(self):
+        """If destination state has no `description` field, the
+        suggested-preset check must not crash and must clone."""
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        dashboards.config.destination_client.put = AsyncMock(side_effect=_read_only_error())
+        dashboards.config.destination_client.post = AsyncMock(return_value={"id": "dst-new"})
+        source = {"title": "src"}
+
+        _id, resp = asyncio.run(dashboards.update_resource("abc-123", source))
+        assert resp["id"] == "dst-new"
+
+    def test_non_403_error_reraised(self):
+        """A 500 during update propagates as-is (no clone attempt)."""
+        dashboards = _make_dashboards_with_destination({"id": "dst-old"})
+        resp = MagicMock()
+        resp.status = 500
+        resp.message = "Internal Server Error"
+        err = CustomClientHTTPError(resp, message="boom")
+        dashboards.config.destination_client.put = AsyncMock(side_effect=err)
+        dashboards.config.destination_client.post = AsyncMock()
+
+        with pytest.raises(CustomClientHTTPError):
+            asyncio.run(dashboards.update_resource("abc-123", {"title": "src"}))
+        dashboards.config.destination_client.post.assert_not_awaited()