Skip to content

fix: enforce idempotency for change requests #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
garvitkaushik-123 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: enforce idempotency for change requests #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 46 additions & 2 deletions src/ad_seller/interfaces/api/routers/change_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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"])
Expand Down
3 changes: 2 additions & 1 deletion src/ad_seller/interfaces/api/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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] = []
Expand Down
119 changes: 119 additions & 0 deletions tests/unit/test_change_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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}],
Expand All @@ -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": [
Expand All @@ -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",
},
Expand All @@ -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",
},
Expand All @@ -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}],
Expand All @@ -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
# =============================================================================
Expand All @@ -281,13 +390,15 @@ 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",
},
)
await client.post(
"/api/v1/change-requests",
json={
"idempotency_key": "idem-list-b",
"order_id": "ORD-B",
"change_type": "creative",
},
Expand All @@ -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",
},
Expand Down Expand Up @@ -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}],
Expand All @@ -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",
Expand Down Expand Up @@ -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",
},
Expand All @@ -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}],
Expand Down Expand Up @@ -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}],
Expand Down Expand Up @@ -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}],
Expand Down Expand Up @@ -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": [
Expand Down
Loading