Improve conflict handling. #3
Conversation
meng-tian
force-pushed
the
improvement
branch
from
acd5e47
to
01bd629
Compare
| } else { | ||
| $lastModificationTime = null; | ||
| $updateTimestamp = \time(); | ||
| $currentTimestamp = \time(); |
There was a problem hiding this comment.
I wonder what use the default timestamp is? This feature is pretty pointless if the client doesn't specify a timestamp. Perhaps we should only invoke the feature if both the message group ID and a timestamp are provided?
| @@ -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) | |||
There was a problem hiding this comment.
Given that we have X-Message-Group-Id, shouldn't this method be called getMessageGroup? And shouldn't the same noun be used elsewhere (e.g. in the table name)?
There was a problem hiding this comment.
I agree. getMessageGroup is better. I am going to make all related names consistent.
| { | ||
| $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); |
There was a problem hiding this comment.
As suggested elsewhere, should this table be called webapi_message_groups?
| ], | ||
| ['identifier = ?' => $identifier, 'timestamp = ?' => $expectedModificationTime] | ||
| ['identifier = ?' => $identifier, 'timestamp = ?' => $expectedModificationTime, 'version = ?' => $expectedVersion] |
meng-tian
force-pushed
the
improvement
branch
from
baefae9
to
95f3dd2
Compare
|
Hi @joshdifabio , 84a2abb and 95f3dd2 are changes I made based on your recommendations. Could you please review? |
|
Hey @joshdifabio, could you please let me know when this PR is merged. Thanks. |
No description provided.