From 7c7172bfddacb959b60b39f4a47952c9de2ecca1 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Wed, 5 Aug 2026 15:54:37 -0400 Subject: [PATCH] fix(rest/python): assign checkout id server side instead of trusting the request A schema-valid checkout create carrying a non-string top-level `id` returns HTTP 500. checkout.json annotates `id` with `ucp_request: omit` (the business assigns it), so the generated CheckoutCreateRequest declares no id field; extra="allow" then admits a client-sent `id` of any JSON type as an extra member, keeping the request schema-valid. create_checkout read that extra (`getattr(checkout_req, "id", None)`) and passed it verbatim into the Checkout response model, where a non-string raised an uncaught pydantic ValidationError: id Input should be a valid string [type=string_type, input_value=123] Observed vs expected, reproduced against main: POST /checkout-sessions UCP-Agent: profile="https://spck.dev/agent" idempotency-key: request-id: Content-Type: application/json {"id":123,"currency":"USD","line_items":[{"id":"li_1","quantity":1, "item":{"id":"bouquet_roses","price":1000},"totals":[]}], "payment":{"instruments":[],"handlers":[]},"status":"incomplete", "ucp":{"version":"2026-04-08"},"totals":[],"links":[]} -> 500 Internal Server Error (expected: 201) Controls: the same request with `id` omitted, or with a string id, returns 201. This is the same defect class as the currency read fixed in #156: the server determines an omit field and never takes it from the request. The create path now always assigns its own uuid and ignores any client-sent id. The update path never read the body id (the id comes from the URL path), so it has no analogue of this defect. Accounting for the other omit-annotated checkout fields at 2026-04-08 (status, totals, links, ucp, expires_at, currency): each is either excluded from the request dump before the response model is constructed or already determined server side (currency since #156), and wrong-typed values for each were verified to return 201 on unmodified main. Why the existing suite missed it: every lifecycle test built its payload through _create_checkout_payload, which always sets a string id, and then addressed follow-up calls with that same id, so a non-string id was never sent and the id echo was baked into the tests as a contract. Those tests now use the server-returned id, matching what the happy-path client already does, and a new test covers non-string, string, and omitted id on create. --- rest/python/server/integration_test.py | 119 +++++++++++++++--- .../server/services/checkout_service.py | 9 +- 2 files changed, 109 insertions(+), 19 deletions(-) diff --git a/rest/python/server/integration_test.py b/rest/python/server/integration_test.py index 97e990c..403fcb8 100644 --- a/rest/python/server/integration_test.py +++ b/rest/python/server/integration_test.py @@ -340,13 +340,16 @@ def test_single_item_checkout(self) -> None: ) self.assertEqual(response.status_code, 201, f"Response: {response.text}") checkout = TestCheckout.model_validate(response.json()) - self.assertEqual(self.get_resource_id(checkout.id), "test_checkout_1") + # `id` is ucp_request: omit, so the server assigns it; follow-up + # operations use the returned id. + checkout_sid = self.get_resource_id(checkout.id) + self.assertIsInstance(checkout_sid, str) self.assertEqual(checkout.status, "ready_for_complete") # 2. Complete Checkout payment_payload = self._create_payment_payload() response = self.client.post( - "/checkout-sessions/test_checkout_1/complete", + f"/checkout-sessions/{checkout_sid}/complete", headers=self._get_headers(idempotency_key="2", request_id="2"), json=payment_payload, ) @@ -393,11 +396,12 @@ def test_double_complete_checkout(self) -> None: json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201) + checkout_sid = self.get_resource_id(response.json()["id"]) # 2. Complete Checkout (First time) payment_payload = self._create_payment_payload() response = self.client.post( - "/checkout-sessions/test_checkout_double/complete", + f"/checkout-sessions/{checkout_sid}/complete", headers=self._get_headers(idempotency_key="2", request_id="2"), json=payment_payload, ) @@ -405,7 +409,7 @@ def test_double_complete_checkout(self) -> None: # 3. Complete Checkout (Second time) - Should fail response = self.client.post( - "/checkout-sessions/test_checkout_double/complete", + f"/checkout-sessions/{checkout_sid}/complete", headers=self._get_headers(idempotency_key="4", request_id="4"), json=payment_payload, ) @@ -432,11 +436,12 @@ def test_multi_item_checkout(self) -> None: json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201) + checkout_sid = self.get_resource_id(response.json()["id"]) # 2. Complete Multi-item Checkout payment_payload = self._create_payment_payload() response = self.client.post( - "/checkout-sessions/test_checkout_multi/complete", + f"/checkout-sessions/{checkout_sid}/complete", headers=self._get_headers(idempotency_key="6", request_id="6"), json=payment_payload, ) @@ -606,6 +611,7 @@ async def seed_discount() -> None: json=body, ) self.assertEqual(create.status_code, 201, f"Response: {create.text}") + checkout_id = self.get_resource_id(create.json()["id"]) applied = (create.json().get("discounts") or {}).get("applied") or [] self.assertEqual(len(applied), 1, "create applies the discount once") @@ -666,10 +672,11 @@ def test_cancel_checkout(self) -> None: json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201) + checkout_sid = self.get_resource_id(response.json()["id"]) # 2. Cancel Checkout response = self.client.post( - "/checkout-sessions/test_checkout_cancel/cancel", + f"/checkout-sessions/{checkout_sid}/cancel", headers=self._get_headers( idempotency_key="cancel_2", request_id="cancel_2" ), @@ -680,7 +687,7 @@ def test_cancel_checkout(self) -> None: # 3. Try to Cancel again (should fail) response = self.client.post( - "/checkout-sessions/test_checkout_cancel/cancel", + f"/checkout-sessions/{checkout_sid}/cancel", headers=self._get_headers( idempotency_key="cancel_3", request_id="cancel_3" ), @@ -703,11 +710,12 @@ def test_cancel_checkout(self) -> None: json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201) + completed_sid = self.get_resource_id(response.json()["id"]) # Complete it payment_payload = self._create_payment_payload() response = self.client.post( - "/checkout-sessions/test_checkout_cancel_completed/complete", + f"/checkout-sessions/{completed_sid}/complete", headers=self._get_headers( idempotency_key="cancel_5", request_id="cancel_5" ), @@ -717,7 +725,7 @@ def test_cancel_checkout(self) -> None: # Try to cancel completed checkout response = self.client.post( - "/checkout-sessions/test_checkout_cancel_completed/cancel", + f"/checkout-sessions/{completed_sid}/cancel", headers=self._get_headers( idempotency_key="cancel_6", request_id="cancel_6" ), @@ -733,22 +741,26 @@ def test_idempotency_key_is_scoped_to_operation_and_checkout(self) -> None: with self.client: for operation in ("update", "complete", "cancel"): with self.subTest(operation=operation): - first_checkout_id = f"idempotency_{operation}_first" - second_checkout_id = f"idempotency_{operation}_second" - - for checkout_id in (first_checkout_id, second_checkout_id): + # The server assigns the checkout ids; the labels only scope the + # idempotency keys of the create calls. + server_ids: dict[str, str] = {} + for label in ("first", "second"): payload = self._create_checkout_payload( - checkout_id, [("rose", "Red Rose", 1000, 1)] + f"idempotency_{operation}_{label}", + [("rose", "Red Rose", 1000, 1)], ) response = self.client.post( "/checkout-sessions", headers=self._get_headers( - idempotency_key=f"create_{checkout_id}", - request_id=f"create_{checkout_id}", + idempotency_key=f"create_idempotency_{operation}_{label}", + request_id=f"create_idempotency_{operation}_{label}", ), json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201, response.text) + server_ids[label] = self.get_resource_id(response.json()["id"]) + first_checkout_id = server_ids["first"] + second_checkout_id = server_ids["second"] shared_key = f"shared_{operation}_key" request_body = None @@ -869,10 +881,11 @@ def test_webhook_delivers_the_bare_order_as_body(self) -> None: json=payload.model_dump(mode="json", exclude_none=True), ) self.assertEqual(response.status_code, 201, response.text) + checkout_sid = self.get_resource_id(response.json()["id"]) payment_payload = self._create_payment_payload() response = self.client.post( - "/checkout-sessions/wh_order_placed/complete", + f"/checkout-sessions/{checkout_sid}/complete", headers=self._get_headers(idempotency_key="wh2", request_id="wh2"), json=payment_payload, ) @@ -1103,6 +1116,78 @@ def test_update_omitting_server_determined_fields(self) -> None: ) self.assertEqual(updated.status_code, 200, f"Response: {updated.text}") + def test_create_assigns_id_server_side(self) -> None: + """A create carrying a non-string top-level `id` must not 500. + + checkout.json marks `id` with `ucp_request: omit` -- the business assigns + it -- so the generated CheckoutCreateRequest declares no id field, and a + request carrying one of any JSON type is still schema-valid because + extra="allow" admits it as an extra member. create_checkout read that + extra attribute and passed it into the Checkout response model verbatim, + so `"id": 123` raised an uncaught pydantic ValidationError (HTTP 500). + Same defect class as the currency read fixed in #156: the server + determines the value and never takes it from the request. + """ + + def _body(**extra: object) -> dict: + body = { + "currency": "USD", + "line_items": [ + { + "id": "li_1", + "quantity": 1, + "item": {"id": "rose", "price": 1000}, + "totals": [], + } + ], + "payment": {"instruments": [], "handlers": []}, + "status": "incomplete", + "ucp": {"version": "2026-04-08"}, + "totals": [], + "links": [], + } + body.update(extra) + return body + + with self.client: + # A non-string id is schema-valid (id is omit, so it arrives as an + # extra member) and must never 500. + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="sid1", request_id="sid1"), + json=_body(id=123), + ) + self.assertEqual(response.status_code, 201, f"Response: {response.text}") + self.assertIsInstance( + response.json().get("id"), + str, + "server must assign a string id when the platform sends a non-string", + ) + + # A string id is ignored the same way: the server assigns its own. + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="sid2", request_id="sid2"), + json=_body(id="client_chosen_id"), + ) + self.assertEqual(response.status_code, 201, f"Response: {response.text}") + body = response.json() + self.assertIsInstance(body.get("id"), str) + self.assertNotIn( + "client_chosen_id", + body["id"], + "id is ucp_request: omit, so the server assigns it", + ) + + # Omitted id keeps working (the conformant request). + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="sid3", request_id="sid3"), + json=_body(), + ) + self.assertEqual(response.status_code, 201, f"Response: {response.text}") + self.assertIsInstance(response.json().get("id"), str) + if __name__ == "__main__": absltest.main() diff --git a/rest/python/server/services/checkout_service.py b/rest/python/server/services/checkout_service.py index 50665d7..c84a13b 100644 --- a/rest/python/server/services/checkout_service.py +++ b/rest/python/server/services/checkout_service.py @@ -167,8 +167,13 @@ async def create_checkout( # Return cached response return Checkout(**existing_record.response_body) - # Initialize full model from request - checkout_id = getattr(checkout_req, "id", None) or str(uuid.uuid4()) + # `id` carries `ucp_request: omit`, so the server assigns it and never + # takes it from the request. The generated CheckoutCreateRequest declares + # no id field, but extra="allow" admits a client-sent `id` of any JSON + # type as an extra attribute; reading it here propagated that raw value + # into the response model, where a non-string raised an uncaught + # ValidationError. Same defect class as the currency read fixed in #156. + checkout_id = str(uuid.uuid4()) # Map line items line_items = []