From 01bd62916cde03cf87ece2cde488ecbb783564d5 Mon Sep 17 00:00:00 2001 From: Meng Tian Date: Wed, 18 Jan 2017 15:47:12 +0000 Subject: [PATCH 1/3] Improve conflict handling. --- Model/ConflictException.php | 7 ++++++ Model/DispatchPlugin.php | 26 ++++++++++---------- Model/ResourceModificationTimeRepository.php | 21 ++++++++-------- Setup/InstallSchema.php | 10 ++++++-- 4 files changed, 39 insertions(+), 25 deletions(-) create mode 100644 Model/ConflictException.php diff --git a/Model/ConflictException.php b/Model/ConflictException.php new file mode 100644 index 0000000..2e7d284 --- /dev/null +++ b/Model/ConflictException.php @@ -0,0 +1,7 @@ +modificationTimeRepo->getLastModificationTime($messageGroupId) - ) { - if (!$this->isUnmodifiedSince($lastModificationTime, $messageTimestamp)) { + $lastModification = $this->modificationTimeRepo->getLastModification($messageGroupId); + $lastModificationTime = $lastModification['timestamp'] ?? null; + $lastVersion = $lastModification['version'] ?? null; + + if ($messageTimestamp !== false) { + if ($lastModificationTime !== null && $messageTimestamp < $lastModificationTime) { $this->response->setStatusCode(412); return $this->response; } - $updateTimestamp = $messageTimestamp; + $currentTimestamp = $messageTimestamp; } else { - $lastModificationTime = null; - $updateTimestamp = \time(); + $currentTimestamp = \time(); } $connection = $this->resourceConnection->getConnection(); @@ -85,7 +86,8 @@ public function aroundDispatch( $this->modificationTimeRepo->updateModificationTime( $messageGroupId, - $updateTimestamp, + $currentTimestamp, + $lastVersion, $lastModificationTime ); $connection->commit(); @@ -94,6 +96,9 @@ public function aroundDispatch( $connection->rollBack(); throw $e; } + } catch (ConflictException $e) { + $this->response->setStatusCode(409); + return $this->response; } catch (\Exception $e) { $e = $this->errorProcessor->maskException($e); $this->response->setStatusCode($e->getHttpCode()); @@ -105,9 +110,4 @@ public function aroundDispatch( $this->lockService->releaseLock("idempotent_api.$messageGroupId"); } } - - private function isUnmodifiedSince(int $modificationTime, int $expectedTime) - { - return $modificationTime <= $expectedTime; - } } diff --git a/Model/ResourceModificationTimeRepository.php b/Model/ResourceModificationTimeRepository.php index ed06b62..9f5cc17 100644 --- a/Model/ResourceModificationTimeRepository.php +++ b/Model/ResourceModificationTimeRepository.php @@ -13,20 +13,21 @@ public function __construct(Context $dbContext, $connectionName = null) $this->dbConnection = $dbContext->getResources()->getConnection($connectionName); } - public function getLastModificationTime(string $identifier) + public function getLastModification(string $identifier) { $identifier = md5($identifier); $select = $this->dbConnection->select() - ->from(['t' => $this->dbConnection->getTableName('webapi_resource_modification_log')], 'timestamp') + ->from(['t' => $this->dbConnection->getTableName('webapi_resource_modification_log')], ['timestamp', 'version']) ->where('t.identifier = ?', $identifier); - $result = $this->dbConnection->fetchOne($select); + $result = $this->dbConnection->fetchAssoc($select); - return $result ? (int) $result : null; + return array_shift($result); } public function updateModificationTime( string $identifier, string $modificationTime, + int $expectedVersion = null, string $expectedModificationTime = null ) { $identifier = md5($identifier); @@ -35,20 +36,20 @@ public function updateModificationTime( $rowsAffected = $this->dbConnection->update( $this->dbConnection->getTableName('webapi_resource_modification_log'), [ - 'timestamp' => $modificationTime + 'timestamp' => $modificationTime, + 'version' => $expectedVersion + 1 ], - ['identifier = ?' => $identifier, 'timestamp = ?' => $expectedModificationTime] + ['identifier = ?' => $identifier, 'timestamp = ?' => $expectedModificationTime, 'version = ?' => $expectedVersion] ); } else { - //new resource that has never been modified $rowsAffected = $this->dbConnection->insert( $this->dbConnection->getTableName('webapi_resource_modification_log'), - ['identifier' => $identifier, 'timestamp' => $modificationTime] + ['identifier' => $identifier, 'timestamp' => $modificationTime, 'version' => 1] ); } - if ($rowsAffected === 0 && ($modificationTime !== $expectedModificationTime)) { - throw new \RuntimeException('There has been a conflict updating the web api resource'); + if ($rowsAffected === 0) { + throw new ConflictException; } } } diff --git a/Setup/InstallSchema.php b/Setup/InstallSchema.php index 31b2286..c6d4b39 100644 --- a/Setup/InstallSchema.php +++ b/Setup/InstallSchema.php @@ -31,10 +31,16 @@ public function install(SchemaSetupInterface $setup, ModuleContextInterface $con 'Resource Identifier' )->addColumn( 'timestamp', - Table::TYPE_DECIMAL, - null, + Table::TYPE_TEXT, + 255, ['unsigned' => true, 'nullable' => false], 'Timestamp' + )->addColumn( + 'version', + Table::TYPE_INTEGER, + 255, + ['unsigned' => true, 'nullable' => false, 'default' => 1], + 'Version' ); $installer->getConnection()->createTable($table); $installer->endSetup(); From 84a2abb36f44bb04b72afe759a372a26660ec4b0 Mon Sep 17 00:00:00 2001 From: Meng Tian Date: Thu, 19 Jan 2017 16:41:28 +0000 Subject: [PATCH 2/3] Change to invoke the plugin only when both group id and timestamp are provided. --- Model/DispatchPlugin.php | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/Model/DispatchPlugin.php b/Model/DispatchPlugin.php index 648b197..e8f96bf 100644 --- a/Model/DispatchPlugin.php +++ b/Model/DispatchPlugin.php @@ -47,7 +47,7 @@ public function aroundDispatch( $messageGroupId = $this->request->getHeader('X-Message-Group-ID', $default = false); $messageTimestamp = $this->request->getHeader('X-Message-Timestamp', $default = false); - if ($messageGroupId === false) { + if ($messageGroupId === false || $messageTimestamp === false) { return $proceed($request); } @@ -61,14 +61,9 @@ public function aroundDispatch( $lastModificationTime = $lastModification['timestamp'] ?? null; $lastVersion = $lastModification['version'] ?? null; - if ($messageTimestamp !== false) { - if ($lastModificationTime !== null && $messageTimestamp < $lastModificationTime) { - $this->response->setStatusCode(412); - return $this->response; - } - $currentTimestamp = $messageTimestamp; - } else { - $currentTimestamp = \time(); + if ($lastModificationTime !== null && $messageTimestamp < $lastModificationTime) { + $this->response->setStatusCode(412); + return $this->response; } $connection = $this->resourceConnection->getConnection(); @@ -86,7 +81,7 @@ public function aroundDispatch( $this->modificationTimeRepo->updateModificationTime( $messageGroupId, - $currentTimestamp, + $messageTimestamp, $lastVersion, $lastModificationTime ); From 95f3dd274c29a041a6eb83ea428db75e6a021595 Mon Sep 17 00:00:00 2001 From: Meng Tian Date: Thu, 19 Jan 2017 16:51:09 +0000 Subject: [PATCH 3/3] Change resource modification to webapi message group. --- Model/DispatchPlugin.php | 18 +++++++-------- ...y.php => WebApiMessageGroupRepository.php} | 22 +++++++++---------- Setup/InstallSchema.php | 8 +++---- Test/Unit/Model/DispatchPluginTest.php | 6 ++--- 4 files changed, 27 insertions(+), 27 deletions(-) rename Model/{ResourceModificationTimeRepository.php => WebApiMessageGroupRepository.php} (61%) diff --git a/Model/DispatchPlugin.php b/Model/DispatchPlugin.php index e8f96bf..5533b50 100644 --- a/Model/DispatchPlugin.php +++ b/Model/DispatchPlugin.php @@ -16,7 +16,7 @@ class DispatchPlugin private $response; private $lockService; private $resourceConnection; - private $modificationTimeRepo; + private $messageGroupRepository; private $errorProcessor; public function __construct( @@ -24,14 +24,14 @@ public function __construct( Response $response, LockService $lockService, ResourceConnection $resourceConnection, - ResourceModificationTimeRepository $resourceTimestampRepository, + WebApiMessageGroupRepository $messageGroupRepository, ErrorProcessor $errorProcessor ) { $this->request = $request; $this->response = $response; $this->lockService = $lockService; $this->resourceConnection = $resourceConnection; - $this->modificationTimeRepo = $resourceTimestampRepository; + $this->messageGroupRepository = $messageGroupRepository; $this->errorProcessor = $errorProcessor; } @@ -57,11 +57,11 @@ public function aroundDispatch( } try { - $lastModification = $this->modificationTimeRepo->getLastModification($messageGroupId); - $lastModificationTime = $lastModification['timestamp'] ?? null; - $lastVersion = $lastModification['version'] ?? null; + $messageGroup = $this->messageGroupRepository->getMessageGroup($messageGroupId); + $lastTimestamp = $messageGroup['timestamp'] ?? null; + $lastVersion = $messageGroup['version'] ?? null; - if ($lastModificationTime !== null && $messageTimestamp < $lastModificationTime) { + if ($lastTimestamp !== null && $messageTimestamp < $lastTimestamp) { $this->response->setStatusCode(412); return $this->response; } @@ -79,11 +79,11 @@ public function aroundDispatch( return $response; } - $this->modificationTimeRepo->updateModificationTime( + $this->messageGroupRepository->updateModificationTime( $messageGroupId, $messageTimestamp, $lastVersion, - $lastModificationTime + $lastTimestamp ); $connection->commit(); return $response; diff --git a/Model/ResourceModificationTimeRepository.php b/Model/WebApiMessageGroupRepository.php similarity index 61% rename from Model/ResourceModificationTimeRepository.php rename to Model/WebApiMessageGroupRepository.php index 9f5cc17..9f553d2 100644 --- a/Model/ResourceModificationTimeRepository.php +++ b/Model/WebApiMessageGroupRepository.php @@ -4,7 +4,7 @@ use Magento\Framework\Model\ResourceModel\Db\Context; -class ResourceModificationTimeRepository +class WebApiMessageGroupRepository { private $dbConnection; @@ -13,38 +13,38 @@ public function __construct(Context $dbContext, $connectionName = null) $this->dbConnection = $dbContext->getResources()->getConnection($connectionName); } - public function getLastModification(string $identifier) + public function getMessageGroup(string $id) { - $identifier = md5($identifier); + $id = md5($id); $select = $this->dbConnection->select() - ->from(['t' => $this->dbConnection->getTableName('webapi_resource_modification_log')], ['timestamp', 'version']) - ->where('t.identifier = ?', $identifier); + ->from(['t' => $this->dbConnection->getTableName('webapi_message_group')], ['timestamp', 'version']) + ->where('t.id = ?', $id); $result = $this->dbConnection->fetchAssoc($select); return array_shift($result); } public function updateModificationTime( - string $identifier, + string $id, string $modificationTime, int $expectedVersion = null, string $expectedModificationTime = null ) { - $identifier = md5($identifier); + $id = md5($id); if ($expectedModificationTime !== null) { $rowsAffected = $this->dbConnection->update( - $this->dbConnection->getTableName('webapi_resource_modification_log'), + $this->dbConnection->getTableName('webapi_message_group'), [ 'timestamp' => $modificationTime, 'version' => $expectedVersion + 1 ], - ['identifier = ?' => $identifier, 'timestamp = ?' => $expectedModificationTime, 'version = ?' => $expectedVersion] + ['id = ?' => $id, 'timestamp = ?' => $expectedModificationTime, 'version = ?' => $expectedVersion] ); } else { $rowsAffected = $this->dbConnection->insert( - $this->dbConnection->getTableName('webapi_resource_modification_log'), - ['identifier' => $identifier, 'timestamp' => $modificationTime, 'version' => 1] + $this->dbConnection->getTableName('webapi_message_group'), + ['id' => $id, 'timestamp' => $modificationTime, 'version' => 1] ); } diff --git a/Setup/InstallSchema.php b/Setup/InstallSchema.php index c6d4b39..76b038b 100644 --- a/Setup/InstallSchema.php +++ b/Setup/InstallSchema.php @@ -22,19 +22,19 @@ public function install(SchemaSetupInterface $setup, ModuleContextInterface $con $installer = $setup; $installer->startSetup(); $table = $installer->getConnection()->newTable( - $installer->getTable('webapi_resource_modification_log') + $installer->getTable('webapi_message_group') )->addColumn( - 'identifier', + 'id', Table::TYPE_TEXT, 255, ['primary' => true, 'nullable' => false], - 'Resource Identifier' + 'Message Group ID' )->addColumn( 'timestamp', Table::TYPE_TEXT, 255, ['unsigned' => true, 'nullable' => false], - 'Timestamp' + 'Message Timestamp' )->addColumn( 'version', Table::TYPE_INTEGER, diff --git a/Test/Unit/Model/DispatchPluginTest.php b/Test/Unit/Model/DispatchPluginTest.php index 4637abf..69a6221 100644 --- a/Test/Unit/Model/DispatchPluginTest.php +++ b/Test/Unit/Model/DispatchPluginTest.php @@ -11,7 +11,7 @@ use PHPUnit_Framework_MockObject_MockObject; use PHPUnit_Framework_TestCase; use SnowIO\IdempotentAPI\Model\DispatchPlugin; -use SnowIO\IdempotentAPI\Model\ResourceModificationTimeRepository; +use SnowIO\IdempotentAPI\Model\WebApiMessageGroupRepository; use SnowIO\Lock\Api\LockService; use Magento\Framework\Webapi\Response; use \Magento\Framework\App\RequestInterface; @@ -20,7 +20,7 @@ class DispatchPluginTest extends PHPUnit_Framework_TestCase { - /** @var ResourceModificationTimeRepository | PHPUnit_Framework_MockObject_MockObject */ + /** @var WebApiMessageGroupRepository | PHPUnit_Framework_MockObject_MockObject */ private $mockResourceTimestampRespository; /** @var LockService | PHPUnit_Framework_MockObject_MockObject */ @@ -56,7 +56,7 @@ public function __construct($name = null, array $data = array(), $dataName = '') public function setUp() { - $this->mockResourceTimestampRespository = $this->getMockBuilder(ResourceModificationTimeRepository::class) + $this->mockResourceTimestampRespository = $this->getMockBuilder(WebApiMessageGroupRepository::class) ->disableOriginalConstructor()->setMethods([ 'getLastModificationTime', 'updateModificationTime'