From a3f8358c43cc753104feab605e956297d91145e8 Mon Sep 17 00:00:00 2001 From: diosmosis <benaka@piwik.pro> Date: Sat, 7 Feb 2015 18:39:10 -0800 Subject: [PATCH] Refactoring entire branch, moved most of logic to command class, turned DuplicateActionRemover into a DAO that only deals w/ duplicate actions, created two new DAO classes (one for log_action manipulation and one for getting relational table metadata), deprecated SHOW COLUMNS method in Db in favor of new table metadata DAO, and added & fixed tests. --- core/DataAccess/Actions.php | 34 +++ core/DataAccess/TableMetadata.php | 48 +++ core/Db.php | 12 +- .../Commands/FixDuplicateLogActions.php | 183 +++++++++++- .../Model/DuplicateActionRemover.php | 193 ++++++++++++ .../Utility/DuplicateActionRemover.php | 280 ------------------ .../DuplicateActions.php} | 142 +-------- .../Integration/FixDuplicateActionsTest.php | 175 +++++++++++ .../Model/DuplicateActionRemoverTest.php | 112 +++++++ .../Integration/DataAccess/ActionsTest.php | 64 ++++ .../DataAccess/TableMetadataTest.php | 55 ++++ 11 files changed, 858 insertions(+), 440 deletions(-) create mode 100644 core/DataAccess/Actions.php create mode 100644 core/DataAccess/TableMetadata.php create mode 100644 plugins/CoreAdminHome/Model/DuplicateActionRemover.php delete mode 100644 plugins/CoreAdminHome/Utility/DuplicateActionRemover.php rename plugins/CoreAdminHome/tests/{Integration/DuplicateActionRemoverTest.php => Fixture/DuplicateActions.php} (51%) create mode 100644 plugins/CoreAdminHome/tests/Integration/FixDuplicateActionsTest.php create mode 100644 plugins/CoreAdminHome/tests/Integration/Model/DuplicateActionRemoverTest.php create mode 100644 tests/PHPUnit/Integration/DataAccess/ActionsTest.php create mode 100644 tests/PHPUnit/Integration/DataAccess/TableMetadataTest.php diff --git a/core/DataAccess/Actions.php b/core/DataAccess/Actions.php new file mode 100644 index 0000000000..9efc4fc2a5 --- /dev/null +++ b/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); + } +} \ No newline at end of file diff --git a/core/DataAccess/TableMetadata.php b/core/DataAccess/TableMetadata.php new file mode 100644 index 0000000000..5f59367580 --- /dev/null +++ b/core/DataAccess/TableMetadata.php @@ -0,0 +1,48 @@ +<?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\Common; +use Piwik\Db; + +/** + * TODO + */ +class TableMetadata +{ + /** + * TODO + */ + 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; + } + + /** + * TODO + */ + public function getIdActionColumnNames($table) + { + $columns = $this->getColumns($table); + + $columns = array_filter($columns, function ($columnName) { + return strpos($columnName, 'idaction') !== false; + }); + + return array_values($columns); + } +} \ No newline at end of file diff --git a/core/Db.php b/core/Db.php index 67618c8f58..a96786e1e3 100644 --- a/core/Db.php +++ b/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 @@ class Db * * @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); } /** diff --git a/plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php b/plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php index c378172769..54b53afabe 100644 --- a/plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php +++ b/plugins/CoreAdminHome/Commands/FixDuplicateLogActions.php @@ -9,20 +9,93 @@ namespace Piwik\Plugins\CoreAdminHome\Commands; use Piwik\Common; +use Piwik\Container\StaticContainer; +use Piwik\DataAccess\Actions; use Piwik\DataAccess\ArchiveInvalidator; use Piwik\Plugin\ConsoleCommand; -use Piwik\Plugins\CoreAdminHome\Utility\DuplicateActionRemover; +use Piwik\Plugins\CoreAdminHome\Model\DuplicateActionRemover; use Piwik\Timer; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; /** - * Removes duplicate log_action rows and fixes references to these duplicate rows - * in + * Finds duplicate actions rows in log_action and removes them. Fixes references to duplicate + * actions in the log_link_visit_action table, log_conversion table, and log_conversion_item + * table. + * + * Prior to version 2.11, there was a race condition in the tracker where it was possible for + * two or more actions with the same name and type to be inserted simultaneously. This resulted + * in inaccurate data. A Piwik database with this problem can be fixed using this class. + * + * With version 2.11 and above, it is still possible for duplicate actions to be inserted, but + * ONLY if the tracker's PHP process fails suddenly right after inserting an action. This is + * very rare, and even if it does happen, report data will not be affected, but the extra + * actions can be deleted w/ this class. */ class FixDuplicateLogActions extends ConsoleCommand { + /** + * Used to invalidate archives. Only used if $shouldInvalidateArchives is true. + * + * @var ArchiveInvalidator + */ + private $archiveInvalidator; + + /** + * DAO used to find duplicate actions in log_action and fix references to them in other tables. + * + * @var DuplicateActionRemover + */ + private $duplicateActionRemover; + + /** + * DAO used to remove actions from the log_action table. + * + * @var Actions + */ + private $actionsAccess; + + /** + * @var LoggerInterface + */ + private $logger; + + /** + * Constructor. + * + * @param ArchiveInvalidator $invalidator + * @param DuplicateActionRemover $duplicateActionRemover + * @param Actions $actionsAccess + * @param LoggerInterface $logger + */ + public function __construct(ArchiveInvalidator $invalidator = null, DuplicateActionRemover $duplicateActionRemover = null, + Actions $actionsAccess = null, LoggerInterface $logger = null) + { + parent::__construct(); + + if ($invalidator === null) { + $invalidator = new ArchiveInvalidator(); + } + $this->archiveInvalidator = $invalidator; + + if ($duplicateActionRemover === null) { + $duplicateActionRemover = new DuplicateActionRemover(); + } + $this->duplicateActionRemover = $duplicateActionRemover; + + if ($actionsAccess === null) { + $actionsAccess = new Actions(); + } + $this->actionsAccess = $actionsAccess; + + if ($logger === null) { + $logger = StaticContainer::get('Psr\Log\LoggerInterface'); + } + $this->logger = $logger; + } + protected function configure() { $this->setName('core:fix-duplicate-log-actions'); @@ -38,23 +111,105 @@ class FixDuplicateLogActions extends ConsoleCommand $timer = new Timer(); - $archiveInvalidator = $invalidateArchives ? new ArchiveInvalidator() : null; - $resolver = new DuplicateActionRemover($archiveInvalidator); + $duplicateActions = $this->duplicateActionRemover->getDuplicateIdActions(); + if (empty($duplicateActions)) { + $output->writeln("Found no duplicate actions."); + return; + } - list($numberRemoved, $archivesAffected) = $resolver->removeDuplicateActionsFromDb(); + $output->writeln("<info>Found " . count($duplicateActions) . " actions with duplicates.</info>"); - if (!$invalidateArchives) { - $output->writeln("The following archives used duplicate actions and should be invalidated if you want correct reports:"); - foreach ($archivesAffected as $archiveInfo) { - $output->writeln("\t[ idSite = {$archiveInfo['idsite']}, date = {$archiveInfo['server_time']} ]"); - } + list($numberRemoved, $allArchivesAffected) = $this->fixDuplicateActionReferences($duplicateActions, $output); + + $this->deleteDuplicatesFromLogAction($output, $duplicateActions); + + if ($invalidateArchives) { + $this->invalidateArchivesUsingActionDuplicates($allArchivesAffected, $output); + } else { + $this->printAffectedArchives($allArchivesAffected, $output); } - $table = Common::prefixTable('log_action'); + $logActionTable = Common::prefixTable('log_action'); $this->writeSuccessMessage($output, array( - "Found and deleted $numberRemoved duplicate action entries in the $table table.", + "Found and deleted $numberRemoved duplicate action entries in the $logActionTable table.", "References in log_link_visit_action, log_conversion and log_conversion_item were corrected.", $timer->__toString() )); } -} + + private function invalidateArchivesUsingActionDuplicates($archivesAffected, OutputInterface $output) + { + $output->write("Invalidating archives affected by duplicates fixed..."); + foreach ($archivesAffected as $archiveInfo) { + $this->archiveInvalidator->markArchivesAsInvalidated( + array($archiveInfo['idsite']), $archiveInfo['server_time'], $period = false); + } + $output->writeln("Done."); + } + + private function printAffectedArchives($allArchivesAffected, OutputInterface $output) + { + $output->writeln("The following archives used duplicate actions and should be invalidated if you want correct reports:"); + foreach ($allArchivesAffected as $archiveInfo) { + $output->writeln("\t[ idSite = {$archiveInfo['idsite']}, date = {$archiveInfo['server_time']} ]"); + } + } + + private function fixDuplicateActionReferences($duplicateActions, OutputInterface $output) + { + $dupeCount = count($duplicateActions); + + $numberRemoved = 0; + $allArchivesAffected = array(); + + foreach ($duplicateActions as $index => $dupeInfo) { + $name = $dupeInfo['name']; + $toIdAction = $dupeInfo['idaction']; + $fromIdActions = $dupeInfo['duplicateIdActions']; + + $numberRemoved += count($fromIdActions); + + $output->writeln("<info>[$index / $dupeCount]</info> Fixing duplicates for '$name'"); + + $this->logger->debug(" idaction = {idaction}, duplicate idactions = {duplicateIdActions}", array( + 'idaction' => $toIdAction, + 'duplicateIdActions' => $fromIdActions + )); + + foreach (DuplicateActionRemover::$tablesWithIdActionColumns as $table) { + $archivesAffected = $this->fixDuplicateActionsInTable($output, $table, $toIdAction, $fromIdActions); + $allArchivesAffected = array_merge($allArchivesAffected, $archivesAffected); + } + } + + $allArchivesAffected = array_values(array_unique($allArchivesAffected, SORT_REGULAR)); + + return array($numberRemoved, $allArchivesAffected); + } + + private function fixDuplicateActionsInTable(OutputInterface $output, $table, $toIdAction, $fromIdActions) + { + $timer = new Timer(); + + $archivesAffected = $this->duplicateActionRemover->getSitesAndDatesOfRowsUsingDuplicates($table, $fromIdActions); + + $this->duplicateActionRemover->fixDuplicateActionsInTable($table, $toIdAction, $fromIdActions); + + $output->writeln("\tFixed duplicates in " . Common::prefixTable($table) . ". <comment>" . $timer->__toString() . "</comment>."); + + return $archivesAffected; + } + + private function deleteDuplicatesFromLogAction(OutputInterface $output, $duplicateActions) + { + $logActionTable = Common::prefixTable('log_action'); + $output->writeln("<info>Deleting duplicate actions from $logActionTable...</info>"); + + $idActions = array(); + foreach ($duplicateActions as $dupeInfo) { + $idActions = array_merge($idActions, $dupeInfo['duplicateIdActions']); + } + + $this->actionsAccess->delete($idActions); + } +} \ No newline at end of file diff --git a/plugins/CoreAdminHome/Model/DuplicateActionRemover.php b/plugins/CoreAdminHome/Model/DuplicateActionRemover.php new file mode 100644 index 0000000000..51a93bab47 --- /dev/null +++ b/plugins/CoreAdminHome/Model/DuplicateActionRemover.php @@ -0,0 +1,193 @@ +<?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\Plugins\CoreAdminHome\Model; + +use Piwik\Common; +use Piwik\Container\StaticContainer; +use Piwik\DataAccess\TableMetadata; +use Piwik\Db; +use Psr\Log\LoggerInterface; + +/** + * Provides methods to find duplicate actions and fix duplicate action references in tables + * that reference log_action rows. + */ +class DuplicateActionRemover +{ + /** + * The tables that contain idaction reference columns. + * + * @var string[] + */ + public static $tablesWithIdActionColumns = array( + 'log_link_visit_action', + 'log_conversion', + 'log_conversion_item' + ); + + /** + * DAO used to get idaction column names in tables that reference log_action rows. + * + * @var TableMetadata + */ + private $tableMetadataAccess; + + /** + * @var LoggerInterface + */ + private $logger; + + /** + * List of idaction columns in each table in $tablesWithIdActionColumns. idaction + * columns are table columns with the string `"idaction"` in them. + * + * @var string[] + */ + private $idactionColumns; + + /** + * Constructor. + * + * @param TableMetadata $tableMetadataAccess + * @param LoggerInterface $logger + */ + public function __construct($tableMetadataAccess = null, $logger = null) + { + if ($tableMetadataAccess === null) { + $tableMetadataAccess = new TableMetadata(); + } + $this->tableMetadataAccess = $tableMetadataAccess; + + if ($logger === null) { + $logger = StaticContainer::get('Psr\Log\LoggerInterface'); + } + $this->logger = $logger; + + $this->idactionColumns = $this->getIdActionTableColumnsFromMetadata(); + } + + /** + * Returns list of all duplicate actions in the log_action table by name and the lowest action ID. + * The duplicate actions are returned with each action. + * + * @return array Contains the following elements: + * + * * **name**: The action's name. + * * **idaction**: The action's ID. + * * **duplicateIdActions**: An array of duplicate action IDs. + */ + public function getDuplicateIdActions() + { + $sql = "SELECT name, COUNT(*) AS count, GROUP_CONCAT(idaction ORDER BY idaction ASC SEPARATOR ',') as idactions + FROM " . Common::prefixTable('log_action') . " + GROUP BY name, hash, type HAVING count > 1"; + + $result = array(); + foreach (Db::fetchAll($sql) as $row) { + $dupeInfo = array('name' => $row['name']); + + $idActions = explode(",", $row['idactions']); + $dupeInfo['idaction'] = array_shift($idActions); + $dupeInfo['duplicateIdActions'] = $idActions; + + $result[] = $dupeInfo; + } + return $result; + } + + /** + * Executes one SQL statement that sets all idaction columns in a table to a single value, if the + * values of those columns are in the specified set (`$duplicateIdActions`). + * + * Notes: + * + * The SQL will look like: + * + * UPDATE $table SET + * col1 = IF((col1 IN ($duplicateIdActions)), $realIdAction, col1), + * col2 = IF((col2 IN ($duplicateIdActions)), $realIdAction, col2), + * ... + * WHERE col1 IN ($duplicateIdActions) OR col2 IN ($duplicateIdActions) OR ... + * + * @param string $table + * @param int $realIdAction The idaction to set column values to. + * @param int[] $duplicateIdActions The idaction values that should be changed. + */ + public function fixDuplicateActionsInTable($table, $realIdAction, $duplicateIdActions) + { + $idactionColumns = array_values($this->idactionColumns[$table]); + $table = Common::prefixTable($table); + + $inFromIdsExpression = $this->getInFromIdsExpression($duplicateIdActions); + $setExpression = "%1\$s = IF(($inFromIdsExpression), $realIdAction, %1\$s)"; + + $sql = "UPDATE $table SET\n"; + foreach ($idactionColumns as $index => $column) { + if ($index != 0) { + $sql .= ",\n"; + } + $sql .= sprintf($setExpression, $column); + } + $sql .= $this->getWhereToGetRowsUsingDuplicateActions($idactionColumns, $duplicateIdActions); + + Db::query($sql); + } + + /** + * Returns the server time and idsite of rows in a log table that reference at least one action + * in a set. + * + * @param string $table + * @param int[] $duplicateIdActions + * @return array with two elements **idsite** and **server_time**. idsite is the site ID and server_time + * is the date of the log. + */ + public function getSitesAndDatesOfRowsUsingDuplicates($table, $duplicateIdActions) + { + $idactionColumns = array_values($this->idactionColumns[$table]); + $table = Common::prefixTable($table); + + $sql = "SELECT idsite, DATE(server_time) as server_time FROM $table "; + $sql .= $this->getWhereToGetRowsUsingDuplicateActions($idactionColumns, $duplicateIdActions); + return Db::fetchAll($sql); + } + + private function getIdActionTableColumnsFromMetadata() + { + $result = array(); + foreach (self::$tablesWithIdActionColumns as $table) { + $columns = $this->tableMetadataAccess->getIdActionColumnNames(Common::prefixTable($table)); + + $this->logger->debug("Found following idactions in {table}: {columns}", array( + 'table' => $table, + 'columns' => implode(',', $columns) + )); + + $result[$table] = $columns; + } + return $result; + } + + private function getWhereToGetRowsUsingDuplicateActions($idactionColumns, $fromIdActions) + { + $sql = "WHERE "; + foreach ($idactionColumns as $index => $column) { + if ($index != 0) { + $sql .= "OR "; + } + + $sql .= sprintf($this->getInFromIdsExpression($fromIdActions), $column) . " "; + } + return $sql; + } + + private function getInFromIdsExpression($fromIdActions) + { + return "%1\$s IN (" . implode(',', $fromIdActions) . ")"; + } +} \ No newline at end of file diff --git a/plugins/CoreAdminHome/Utility/DuplicateActionRemover.php b/plugins/CoreAdminHome/Utility/DuplicateActionRemover.php deleted file mode 100644 index 76c327d52e..0000000000 --- a/plugins/CoreAdminHome/Utility/DuplicateActionRemover.php +++ /dev/null @@ -1,280 +0,0 @@ -<?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\Plugins\CoreAdminHome\Utility; - -use Piwik\Common; -use Piwik\Container\StaticContainer; -use Piwik\DataAccess\ArchiveInvalidator; -use Piwik\Db; -use Psr\Log\LoggerInterface; - -/** - * Finds duplicate actions rows in log_action and removes them. Fixes references to duplicate - * actions in the log_link_visit_action table, log_conversion table, and log_conversion_item - * table. - * - * Prior to version 2.11, there was a race condition in the tracker where it was possible for - * two or more actions with the same name and type to be inserted simultaneously. This resulted - * in inaccurate data. A Piwik database with this problem can be fixed using this class. - * - * With version 2.11 and above, it is still possible for duplicate actions to be inserted, but - * ONLY if the tracker's PHP process fails suddenly right after inserting an action. This is - * very rare, and even if it does happen, report data will not be affected, but the extra - * actions can be deleted w/ this class. - */ -class DuplicateActionRemover -{ - /** - * The tables that contain idaction reference columns. - * - * @var string[] - */ - private static $tablesWithIdActionColumns = array( - 'log_link_visit_action', - 'log_conversion', - 'log_conversion_item' - ); - - /** - * The logger. Used to log status updates. - * - * @var LoggerInterface - */ - private $logger; - - /** - * List of idaction columns in each table in $tablesWithIdActionColumns. idaction - * columns are table columns with the string `"idaction"` in them. - * - * @var string[] - */ - private $idactionColumns; - - /** - * Used to invalidate archives. Only used if $shouldInvalidateArchives is true. - * - * @var ArchiveInvalidator - */ - private $archiveInvalidator; - - /** - * Constructor. - * - * @param ArchiveInvalidator|null $archiveInvalidator If null, archives are not invalidated. - */ - public function __construct(ArchiveInvalidator $archiveInvalidator = null) - { - $this->logger = StaticContainer::get('Psr\Log\LoggerInterface'); - $this->archiveInvalidator = $archiveInvalidator; - } - - /** - * Performs the duplicate row removal & reference fixing. - * - * @return int The number of duplicates removed. - */ - public function removeDuplicateActionsFromDb() - { - $this->getIdActionTableColumnsFromMetadata(); - - $duplicateActions = $this->getDuplicateIdActions(); - - $this->logger->info("<info>Found {count} actions with duplicates.</info>", array( - 'count' => count($duplicateActions) - )); - - $dupeCount = 0; - - $archivesAffected = array(); - if (!empty($duplicateActions)) { - foreach ($duplicateActions as $index => $dupeInfo) { - $name = $dupeInfo['name']; - $idactions = $dupeInfo['idactions']; - - $this->logger->info("<info>[$index / $dupeCount]</info> Fixing duplicates for '{name}'", array( - 'name' => $name - )); - $this->logger->debug(" idactions = [ {idactions} ]", array('idactions' => $idactions)); - - $idactions = explode(',', $idactions); - - $dupeCount += count($idactions) - 1; // -1, because the first idaction is the one that isn't removed - - $this->fixDuplicateActions($idactions, $archivesAffected); - } - - $this->deleteDuplicatesFromLogAction($duplicateActions); - - $archivesAffected = array_values(array_unique($archivesAffected, SORT_REGULAR)); - if (!empty($this->archiveInvalidator)) { - $this->invalidateArchivesUsingActionDuplicates($archivesAffected); - } - } - - return array($dupeCount, $archivesAffected); - } - - private function getDuplicateIdActions() - { - $sql = "SELECT name, hash, type, COUNT(*) AS count, GROUP_CONCAT(idaction ORDER BY idaction ASC SEPARATOR ',') as idactions - FROM " . Common::prefixTable('log_action') . " - GROUP BY name, hash, type HAVING count > 1"; - return Db::fetchAll($sql); - } - - private function fixDuplicateActions($idactions, &$archivesAffected) - { - $toIdAction = array_shift($idactions); - $fromIdActions = $idactions; - - foreach (self::$tablesWithIdActionColumns as $table) { - $this->fixDuplicateActionsInTable($table, $toIdAction, $fromIdActions, $archivesAffected); - } - } - - private function fixDuplicateActionsInTable($table, $toIdAction, $fromIdActions, &$archivesAffected) - { - $startTime = microtime(true); - - $sql = $this->getSitesAndDatesOfRowsUsingDuplicates($table, $fromIdActions); - - foreach (Db::fetchAll($sql) as $row) { - $archivesAffected[] = $row; - } - - $sql = $this->getSqlToFixDuplicates($table, $toIdAction, $fromIdActions); - - Db::query($sql); - - $endTime = microtime(true); - $elapsed = $endTime - $startTime; - - $this->logger->info("\tFixed duplicates in {table} in <comment>{elapsed}s</comment>.", array( - 'table' => Common::prefixTable($table), - 'elapsed' => $elapsed - )); - } - - private function deleteDuplicatesFromLogAction($duplicateActions) - { - $table = Common::prefixTable('log_action'); - - $this->logger->info("<info>Deleting duplicate actions from {table}...</info>", array( - 'table' => $table - )); - - $sql = "DELETE FROM $table WHERE idaction IN ("; - foreach ($duplicateActions as $index => $dupeInfos) { - if ($index != 0) { - $sql .= ","; - } - - $restIdActions = $dupeInfos['idactions']; - - $commaPos = strpos($restIdActions, ','); - if ($commaPos !== false) { - $restIdActions = substr($restIdActions, $commaPos + 1); - } - - $sql .= $restIdActions; - } - $sql .= ")"; - - Db::query($sql); - } - - /** - * Creates SQL that sets multiple columns in a table to a single value, if those - * columns are set to certain values. - * - * The SQL will look like: - * - * UPDATE $table SET - * col1 = IF((col1 IN ($fromIdActions)), $toIdAction, col1), - * col2 = IF((col2 IN ($fromIdActions)), $toIdAction, col2), - * ... - * WHERE col1 IN ($fromIdActions) OR col2 IN ($fromIdActions) OR ... - */ - private function getSqlToFixDuplicates($table, $toIdAction, $fromIdActions) - { - $idactionColumns = array_values($this->idactionColumns[$table]); - $table = Common::prefixTable($table); - - $inFromIdsExpression = $this->getInFromIdsExpression($fromIdActions); - $setExpression = "%1\$s = IF(($inFromIdsExpression), $toIdAction, %1\$s)"; - - $sql = "UPDATE $table SET\n"; - foreach ($idactionColumns as $index => $column) { - if ($index != 0) { - $sql .= ",\n"; - } - $sql .= sprintf($setExpression, $column); - } - $sql .= $this->getWhereToGetRowsUsingDuplicateActions($idactionColumns, $fromIdActions); - - return $sql; - } - - private function getSitesAndDatesOfRowsUsingDuplicates($table, $fromIdActions) - { - $idactionColumns = array_values($this->idactionColumns[$table]); - $table = Common::prefixTable($table); - - $sql = "SELECT idsite, DATE(server_time) as server_time FROM $table "; - $sql .= $this->getWhereToGetRowsUsingDuplicateActions($idactionColumns, $fromIdActions); - return $sql; - } - - private function getIdActionTableColumnsFromMetadata() - { - foreach (self::$tablesWithIdActionColumns as $table) { - $columns = Db::fetchAll("SHOW COLUMNS FROM " . Common::prefixTable($table)); - - $columns = array_map(function ($row) { return $row['Field']; }, $columns); - - $columns = array_filter($columns, function ($columnName) { - return strpos($columnName, 'idaction') !== false; - }); - - $this->logger->debug("Found following idactions in {table}: {columns}", array( - 'table' => $table, - 'columns' => implode(',', $columns) - )); - - $this->idactionColumns[$table] = $columns; - } - } - - private function getWhereToGetRowsUsingDuplicateActions($idactionColumns, $fromIdActions) - { - $sql = "WHERE "; - foreach ($idactionColumns as $index => $column) { - if ($index != 0) { - $sql .= "OR "; - } - - $sql .= sprintf($this->getInFromIdsExpression($fromIdActions), $column) . " "; - } - return $sql; - } - - private function getInFromIdsExpression($fromIdActions) - { - return "%1\$s IN (" . implode(',', $fromIdActions) . ")"; - } - - private function invalidateArchivesUsingActionDuplicates($archivesAffected) - { - $this->logger->info("Invalidating archives affected by duplicates fixed..."); - foreach ($archivesAffected as $archiveInfo) { - $this->archiveInvalidator->markArchivesAsInvalidated( - array($archiveInfo['idsite']), $archiveInfo['server_time'], $period = false); - } - $this->logger->info("...Done."); - } -} \ No newline at end of file diff --git a/plugins/CoreAdminHome/tests/Integration/DuplicateActionRemoverTest.php b/plugins/CoreAdminHome/tests/Fixture/DuplicateActions.php similarity index 51% rename from plugins/CoreAdminHome/tests/Integration/DuplicateActionRemoverTest.php rename to plugins/CoreAdminHome/tests/Fixture/DuplicateActions.php index cca9fdbe76..7fbb1225ee 100644 --- a/plugins/CoreAdminHome/tests/Integration/DuplicateActionRemoverTest.php +++ b/plugins/CoreAdminHome/tests/Fixture/DuplicateActions.php @@ -5,19 +5,16 @@ * @link http://piwik.org * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later */ -namespace Piwik\Plugins\CoreAdminHome\tests\Integration; +namespace Piwik\Plugins\CoreAdminHome\tests\Fixture; use Piwik\Common; -use Piwik\DataAccess\ArchiveInvalidator; use Piwik\Db; -use Piwik\Plugin\Manager as PluginManager; -use Piwik\Plugins\CoreAdminHome\Utility\DuplicateActionRemover; -use Piwik\Tests\Framework\TestCase\IntegrationTestCase; +use Piwik\Tests\Framework\Fixture; /** - * @group Core + * Fixture that adds log table rows that use duplicate actions. */ -class DuplicateActionRemoverTest extends IntegrationTestCase +class DuplicateActions extends Fixture { const DUMMY_IDVISITOR = '008c5926ca861023c1d2a36653fd88e2'; @@ -143,11 +140,6 @@ class DuplicateActionRemoverTest extends IntegrationTestCase ) ); - /** - * @var DuplicateActionRemover - */ - private $remover; - public function setUp() { parent::setUp(); @@ -155,30 +147,6 @@ class DuplicateActionRemoverTest extends IntegrationTestCase foreach (self::$dataToInsert as $table => $rows) { self::insertRowData($table, $rows); } - - $archiveInvalidator = new ArchiveInvalidator(); - $this->remover = new DuplicateActionRemover($archiveInvalidator); - } - - public function test_DuplicateActionRemover_CorrectlyRemovesDuplicates_AndFixesReferencesInOtherTables() - { - list($duplicatesCount, $affectedArchives) = $this->remover->removeDuplicateActionsFromDb(); - - $this->assertDuplicateActionsRemovedFromLogActionTable(); - $this->assertDuplicatesFixedInLogLinkVisitActionTable(); - $this->assertDuplicatesFixedInLogConversionTable(); - $this->assertDuplicatesFixedInLogConversionItemTable(); - - $this->assertEquals(7, $duplicatesCount); - - $expectedAffectedArchives = array( - array('idsite' => '1', 'server_time' => '2012-01-01'), - array('idsite' => '2', 'server_time' => '2013-01-01'), - array('idsite' => '1', 'server_time' => '2012-02-01'), - array('idsite' => '2', 'server_time' => '2012-01-09'), - array('idsite' => '2', 'server_time' => '2012-03-01'), - ); - $this->assertEquals($expectedAffectedArchives, $affectedArchives); } private static function insertRowData($unprefixedTable, $rows) @@ -198,106 +166,4 @@ class DuplicateActionRemoverTest extends IntegrationTestCase Db::query($sql, array_values($row)); } } - - private function assertDuplicateActionsRemovedFromLogActionTable() - { - $actions = Db::fetchAll("SELECT idaction, name FROM " . Common::prefixTable('log_action')); - $expectedActions = array( - array('idaction' => 1, 'name' => 'action1'), - array('idaction' => 4, 'name' => 'action2'), - array('idaction' => 5, 'name' => 'ACTION2'), - array('idaction' => 6, 'name' => 'action4'), - array('idaction' => 8, 'name' => 'action5'), - ); - $this->assertEquals($expectedActions, $actions); - } - - private function assertDuplicatesFixedInLogLinkVisitActionTable() - { - $columns = array( - 'idaction_url_ref', - 'idaction_name_ref', - 'idaction_name', - 'idaction_url', - 'idaction_event_action', - 'idaction_event_category', - 'idaction_content_interaction', - 'idaction_content_name', - 'idaction_content_piece', - 'idaction_content_target' - ); - $rows = Db::fetchAll("SELECT " . implode(',', $columns) . " FROM " . Common::prefixTable('log_link_visit_action')); - $expectedRows = array( - array( - 'idaction_url_ref' => '1', - 'idaction_name_ref' => '1', - 'idaction_name' => '1', - 'idaction_url' => '4', - 'idaction_event_action' => '5', - 'idaction_event_category' => '6', - 'idaction_content_interaction' => '5', - 'idaction_content_name' => '8', - 'idaction_content_piece' => '4', - 'idaction_content_target' => '6' - ), - array( - 'idaction_url_ref' => '1', - 'idaction_name_ref' => '1', - 'idaction_name' => '5', - 'idaction_url' => '5', - 'idaction_event_action' => '4', - 'idaction_event_category' => '6', - 'idaction_content_interaction' => '5', - 'idaction_content_name' => '5', - 'idaction_content_piece' => '6', - 'idaction_content_target' => '6' - ) - ); - $this->assertEquals($expectedRows, $rows); - } - - private function assertDuplicatesFixedInLogConversionTable() - { - $rows = Db::fetchAll("SELECT idaction_url FROM " . Common::prefixTable('log_conversion')); - $expectedRows = array( - array('idaction_url' => 4), - array('idaction_url' => 5) - ); - $this->assertEquals($expectedRows, $rows); - } - - private function assertDuplicatesFixedInLogConversionItemTable() - { - $columns = array( - 'idaction_sku', - 'idaction_name', - 'idaction_category', - 'idaction_category2', - 'idaction_category3', - 'idaction_category4', - 'idaction_category5' - ); - $rows = Db::fetchAll("SELECT " . implode(',', $columns) . " FROM " . Common::prefixTable('log_conversion_item')); - $expectedRows = array( - array( - 'idaction_sku' => '1', - 'idaction_name' => '1', - 'idaction_category' => '1', - 'idaction_category2' => '4', - 'idaction_category3' => '5', - 'idaction_category4' => '6', - 'idaction_category5' => '5' - ), - array( - 'idaction_sku' => '1', - 'idaction_name' => '1', - 'idaction_category' => '5', - 'idaction_category2' => '5', - 'idaction_category3' => '8', - 'idaction_category4' => '4', - 'idaction_category5' => '6' - ) - ); - $this->assertEquals($expectedRows, $rows); - } } \ No newline at end of file diff --git a/plugins/CoreAdminHome/tests/Integration/FixDuplicateActionsTest.php b/plugins/CoreAdminHome/tests/Integration/FixDuplicateActionsTest.php new file mode 100644 index 0000000000..ca56f3b026 --- /dev/null +++ b/plugins/CoreAdminHome/tests/Integration/FixDuplicateActionsTest.php @@ -0,0 +1,175 @@ +<?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\Plugins\CoreAdminHome\tests\Integration; + +use Piwik\Common; +use Piwik\Console; +use Piwik\Db; +use Piwik\Plugin\Manager as PluginManager; +use Piwik\Plugins\CoreAdminHome\tests\Fixture\DuplicateActions; +use Piwik\Plugins\QueuedTracking\tests\Framework\TestCase\IntegrationTestCase; +use Symfony\Component\Console\Tester\ApplicationTester; + +/** + * @group Core + */ +class FixDuplicateActionsTest extends IntegrationTestCase +{ + /** + * @var DuplicateActions + */ + public static $fixture = null; + + /** + * @var ApplicationTester + */ + protected $applicationTester = null; + + public function setUp() + { + parent::setUp(); + + $application = new Console(); + $application->setAutoExit(false); + + $this->applicationTester = new ApplicationTester($application); + } + + public function test_FixDuplicateLogActions_CorrectlyRemovesDuplicates_AndFixesReferencesInOtherTables() + { + $result = $this->applicationTester->run(array( + 'command' => 'core:fix-duplicate-log-actions', + '--invalidate-archives' => 0, + '-vvv' => false + )); + + $this->assertEquals(0, $result, "Command failed: " . $this->applicationTester->getDisplay()); + + $this->assertDuplicateActionsRemovedFromLogActionTable(); + $this->assertDuplicatesFixedInLogLinkVisitActionTable(); + $this->assertDuplicatesFixedInLogConversionTable(); + $this->assertDuplicatesFixedInLogConversionItemTable(); + + $this->assertContains("Found and deleted 7 duplicate action entries", $this->applicationTester->getDisplay()); + + $expectedAffectedArchives = array( + array('idsite' => '1', 'server_time' => '2012-01-01'), + array('idsite' => '2', 'server_time' => '2013-01-01'), + array('idsite' => '1', 'server_time' => '2012-02-01'), + array('idsite' => '2', 'server_time' => '2012-01-09'), + array('idsite' => '2', 'server_time' => '2012-03-01'), + ); + foreach ($expectedAffectedArchives as $archive) { + $this->assertContains("[ idSite = {$archive['idsite']}, date = {$archive['server_time']} ]", $this->applicationTester->getDisplay()); + } + } + + private function assertDuplicateActionsRemovedFromLogActionTable() + { + $actions = Db::fetchAll("SELECT idaction, name FROM " . Common::prefixTable('log_action')); + $expectedActions = array( + array('idaction' => 1, 'name' => 'action1'), + array('idaction' => 4, 'name' => 'action2'), + array('idaction' => 5, 'name' => 'ACTION2'), + array('idaction' => 6, 'name' => 'action4'), + array('idaction' => 8, 'name' => 'action5'), + ); + $this->assertEquals($expectedActions, $actions); + } + + private function assertDuplicatesFixedInLogLinkVisitActionTable() + { + $columns = array( + 'idaction_url_ref', + 'idaction_name_ref', + 'idaction_name', + 'idaction_url', + 'idaction_event_action', + 'idaction_event_category', + 'idaction_content_interaction', + 'idaction_content_name', + 'idaction_content_piece', + 'idaction_content_target' + ); + $rows = Db::fetchAll("SELECT " . implode(',', $columns) . " FROM " . Common::prefixTable('log_link_visit_action')); + $expectedRows = array( + array( + 'idaction_url_ref' => '1', + 'idaction_name_ref' => '1', + 'idaction_name' => '1', + 'idaction_url' => '4', + 'idaction_event_action' => '5', + 'idaction_event_category' => '6', + 'idaction_content_interaction' => '5', + 'idaction_content_name' => '8', + 'idaction_content_piece' => '4', + 'idaction_content_target' => '6' + ), + array( + 'idaction_url_ref' => '1', + 'idaction_name_ref' => '1', + 'idaction_name' => '5', + 'idaction_url' => '5', + 'idaction_event_action' => '4', + 'idaction_event_category' => '6', + 'idaction_content_interaction' => '5', + 'idaction_content_name' => '5', + 'idaction_content_piece' => '6', + 'idaction_content_target' => '6' + ) + ); + $this->assertEquals($expectedRows, $rows); + } + + private function assertDuplicatesFixedInLogConversionTable() + { + $rows = Db::fetchAll("SELECT idaction_url FROM " . Common::prefixTable('log_conversion')); + $expectedRows = array( + array('idaction_url' => 4), + array('idaction_url' => 5) + ); + $this->assertEquals($expectedRows, $rows); + } + + private function assertDuplicatesFixedInLogConversionItemTable() + { + $columns = array( + 'idaction_sku', + 'idaction_name', + 'idaction_category', + 'idaction_category2', + 'idaction_category3', + 'idaction_category4', + 'idaction_category5' + ); + $rows = Db::fetchAll("SELECT " . implode(',', $columns) . " FROM " . Common::prefixTable('log_conversion_item')); + $expectedRows = array( + array( + 'idaction_sku' => '1', + 'idaction_name' => '1', + 'idaction_category' => '1', + 'idaction_category2' => '4', + 'idaction_category3' => '5', + 'idaction_category4' => '6', + 'idaction_category5' => '5' + ), + array( + 'idaction_sku' => '1', + 'idaction_name' => '1', + 'idaction_category' => '5', + 'idaction_category2' => '5', + 'idaction_category3' => '8', + 'idaction_category4' => '4', + 'idaction_category5' => '6' + ) + ); + $this->assertEquals($expectedRows, $rows); + } +} + +FixDuplicateActionsTest::$fixture = new DuplicateActions(); \ No newline at end of file diff --git a/plugins/CoreAdminHome/tests/Integration/Model/DuplicateActionRemoverTest.php b/plugins/CoreAdminHome/tests/Integration/Model/DuplicateActionRemoverTest.php new file mode 100644 index 0000000000..78a988d908 --- /dev/null +++ b/plugins/CoreAdminHome/tests/Integration/Model/DuplicateActionRemoverTest.php @@ -0,0 +1,112 @@ +<?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\Plugins\CoreAdminHome\tests\Integration\Model; + +use Piwik\Common; +use Piwik\Db; +use Piwik\Plugins\CoreAdminHome\Model\DuplicateActionRemover; +use Piwik\Plugins\CoreAdminHome\tests\Fixture\DuplicateActions; +use Piwik\Tests\Framework\TestCase\IntegrationTestCase; + +/** + * @group Core + */ +class DuplicateActionRemoverTest extends IntegrationTestCase +{ + /** + * @var DuplicateActions + */ + public static $fixture = null; + + /** + * @var DuplicateActionRemover + */ + private $duplicateActionRemover; + + public function setUp() + { + parent::setUp(); + + $this->duplicateActionRemover = new DuplicateActionRemover(); + } + + public function test_getDuplicateIdActions_ReturnsDuplicateIdActions_AndTreatsLowestIdActionAsOriginal() + { + $expectedResult = array( + array('name' => 'action1', 'idaction' => 1, 'duplicateIdActions' => array(2, 3)), + array('name' => 'ACTION2', 'idaction' => 5, 'duplicateIdActions' => array(7, 11)), + array('name' => 'action2', 'idaction' => 4, 'duplicateIdActions' => array(9)), + array('name' => 'action4', 'idaction' => 6, 'duplicateIdActions' => array(10, 12)), + ); + $actualResult = $this->duplicateActionRemover->getDuplicateIdActions(); + $this->assertEquals($expectedResult, $actualResult); + } + + public function test_fixDuplicateActionsInTable_CorrectlyUpdatesIdActionColumns_InSpecifiedTable() + { + $this->duplicateActionRemover->fixDuplicateActionsInTable('log_conversion_item', 5, array(3, 6, 7, 10)); + + $columns = array('idaction_sku', 'idaction_name', 'idaction_category', 'idaction_category2', + 'idaction_category3', 'idaction_category4', 'idaction_category5'); + + $expectedResult = array( + array( + 'idaction_sku' => '1', + 'idaction_name' => '2', + 'idaction_category' => '5', + 'idaction_category2' => '4', + 'idaction_category3' => '5', + 'idaction_category4' => '5', + 'idaction_category5' => '5' + ), + array( + 'idaction_sku' => '2', + 'idaction_name' => '5', + 'idaction_category' => '5', + 'idaction_category2' => '5', + 'idaction_category3' => '8', + 'idaction_category4' => '9', + 'idaction_category5' => '5' + ), + ); + $actualResult = Db::fetchAll("SELECT " . implode(", ", $columns) . " FROM " . Common::prefixTable('log_conversion_item')); + $this->assertEquals($expectedResult, $actualResult); + } + + public function test_getSitesAndDatesOfRowsUsingDuplicates_ReturnsTheServerTimeAndIdSite_OfRowsUsingSpecifiedActionIds() + { + $row = array( + 'idsite' => 3, + 'idvisitor' => DuplicateActions::DUMMY_IDVISITOR, + 'server_time' => '2012-02-13 00:00:00', + 'idvisit' => 5, + 'idorder' => 6, + 'price' => 15, + 'quantity' => 21, + 'deleted' => 1, + 'idaction_sku' => 3, + 'idaction_name' => 3, + 'idaction_category' => 12, + 'idaction_category2' => 3, + 'idaction_category3' => 3, + 'idaction_category4' => 3, + 'idaction_category5' => 3, + ); + Db::query("INSERT INTO " . Common::prefixTable('log_conversion_item') . " (" . implode(", ", array_keys($row)) + . ") VALUES ('" . implode("', '", array_values($row)) . "')"); + + $expectedResult = array( + array('idsite' => 1, 'server_time' => '2012-02-01'), + array('idsite' => 3, 'server_time' => '2012-02-13') + ); + $actualResult = $this->duplicateActionRemover->getSitesAndDatesOfRowsUsingDuplicates('log_conversion_item', array(4, 6, 12)); + $this->assertEquals($expectedResult, $actualResult); + } +} + +DuplicateActionRemoverTest::$fixture = new DuplicateActions(); \ No newline at end of file diff --git a/tests/PHPUnit/Integration/DataAccess/ActionsTest.php b/tests/PHPUnit/Integration/DataAccess/ActionsTest.php new file mode 100644 index 0000000000..77ce6b08f1 --- /dev/null +++ b/tests/PHPUnit/Integration/DataAccess/ActionsTest.php @@ -0,0 +1,64 @@ +<?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\Integration\DataAccess; + +use Piwik\Common; +use Piwik\DataAccess\Actions; +use Piwik\Db; +use Piwik\Tests\Framework\TestCase\IntegrationTestCase; + +/** + * @group Core + */ +class ActionsTest extends IntegrationTestCase +{ + /** + * @var Actions + */ + private $actionsAccess; + + public function setUp() + { + parent::setUp(); + + $this->insertTestActions(); + + $this->actionsAccess = new Actions(); + } + + public function test_delete_DeletesSpecifiedActions() + { + $this->actionsAccess->delete(array(2,3,4,5)); + + $expectedActions = array( + array('name' => 'action1') + ); + $actualActions = Db::fetchAll("SELECT name FROM ".Common::prefixTable('log_action')); + $this->assertEquals($expectedActions, $actualActions); + } + + public function test_delete_ConvertsIdActionsToInt() + { + $this->actionsAccess->delete(array("2", "0, 1")); + + $expectedActions = array( + array('name' => 'action1'), + array('name' => 'action3') + ); + $actualActions = Db::fetchAll("SELECT name FROM ".Common::prefixTable('log_action')); + $this->assertEquals($expectedActions, $actualActions); + } + + private function insertTestActions() + { + Db::query("INSERT INTO " . Common::prefixTable('log_action') . " (name, type, hash) + VALUES ('action1', 1, CRC32('action1')), + ('action2', 1, CRC32('action2')), + ('action3', 1, CRC32('action3'))"); + } +} \ No newline at end of file diff --git a/tests/PHPUnit/Integration/DataAccess/TableMetadataTest.php b/tests/PHPUnit/Integration/DataAccess/TableMetadataTest.php new file mode 100644 index 0000000000..47c911bc2b --- /dev/null +++ b/tests/PHPUnit/Integration/DataAccess/TableMetadataTest.php @@ -0,0 +1,55 @@ +<?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\Integration\DataAccess; + +use Piwik\Common; +use Piwik\DataAccess\TableMetadata; +use Piwik\Tests\Framework\TestCase\IntegrationTestCase; + +/** + * @group Core + */ +class TableMetadataTest extends IntegrationTestCase +{ + /** + * @var TableMetadata + */ + private $tableMetadataAccess; + + public function setUp() + { + parent::setUp(); + + $this->tableMetadataAccess = new TableMetadata(); + } + + public function test_getColumns_CorrectlyReturnsListOfColumnNames() + { + $expectedColumns = array('option_name', 'option_value', 'autoload'); + $columns = $this->tableMetadataAccess->getColumns(Common::prefixTable('option')); + $this->assertEquals($expectedColumns, $columns); + } + + /** + * @dataProvider getTablesWithIdActionColumnsToTest + */ + public function test_getIdActionColumnNames_CorrectlyReturnsColumnsWithIdActionName($table, $expectedColumns) + { + $columns = $this->tableMetadataAccess->getIdActionColumnNames(Common::prefixTable($table)); + $this->assertEquals($expectedColumns, $columns); + } + + public function getTablesWithIdActionColumnsToTest() + { + return array( + array('log_conversion', array('idaction_url')), + array('log_conversion_item', array('idaction_sku', 'idaction_name', 'idaction_category', 'idaction_category2', + 'idaction_category3', 'idaction_category4', 'idaction_category5')) + ); + } +} \ No newline at end of file -- GitLab