Fixes #6436, fix concurrency issue regarding duplicate actions by alw…

…ays 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

@@ -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 = ? )";
+    }
 }

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 '=@':

tests/PHPUnit/System/DuplicateActionsTest.php

@@ -0,0 +1,75 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link    http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+namespace Piwik\Tests\System;
+
+use Piwik\Common;
+use Piwik\Db;
+use Piwik\Tests\Fixtures\OneVisitorTwoVisits;
+use Piwik\Tests\Framework\TestCase\SystemTestCase;
+
+/**
+ * The tracker inserts actions in separate SQL queries which can cause
+ * duplicate actions to be in the DB (actions w/ the same name, hash + type, but
+ * different idaction). The tracker will delete duplicate actions, but
+ * if for some reason the tracker fails before the DELETE occurs, there can be
+ * stray duplicate actions. This test is there to ensure reports are not affected
+ * by duplicate action entries.
+ *
+ * @group Core
+ * @group DuplicateActionsTest
+ */
+class DuplicateActionsTest extends SystemTestCase
+{
+    /**
+     * @var OneVisitorTwoVisits
+     */
+    public static $fixture = null; // initialized below class
+
+    public static function setUpBeforeClass()
+    {
+        parent::setUpBeforeClass();
+
+        // add duplicates for every action
+        $table = Common::prefixTable('log_action');
+        foreach (Db::fetchAll("SELECT * FROM $table") as $row) {
+            $insertSql = "INSERT INTO $table (name, type, hash, url_prefix)
+                               VALUES (?, ?, CRC32(?), ?)";
+
+            Db::query($insertSql, array($row['name'], $row['type'], $row['name'], $row['url_prefix']));
+        }
+    }
+
+    /**
+     * @dataProvider getApiForTesting
+     */
+    public function testApi($api, $params)
+    {
+        $this->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;

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