diff --git a/src/ad_seller/interfaces/api/routers/change_requests.py b/src/ad_seller/interfaces/api/routers/change_requests.py index 534efb9..9a00a15 100644 --- a/src/ad_seller/interfaces/api/routers/change_requests.py +++ b/src/ad_seller/interfaces/api/routers/change_requests.py @@ -5,9 +5,10 @@ from typing import Optional -from fastapi import APIRouter, Depends +from fastapi import APIRouter, Depends, HTTPException from ....services import order_service +from .. import contract_mappers as cm from .. import deps from ..schemas import CreateChangeRequestModel, ReviewChangeRequestModel @@ -23,8 +24,51 @@ async def create_change_request( Validates the change against the current order state, classifies severity, and routes to approval if needed. + + **Idempotency (FD-12):** the request carries a required + ``idempotency_key``. An identical replay returns the original change + request without creating a second approval; reusing the key with a + different body returns ``idempotency_conflict`` (HTTP 409). Keys are + scoped per order and expire after 24 hours. """ - return await order_service.create_change_request(request) + from ....storage.factory import get_storage + + storage_key = f"idempotency:change-request:{request.order_id}:{request.idempotency_key}" + payload_hash = cm.request_payload_hash( + request.model_dump(mode="json", exclude={"idempotency_key"}) + ) + storage = await get_storage() + try: + prior = await storage.get(storage_key) + except Exception: + prior = None + + if isinstance(prior, dict) and prior.get("change_request_id"): + if prior.get("payload_hash") != payload_hash: + raise HTTPException( + status_code=409, + detail=cm.idempotency_conflict_detail( + f"idempotency_key '{request.idempotency_key}' was already used " + "for a different change request." + ), + ) + return await order_service.get_change_request(prior["change_request_id"]) + + result = await order_service.create_change_request(request) + + try: + await storage.set( + storage_key, + { + "change_request_id": result["change_request_id"], + "payload_hash": payload_hash, + }, + ttl=86400, + ) + except Exception: + pass + + return result @router.get("/api/v1/change-requests", tags=["Change Requests"]) diff --git a/src/ad_seller/interfaces/api/schemas.py b/src/ad_seller/interfaces/api/schemas.py index 644ad60..f218a07 100644 --- a/src/ad_seller/interfaces/api/schemas.py +++ b/src/ad_seller/interfaces/api/schemas.py @@ -38,7 +38,7 @@ from iab_agentic_primitives.protocol import ( ProductAvailsSearch as ProductAvailsSearch, # noqa: PLC0414 — explicit re-export ) -from pydantic import BaseModel +from pydantic import BaseModel, Field class PricingRequest(BaseModel): @@ -355,6 +355,7 @@ class FieldDiffModel(BaseModel): class CreateChangeRequestModel(BaseModel): """Request to create a change request for an order.""" + idempotency_key: str = Field(min_length=1) order_id: str change_type: str diffs: list[FieldDiffModel] = [] diff --git a/tests/unit/test_change_request.py b/tests/unit/test_change_request.py index ce0a147..39884f7 100644 --- a/tests/unit/test_change_request.py +++ b/tests/unit/test_change_request.py @@ -170,6 +170,7 @@ async def test_create_minor_auto_approved(self, client, mock_storage): resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-minor-1", "order_id": "ORD-TEST001", "change_type": "creative", "reason": "Swap banner creative", @@ -190,6 +191,7 @@ async def test_create_material_needs_approval(self, client, mock_storage): resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-material-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], @@ -208,6 +210,7 @@ async def test_create_with_rollback_snapshot(self, client, mock_storage): resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-rollback-1", "order_id": "ORD-TEST001", "change_type": "flight_dates", "diffs": [ @@ -228,6 +231,7 @@ async def test_order_not_found(self, client, mock_storage): resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-missing-order-1", "order_id": "ORD-NOPE", "change_type": "creative", }, @@ -240,6 +244,7 @@ async def test_invalid_change_type(self, client, mock_storage): resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-invalid-type-1", "order_id": "ORD-TEST001", "change_type": "banana", }, @@ -253,6 +258,7 @@ async def test_validation_failure_on_completed_order(self, client, mock_storage) resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-invalid-state-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "new_value": 1000000}], @@ -262,6 +268,109 @@ async def test_validation_failure_on_completed_order(self, client, mock_storage) assert resp.json()["detail"]["error"] == "validation_failed" +class TestChangeRequestIdempotency: + async def test_missing_idempotency_key_is_rejected(self, client): + resp = await client.post( + "/api/v1/change-requests", + json={"order_id": "ORD-TEST001", "change_type": "creative"}, + ) + + assert resp.status_code == 422 + + async def test_identical_replay_returns_original_change_request(self, client, mock_storage): + _seed_order(mock_storage) + body = { + "idempotency_key": "idem-replay-1", + "order_id": "ORD-TEST001", + "change_type": "impressions", + "diffs": [{"field": "impressions", "old_value": 5_000_000, "new_value": 8_000_000}], + "reason": "Increase campaign reach", + } + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + first = await client.post("/api/v1/change-requests", json=body) + second = await client.post("/api/v1/change-requests", json=body) + + assert first.status_code == 200 + assert second.status_code == 200 + assert first.json() == second.json() + change_request_keys = [ + key for key in mock_storage._store if key.startswith("change_request:") + ] + assert len(change_request_keys) == 1 + + async def test_reused_key_with_changed_payload_returns_conflict(self, client, mock_storage): + _seed_order(mock_storage) + body = { + "idempotency_key": "idem-conflict-1", + "order_id": "ORD-TEST001", + "change_type": "impressions", + "reason": "Initial request", + } + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + first = await client.post("/api/v1/change-requests", json=body) + second = await client.post( + "/api/v1/change-requests", + json={**body, "reason": "Materially different request"}, + ) + + assert first.status_code == 200 + assert second.status_code == 409 + assert second.json()["detail"]["error"] == "idempotency_conflict" + change_request_keys = [ + key for key in mock_storage._store if key.startswith("change_request:") + ] + assert len(change_request_keys) == 1 + + async def test_same_key_is_independent_across_orders(self, client, mock_storage): + _seed_order(mock_storage, order_id="ORD-A") + _seed_order(mock_storage, order_id="ORD-B") + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + first = await client.post( + "/api/v1/change-requests", + json={ + "idempotency_key": "idem-shared", + "order_id": "ORD-A", + "change_type": "creative", + }, + ) + second = await client.post( + "/api/v1/change-requests", + json={ + "idempotency_key": "idem-shared", + "order_id": "ORD-B", + "change_type": "creative", + }, + ) + + assert first.status_code == 200 + assert second.status_code == 200 + assert first.json()["change_request_id"] != second.json()["change_request_id"] + + async def test_idempotency_record_uses_24_hour_ttl(self, client, mock_storage): + _seed_order(mock_storage) + with patch("ad_seller.storage.factory.get_storage", return_value=mock_storage): + resp = await client.post( + "/api/v1/change-requests", + json={ + "idempotency_key": "idem-ttl-1", + "order_id": "ORD-TEST001", + "change_type": "creative", + }, + ) + + assert resp.status_code == 200 + mock_storage.set.assert_any_await( + "idempotency:change-request:ORD-TEST001:idem-ttl-1", + { + "change_request_id": resp.json()["change_request_id"], + "payload_hash": mock_storage._store[ + "idempotency:change-request:ORD-TEST001:idem-ttl-1" + ]["payload_hash"], + }, + ttl=86400, + ) + + # ============================================================================= # GET /api/v1/change-requests # ============================================================================= @@ -281,6 +390,7 @@ async def test_list_filtered_by_order(self, client, mock_storage): await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-list-a", "order_id": "ORD-A", "change_type": "creative", }, @@ -288,6 +398,7 @@ async def test_list_filtered_by_order(self, client, mock_storage): await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-list-b", "order_id": "ORD-B", "change_type": "creative", }, @@ -310,6 +421,7 @@ async def test_retrieve(self, client, mock_storage): create_resp = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-retrieve-1", "order_id": "ORD-TEST001", "change_type": "creative", }, @@ -338,6 +450,7 @@ async def test_approve_pending_request(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-review-approve-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], @@ -364,6 +477,7 @@ async def test_reject_pending_request(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-review-reject-1", "order_id": "ORD-TEST001", "change_type": "cancellation", "reason": "Client wants out", @@ -391,6 +505,7 @@ async def test_cannot_review_already_approved(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-review-approved-1", "order_id": "ORD-TEST001", "change_type": "creative", }, @@ -414,6 +529,7 @@ async def test_invalid_decision(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-review-invalid-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], @@ -444,6 +560,7 @@ async def test_apply_approved_request(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-apply-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], @@ -481,6 +598,7 @@ async def test_cannot_apply_unapproved(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-apply-unapproved-1", "order_id": "ORD-TEST001", "change_type": "impressions", "diffs": [{"field": "impressions", "old_value": 5000000, "new_value": 8000000}], @@ -512,6 +630,7 @@ async def test_full_material_change_flow(self, client, mock_storage): cr = await client.post( "/api/v1/change-requests", json={ + "idempotency_key": "idem-full-flow-1", "order_id": "ORD-TEST001", "change_type": "flight_dates", "diffs": [