Make the callback flow reusable for secondary payment models - #492
Open
PetrDlouhy wants to merge 2 commits into
Open
Make the callback flow reusable for secondary payment models#492PetrDlouhy wants to merge 2 commits into
PetrDlouhy wants to merge 2 commits into
Conversation
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>
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.
The problem
PAYMENT_MODELnames 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 reuseprocess_datafor 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_datalearned to lock the payment row (#PR1), none of the copies did, and all four kept the race that had already demoted a captured payment toerrorin 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:
process_data's behavior is unchanged whenpayment_modelis not passed.A project whose extra model needs no custom logic routes the real view and inherits later fixes automatically:
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 aquerysetargument. A queryset parameter would let callers pass an unlocked or pre-filtered one and silently lose the serialization the fetch exists to provide._default_managerinstead of.objects. That is whatget_object_or_404uses 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].objectsdoes 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;ruffandmypyclean.