diff --git a/rest/python/server/integration_test.py b/rest/python/server/integration_test.py index 1e55400..9b17c81 100644 --- a/rest/python/server/integration_test.py +++ b/rest/python/server/integration_test.py @@ -1772,6 +1772,111 @@ def test_validation_failure_answers_with_ucp_envelope(self) -> None: "content must name the offending member", ) + def test_update_applies_payment_instruments(self) -> None: + """An update carrying payment.instruments must apply them, not 500. + + checkout.json marks `payment` as `ucp_request: {update: "optional"}`, and + checkout.md says submitting payment populates payment.instruments with + the collected instrument data -- update is a normal place for a platform + to submit payment. update_checkout built the response instead as + `PaymentResponse(instruments=checkout_req.payment.instruments)`, handing + the response model a list of + payment_instrument_update_request.SelectedPaymentInstrument instances. + The response model is typed for the sibling response class + payment_instrument.SelectedPaymentInstrument, so pydantic rejects the + construction as an unhandled ValidationError -- a bare 500, not the UCP + error envelope, whenever a request supplies a non-empty instruments + array. An update that omits payment (or sends instruments: []) never + exercises the mismatch, which is why this shipped unnoticed. + """ + with self.client: + created = self.client.post( + "/checkout-sessions", + headers=self._get_headers( + idempotency_key="pay_upd_1", request_id="pay_upd_1" + ), + json={"line_items": [{"item": {"id": "rose"}, "quantity": 1}]}, + ) + self.assertEqual(created.status_code, 201, f"Response: {created.text}") + checkout_id = self.get_resource_id(created.json()["id"]) + + instrument = { + "id": "instr_upd_1", + "handler_id": "mock_payment_handler", + "type": "card", + "display": {"brand": "Visa", "last_digits": "4242"}, + "selected": True, + } + updated = self.client.put( + f"/checkout-sessions/{checkout_id}", + headers=self._get_headers( + idempotency_key="pay_upd_2", request_id="pay_upd_2" + ), + json={ + "line_items": [{"item": {"id": "rose"}, "quantity": 1}], + "payment": {"instruments": [instrument]}, + }, + ) + self.assertEqual( + updated.status_code, + 200, + f"an update carrying payment.instruments must not 500: {updated.text}", + ) + returned = (updated.json().get("payment") or {}).get("instruments") or [] + self.assertEqual( + len(returned), 1, "the submitted instrument must be applied" + ) + self.assertEqual(returned[0].get("id"), "instr_upd_1") + self.assertEqual(returned[0].get("handler_id"), "mock_payment_handler") + self.assertEqual(returned[0].get("type"), "card") + self.assertEqual( + returned[0].get("display"), + {"brand": "Visa", "last_digits": "4242"}, + ) + self.assertTrue(returned[0].get("selected")) + + def test_create_applies_payment_instruments(self) -> None: + """A create carrying payment.instruments must apply them, not 500. + + Same class as test_update_applies_payment_instruments: create_checkout + builds `PaymentResponse(instruments=checkout_req.payment.instruments)` + from the create request, handing the response model a list of + payment_instrument_create_request.SelectedPaymentInstrument instances + instead of the response class it declares. Every existing create test + that touches payment sends `instruments: []`, so the mismatch never + triggers pydantic's model-type check. + """ + with self.client: + instrument = { + "id": "instr_create_1", + "handler_id": "mock_payment_handler", + "type": "card", + "display": {"brand": "Visa", "last_digits": "4242"}, + "selected": True, + } + response = self.client.post( + "/checkout-sessions", + headers=self._get_headers( + idempotency_key="pay_create_1", request_id="pay_create_1" + ), + json={ + "line_items": [{"item": {"id": "rose"}, "quantity": 1}], + "payment": {"instruments": [instrument]}, + }, + ) + self.assertEqual( + response.status_code, + 201, + f"a create carrying payment.instruments must not 500: {response.text}", + ) + returned = (response.json().get("payment") or {}).get("instruments") or [] + self.assertEqual( + len(returned), 1, "the submitted instrument must be applied" + ) + self.assertEqual(returned[0].get("id"), "instr_create_1") + self.assertEqual(returned[0].get("handler_id"), "mock_payment_handler") + self.assertEqual(returned[0].get("type"), "card") + if __name__ == "__main__": absltest.main() diff --git a/rest/python/server/services/checkout_service.py b/rest/python/server/services/checkout_service.py index ad7f376..5f8cc51 100644 --- a/rest/python/server/services/checkout_service.py +++ b/rest/python/server/services/checkout_service.py @@ -392,10 +392,14 @@ async def create_checkout( {"type": "total", "amount": 0}, ], links=[], + # Same request/response class collision as update_checkout below: + # checkout_req.payment.instruments holds + # payment_instrument_create_request.SelectedPaymentInstrument + # instances, not the payment_instrument.SelectedPaymentInstrument the + # response field declares. Dump to a dict first so PaymentResponse + # parses it rather than rejecting a foreign model instance. payment=PaymentResponse( - instruments=checkout_req.payment.instruments - if checkout_req.payment - else None, + **checkout_req.payment.model_dump(exclude_none=True) ) if checkout_req.payment else None, @@ -529,8 +533,19 @@ async def update_checkout( # is the same defect as the create path. if checkout_req.payment: + # checkout_req.payment.instruments holds + # payment_instrument_update_request.SelectedPaymentInstrument + # instances, a sibling of the response class + # payment_instrument.SelectedPaymentInstrument that PaymentResponse + # declares for the same field. Passing the request instances straight + # through fails pydantic's model-type check (a different class, not a + # dict), which raised an unhandled ValidationError -- a bare 500 -- + # whenever an update carried a non-empty instruments array. Dumping to + # a dict first and letting PaymentResponse parse it mirrors how buyer, + # context, signals, and discounts already cross this same request to + # response boundary below and in create_checkout. existing.payment = PaymentResponse( - instruments=checkout_req.payment.instruments, + **checkout_req.payment.model_dump(exclude_none=True) ) if checkout_req.buyer: