From e43ed30926102a009985a8a6b889b20c89179ce0 Mon Sep 17 00:00:00 2001 From: diosmosis Date: Tue, 27 Jan 2015 22:59:06 -0800 Subject: [PATCH] Fixes #6436, fix concurrency issue regarding duplicate actions by always using least idaction and deleting duplicates when they are found to be inserted. Since tracker process can potentially fail before duplicates are removed, added test to make sure reports work when duplicate actions exist in the DB. --- core/Tracker/Model.php | 58 ++++++++++++-- core/Tracker/TableLogAction.php | 5 +- tests/PHPUnit/System/DuplicateActionsTest.php | 75 +++++++++++++++++++ .../System/OneVisitorTwoVisitsTest.php | 2 +- 4 files changed, 133 insertions(+), 7 deletions(-) create mode 100644 tests/PHPUnit/System/DuplicateActionsTest.php diff --git a/core/Tracker/Model.php b/core/Tracker/Model.php index 090cd8be596..89ee45b27d4 100644 --- a/core/Tracker/Model.php +++ b/core/Tracker/Model.php @@ -9,10 +9,8 @@ namespace Piwik\Tracker; use Exception; -use PDOStatement; use Piwik\Common; use Piwik\Tracker; -use Piwik\Tracker\Db\DbException; class Model { @@ -142,7 +140,31 @@ public function createEcommerceItems($ecommerceItems) Common::printDebug($bind); } + /** + * Inserts a new action into the log_action table. If there is an existing action that was inserted + * due to another request pre-empting this one, the newly inserted action is deleted. + * + * @param string $name + * @param int $type + * @param string $urlPrefix + * @return int The ID of the action (can be for an existing action or new action). + */ public function createNewIdAction($name, $type, $urlPrefix) + { + $newActionId = $this->insertNewAction($name, $type, $urlPrefix); + + $realFirstActionId = $this->getIdActionMatchingNameAndType($name, $type); + + // if the inserted action ID is not the same as the queried action ID, then that means we inserted + // a duplicate, so remove it now + if ($realFirstActionId != $newActionId) { + $this->deleteDuplicateAction($newActionId); + } + + return $realFirstActionId; + } + + private function insertNewAction($name, $type, $urlPrefix) { $table = Common::prefixTable('log_action'); $sql = "INSERT INTO $table (name, hash, type, url_prefix) VALUES (?,CRC32(?),?,?)"; @@ -157,8 +179,11 @@ public function createNewIdAction($name, $type, $urlPrefix) private function getSqlSelectActionId() { + // it is possible for multiple actions to exist in the DB (due to rare concurrency issues), so the ORDER BY and + // LIMIT are important $sql = "SELECT idaction, type, name FROM " . Common::prefixTable('log_action') - . " WHERE ( hash = CRC32(?) AND name = ? AND type = ? ) "; + . " WHERE " . $this->getSqlConditionToMatchSingleAction() . " " + . "ORDER BY idaction ASC LIMIT 1"; return $sql; } @@ -173,9 +198,16 @@ public function getIdActionMatchingNameAndType($name, $type) return $idAction; } + /** + * Returns the IDs for multiple actions based on name + type values. + * + * @param array $actionsNameAndType Array like `array( array('name' => '...', 'type' => 1), ... )` + * @return array|false Array of DB rows w/ columns: **idaction**, **type**, **name**. + */ public function getIdsAction($actionsNameAndType) { - $sql = $this->getSqlSelectActionId(); + $sql = "SELECT MIN(idaction) as idaction, type, name FROM " . Common::prefixTable('log_action') + . " WHERE"; $bind = array(); $i = 0; @@ -187,15 +219,19 @@ public function getIdsAction($actionsNameAndType) } if ($i > 0) { - $sql .= " OR ( hash = CRC32(?) AND name = ? AND type = ? ) "; + $sql .= " OR"; } + $sql .= " " . $this->getSqlConditionToMatchSingleAction() . " "; + $bind[] = $name; $bind[] = $name; $bind[] = $actionNameType['type']; $i++; } + $sql .= " GROUP BY type, name"; + // Case URL & Title are empty if (empty($bind)) { return false; @@ -375,9 +411,21 @@ private function visitFieldsToQuery($valuesToUpdate) return array($updateParts, $sqlBind); } + private function deleteDuplicateAction($newActionId) + { + $sql = "DELETE FROM " . Common::prefixTable('log_action') . " WHERE idaction = ?"; + + $db = $this->getDb(); + $db->query($sql, array($newActionId)); + } + private function getDb() { return Tracker::getDatabase(); } + private function getSqlConditionToMatchSingleAction() + { + return "( hash = CRC32(?) AND name = ? AND type = ? )"; + } } diff --git a/core/Tracker/TableLogAction.php b/core/Tracker/TableLogAction.php index fe620035d72..346407150fd 100644 --- a/core/Tracker/TableLogAction.php +++ b/core/Tracker/TableLogAction.php @@ -61,7 +61,10 @@ private static function getSelectQueryWhereNameContains($matchType, $actionType) { // now, we handle the cases =@ (contains) and !@ (does not contain) // build the expression based on the match type - $sql = 'SELECT idaction FROM ' . Common::prefixTable('log_action') . ' WHERE %s AND type = ' . $actionType . ' )'; + $sql = 'SELECT MIN(idaction) AS idaction FROM ' . Common::prefixTable('log_action') . ' WHERE %s AND type = ' . $actionType . ' )' + . ' GROUP BY name, type'; // group by is to avoid possible case of duplicates in log_action table + // (duplicates can exist if php tracker fails right after inserting a duplicate in + // Tracker\Model::insertNewAction()) switch ($matchType) { case '=@': diff --git a/tests/PHPUnit/System/DuplicateActionsTest.php b/tests/PHPUnit/System/DuplicateActionsTest.php new file mode 100644 index 00000000000..a9e1c028031 --- /dev/null +++ b/tests/PHPUnit/System/DuplicateActionsTest.php @@ -0,0 +1,75 @@ +runApiTests($api, $params); + } + + public function getApiForTesting() + { + $idSite = self::$fixture->idSite; + $dateTime = self::$fixture->dateTime; + + $api = array('VisitsSummary', 'Actions', 'Contents', 'Events'); + return array( + array($api, array('idSite' => $idSite, + 'periods' => 'day', + 'date' => $dateTime, + 'compareAgainst' => 'OneVisitorTwoVisits', + 'otherRequestParameters' => array( + 'hideColumns' => 'nb_users', + ) + )) + ); + } +} + +DuplicateActionsTest::$fixture = new OneVisitorTwoVisits(); +DuplicateActionsTest::$fixture->excludeMozilla = true; \ No newline at end of file diff --git a/tests/PHPUnit/System/OneVisitorTwoVisitsTest.php b/tests/PHPUnit/System/OneVisitorTwoVisitsTest.php index 287e6799f14..8ca82598466 100755 --- a/tests/PHPUnit/System/OneVisitorTwoVisitsTest.php +++ b/tests/PHPUnit/System/OneVisitorTwoVisitsTest.php @@ -29,7 +29,7 @@ class OneVisitorTwoVisitsTest extends SystemTestCase { /** - * @var Test_Piwik_Fixture_OneVisitorTwoVisits + * @var OneVisitorTwoVisits */ public static $fixture = null; // initialized below class