From 5f3f78521c5b7343bc8ccadb408f016616c79a11 Mon Sep 17 00:00:00 2001 From: "michael.richey" Date: Thu, 14 May 2026 15:20:51 -0400 Subject: [PATCH] fix(synthetics_global_variables): key on (name, type) to resolve 409 conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Datadog API enforces uniqueness on the (name, type) tuple — two global variables can share a name as long as their types differ. Keying `_existing_resources_map` by `name` alone caused last-write-wins map collisions: when the destination had two variables with the same name but different types, only one survived in the prefetch map and the source matching the other type fell through to POST and hit "409 Conflict — Synthetics variable with same name and type already exists". Switch `resource_mapping_key` to `lambda r: f"{r['name']}:{r['type']}"`, mirroring the composite-key pattern Teams already uses for (name, handle). The existing prefetch + remap path in `create_resource` then resolves conflicts before any POST is issued. Adds six unit tests in tests/unit/test_synthetics_global_variables.py covering: map_existing_resources retains both same-name variants, routing by composite key, key-resolves-to-string + missing-type-returns-None guards, unique-name regression, and the partial-collision case that reproduces the production bug. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../model/synthetics_global_variables.py | 6 +- .../unit/test_synthetics_global_variables.py | 160 ++++++++++++++++++ 2 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_synthetics_global_variables.py diff --git a/datadog_sync/model/synthetics_global_variables.py b/datadog_sync/model/synthetics_global_variables.py index 8b616c12..f3cf5266 100644 --- a/datadog_sync/model/synthetics_global_variables.py +++ b/datadog_sync/model/synthetics_global_variables.py @@ -33,7 +33,11 @@ class SyntheticsGlobalVariables(BaseResource): "editor", ], tagging_config=TaggingConfig(path="tags"), - resource_mapping_key="name", + # Datadog enforces uniqueness on (name, type) — two variables can share + # a name only when their types differ (e.g. "variable" vs "secret_token"). + # Keying by name alone collapses same-name/different-type entries in + # _existing_resources_map and triggers a 409 on the unmapped source. + resource_mapping_key=lambda r: f"{r['name']}:{r['type']}", ) # Additional SyntheticsGlobalVariables specific attributes diff --git a/tests/unit/test_synthetics_global_variables.py b/tests/unit/test_synthetics_global_variables.py new file mode 100644 index 00000000..55336152 --- /dev/null +++ b/tests/unit/test_synthetics_global_variables.py @@ -0,0 +1,160 @@ +# 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 the synthetics_global_variables composite (name, type) mapping key. + +The Datadog API enforces uniqueness on the (name, type) tuple — two global +variables can share a name as long as their types differ (e.g. "variable" vs +"secret_token"). Keying ``_existing_resources_map`` by ``name`` alone caused +last-write-wins map collisions and a production 409 Conflict on sync. + +Tests 1, 2, 3a, 5 are red against ``resource_mapping_key="name"`` and green +after the fix to ``lambda r: f"{r['name']}:{r['type']}"``. +Tests 3b and 4 are green/green guards. +""" + +import asyncio +from unittest.mock import AsyncMock + +from datadog_sync.model.synthetics_global_variables import SyntheticsGlobalVariables + + +def _make_variable(name, type_, var_id, description="d", value_value="v"): + """Build a synthetics global variable dict shaped like the GET response. + + Pre-populates ``value.value`` so ``_inject_secret_value`` short-circuits + on the ``if "value" not in resource.get("value", {})`` guard at + synthetics_global_variables.py:62 — no source_client.get call. + """ + return { + "id": var_id, + "name": name, + "type": type_, + "description": description, + "tags": [], + "value": {"value": value_value, "secure": False, "options": {}}, + } + + +class TestCompositeKeyResolution: + """3a: composite key resolves to ":" string. + 3b: missing type returns None silently (green/green guard).""" + + def test_composite_key_resolves_to_string(self, mock_config): + """3a (red→green): full input yields composite "X:variable".""" + instance = SyntheticsGlobalVariables(mock_config) + resource = _make_variable("X", "variable", "abc") + assert instance.get_resource_mapping_key(resource) == "X:variable" + + def test_missing_type_returns_none(self, mock_config): + """3b (green/green): missing ``type`` returns None — pins the silent + KeyError handling at base_resource.py:111-115. Behavior unchanged by + the fix; guards against future refactors dropping the try/except or + an API rename moving ``type`` under ``attributes``. + """ + instance = SyntheticsGlobalVariables(mock_config) + resource = {"name": "X"} + assert instance.get_resource_mapping_key(resource) is None + + +class TestMapExistingResourcesRetainsBothTypes: + """1: ``map_existing_resources()`` keeps both same-name variants under + distinct composite keys.""" + + def test_two_same_name_different_type_both_in_map(self, mock_config): + """1 (red→green): destination has X/variable and X/secret_token. + Under old ``resource_mapping_key="name"`` the map collapses to one + entry. Under the composite key both entries are retained. + """ + instance = SyntheticsGlobalVariables(mock_config) + dest_resources = [ + _make_variable("X", "variable", "dest-1"), + _make_variable("X", "secret_token", "dest-2"), + ] + instance.get_resources = AsyncMock(return_value=dest_resources) + + asyncio.run(instance.map_existing_resources()) + + assert len(instance._existing_resources_map) == 2 + assert instance._existing_resources_map["X:variable"]["id"] == "dest-1" + assert instance._existing_resources_map["X:secret_token"]["id"] == "dest-2" + + +class TestRoutingByCompositeKey: + """2: routing picks the right destination entry by (name, type).""" + + def test_source_routes_to_matching_type_destination(self, mock_config): + """2 (red→green): ``_existing_resources_map`` has both entries, source + is X/secret_token — must remap to dest-2 and PUT, not POST. + """ + instance = SyntheticsGlobalVariables(mock_config) + dest_variable = _make_variable("X", "variable", "dest-1") + dest_secret = _make_variable("X", "secret_token", "dest-2") + instance._existing_resources_map = { + "X:variable": dest_variable, + "X:secret_token": dest_secret, + } + + mock_config.destination_client.post = AsyncMock(return_value=dest_secret) + mock_config.destination_client.put = AsyncMock(return_value=dest_secret) + + source = _make_variable("X", "secret_token", "src-A") + asyncio.run(instance.create_resource("src-A", source)) + + assert mock_config.state.destination["synthetics_global_variables"]["src-A"]["id"] == "dest-2" + mock_config.destination_client.post.assert_not_called() + mock_config.destination_client.put.assert_called_once() + put_path = mock_config.destination_client.put.call_args[0][0] + assert put_path == "/api/v1/synthetics/variables/dest-2" + + +class TestUniqueNameStillPosts: + """4: green/green regression — a name not in the map still POSTs.""" + + def test_unique_name_routes_to_post(self, mock_config): + """4 (green/green): source name "Y" has no match — POST is called + once and the response lands in state. Behavior unchanged by the fix + when there is no type collision. + """ + instance = SyntheticsGlobalVariables(mock_config) + instance._existing_resources_map = {} + + source = _make_variable("Y", "variable", "src-Y") + response = _make_variable("Y", "variable", "dest-Y") + mock_config.destination_client.post = AsyncMock(return_value=response) + mock_config.destination_client.put = AsyncMock() + + _, resp = asyncio.run(instance.create_resource("src-Y", source)) + + mock_config.destination_client.post.assert_called_once() + mock_config.destination_client.put.assert_not_called() + assert resp["id"] == "dest-Y" + + +class TestPartialCollisionPosts: + """5: the literal production bug — destination has X/variable, source is + X/secret_token. Under old name-only key this incorrectly hit PUT on the + wrong type and the API returned 409. Under the composite key the source + correctly falls through to POST as a new variable.""" + + def test_partial_collision_routes_to_post(self, mock_config): + """5 (red→green): destination only has the X/variable entry. Source + is X/secret_token. Must POST (new variable), not PUT (wrong type). + """ + instance = SyntheticsGlobalVariables(mock_config) + instance._existing_resources_map = { + "X:variable": _make_variable("X", "variable", "dest-1"), + } + + source = _make_variable("X", "secret_token", "src-A") + response = _make_variable("X", "secret_token", "dest-new") + mock_config.destination_client.post = AsyncMock(return_value=response) + mock_config.destination_client.put = AsyncMock() + + _, resp = asyncio.run(instance.create_resource("src-A", source)) + + mock_config.destination_client.post.assert_called_once() + mock_config.destination_client.put.assert_not_called() + assert resp["id"] == "dest-new"