fix(rest/python): apply update request payment instruments instead of failing construction - #220
Open
vishkaty wants to merge 2 commits into
Conversation
…test helper _create_checkout_payload built shipping_destination.ShippingDestination for FulfillmentMethodCreateRequest.destinations, a field typed as the FulfillmentDestinationCreateRequest union. ucp-sdk 0.4.5 resolved that union to the response variants (ShippingDestination, RetailLocation), so the construction validated. 0.4.6 corrected the oneOf/anyOf codegen (python-sdk#83) and the union now resolves to the matching *CreateRequest variants, so the response class no longer validates against a request field. Since ucp-sdk is an unpinned dependency here, a fresh install now resolves 0.4.6 and the shared helper fails for every caller (integration_test.py, cart_test.py, signature_integration_test.py). Swap the import and construction to shipping_destination_create_request.ShippingDestinationCreateRequest, matching the corrected union.
… failing construction
update_checkout built the response payment as
PaymentResponse(instruments=checkout_req.payment.instruments), handing the
response model a list of payment_instrument_update_request.
SelectedPaymentInstrument instances. PaymentResponse declares that field as
list[payment_instrument.SelectedPaymentInstrument], a sibling response
class with the same shape but a different identity, so pydantic rejects
the construction with an unhandled ValidationError: a bare 500, not the
UCP error envelope, on any update whose payment carries a non-empty
instruments array. create_checkout builds PaymentResponse the same way
from the create request and collapses the same way; every existing test
that touches payment sends instruments: [], so neither site was ever
exercised with a real instrument.
checkout.json marks payment ucp_request: {create: optional, update:
optional}, and checkout.md documents that submitting payment populates
payment.instruments with the collected instrument data, so update is a
normal place for a platform to submit payment and the server must apply
it, not 500.
Fix both sites by dumping the request-side payment to a dict and letting
PaymentResponse parse that, mirroring how buyer, context, signals, and
discounts already cross this same request to response boundary elsewhere
in this file.
Adds test_update_applies_payment_instruments and
test_create_applies_payment_instruments to integration_test.py, which
signature_integration_test.py and cart_test.py also pick up, giving 8
passing instances across the create and cart-to-checkout paths in both
signed and unsigned harness modes. Kill tested: reverting the service
change with the tests kept turns all 8 red with the same ValidationError;
restoring turns them green.
Builds on fix/test-helper-request-variants (samples 9c0ebef), cherry
picked here as its own commit, since a fresh ucp-sdk install resolves
0.4.6 and the shared checkout test helper needs that fix to import.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A checkout update that submits
payment.instrumentsreturns a bare HTTP 500instead of applying the instruments. A checkout create with the same shape
has the identical defect, unexercised by any existing test.
Root cause
rest/python/server/services/checkout_service.py:532inupdate_checkout:checkout_req.payment.instrumentsholdspayment_instrument_update_request.SelectedPaymentInstrumentinstances,the request side model that
ucp_sdkgenerates forPaymentUpdateRequest.PaymentResponse(imported asucp_sdk.models.schemas.shopping.payment.Payment)declares that same field as
list[payment_instrument.SelectedPaymentInstrument], the sibling responseclass. The two classes share every field name and shape (
id,handler_id,type,billing_address,credential,display,selected) but are notthe same class and not a subclass of one another, so pydantic refuses to
construct
PaymentResponsefrom an instance of the wrong one:That
ValidationErroris neither aRequestValidationErrornor aUcpError,the two exception types this server maps to the UCP error envelope, so
FastAPI answers with its default unhandled exception response: a bare 500
with no body a caller can act on.
create_checkoutat line 395 buildsPaymentResponsethe same way fromcheckout_req.payment.instruments, where the request side class is insteadpayment_instrument_create_request.SelectedPaymentInstrument. Sameconstruction, same collision, same 500. Every test in this repository that
sends
paymenton create sendsinstruments: [], and an empty list neverreaches the per item type check, so the create path 500 has been present
and untested since
payment.instrumentswas first threaded through thisfile.
Spec grounding
checkout.jsonmarks thepaymentmember of the update requestucp_request: {create: "optional", update: "optional"}, so a platformsubmitting payment on update is a conformant request, not an edge case.
checkout.md is explicit about what the field is for: "When the buyer
submits payment, the platform populates the payment.instruments array with
the collected instrument data." An update carrying instruments must apply
them and answer 200 with the instruments reflected, which is what the
added tests assert (checked against the currently pinned 2026-04-08 schema
and spec).
The fix
Both sites now dump the request side payment object to a dict first and
let
PaymentResponseparse that dict, rather than handing it a foreignmodel instance:
This mirrors the pattern this file already uses at the same request to
response boundary for
buyer,context,signals, anddiscountsa fewlines below and above it (
source_context.model_dump(exclude_none=True),and so on), so the fix follows the established convention already in this file
rather than introducing a new one.
Class sweep table
The defect class is: a response model constructed from a request typed
object fields, where the two sides are different generated classes.
Every
...Response(...)construction incheckout_service.pythat reachesinto request data was checked:
create_checkoutpayment (line 395)checkout_req.payment.instrumentsupdate_checkoutpayment (line 532)checkout_req.payment.instrumentscreate_checkout/update_checkoutLineItemResponse,ItemResponseli_req.id,li_req.item.id,li_req.quantity)create_checkoutShippingDestinationResponse(line 342)dest_req.address_country, and so on)update_checkoutShippingDestinationResponse(line 625)dest_req.model_dump(exclude_none=True)update_checkoutShippingDestinationResponsefrom stored customer address (line 634)FulfillmentResponseClass,FulfillmentMethod,FulfillmentGroupTotalResponse(totals rebuild)amount,type) computed by the servercart_service.pypaymenthandling anywhere in the filepaymentis the only field in this file where the response type is builtdirectly from a same shaped sibling model instead of a dict or scalars, on
both the create and the update path.
Testing
test_update_applies_payment_instrumentsandtest_create_applies_payment_instrumentstointegration_test.py.signature_integration_test.pyandcart_test.pyboth import andsubclass
IntegrationTest, so the two new tests also run signed and aspart of the cart to checkout conversion path, for 8 passing test
instances total from the 2 new test bodies.
the 8 new instances; 0 regressions).
checkout_service.pychange and keepingthe tests turns all 8 instances red with the same
pydantic_core._pydantic_core.ValidationError: ... model_typethis PRfixes. Restoring the fix turns them green again.
pre-commit runon both changed files: all hooks pass, includingruffandruff-formatas separate hooks.simple_happy_path_client.pyagainst a freshlyseeded server completes discovery through order completion.
create followed by
PUT /checkout-sessions/{id}carrying a non emptypayment.instrumentsnow answers200with the instrument reflected inthe response body, where it previously answered
500.Dependency note
This branch is built on top of the fixture correction proposed in #219
(cherry picked here as
0da1797), which fixes an unrelated collision inthe shared checkout test helper (
ShippingDestinationCreateRequestversusits response sibling) that otherwise makes the whole suite fail to import
once a fresh install resolves
ucp-sdk0.4.6. If that PR has alreadymerged by the time this one is reviewed, this branch should rebase onto
main and drop the cherry picked commit; if not, this PR depends on it
landing first, or the same fix should be folded in ahead of merge.
Fixes #218