From 6d1b5eed3f65f13c5faa3aa277e2b559f6455388 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Sat, 29 Aug 2026 12:30:57 -0400 Subject: [PATCH] fix(rest/python): stop null-padding unset optional fields in order responses The order schemas type unset optional fields as bare non-nullable string/object/array, the same shape #115/#117 fixed for checkout responses against the 2026-01-23 schema. The order route has three independent write sites with the identical gap: the initial persist in complete_checkout, the order update route, and the order-event webhook receiver each model_dump an Order without exclude_none, so GET/PUT /orders/{id} and the outbound order-event webhook body (which reads the same stored row) all emit explicit null for every field a client or partner left unset. Adds exclude_none=True to all three model_dump call sites, following the #117 idiom exactly. Adds three new tests asserting no null anywhere in the order GET, order PUT, and post-webhook-receiver bodies; each was watched red before the corresponding fix and confirmed to go red again with that fix excised (kill-tested individually). --- rest/python/server/integration_test.py | 189 ++++++++++++++++++ rest/python/server/routes/order.py | 2 +- .../server/routes/ucp_implementation.py | 4 +- .../server/services/checkout_service.py | 2 +- 4 files changed, 194 insertions(+), 3 deletions(-) diff --git a/rest/python/server/integration_test.py b/rest/python/server/integration_test.py index 55b0ba63..1e2c5e17 100644 --- a/rest/python/server/integration_test.py +++ b/rest/python/server/integration_test.py @@ -87,6 +87,31 @@ FLAGS = flags.FLAGS +def _find_nulls(node: object, path: str = "") -> list[str]: + """Return the JSON-pointer path of every explicit `null` in a decoded body. + + The order schema types unset optional fields as bare non-nullable + string/object/array, so under JSON Schema 2020-12 an explicit `null` is a + distinct type and fails validation the way an omitted key does not. A + response must OMIT an unset optional field, never emit `null` for it. + `Order.model_validate` (used by every other order test in this file, e.g. + test_shipping_event_matches_order_schema and + test_webhook_delivers_the_bare_order_as_body) cannot see this defect: the + generated pydantic model types every one of these fields + `Optional[...] = None`, so it accepts a `null` the wire schema forbids. + """ + paths: list[str] = [] + if node is None: + return [path or "/"] + if isinstance(node, dict): + for key, value in node.items(): + paths.extend(_find_nulls(value, f"{path}/{key}")) + elif isinstance(node, list): + for index, value in enumerate(node): + paths.extend(_find_nulls(value, f"{path}/{index}")) + return paths + + class TestCheckout( BuyerConsentCheckoutResp, FulfillmentCheckout, @@ -522,6 +547,170 @@ def test_shipping_event_matches_order_schema(self) -> None: ], ) + def test_get_order_omits_unset_optional_fields_as_null(self) -> None: + """GET /orders/{id} must omit unset optional fields, never emit null. + + Mirrors #115/#117 (checkout responses null-padded unset optional + fields against the 2026-01-23 schema): the order route has the same + defect. Validated separately against the official ucp-schema validator + and an independent jsonschema referee, both spec corpora (2026-04-08, + 2026-08-25); this in-repo test pins the narrower, dependency-free + signature -- no `null` anywhere in the response body. + """ + with self.client: + payload = self._create_checkout_payload( + "order_get_nulls", + [("rose", "Red Rose", 1000, 2), ("tulip", "White Tulip", 800, 1)], + ) + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="ogn1", request_id="ogn1"), + 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"]) + + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="ogn2", request_id="ogn2"), + json=self._create_payment_payload(), + ) + self.assertEqual(response.status_code, 200, response.text) + order_id = response.json()["order"]["id"] + + response = self.client.get( + f"/orders/{order_id}", headers=self._get_headers() + ) + self.assertEqual(response.status_code, 200, response.text) + null_paths = _find_nulls(response.json()) + self.assertEqual( + null_paths, + [], + "order GET must omit unset optional fields, not emit null for them", + ) + + def test_update_order_omits_unset_optional_fields_as_null(self) -> None: + """PUT /orders/{id} must not write null-padded fields back to storage. + + Companion to test_get_order_omits_unset_optional_fields_as_null: a fix + scoped only to the order's initial persist (inside complete_checkout) + would leave this update path free to reintroduce nulls on the very + next PUT -- the create/update-path split this suite exists to catch + (the shape of conformance#59 upstream: an issue named three sites, a + maintainer found the fourth of the same class). + """ + with self.client: + payload = self._create_checkout_payload( + "order_put_nulls", [("rose", "Red Rose", 1000, 1)] + ) + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="opn1", request_id="opn1"), + 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"]) + + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="opn2", request_id="opn2"), + json=self._create_payment_payload(), + ) + self.assertEqual(response.status_code, 200, response.text) + order_id = response.json()["order"]["id"] + + # Round-trip the order body through PUT unchanged (a real client + # updating one field would carry the rest of the order along, since + # the route is typed to accept a whole UnifiedOrder). + order_body = self.client.get( + f"/orders/{order_id}", headers=self._get_headers() + ).json() + + response = self.client.put( + f"/orders/{order_id}", + headers=self._get_headers(idempotency_key="opn3", request_id="opn3"), + json=order_body, + ) + self.assertEqual(response.status_code, 200, response.text) + null_paths = _find_nulls(response.json()) + self.assertEqual( + null_paths, + [], + "order PUT response must omit unset optional fields, not emit " + "null for them", + ) + + # And the stored copy the next GET serves must stay null-free too. + response = self.client.get( + f"/orders/{order_id}", headers=self._get_headers() + ) + null_paths = _find_nulls(response.json()) + self.assertEqual( + null_paths, + [], + "order PUT must not write null-padded fields back to storage", + ) + + def test_order_webhook_receiver_omits_unset_optional_fields_as_null( + self, + ) -> None: + """The order-event webhook receiver must not write nulls to storage. + + A third site of the same class: `routes/ucp_implementation.py`'s + order_event_webhook parses an inbound partner Order payload and + persists `payload.model_dump(...)` directly. A partner naturally omits + unset optional fields; pydantic parses those as None, and dumping + without exclude_none writes them back as explicit null -- corrupting + storage the same way the create and update paths did, but reachable + without ever calling GET or PUT /orders/{id} first. + """ + with self.client: + payload = self._create_checkout_payload( + "order_webhook_nulls", [("rose", "Red Rose", 1000, 1)] + ) + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers(idempotency_key="own1", request_id="own1"), + 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"]) + + response = self.client.post( + f"/checkout-sessions/{checkout_sid}/complete", + headers=self._get_headers(idempotency_key="own2", request_id="own2"), + json=self._create_payment_payload(), + ) + self.assertEqual(response.status_code, 200, response.text) + order_id = response.json()["order"]["id"] + + # A partner's own event payload: only the fields it actually knows + # about, exactly as a real notifier would send it (the just-created, + # already null-free order body is the fixture: it naturally omits + # every unset optional field, so pydantic parses them as None on the + # way in). + order_body = self.client.get( + f"/orders/{order_id}", headers=self._get_headers() + ).json() + + response = self.client.post( + "/webhooks/partners/partner_1/events/order", + headers=self._get_headers(), + json=order_body, + ) + self.assertEqual(response.status_code, 200, response.text) + + stored = self.client.get( + f"/orders/{order_id}", headers=self._get_headers() + ).json() + null_paths = _find_nulls(stored) + self.assertEqual( + null_paths, + [], + "the order-event webhook receiver must not write null-padded " + "fields back to storage", + ) + def test_missing_ucp_agent_header(self) -> None: """Tests that requests missing mandatory headers are rejected.""" with self.client: diff --git a/rest/python/server/routes/order.py b/rest/python/server/routes/order.py index dc4d8b64..42c45fb4 100644 --- a/rest/python/server/routes/order.py +++ b/rest/python/server/routes/order.py @@ -86,5 +86,5 @@ async def update_order( del common_headers # Unused # We convert to dict to match service signature and DB storage which expects # JSON-able dict - order_data = order.model_dump(mode="json", by_alias=True) + order_data = order.model_dump(mode="json", by_alias=True, exclude_none=True) return await checkout_service.update_order(order_id, order_data) diff --git a/rest/python/server/routes/ucp_implementation.py b/rest/python/server/routes/ucp_implementation.py index 06d8a7c8..1a0460e3 100644 --- a/rest/python/server/routes/ucp_implementation.py +++ b/rest/python/server/routes/ucp_implementation.py @@ -318,7 +318,9 @@ async def order_event_webhook( ) -> dict[str, Any]: """Order Event Webhook Implementation.""" del partner_id, signature # Unused - payload_dict = payload.model_dump(mode="json", by_alias=True) + payload_dict = payload.model_dump( + mode="json", by_alias=True, exclude_none=True + ) await checkout_service.update_order(payload.id, payload_dict) return {"status": "ok"} diff --git a/rest/python/server/services/checkout_service.py b/rest/python/server/services/checkout_service.py index ad7f376e..a2242017 100644 --- a/rest/python/server/services/checkout_service.py +++ b/rest/python/server/services/checkout_service.py @@ -882,7 +882,7 @@ async def complete_checkout( await db.save_order( self.transactions_session, order.id, - order.model_dump(mode="json", by_alias=True), + order.model_dump(mode="json", by_alias=True, exclude_none=True), ) await db.save_checkout(