From 15111cea565f33476b37e17279ba8e9b3a264ef3 Mon Sep 17 00:00:00 2001 From: "riyaz.shiraguppi" Date: Tue, 28 Jul 2026 17:46:12 -0500 Subject: [PATCH] fix(slo_corrections): strip null-valued mutually-exclusive fields before POST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The destination `POST /api/v1/slo/correction` endpoint rejects payloads that carry `slo_id`, `slo_query`, or `end` explicitly set to null with 'API input validation failed: {"": ["Field may not be null."]}'. Source resources round-tripped from `GET /api/v1/slo/correction/{id}` carry both slo_id and slo_query with one explicitly null (they are mutually exclusive — a time-based correction sets slo_id + null slo_query, and a query-based correction sets slo_query + null slo_id). Similarly, recurring rrule corrections carry end=null. Add slo_id, slo_query, and end to non_nullable_attr so the existing del_null_attr helper strips these fields when they are null. Symmetric prep_resource application on both sides of check_diff (via resources_handler.py:503,506 and :657,658) means no spurious update churn. Unit tests cover: time-based correction (slo_query stripped), query- based correction (slo_id + end stripped), populated fields preserved, 0-end preserved (is-None check semantics), symmetric strip yields no check_diff churn, existing duration/rrule regression preserved. --- datadog_sync/model/slo_corrections.py | 16 ++- tests/unit/test_slo_corrections.py | 169 ++++++++++++++++++++++++++ 2 files changed, 184 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_slo_corrections.py diff --git a/datadog_sync/model/slo_corrections.py b/datadog_sync/model/slo_corrections.py index 15e4bc144..940811441 100644 --- a/datadog_sync/model/slo_corrections.py +++ b/datadog_sync/model/slo_corrections.py @@ -20,7 +20,21 @@ class SLOCorrections(BaseResource): resource_connections={"service_level_objectives": ["attributes.slo_id"]}, base_path="/api/v1/slo/correction", excluded_attributes=["id", "attributes.creator", "attributes.created_at", "attributes.modified_at"], - non_nullable_attr=["attributes.duration", "attributes.rrule"], + non_nullable_attr=[ + "attributes.duration", + "attributes.rrule", + # slo_id and slo_query are mutually exclusive: a time-based + # correction sets slo_id (with slo_query=null) and a + # query-based correction sets slo_query (with slo_id=null). + # The destination API rejects null on either field with + # "Field may not be null" — strip whichever is null before POST. + "attributes.slo_id", + "attributes.slo_query", + # end is optional (recurring corrections omit it); when the + # source resource has end=null the destination API rejects + # the payload rather than treating null as "unset". Strip. + "attributes.end", + ], skip_resource_mapping=True, ) # Additional SLOCorrections specific attributes diff --git a/tests/unit/test_slo_corrections.py b/tests/unit/test_slo_corrections.py new file mode 100644 index 000000000..cd0bf28a9 --- /dev/null +++ b/tests/unit/test_slo_corrections.py @@ -0,0 +1,169 @@ +# 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. + +""" +Unit tests for slo_corrections resource handling. + +These tests verify that null-valued mutually-exclusive fields +(`slo_id` vs `slo_query`, and the optional `end`) are stripped from the +destination payload before POST, matching the destination API's +"Field may not be null" rejection semantics. + +The destination `POST /api/v1/slo/correction` endpoint accepts either +a time-based correction (populated `slo_id`, populated `end`) or a +query-based correction (populated `slo_query`). It rejects payloads +that explicitly carry the alternative field with value `null` — the +error reads `API input validation failed: {'slo_query': ['Field may +not be null.']}` (or the corresponding key). Source resources round- +tripped from `GET /api/v1/slo/correction/{id}` carry both fields with +one of them explicitly null; those need to be stripped before the +destination POST. +""" + +from datadog_sync.model.slo_corrections import SLOCorrections +from datadog_sync.utils.resource_utils import prep_resource + + +class TestSLOCorrectionsNonNullableStripping: + """prep_resource must strip null-valued mutually-exclusive fields.""" + + def test_time_based_correction_strips_null_slo_query(self): + """A time-based correction (slo_id set, slo_query=null) must have + slo_query removed from the payload before POST.""" + resource = { + "type": "correction", + "attributes": { + "slo_id": "abc123", + "slo_query": None, + "start": 1700000000, + "end": 1700010000, + "category": "Scheduled Maintenance", + "duration": None, + "rrule": None, + "description": "planned maintenance window", + }, + } + prep_resource(SLOCorrections.resource_config, resource) + assert "slo_query" not in resource["attributes"] + assert resource["attributes"]["slo_id"] == "abc123" + assert resource["attributes"]["end"] == 1700010000 + + def test_query_based_correction_strips_null_slo_id_and_null_end(self): + """A query-based correction (slo_query set, slo_id=null, end=null) + must have both slo_id and end removed from the payload.""" + resource = { + "type": "correction", + "attributes": { + "slo_id": None, + "slo_query": {"query_id": "query-123"}, + "start": 1700000000, + "end": None, + "category": "Scheduled Maintenance", + "duration": None, + "rrule": None, + "description": "recurring correction", + }, + } + prep_resource(SLOCorrections.resource_config, resource) + assert "slo_id" not in resource["attributes"] + assert "end" not in resource["attributes"] + assert resource["attributes"]["slo_query"] == {"query_id": "query-123"} + + def test_populated_fields_are_preserved(self): + """Fields that ARE populated must NOT be stripped.""" + resource = { + "type": "correction", + "attributes": { + "slo_id": "def456", + "slo_query": None, + "start": 1700000000, + "end": 1700010000, + "category": "Other", + "duration": 3600, + "rrule": "FREQ=DAILY", + "description": "populated correction", + }, + } + prep_resource(SLOCorrections.resource_config, resource) + assert resource["attributes"]["slo_id"] == "def456" + assert resource["attributes"]["end"] == 1700010000 + assert resource["attributes"]["duration"] == 3600 + assert resource["attributes"]["rrule"] == "FREQ=DAILY" + assert "slo_query" not in resource["attributes"] + + def test_zero_end_is_preserved_not_stripped(self): + """del_null_attr uses `is None` so integer 0 and empty string "" on + end/slo_id/slo_query are preserved. Regression pin — a stricter strip + would break time-based corrections whose end unix-timestamp is 0. + (No such correction should exist in practice, but the type semantics + should not conflate 0 with null.)""" + resource = { + "type": "correction", + "attributes": { + "slo_id": "abc123", + "slo_query": None, + "start": 1700000000, + "end": 0, + }, + } + prep_resource(SLOCorrections.resource_config, resource) + assert resource["attributes"]["end"] == 0 + assert resource["attributes"]["slo_id"] == "abc123" + assert "slo_query" not in resource["attributes"] + + def test_check_diff_symmetric_strip_no_spurious_diff(self): + """When both `state` and `resource` are `prep_resource`'d before + `check_diff` (as done in resources_handler.py:503-507 and :657-659), + the diff is empty even if only one side has null slo_query/end. + Regression pin against a future asymmetric-prep refactor that could + cause spurious no-op updates on every dispatch.""" + from datadog_sync.utils.resource_utils import check_diff + + state = { + "type": "correction", + "attributes": { + "slo_id": "abc123", + "slo_query": None, # would round-trip as null from destination GET + "start": 1700000000, + "end": 1700010000, + "duration": None, + "rrule": None, + "category": "Scheduled Maintenance", + }, + } + resource = { + "type": "correction", + "attributes": { + "slo_id": "abc123", + "slo_query": None, + "start": 1700000000, + "end": 1700010000, + "duration": None, + "rrule": None, + "category": "Scheduled Maintenance", + }, + } + prep_resource(SLOCorrections.resource_config, state) + prep_resource(SLOCorrections.resource_config, resource) + diff = check_diff(SLOCorrections.resource_config, resource, state) + assert not diff, f"expected empty diff after symmetric strip, got: {diff}" + + def test_null_duration_and_rrule_still_stripped_regression(self): + """Regression pin for existing behavior: duration and rrule + remain in non_nullable_attr and null values are stripped.""" + resource = { + "type": "correction", + "attributes": { + "slo_id": "abc123", + "slo_query": None, + "start": 1700000000, + "end": 1700010000, + "duration": None, + "rrule": None, + }, + } + prep_resource(SLOCorrections.resource_config, resource) + assert "duration" not in resource["attributes"] + assert "rrule" not in resource["attributes"]