Merge pull request #7112 from piwik/6436_action_dupes

Refactor new action insertion code so duplicate actions will not exist + provide means to fix duplicates

core/DataAccess/Actions.php

@@ -0,0 +1,34 @@
+<?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\DataAccess;
+
+use Piwik\Db;
+use Piwik\Common;
+
+/**
+ * Data Access Object for operations dealing with the log_action table.
+ */
+class Actions
+{
+    /**
+     * Removes a list of actions from the log_action table by ID.
+     *
+     * @param int[] $idActions
+     */
+    public function delete($idActions)
+    {
+        foreach ($idActions as &$id) {
+            $id = (int)$id;
+        }
+
+        $table = Common::prefixTable('log_action');
+
+        $sql = "DELETE FROM $table WHERE idaction IN (" . implode(",", $idActions) . ")";
+        Db::query($sql);
+    }
+}

core/DataAccess/TableMetadata.php

@@ -0,0 +1,56 @@
+<?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\DataAccess;
+
+use Piwik\Db;
+
+/**
+ * Data Access Object that can be used to get metadata information about
+ * the MySQL tables Piwik uses.
+ */
+class TableMetadata
+{
+    /**
+     * Returns the list of column names for a table.
+     *
+     * @param string $table Prefixed table name.
+     * @return string[] List of column names..
+     */
+    public function getColumns($table)
+    {
+        $table = str_replace("`", "", $table);
+
+        $columns = Db::fetchAll("SHOW COLUMNS FROM `" . $table . "`");
+
+        $columnNames = array();
+        foreach ($columns as $column) {
+            $columnNames[] = $column['Field'];
+        }
+
+        return $columnNames;
+    }
+
+    /**
+     * Returns the list of idaction columns in a table. A column is
+     * assumed to be an idaction reference if it has `"idaction"` in its
+     * name (eg, `"idaction_url"` or `"idaction_content_name"`.
+     *
+     * @param string $table Prefixed table name.
+     * @return string[]
+     */
+    public function getIdActionColumnNames($table)
+    {
+        $columns = $this->getColumns($table);
+
+        $columns = array_filter($columns, function ($columnName) {
+            return strpos($columnName, 'idaction') !== false;
+        });
+
+        return array_values($columns);
+    }
+}

core/Db.php

@@ -9,6 +9,7 @@
 namespace Piwik;
 
 use Exception;
+use Piwik\DataAccess\TableMetadata;
 use Piwik\Db\Adapter;
 use Piwik\Tracker;
 
@@ -387,17 +388,12 @@ public static function dropAllTables()
      *
      * @param string|array $table The name of the table you want to get the columns definition for.
      * @return \Zend_Db_Statement
+     * @deprecated since 2.11.0
      */
     public static function getColumnNamesFromTable($table)
     {
-        $columns = self::fetchAll("SHOW COLUMNS FROM `" . $table . "`");
-
-        $columnNames = array();
-        foreach ($columns as $column) {
-            $columnNames[] = $column['Field'];
-        }
-
-        return $columnNames;
+        $tableMetadataAccess = new TableMetadata();
+        return $tableMetadataAccess->getColumns($table);
     }
 
     /**

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 int $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, hash, 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, hash, 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 '=@':

core/Updates/2.11.0-b2.php

@@ -6,7 +6,6 @@
  * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
  *
  */
-
 namespace Piwik\Updates;
 
 use Piwik\Common;