Skip to content

Make the callback flow reusable for secondary payment models - #492

Open
PetrDlouhy wants to merge 2 commits into
jazzband:mainfrom
PetrDlouhy:process-data-multiple-payment-models
Open

Make the callback flow reusable for secondary payment models#492
PetrDlouhy wants to merge 2 commits into
jazzband:mainfrom
PetrDlouhy:process-data-multiple-payment-models

Conversation

@PetrDlouhy

Copy link
Copy Markdown
Contributor

Depends on #PR1 — this branch contains that commit until it merges. Please review it after (or alongside) that one.

The problem

PAYMENT_MODEL names a single model, but projects commonly charge through several — subscriptions, marketplace orders, card verification — each with its own model and its own callback URL. There is currently no way to reuse process_data for the additional ones, so those projects copy the view into their codebase.

Copies drift, and the drift is invisible until it costs something. Concretely: a project of ours carries four such copies. When process_data learned to lock the payment row (#PR1), none of the copies did, and all four kept the race that had already demoted a captured payment to error in production. Any future fix here has the same problem.

The change

Split the view into the two steps it always was, and let it take the model:

def get_payment_or_404(token, payment_model=None):      # the locked fetch
def process_payment_data(payment, request, provider=None):  # the dispatch
def process_data(request, token, provider=None, payment_model=None):

process_data's behavior is unchanged when payment_model is not passed.

  • A project whose extra model needs no custom logic routes the real view and inherits later fixes automatically:

    path("marketplace/process/<uuid:token>/", process_data,
         {"payment_model": MarketplacePayment}, name="process_marketplace_payment"),
  • One that needs logic around the provider call composes the two helpers and still gets the locked fetch.

Both are documented under Using more than one payment model in docs/payment-model.rst.

Two deliberate choices:

  • payment_model, not a queryset argument. A queryset parameter would let callers pass an unlocked or pre-filtered one and silently lose the serialization the fetch exists to provide.
  • _default_manager instead of .objects. That is what get_object_or_404 uses when handed a model class, so adding the lock does not change which manager a project's payment model is read through. (It also types correctly — type[Model].objects does not.)

Tests

Written test-first; the new tests fail without this change. They cover the default and explicit model paths, provider resolution and its Http404, an explicitly passed provider, and the view with an explicit model. Full suite passes; ruff and mypy clean.

PetrDlouhy and others added 2 commits July 30, 2026 06:52
process_data already runs inside @atomic, but fetched the payment
without a row lock. Two concurrent callbacks for the same payment -
typically the provider's asynchronous webhook and the customer's
browser POSTing to the same process URL (widget/wallet token, 3DS
return, refresh) - therefore interleave on instances loaded before
each other's commit, and the loser overwrites the winner's state.

This is not theoretical. Observed in production with the PayU
provider (2026-07-27 and 2026-07-28): the webhook confirmed a
captured payment and completed its order; two seconds later the
customer's duplicate request, holding an instance loaded before that
commit, failed its create_order retry and demoted the payment to
ERROR - a paid, captured order recorded as failed. Every provider
sharing the process URL for webhooks and browser callbacks is
exposed; provider-level in-memory status guards cannot close this,
because the instance they check is exactly what is stale.

Fetch the payment with select_for_update() so the second handler
blocks until the first commits and then observes the fresh state.
The lock spans the same duration the transaction already did, and it
only ever contends between requests for the same payment;
cross-payment traffic is unaffected. static_callback delegates to
process_data and inherits the lock. On databases without
SELECT ... FOR UPDATE support (such as SQLite), Django documents
select_for_update() as having no effect, so behavior there is
unchanged.

Written test-first: the new test asserts the locked fetch and failed
against the previous code ("Expected 'select_for_update' to be
called once. Called 0 times.") before the fix.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PAYMENT_MODEL names a single model, but projects commonly charge
through several (subscriptions, marketplace orders, card verification
...), each with its own model and callback URL. Today the only way to
handle the additional ones is to copy process_data into the project.

Copies drift. The row locking added in the previous commit is a
concrete example: a project carrying four such copies kept all four
unprotected, and the race cost a captured payment its CONFIRMED state
in production. Anything fixed here in future has the same problem.

Split the view into the two steps it always was and let it take the
model:

* get_payment_or_404(token, payment_model=None) - the locked fetch,
* process_payment_data(payment, request, provider=None) - the dispatch,
* process_data(..., payment_model=None) - unchanged behaviour, now
  routable for another model from the project's URLconf.

A project whose extra model needs no custom logic can now route this
view directly and inherit later fixes; one that needs logic around the
provider call composes the two helpers and still gets the locked fetch
that makes concurrent callbacks safe. Both are documented under
"Using more than one payment model".

The fetch goes through _default_manager rather than .objects, which is
what get_object_or_404 does when handed a model class - locking must
not change which manager a project's payment model is read through.

A payment_model argument is deliberately narrower than accepting a
queryset: a queryset parameter would let callers pass an unlocked or
pre-filtered one and silently lose the serialization.

Written test-first; the new tests fail without this change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PetrDlouhy
PetrDlouhy requested a review from WhyNotHugo July 30, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant