Orders need a strict guarantee that they won't be accidentally overwritten in case two users (admin/customer, or customer/script) collide.
There are two ways to accomplish this: pessimistic and optimistic locking.
Pessimistic locking ensures that only one side can load the order for editing, all other loads are blocked.
Optimistic locking allows all sides to load, but will only allow one to save, the others will fail.
This conflict is not common, and is usually one of two scenarios:
1) Admin editing a customer order (which is still a cart or in checkout)
2) A script (cron, queue) modifying an order being edited by either the admin or the customer.
Commerce 1.x
Commerce 1.x implemented pessimistic locking. This proved to be too strict, because it's hard to identify an edit context in Drupal. To be safe, the code would lock all orders viewed in an admin view, on the order view page, etc. This problem was identified and is being addressed in #2240427: Improve commerce entities locking.
Drupal 8 core
Drupal 8 implements optimistic locking (kinda) for content entities. Any content entity that implements EntityChangedInterface has the EntityChanged constraint added.
The EntityChangedConstraintValidator ensures that validation fails if the entity changed timestamp changed in the meantime.
The message shown is "The content has either been modified by another user, or you have already submitted modifications. As a result, your changes cannot be saved."
There are two problems with this:
- Created is not granular enough, since it's in seconds. There can be multiple entity saves in the same second.
- The constraint is not enforced on entity save, only on the entity form.
Commerce 2.x
We want to go with optimistic locking, since it will provide better performance. We'll go with an explicit version field, to ensure ideal granularity.
Plan:
- Unset the EntityChanged constraint.
- Create a version integer field that gets incremented on each save (in preSave())
- Create an OrderVersionConstraintValidator that does the same thing as EntityChangedConstraintValidator (load original order, compare versions), for order forms.
- Before incrementing the version field in preSave() repeat the logic from OrderVersionConstraintValidator, and throw an exception (OrderVersionMismatchException).
This ensures that the overwrite is caught even if the validator isn't invoked.
- From commerce_cart add an order form alter that shows a warning for all cart orders active in the last hour. "This order has been modified by the customer in the last hour. Modifying it here could cause their future changes to be lost. Proceed with caution.".
We also need explicit $order->lock() and $order->unlock() methods, for cases where optimistic locking isn't strict enough (payment page at checkout, for example). Trying to save a locked order throws an exception.
Comment | File | Size | Author |
---|---|---|---|
#66 | interdiff_62-66.txt | 4.49 KB | jsacksick |
#66 | 2656818-66.patch | 21.44 KB | jsacksick |
| |||
#50 | Screen Shot 2020-11-11 at 4.39.37 PM.png | 45.66 KB | mglaman |
#50 | 2656818-50.patch | 24.41 KB | mglaman |
| |||
#48 | 2656818-48.patch | 17.8 KB | mglaman |
|
Comments
Comment #2
bojanz CreditAttribution: bojanz at Centarro commentedAdd a node that we need to unset the EntityChanged constraint.
Comment #3
vasikehere is patch for this that tries to implement what's requested in the issue summary.
However i wasn't able to test it, maybe there's something missing.
Comment #4
bojanz CreditAttribution: bojanz at Centarro commented+ * "version" = "version",
version is not an entity key
entity keys are a fixed list (id, label, uuid, uid, bundle)
nothing else
+ $this->setVersion(1); -> this should be a default value set in baseFieldDefinitions
The next step will be to write a test.
We are also missing "- From commerce_cart add an order form alter that shows a warning for all cart orders active in the last hour. "This order has been modified by the customer in the last hour. Modifying it here could cause their future changes to be lost. Proceed with caution."
Comment #5
vasikehere is patch with bojanz (#4) corrections and suggestions:
implement OrderConstraintsTest
it seems the order validation requires also a value for store_id, which i added to this test.
Should we update for other Order tests?
todo: commerce_cart add an order form alter
Comment #6
vasikenew update for the order edit message for the orders that are carts and modified in the last hour.
Comment #7
vasikeNew patch update:
- Fix for order edit message: "<" instead of ">".
- Implements hook_form_alter() for Views commerce cart forms
- - Check for a Views commerce cart forms, check is a Views form and then check if it's the cart form
- - Replace the submit button label, moved from Edit quantity Views field handler
- - Add (& build) validation for submit buttons, also for remove buttons if present.
For checking "Views commerce cart forms" - there is a new helper function, also used for "commerce_order_type_form" alter.
It checks if View has 'commerce_cart_form' like tag, has commerce_order as base table and the order_id as the first argument.
Comment #8
vasikealso created a PR with the latest patch
https://github.com/drupalcommerce/commerce/pull/317
Comment #9
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedSlight re-roll so patch applies
Comment #10
smccabe CreditAttribution: smccabe as a volunteer and at Acro Commerce commentedFew notes, although it is a really big patch to review so probably needs more than 1 pass.
small change, should be "if"
Isn't this a message that would be seen by the customer? Should it say something like "Your order has been modified" and not "by the customer"? Also why do we only care about the last hour?
Why do we remove this button in the context of this patch? I don't follow.
Could this cause an issue where it has been updated but the version hasn't been saved yet?
Comment #11
bojanz CreditAttribution: bojanz at Centarro commentedExpanded architecture with explicit lock/unlock methods.
Comment #12
vasikenew PR available
https://github.com/drupalcommerce/commerce/pull/649
Updates from previous:
- Added $order->lock() and $order->unlock() methods (not sure about them yet).
- post_update hook to add the version field.
- Update Helper function to check if a view is a 'commerce_cart' view - it takes the tag as argument.
- Update the test - check for Entity validation only
Maybe here we should build other Test for all kind of validation for Orders - plan needed???.
Closed the previous pr.
Comment #13
vasikenew commits for the latest PR to make it green
https://github.com/drupalcommerce/commerce/pull/649
Comment #14
vasikePR updated with requested changes there.
Comment #15
vasikenew updates for the latest PR to make it green
- There were some issues with 8.x-2.x updates.
- (bonus) Extend Order Constraints tests to Order validations tests.
As i noticed the Order has other thing to validate - Order (fields) data validation.
Comment #16
vasikePR "reroll"
Comment #17
bojanz CreditAttribution: bojanz at Centarro commentedNote that we committed a related fix in #2906563: Provide a way to explicitly lock the order.
Comment #18
vasikePr updated - according with module latest updates on locking.
Comment #19
mglamanAfter #3011667: Saving the order before its payment in PaymentGateway::onReturn() can cause data loss, picking this up.
Comment #20
mglamanHere is a re-roll of the PR against HEAD. It is missing the logic from commerce_cart.module to check versions when updating the cart.
Comment #22
mglamanComment #23
mglamanThis provides a test that uncovers a fun problem in \Drupal\commerce_order\Entity\OrderItem::postSave
With the current CartManager implementation, this causes a save that is out of sequence and breaks our version numbers. I wrote a test to demonstrate and commented out
$order_item->set('order_id', $cart->id());
in CartManager to test.Comment #25
mglamanForgot test group.
Comment #27
mglamanAgain, another post* in an entity's life cycle burns us. This time it is order items trying to clean up their references
Comment #28
mglamanIf we undo the work in #3005070: [Reverted] Make the order-order_item relationship more robust, most of our problems are solved.
Comment #29
mglamanThis undoes the order item postSave and postDelete. In reality, we should have the order saving or deleting items at least since the order controls the relationship.
Comment #31
mglamanFixes test assert which throws version mismatch.
Comment #32
bojanz CreditAttribution: bojanz at Centarro commented#3005070: [Reverted] Make the order-order_item relationship more robust has now been reverted, this patch can be rerolled.
Comment #33
mglamanHere is a reroll.
Comment #35
mglamanRemove php7ness
Comment #37
skyredwangAdding #3119157: CartManager bug in asynchronous multi-threads to the related issues.
Comment #38
amateescu CreditAttribution: amateescu as a volunteer commentedIf the patch committed in #3130011: Clear broken order items reference on presave is not reverted in that issue, I think the patch here should revert it.
Comment #39
jsacksick CreditAttribution: jsacksick at Centarro commentedWe'll have to come back to this as I'm currently experiencing several issues due to the order not being locked, See #3119157: CartManager bug in asynchronous multi-threads and #3130011: Clear broken order items reference on presave.
IMO this is a critical feature that needs to be implemented, especially in a headless context where you have async requests potentially saving the same order.
Not having locking opens the door to data integrity issues.
That said, my main concern is that it requires to update any code that's saving the order and I suggest adding a setting (disabled by default) that toggles whether we throw an exception vs just logging.
When the setting is off, we should add a big warning that links to a documentation page explaining why locking is essential.
Bumping the priority.
Comment #40
jsacksick CreditAttribution: jsacksick at Centarro commentedI think the logic should be moved to
OrderStorage::doOrderPresave()
since I believe the version check should be happening before refreshing the order.Also, the order now implements the EntityChangedInterface + Trait so the patch definetly needs a reroll.
Comment #41
mglamanDiscussing with @agoradesign, we'd definitely need to make this opt-in and not a default behavior. Just because most projects have code that don't support this.
Something like
$settings['enable_order_locking']
. We should make it enabled by default in tests to help improve future code. Kind of like the config schema checker in tests.Comment #42
bojanz CreditAttribution: bojanz at Centarro commentedMaybe put it on the same page as the checkbox for enabling product revisions? And default both to TRUE on new installs?
Comment #43
mglamanHere's a reroll. I need to add the settings as proposed in #42.
Comment #45
mglamanFailure is due to forgetting to bring this over 🤦♂️
Comment #46
mglaman#40:
The order preSave runs before the storage methods – so before refresh
Comment #47
mglaman#42
I keep thinking about this and I'm afraid it will cause so many bug reports, the same reason we are afraid of turning it on by default for existing sites.
Comment #48
mglamanOkay, here's an attempt at making it a setting that is stored in the entity type's definition. It is set to log by default and tests turn it out by default. The next step is a setting form to control this.
Comment #49
mglamanThis should be
Next is to provide a settings form at
/admin/commerce/config/orders/settings
that will live alongside the order types, order item types, and number patterns pages.Comment #50
mglamanThis adds a setting form to disable logging and allow the exception to be thrown when saving an order out of sync.
Screenshots of the settings form. Words to be bikeshed.
Default:
Disable logging:
I'm beginning to think it needs to be a radio, not a checkbox:
Comment #51
jsacksick CreditAttribution: jsacksick at Centarro commentedWhy this?
- $order_item->set('order_id', $cart->id());
.I also think this makes more sense.
Comment #52
mglaman@jsacksick see #23
Comment #54
mglamanNew screenshots. I also added the comment back per #52, so that we know why this change was made.
Crediting lisastreeter for words help :)
Comment #55
mglamanThe post-update in
commerce_order_post_update_14
fails... apparently the config importer doesn't work for regular config objects (even though we have code to.) It doesn't detect the config type forcommerce_order.settings
and tries to load a nil config entity type.Comment #56
mglamanThis fixes the config import by using the config factory directly. I'll open a follow up issue for the config importer.
Comment #57
mglamanLinking follow up on config importer: #3182252: ConfigUpdater doesn't support basic config objects
Comment #58
jsacksick CreditAttribution: jsacksick at Centarro commentedWe need to put back
$order_item->set('order_id', $cart->id());
, this was causing an issue whenOrderItem::postSave()
was trying to set the reference from the order to the order item, but this is no longer the case.I've been testing #3179312: Add an option to Buy X Get Y to auto-add the "Get" product variation where I also advised to remove
$order_item->set('order_id', $cart->id());
and this is causing a fatal error when the tax module is trying to resolve the customer profile inTaxTypeBase::resolveCustomerProfile()
Comment #59
jsacksick CreditAttribution: jsacksick at Centarro commentedI put back
$order_item->set('order_id', $cart->id());
which was removed because of #3005070: [Reverted] Make the order-order_item relationship more robust that was reverted since.It's probably unnecessary though... As since #3143382: AddToCartForm should reference the destination cart when building the order item the order item references the order when the Add to cart form is built.
I fixed the post update / update numbers (to account for the updates that occurred since).
Additionally, I believe we stopped implementing the
EntityChangedInterface
in order to skip theEntityChanged
constraint which is automatically added from the EntityType constructor:However, I believe we could simply remove the constraint from
commerce_order_entity_type_alter()
.I'm tempted to commit the patch... Though what's making me hesitant is that we don't actually have any try catch blocks in our code expecting that exception... (Shouldn't we expect it from several places like
AddToCartForm::submitForm()
for example which saves the order?PaymentCheckoutController::returnPage()
would be a good candidate as well...The main reason why we could go ahead and commit this is that we don't throw the exception by default and we simply log it... That is what's making this feature "safe" IMO.
Perhaps it's fine to commit the patch as is and wait for actual bug reports... Thoughts?
I have some tests failing locally for some reason... Let's see what d.org says.
Also... Does it actually make sense to expose a setting in the UI for this vs just setting it from the settings.php? (I'm afraid the form/settings are too technical for non developers...
Comment #60
mglamanLETS DO THIS
The only downside is that this can be such a performance killer (load unchanged.) But I cannot fathom a better alternative.
Comment #61
agoradesign CreditAttribution: agoradesign commentedyeah, as long as we only log by default, I'm also for committing this! Hopefully some brave boys and girls will test the exception configuration in the wild then :D imho any step towards solving that problem is very important, as this is the most annoying Commerce bug I know so far!
@mglaman: well yes, but I don't think that the impact is that big, and a write operation (especially on an order) should be allowed to do some non-trivial stuff
Comment #62
jsacksick CreditAttribution: jsacksick at Centarro commented@mglaman: The EntityChangedConstraint is also doing this... So it's not like we're introducing a performance regression...
As pointed out by Ryan, the path for the settings form is "wrong" and should contain /orders (so that the breadcrumb allows you to go back to the orders config, and not the general commerce config.
Comment #63
jsacksick CreditAttribution: jsacksick at Centarro commentedReviewing the exception message, and I think it should be changed, since finding that exception in logs isn't going to be helpful...
What about...
Instead...
So if the exception is found in logs, we at least know which order ID it relates to... Thoughts?
Comment #64
rszrama CreditAttribution: rszrama at Centarro commentedGood from a functionality standpoint. From a UX standpoint, proposed a simpler order settings form:
Comment #65
rszrama CreditAttribution: rszrama at Centarro commentedSorry, wrong image.
Comment #66
jsacksick CreditAttribution: jsacksick at Centarro commentedAttached patch addresses comment #63 + the settings form improvements from @rzsrama.
Hopefully this is good to go now!
Comment #68
jsacksick CreditAttribution: jsacksick at Centarro commentedCommitted 🎉 🎉 !!! Thanks everyone!
Comment #70
newaytech CreditAttribution: newaytech commentedHi folks,
Thanks for the awesome work on Commerce - it's a great product. I've noticed an error in my log files following the latest Optimistic locking patch - so I flicked the switch to fatal error to get a full stack trace. Nothing I can see in my custom code here - so thought I'd share here as an example of issues in the wild:
This is thrown on "/checkout/7051/payment" - right after adding credit card details and submitting the form.
Comment #71
agoradesign CreditAttribution: agoradesign commented@newaytech which payment gateway plugin do you use?
Comment #72
newaytech CreditAttribution: newaytech commentedHi @agoradesign - Stripe - which for every error was in use (we have manual payment as option too)
Comment #73
John Pitcairn CreditAttribution: John Pitcairn commentedI'm also seeing this exception thrown and logged when commerce recurring processes a queued draft order from a cron run. We are also using Stripe.
Comment #74
Ambient.ImpactI'm also getting the same thing as #70 when I switch it to throw an exception, also using Stripe (though not recurring). The order seems to complete fine and our order paid event subscriber is correctly invoked if set to the default to just log it, so it doesn't seem to be causing issues but thought I'd confirm it.
Comment #75
John Pitcairn CreditAttribution: John Pitcairn commentedFor code that is saving orders somewhere and triggering this exception, is there something the code should now be doing to avoid that? Is that documented with an example? There's no change record.
Comment #76
BerdirMake sure you are loading the latest version of an order before saving it. There are issues with race conditions, for example between the notify and return routes/callbacks of a payment gateway. See #3043180: The changes made to the order on the onNotify method are not applied on the onReturn method for my attempt at avoiding that, but it will require changes to the affected gateways. Feedback very welcome there.