More generic raw log access (#11775)

8 files changed, 134 insertions, 28 deletions

diff --git a/core/DataAccess/RawLogDao.php b/core/DataAccess/RawLogDao.php
index c933d17f63..27bb33563b 100644
--- a/core/DataAccess/RawLogDao.php
+++ b/core/DataAccess/RawLogDao.php
@@ -12,6 +12,7 @@ use Piwik\Common;
 use Piwik\Container\StaticContainer;
 use Piwik\Db;
 use Piwik\Plugin\Dimension\DimensionMetadataProvider;
+use Piwik\Plugin\LogTablesProvider;
 
 /**
  * DAO that queries log tables.
@@ -25,9 +26,15 @@ class RawLogDao
      */
     private $dimensionMetadataProvider;
 
-    public function __construct(DimensionMetadataProvider $provider = null)
+    /**
+     * @var LogTablesProvider
+     */
+    private $logTablesProvider;
+
+    public function __construct(DimensionMetadataProvider $provider = null, LogTablesProvider $logTablesProvider = null)
     {
         $this->dimensionMetadataProvider = $provider ?: StaticContainer::get('Piwik\Plugin\Dimension\DimensionMetadataProvider');
+        $this->logTablesProvider = $logTablesProvider ?: StaticContainer::get('Piwik\Plugin\LogTablesProvider');
     }
 
     /**
@@ -226,22 +233,15 @@ class RawLogDao
         return Db::query($sql, array_merge(array_values($values), array($idVisit)));
     }
 
-    private function getIdFieldForLogTable($logTable)
+    protected function getIdFieldForLogTable($logTable)
     {
-        switch ($logTable) {
-            case 'log_visit':
-                return 'idvisit';
-            case 'log_link_visit_action':
-                return 'idlink_va';
-            case 'log_conversion':
-                return 'idvisit';
-            case 'log_conversion_item':
-                return 'idvisit';
-            case 'log_action':
-                return 'idaction';
-            default:
-                throw new \InvalidArgumentException("Unknown log table '$logTable'.");
+        $idColumns = $this->getTableIdColumns();
+
+        if (isset($idColumns[$logTable])) {
+            return $idColumns[$logTable];
         }
+
+        throw new \InvalidArgumentException("Unknown log table '$logTable'.");
     }
 
     // TODO: instead of creating a log query like this, we should re-use segments. to do this, however, there must be a 1-1
@@ -291,11 +291,10 @@ class RawLogDao
         return $sql;
     }
 
-
-    private function getMaxIdsInLogTables()
+    protected function getMaxIdsInLogTables()
     {
-        $tables = array('log_conversion', 'log_link_visit_action', 'log_visit', 'log_conversion_item');
         $idColumns = $this->getTableIdColumns();
+        $tables = array_keys($idColumns);
 
         $result = array();
         foreach ($tables as $table) {
@@ -349,8 +348,17 @@ class RawLogDao
 
     private function lockLogTables()
     {
+        $tables = $this->getTableIdColumns();
+        unset($tables['log_action']); // we write lock it
+        $tableNames = array_keys($tables);
+
+        $readLocks = array();
+        foreach ($tableNames as $tableName) {
+            $readLocks[] = Common::prefixTable($tableName);
+        }
+
         Db::lockTables(
-            $readLocks = Common::prefixTables('log_conversion', 'log_link_visit_action', 'log_visit', 'log_conversion_item'),
+            $readLocks,
             $writeLocks = Common::prefixTables('log_action')
         );
     }
@@ -367,13 +375,18 @@ class RawLogDao
         Db::query($deleteSql);
     }
 
-    private function getTableIdColumns()
+    protected function getTableIdColumns()
     {
-        return array(
-            'log_link_visit_action' => 'idlink_va',
-            'log_conversion'        => 'idvisit',
-            'log_visit'             => 'idvisit',
-            'log_conversion_item'   => 'idvisit'
-        );
+        $columns = array();
+
+        foreach ($this->logTablesProvider->getAllLogTables() as $logTable) {
+            $idColumn = $logTable->getIdColumn();
+
+            if (!empty($idColumn)) {
+                $columns[$logTable->getName()] = $idColumn;
+            }
+        }
+
+        return $columns;
     }
 }
diff --git a/core/Tracker/LogTable.php b/core/Tracker/LogTable.php
index 8e66f43463..3fedba6945 100644
--- a/core/Tracker/LogTable.php
+++ b/core/Tracker/LogTable.php
@@ -21,6 +21,16 @@ abstract class LogTable {
     abstract public function getName();
 
     /**
+     * Get the name of the column that represents the primary key. For example "idvisit" or "idlink_va". If the table
+     * does not have a unique ID for each row, you may choose a column that comes closest to it, for example "idvisit".
+     * @return string
+     */
+    public function getIdColumn()
+    {
+        return '';
+    }
+
+    /**
      * Get the name of the column that can be used to join a visit with another table. This is the name of the column
      * that represents the "idvisit".
      * @return string
diff --git a/plugins/CoreHome/Tracker/LogTable/Action.php b/plugins/CoreHome/Tracker/LogTable/Action.php
index fdb69e5ec6..66f1d1fd81 100644
--- a/plugins/CoreHome/Tracker/LogTable/Action.php
+++ b/plugins/CoreHome/Tracker/LogTable/Action.php
@@ -17,6 +17,11 @@ class Action extends LogTable
         return 'log_action';
     }
 
+    public function getIdColumn()
+    {
+        return 'idaction';
+    }
+
     public function getColumnToJoinOnIdAction()
     {
         return 'idaction';
diff --git a/plugins/CoreHome/Tracker/LogTable/Conversion.php b/plugins/CoreHome/Tracker/LogTable/Conversion.php
index e920864088..b579b32d81 100644
--- a/plugins/CoreHome/Tracker/LogTable/Conversion.php
+++ b/plugins/CoreHome/Tracker/LogTable/Conversion.php
@@ -17,6 +17,11 @@ class Conversion extends LogTable
         return 'log_conversion';
     }
 
+    public function getIdColumn()
+    {
+        return 'idvisit';
+    }
+
     public function getColumnToJoinOnIdVisit()
     {
         return 'idvisit';
diff --git a/plugins/CoreHome/Tracker/LogTable/ConversionItem.php b/plugins/CoreHome/Tracker/LogTable/ConversionItem.php
index 566841b4fa..cf24614458 100644
--- a/plugins/CoreHome/Tracker/LogTable/ConversionItem.php
+++ b/plugins/CoreHome/Tracker/LogTable/ConversionItem.php
@@ -17,6 +17,11 @@ class ConversionItem extends LogTable
         return 'log_conversion_item';
     }
 
+    public function getIdColumn()
+    {
+        return 'idvisit';
+    }
+
     public function getColumnToJoinOnIdVisit()
     {
         return 'idvisit';
diff --git a/plugins/CoreHome/Tracker/LogTable/LinkVisitAction.php b/plugins/CoreHome/Tracker/LogTable/LinkVisitAction.php
index cb102d407c..8dabf66a66 100644
--- a/plugins/CoreHome/Tracker/LogTable/LinkVisitAction.php
+++ b/plugins/CoreHome/Tracker/LogTable/LinkVisitAction.php
@@ -17,6 +17,11 @@ class LinkVisitAction extends LogTable
         return 'log_link_visit_action';
     }
 
+    public function getIdColumn()
+    {
+        return 'idlink_va';
+    }
+
     public function getColumnToJoinOnIdAction()
     {
         return 'idaction_url';
diff --git a/plugins/CoreHome/Tracker/LogTable/Visit.php b/plugins/CoreHome/Tracker/LogTable/Visit.php
index f403c37697..a72fb24d11 100644
--- a/plugins/CoreHome/Tracker/LogTable/Visit.php
+++ b/plugins/CoreHome/Tracker/LogTable/Visit.php
@@ -17,6 +17,11 @@ class Visit extends LogTable
         return 'log_visit';
     }
 
+    public function getIdColumn()
+    {
+        return 'idvisit';
+    }
+
     public function getColumnToJoinOnIdVisit()
     {
         return 'idvisit';
diff --git a/tests/PHPUnit/System/RawLogDaoTest.php b/tests/PHPUnit/System/RawLogDaoTest.php
index e0210467e2..7046c53a34 100644
--- a/tests/PHPUnit/System/RawLogDaoTest.php
+++ b/tests/PHPUnit/System/RawLogDaoTest.php
@@ -12,6 +12,24 @@ use Piwik\DataAccess\RawLogDao;
 use Piwik\Tests\Framework\Fixture;
 use Piwik\Tests\Framework\TestCase\SystemTestCase;
 
+class CustomRawLogDao extends RawLogDao {
+
+    public function getTableIdColumns()
+    {
+        return parent::getTableIdColumns();
+    }
+
+    public function getIdFieldForLogTable($logTable)
+    {
+        return parent::getIdFieldForLogTable($logTable);
+    }
+
+    public function getMaxIdsInLogTables()
+    {
+        return parent::getMaxIdsInLogTables();
+    }
+}
+
 /**
  * @group Core
  * @group RawLogDao
@@ -20,7 +38,7 @@ use Piwik\Tests\Framework\TestCase\SystemTestCase;
 class RawLogDaoTest extends SystemTestCase
 {
     /**
-     * @var RawLogDao
+     * @var CustomRawLogDao
      */
     private $dao;
 
@@ -34,7 +52,7 @@ class RawLogDaoTest extends SystemTestCase
             Fixture::createWebsite('2010-00-00 00:00:00');
         }
 
-        $this->dao = new RawLogDao();
+        $this->dao = new CustomRawLogDao();
     }
 
     /**
@@ -48,6 +66,46 @@ class RawLogDaoTest extends SystemTestCase
         $this->assertSame($expectedHasVisits, $hasVisits);
     }
 
+    public function test_getIdColumns()
+    {
+        $expected = array(
+            'log_action' => 'idaction',
+            'log_conversion_item' => 'idvisit',
+            'log_conversion' => 'idvisit',
+            'log_link_visit_action' => 'idlink_va',
+            'log_visit' => 'idvisit',
+        );
+        $this->assertSame($expected, $this->dao->getTableIdColumns());
+    }
+
+    public function test_getIdFieldForLogTable()
+    {
+        $this->assertSame('idaction', $this->dao->getIdFieldForLogTable('log_action'));
+        $this->assertSame('idlink_va', $this->dao->getIdFieldForLogTable('log_link_visit_action'));
+        $this->assertSame('idvisit', $this->dao->getIdFieldForLogTable('log_visit'));
+    }
+
+    /**
+     * @expectedException \Exception
+     * @expectedExceptionMessage Unknown log table 'log_foobarbaz'
+     */
+    public function test_getIdFieldForLogTable_whenUnknownTable()
+    {
+        $this->dao->getIdFieldForLogTable('log_foobarbaz');
+    }
+
+    public function test_getMaxIdsInLogTables()
+    {
+        $expected = array(
+            'log_action' => '2',
+            'log_conversion_item' => null,
+            'log_conversion' => null,
+            'log_link_visit_action' => '11',
+            'log_visit' => '1',
+        );
+        $this->assertSame($expected, $this->dao->getMaxIdsInLogTables());
+    }
+
     public function getVisitsInTimeFrameData()
     {
         return array(