Skip to content

Commit 48a4844

Browse files
committed
MAGETWO-69701: [GitHub] Concurrent checkouts can lead to negative stock #6363 [backport 2.1]
1 parent e738316 commit 48a4844

File tree

5 files changed

+169
-23
lines changed

5 files changed

+169
-23
lines changed

app/code/Magento/CatalogInventory/Model/ResourceModel/Stock.php

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public function lockProductsStock($productIds, $websiteId)
126126
}
127127
$itemTable = $this->getTable('cataloginventory_stock_item');
128128
$select = $this->getConnection()->select()->from(['si' => $itemTable])
129-
->where('website_id=?', $websiteId)
129+
->where('website_id = ?', $websiteId)
130130
->where('product_id IN(?)', $productIds)
131131
->forUpdate(true);
132132

@@ -139,9 +139,15 @@ public function lockProductsStock($productIds, $websiteId)
139139
'type_id' => 'type_id'
140140
]
141141
);
142-
$this->getConnection()->query($select);
142+
$items = [];
143143

144-
return $this->getConnection()->fetchAll($selectProducts);
144+
foreach ($this->getConnection()->query($select)->fetchAll() as $si) {
145+
$items[$si['product_id']] = $si;
146+
}
147+
foreach ($this->getConnection()->fetchAll($selectProducts) as $p) {
148+
$items[$p['product_id']]['type_id'] = $p['type_id'];
149+
}
150+
return $items;
145151
}
146152

147153
/**

app/code/Magento/CatalogInventory/Model/StockManagement.php

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,28 +48,37 @@ class StockManagement implements StockManagementInterface
4848
*/
4949
private $qtyCounter;
5050

51+
/**
52+
* @var StockRegistryStorage
53+
*/
54+
private $stockRegistryStorage;
55+
5156
/**
5257
* @param ResourceStock $stockResource
5358
* @param StockRegistryProviderInterface $stockRegistryProvider
5459
* @param StockState $stockState
5560
* @param StockConfigurationInterface $stockConfiguration
5661
* @param ProductRepositoryInterface $productRepository
5762
* @param QtyCounterInterface $qtyCounter
63+
* @param StockRegistryStorage|null $stockRegistryStorage
5864
*/
5965
public function __construct(
6066
ResourceStock $stockResource,
6167
StockRegistryProviderInterface $stockRegistryProvider,
6268
StockState $stockState,
6369
StockConfigurationInterface $stockConfiguration,
6470
ProductRepositoryInterface $productRepository,
65-
QtyCounterInterface $qtyCounter
71+
QtyCounterInterface $qtyCounter,
72+
StockRegistryStorage $stockRegistryStorage = null
6673
) {
6774
$this->stockRegistryProvider = $stockRegistryProvider;
6875
$this->stockState = $stockState;
6976
$this->stockConfiguration = $stockConfiguration;
7077
$this->productRepository = $productRepository;
7178
$this->qtyCounter = $qtyCounter;
7279
$this->resource = $stockResource;
80+
$this->stockRegistryStorage = $stockRegistryStorage ?: \Magento\Framework\App\ObjectManager::getInstance()
81+
->get(StockRegistryStorage::class);
7382
}
7483

7584
/**
@@ -92,9 +101,12 @@ public function registerProductsSale($items, $websiteId = null)
92101
$fullSaveItems = $registeredItems = [];
93102
foreach ($lockedItems as $lockedItemRecord) {
94103
$productId = $lockedItemRecord['product_id'];
104+
$this->stockRegistryStorage->removeStockItem($productId, $websiteId);
105+
95106
/** @var StockItemInterface $stockItem */
96107
$orderedQty = $items[$productId];
97108
$stockItem = $this->stockRegistryProvider->getStockItem($productId, $websiteId);
109+
$stockItem->setQty($lockedItemRecord['qty']); // update data from locked item
98110
$canSubtractQty = $stockItem->getItemId() && $this->canSubtractQty($stockItem);
99111
if (!$canSubtractQty || !$this->stockConfiguration->isQty($lockedItemRecord['type_id'])) {
100112
continue;
@@ -180,7 +192,7 @@ protected function getProductType($productId)
180192
}
181193

182194
/**
183-
* @return Stock
195+
* @return ResourceStock
184196
*/
185197
protected function getResource()
186198
{

app/code/Magento/CatalogInventory/Test/Unit/Model/ResourceModel/StockTest.php

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,11 @@ class StockTest extends \PHPUnit_Framework_TestCase
6767
*/
6868
protected $selectMock;
6969

70+
/**
71+
* @var \PHPUnit_Framework_MockObject_MockObject|\Zend_Db_Statement_Interface
72+
*/
73+
protected $statementMock;
74+
7075
/**
7176
* Prepare subjects for tests.
7277
*
@@ -95,6 +100,7 @@ protected function setUp()
95100
$this->connectionMock = $this->getMockBuilder(Mysql::class)
96101
->disableOriginalConstructor()
97102
->getMock();
103+
$this->statementMock = $this->getMockForAbstractClass(\Zend_Db_Statement_Interface::class);
98104
$this->stock = $this->getMockBuilder(Stock::class)
99105
->setMethods(['getTable', 'getConnection'])
100106
->setConstructorArgs(
@@ -119,7 +125,21 @@ public function testLockProductsStock()
119125
{
120126
$websiteId = 0;
121127
$productIds = [1, 2, 3];
122-
$result = ['testResult'];
128+
$result = [
129+
1 => [
130+
'product_id' => 1,
131+
'type_id' => 'simple'
132+
],
133+
2 => [
134+
'product_id' => 2,
135+
'type_id' => 'simple'
136+
],
137+
3 => [
138+
'product_id' => 3,
139+
'type_id' => 'simple'
140+
]
141+
];
142+
123143
$this->selectMock->expects(self::exactly(2))
124144
->method('from')
125145
->withConsecutive(
@@ -130,7 +150,7 @@ public function testLockProductsStock()
130150
$this->selectMock->expects(self::exactly(3))
131151
->method('where')
132152
->withConsecutive(
133-
[self::identicalTo('website_id=?'), self::identicalTo($websiteId)],
153+
[self::identicalTo('website_id = ?'), self::identicalTo($websiteId)],
134154
[self::identicalTo('product_id IN(?)'), self::identicalTo($productIds)],
135155
[self::identicalTo('entity_id IN (?)'), self::identicalTo($productIds)]
136156
)
@@ -149,10 +169,19 @@ public function testLockProductsStock()
149169
->willReturn($this->selectMock);
150170
$this->connectionMock->expects(self::once())
151171
->method('query')
152-
->with(self::identicalTo($this->selectMock));
172+
->with(self::identicalTo($this->selectMock))
173+
->willReturn($this->statementMock);
174+
$this->statementMock->expects(self::once())
175+
->method('fetchAll')
176+
->willReturn([
177+
1 => ['product_id' => 1],
178+
2 => ['product_id' => 2],
179+
3 => ['product_id' => 3]
180+
]);
181+
153182
$this->connectionMock->expects(self::once())
154183
->method('fetchAll')
155-
->with($this->selectMock)
184+
->with(self::identicalTo($this->selectMock))
156185
->willReturn($result);
157186

158187
$this->stock->expects(self::exactly(2))

app/code/Magento/Checkout/Model/GuestPaymentInformationManagement.php

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66

77
namespace Magento\Checkout\Model;
88

9+
use Magento\Framework\App\ObjectManager;
10+
use Magento\Framework\App\ResourceConnection;
911
use Magento\Quote\Api\CartRepositoryInterface;
1012
use Magento\Framework\Exception\CouldNotSaveException;
1113

@@ -51,13 +53,19 @@ class GuestPaymentInformationManagement implements \Magento\Checkout\Api\GuestPa
5153
*/
5254
private $logger;
5355

56+
/**
57+
* @var ResourceConnection
58+
*/
59+
private $connectionPull;
60+
5461
/**
5562
* @param \Magento\Quote\Api\GuestBillingAddressManagementInterface $billingAddressManagement
5663
* @param \Magento\Quote\Api\GuestPaymentMethodManagementInterface $paymentMethodManagement
5764
* @param \Magento\Quote\Api\GuestCartManagementInterface $cartManagement
5865
* @param \Magento\Checkout\Api\PaymentInformationManagementInterface $paymentInformationManagement
5966
* @param \Magento\Quote\Model\QuoteIdMaskFactory $quoteIdMaskFactory
6067
* @param CartRepositoryInterface $cartRepository
68+
* @param ResourceConnection|null $connectionPull
6169
* @codeCoverageIgnore
6270
*/
6371
public function __construct(
@@ -66,14 +74,16 @@ public function __construct(
6674
\Magento\Quote\Api\GuestCartManagementInterface $cartManagement,
6775
\Magento\Checkout\Api\PaymentInformationManagementInterface $paymentInformationManagement,
6876
\Magento\Quote\Model\QuoteIdMaskFactory $quoteIdMaskFactory,
69-
CartRepositoryInterface $cartRepository
77+
CartRepositoryInterface $cartRepository,
78+
ResourceConnection $connectionPull = null
7079
) {
7180
$this->billingAddressManagement = $billingAddressManagement;
7281
$this->paymentMethodManagement = $paymentMethodManagement;
7382
$this->cartManagement = $cartManagement;
7483
$this->paymentInformationManagement = $paymentInformationManagement;
7584
$this->quoteIdMaskFactory = $quoteIdMaskFactory;
7685
$this->cartRepository = $cartRepository;
86+
$this->connectionPull = $connectionPull ?: ObjectManager::getInstance()->get(ResourceConnection::class);
7787
}
7888

7989
/**
@@ -85,20 +95,33 @@ public function savePaymentInformationAndPlaceOrder(
8595
\Magento\Quote\Api\Data\PaymentInterface $paymentMethod,
8696
\Magento\Quote\Api\Data\AddressInterface $billingAddress = null
8797
) {
88-
$this->savePaymentInformation($cartId, $email, $paymentMethod, $billingAddress);
98+
$salesConnection = $this->connectionPull->getConnection('sales');
99+
$checkoutConnection = $this->connectionPull->getConnection('checkout');
100+
$salesConnection->beginTransaction();
101+
$checkoutConnection->beginTransaction();
102+
89103
try {
90-
$orderId = $this->cartManagement->placeOrder($cartId);
91-
} catch (\Magento\Framework\Exception\LocalizedException $e) {
92-
throw new CouldNotSaveException(
93-
__($e->getMessage()),
94-
$e
95-
);
104+
$this->savePaymentInformation($cartId, $email, $paymentMethod, $billingAddress);
105+
try {
106+
$orderId = $this->cartManagement->placeOrder($cartId);
107+
} catch (\Magento\Framework\Exception\LocalizedException $e) {
108+
throw new CouldNotSaveException(
109+
__($e->getMessage()),
110+
$e
111+
);
112+
} catch (\Exception $e) {
113+
$this->getLogger()->critical($e);
114+
throw new CouldNotSaveException(
115+
__('An error occurred on the server. Please try to place the order again.'),
116+
$e
117+
);
118+
}
119+
$salesConnection->commit();
120+
$checkoutConnection->commit();
96121
} catch (\Exception $e) {
97-
$this->getLogger()->critical($e);
98-
throw new CouldNotSaveException(
99-
__('An error occurred on the server. Please try to place the order again.'),
100-
$e
101-
);
122+
$salesConnection->rollBack();
123+
$checkoutConnection->rollBack();
124+
throw $e;
102125
}
103126

104127
return $orderId;

app/code/Magento/Checkout/Test/Unit/Model/GuestPaymentInformationManagementTest.php

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66

77
namespace Magento\Checkout\Test\Unit\Model;
88

9+
use Magento\Framework\App\ResourceConnection;
10+
use Magento\Framework\DB\Adapter\AdapterInterface;
11+
912
/**
1013
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
1114
*/
@@ -46,6 +49,11 @@ class GuestPaymentInformationManagementTest extends \PHPUnit_Framework_TestCase
4649
*/
4750
private $loggerMock;
4851

52+
/**
53+
* @var ResourceConnection|\PHPUnit_Framework_MockObject_MockObject
54+
*/
55+
private $resourceConnectionMock;
56+
4957
protected function setUp()
5058
{
5159
$objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
@@ -66,14 +74,19 @@ protected function setUp()
6674
false
6775
);
6876
$this->loggerMock = $this->getMock(\Psr\Log\LoggerInterface::class);
77+
$this->resourceConnectionMock = $this->getMockBuilder(ResourceConnection::class)
78+
->disableOriginalConstructor()
79+
->getMock();
80+
6981
$this->model = $objectManager->getObject(
7082
\Magento\Checkout\Model\GuestPaymentInformationManagement::class,
7183
[
7284
'billingAddressManagement' => $this->billingAddressManagementMock,
7385
'paymentMethodManagement' => $this->paymentMethodManagementMock,
7486
'cartManagement' => $this->cartManagementMock,
7587
'cartRepository' => $this->cartRepositoryMock,
76-
'quoteIdMaskFactory' => $this->quoteIdMaskFactoryMock
88+
'quoteIdMaskFactory' => $this->quoteIdMaskFactoryMock,
89+
'connectionPull' => $this->resourceConnectionMock
7790
]
7891
);
7992
$objectManager->setBackwardCompatibleProperty($this->model, 'logger', $this->loggerMock);
@@ -89,6 +102,27 @@ public function testSavePaymentInformationAndPlaceOrder()
89102

90103
$billingAddressMock->expects($this->once())->method('setEmail')->with($email)->willReturnSelf();
91104

105+
$adapterMockForSales = $this->getMockBuilder(AdapterInterface::class)
106+
->disableOriginalConstructor()
107+
->getMockForAbstractClass();
108+
$adapterMockForCheckout = $this->getMockBuilder(AdapterInterface::class)
109+
->disableOriginalConstructor()
110+
->getMockForAbstractClass();
111+
112+
$this->resourceConnectionMock->expects($this->at(0))
113+
->method('getConnection')
114+
->with('sales')
115+
->willReturn($adapterMockForSales);
116+
$adapterMockForSales->expects($this->once())->method('beginTransaction');
117+
$adapterMockForSales->expects($this->once())->method('commit');
118+
119+
$this->resourceConnectionMock->expects($this->at(1))
120+
->method('getConnection')
121+
->with('checkout')
122+
->willReturn($adapterMockForCheckout);
123+
$adapterMockForCheckout->expects($this->once())->method('beginTransaction');
124+
$adapterMockForCheckout->expects($this->once())->method('commit');
125+
92126
$this->billingAddressManagementMock->expects($this->once())
93127
->method('assign')
94128
->with($cartId, $billingAddressMock);
@@ -114,6 +148,27 @@ public function testSavePaymentInformationAndPlaceOrderException()
114148

115149
$billingAddressMock->expects($this->once())->method('setEmail')->with($email)->willReturnSelf();
116150

151+
$adapterMockForSales = $this->getMockBuilder(AdapterInterface::class)
152+
->disableOriginalConstructor()
153+
->getMockForAbstractClass();
154+
$adapterMockForCheckout = $this->getMockBuilder(AdapterInterface::class)
155+
->disableOriginalConstructor()
156+
->getMockForAbstractClass();
157+
158+
$this->resourceConnectionMock->expects($this->at(0))
159+
->method('getConnection')
160+
->with('sales')
161+
->willReturn($adapterMockForSales);
162+
$adapterMockForSales->expects($this->once())->method('beginTransaction');
163+
$adapterMockForSales->expects($this->once())->method('rollback');
164+
165+
$this->resourceConnectionMock->expects($this->at(1))
166+
->method('getConnection')
167+
->with('checkout')
168+
->willReturn($adapterMockForCheckout);
169+
$adapterMockForCheckout->expects($this->once())->method('beginTransaction');
170+
$adapterMockForCheckout->expects($this->once())->method('rollback');
171+
117172
$this->billingAddressManagementMock->expects($this->once())
118173
->method('assign')
119174
->with($cartId, $billingAddressMock);
@@ -176,6 +231,27 @@ public function testSavePaymentInformationAndPlaceOrderWithLocalizedException()
176231

177232
$billingAddressMock->expects($this->once())->method('setEmail')->with($email)->willReturnSelf();
178233

234+
$adapterMockForSales = $this->getMockBuilder(AdapterInterface::class)
235+
->disableOriginalConstructor()
236+
->getMockForAbstractClass();
237+
$adapterMockForCheckout = $this->getMockBuilder(AdapterInterface::class)
238+
->disableOriginalConstructor()
239+
->getMockForAbstractClass();
240+
241+
$this->resourceConnectionMock->expects($this->at(0))
242+
->method('getConnection')
243+
->with('sales')
244+
->willReturn($adapterMockForSales);
245+
$adapterMockForSales->expects($this->once())->method('beginTransaction');
246+
$adapterMockForSales->expects($this->once())->method('rollback');
247+
248+
$this->resourceConnectionMock->expects($this->at(1))
249+
->method('getConnection')
250+
->with('checkout')
251+
->willReturn($adapterMockForCheckout);
252+
$adapterMockForCheckout->expects($this->once())->method('beginTransaction');
253+
$adapterMockForCheckout->expects($this->once())->method('rollback');
254+
179255
$this->billingAddressManagementMock->expects($this->once())
180256
->method('assign')
181257
->with($cartId, $billingAddressMock);

0 commit comments

Comments
 (0)