Merge pull request matomo-org#6836 from piwik/3414

refactor to prevent API error "This report has been reprocessed since your last click"
d19dotca · Dec 10, 2014 · 8b1adfe · 8b1adfe
2 parents 490990f + 54abfe3
commit 8b1adfe
Show file tree

Hide file tree

Showing 28 changed files with 184 additions and 335 deletions.
diff --git a/core/DataTable.php b/core/DataTable.php
@@ -356,10 +356,11 @@ public function sort($functionCallback, $columnSortedBy)
 
         if ($this->enableRecursiveSort === true) {
             foreach ($this->getRows() as $row) {
-                if (($idSubtable = $row->getIdSubDataTable()) !== null) {
-                    $table = Manager::getInstance()->getTable($idSubtable);
-                    $table->enableRecursiveSort();
-                    $table->sort($functionCallback, $columnSortedBy);
+
+                $subTable = $row->getSubtable();
+                if ($subTable) {
+                    $subTable->enableRecursiveSort();
+                    $subTable->sort($functionCallback, $columnSortedBy);
                 }
             }
         }
@@ -868,8 +869,8 @@ public function getRowsCountRecursive()
     {
         $totalCount = 0;
         foreach ($this->rows as $row) {
-            if (($idSubTable = $row->getIdSubDataTable()) !== null) {
-                $subTable = Manager::getInstance()->getTable($idSubTable);
+            $subTable = $row->getSubtable();
+            if ($subTable) {
                 $count = $subTable->getRowsCountRecursive();
                 $totalCount += $count;
             }
@@ -907,8 +908,9 @@ public function renameColumn($oldName, $newName, $doRenameColumnsOfSubTables = t
             $row->renameColumn($oldName, $newName);
 
             if ($doRenameColumnsOfSubTables) {
-                if (($idSubDataTable = $row->getIdSubDataTable()) !== null) {
-                    Manager::getInstance()->getTable($idSubDataTable)->renameColumn($oldName, $newName);
+                $subTable = $row->getSubtable();
+                if ($subTable) {
+                    $subTable->renameColumn($oldName, $newName);
                 }
             }
         }
@@ -929,8 +931,9 @@ public function deleteColumns($names, $deleteRecursiveInSubtables = false)
             foreach ($names as $name) {
                 $row->deleteColumn($name);
             }
-            if (($idSubDataTable = $row->getIdSubDataTable()) !== null) {
-                Manager::getInstance()->getTable($idSubDataTable)->deleteColumns($names, $deleteRecursiveInSubtables);
+            $subTable = $row->getSubtable();
+            if ($subTable) {
+                $subTable->deleteColumns($names, $deleteRecursiveInSubtables);
             }
         }
         if (!is_null($this->summaryRow)) {
@@ -1110,20 +1113,13 @@ public function getSerialized($maximumRowsInDataTable = null,
         // but returns all serialized tables and subtable in an array of 1 dimension
         $aSerializedDataTable = array();
         foreach ($this->rows as $row) {
-            if (($idSubTable = $row->getIdSubDataTable()) !== null) {
-                $subTable = null;
-                try {
-                    $subTable = Manager::getInstance()->getTable($idSubTable);
-                } catch(TableNotFoundException $e) {
-                    // This occurs is an unknown & random data issue. Catch Exception and remove subtable from the row.
-                    $row->removeSubtable();
-                    // Go to next row
-                    continue;
-                }
-
+            $subTable = $row->getSubtable();
+            if ($subTable) {
                 $depth++;
                 $aSerializedDataTable = $aSerializedDataTable + $subTable->getSerialized($maximumRowsInSubDataTable, $maximumRowsInSubDataTable, $columnToSortByBeforeTruncation);
                 $depth--;
+            } else {
+                $row->removeSubtable();
             }
         }
         // we load the current Id of the DataTable
@@ -1616,8 +1612,8 @@ protected function aggregateRowWithLabel(Row $row, $doAggregateSubTables = true)
                 // we simply add it (cloning the subtable)
                 // if the row has the subtable already
                 // then we have to recursively sum the subtables
-                if (($idSubTable = $row->getIdSubDataTable()) !== null) {
-                    $subTable = Manager::getInstance()->getTable($idSubTable);
+                $subTable = $row->getSubtable();
+                if ($subTable) {
                     $subTable->metadata[self::COLUMN_AGGREGATION_OPS_METADATA_NAME]
                         = $this->getMetadata(self::COLUMN_AGGREGATION_OPS_METADATA_NAME);
                     $rowFound->sumSubtable($subTable);

diff --git a/core/DataTable/BaseFilter.php b/core/DataTable/BaseFilter.php
@@ -73,8 +73,8 @@ public function filterSubTable(Row $row)
         if (!$this->enableRecursive) {
             return;
         }
-        if ($row->isSubtableLoaded()) {
-            $subTable = Manager::getInstance()->getTable($row->getIdSubDataTable());
+        $subTable = $row->getSubtable();
+        if ($subTable) {
             $this->filter($subTable);
         }
     }

diff --git a/core/DataTable/Filter/PatternRecursive.php b/core/DataTable/Filter/PatternRecursive.php
@@ -62,18 +62,15 @@ public function filter($table)
             // AND 2 - the label is not found in the children
             $patternNotFoundInChildren = false;
 
-            try {
-                $idSubTable = $row->getIdSubDataTable();
-                $subTable = Manager::getInstance()->getTable($idSubTable);
-
+            $subTable = $row->getSubtable();
+            if(!$subTable) {
+                $patternNotFoundInChildren = true;
+            } else {
                 // we delete the row if we couldn't find the pattern in any row in the
                 // children hierarchy
                 if ($this->filter($subTable) == 0) {
                     $patternNotFoundInChildren = true;
                 }
-            } catch (Exception $e) {
-                // there is no subtable loaded for example
-                $patternNotFoundInChildren = true;
             }
 
             if ($patternNotFoundInChildren

diff --git a/core/DataTable/Filter/ReplaceSummaryRowLabel.php b/core/DataTable/Filter/ReplaceSummaryRowLabel.php
@@ -65,8 +65,8 @@ public function filter($table)
 
         // recurse
         foreach ($rows as $row) {
-            if ($row->isSubtableLoaded()) {
-                $subTable = Manager::getInstance()->getTable($row->getIdSubDataTable());
+            $subTable = $row->getSubtable();
+            if ($subTable) {
                 $this->filter($subTable);
             }
         }

diff --git a/core/DataTable/Manager.php b/core/DataTable/Manager.php
@@ -61,7 +61,7 @@ public function addTable($table)
     public function getTable($idTable)
     {
         if (!isset($this->tables[$idTable])) {
-            throw new TableNotFoundException(sprintf("This report has been reprocessed since your last click. To see this error less often, please increase the timeout value in seconds in Settings > General Settings. (error: id %s not found).", $idTable));
+            throw new TableNotFoundException(sprintf("Error: table id %s not found in memory. (If this error is causing you problems in production, please report it in Piwik issue tracker.)", $idTable));
         }
 
         return $this->tables[$idTable];

diff --git a/core/DataTable/Renderer/Console.php b/core/DataTable/Renderer/Console.php
@@ -120,14 +120,10 @@ protected function renderTable($table, $prefix = "")
                 . $row->getIdSubDataTable() . "]<br />\n";
 
             if (!is_null($row->getIdSubDataTable())) {
-                if ($row->isSubtableLoaded()) {
+                $subTable = $row->getSubtable();
+                if ($subTable) {
                     $depth++;
-                    $output .= $this->renderTable(
-                        Manager::getInstance()->getTable(
-                            $row->getIdSubDataTable()
-                        ),
-                        $prefix . '&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;'
-                    );
+                    $output .= $this->renderTable($subTable, $prefix . '&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;');
                     $depth--;
                 } else {
                     $output .= "-- Sub DataTable not loaded<br />\n";

diff --git a/core/DataTable/Renderer/Php.php b/core/DataTable/Renderer/Php.php
@@ -206,10 +206,11 @@ protected function renderTable($table)
                 $newRow['issummaryrow'] = true;
             }
 
+            $subTable = $row->getSubtable();
             if ($this->isRenderSubtables()
-                && $row->isSubtableLoaded()
+                && $subTable
             ) {
-                $subTable = $this->renderTable(Manager::getInstance()->getTable($row->getIdSubDataTable()));
+                $subTable = $this->renderTable($subTable);
                 $newRow['subtable'] = $subTable;
                 if ($this->hideIdSubDatatable === false
                     && isset($newRow['metadata']['idsubdatatable_in_db'])

diff --git a/core/DataTable/Row.php b/core/DataTable/Row.php
@@ -317,7 +317,11 @@ public function getIdSubDataTable()
     public function getSubtable()
     {
         if ($this->isSubtableLoaded()) {
-            return Manager::getInstance()->getTable($this->getIdSubDataTable());
+            try {
+                return Manager::getInstance()->getTable($this->getIdSubDataTable());
+            } catch(TableNotFoundException $e) {
+                // edge case
+            }
         }
         return false;
     }

diff --git a/core/DataTable/Row/DataTableSummaryRow.php b/core/DataTable/Row/DataTableSummaryRow.php
@@ -47,9 +47,8 @@ public function __construct($subTable = null)
      */
     public function recalculate()
     {
-        $id = $this->getIdSubDataTable();
-        if ($id !== null) {
-            $subTable = Manager::getInstance()->getTable($id);
+        $subTable = $this->getSubtable();
+        if ($subTable) {
             $this->sumTable($subTable);
         }
     }

diff --git a/plugins/Actions/ArchivingHelper.php b/plugins/Actions/ArchivingHelper.php
@@ -198,9 +198,9 @@ public static function deleteInvalidSummedColumnsFromDataTable($dataTable)
             if (($idSubtable = $row->getIdSubDataTable()) !== null
                 || $id === DataTable::ID_SUMMARY_ROW
             ) {
-                if ($idSubtable !== null) {
-                    $subtable = Manager::getInstance()->getTable($idSubtable);
-                    self::deleteInvalidSummedColumnsFromDataTable($subtable);
+                $subTable = $row->getSubtable();
+                if ($subTable) {
+                    self::deleteInvalidSummedColumnsFromDataTable($subTable);
                 }
 
                 if ($row instanceof DataTableSummaryRow) {

diff --git a/plugins/Transitions/API.php b/plugins/Transitions/API.php
@@ -523,8 +523,8 @@ private function addExternalReferrers($logAggregator, &$report, $idaction, $acti
             if ($visits) {
                 // load details (i.e. subtables)
                 $details = array();
-                if ($idSubTable = $row->getIdSubDataTable()) {
-                    $subTable = Manager::getInstance()->getTable($idSubTable);
+                $subTable = $row->getSubtable();
+                if ($subTable) {
                     foreach ($subTable->getRows() as $subRow) {
                         $details[] = array(
                             'label'     => $subRow->getColumn('label'),

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/AddSummaryRowTest.php b/tests/PHPUnit/Unit/DataTable/Filter/AddSummaryRowTest.php
@@ -11,6 +11,9 @@
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
 
+/**
+ * @group DataTableTest
+ */
 class DataTable_Filter_AddSummaryRowTest extends \PHPUnit_Framework_TestCase
 {
     /**

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/ExcludeLowPopulationTest.php b/tests/PHPUnit/Unit/DataTable/Filter/ExcludeLowPopulationTest.php
@@ -12,6 +12,9 @@
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
 
+/**
+ * @group DataTableTest
+ */
 class DataTable_Filter_ExcludeLowPopulationTest extends \PHPUnit_Framework_TestCase
 {
     protected function getTestDataTable()

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/LimitTest.php b/tests/PHPUnit/Unit/DataTable/Filter/LimitTest.php
@@ -12,6 +12,9 @@
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
 
+/**
+ * @group DataTableTest
+ */
 class DataTable_Filter_LimitTest extends \PHPUnit_Framework_TestCase
 {
     /**
@@ -39,9 +42,7 @@ protected function getDataTableCount10()
         return $table;
     }
 
-    /**
-     * @group Core
-     */
+
     public function testNormal()
     {
         $offset = 2;
@@ -55,9 +56,7 @@ public function testNormal()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testLimitLessThanCountShouldReturnCountLimit()
     {
         $offset = 2;
@@ -71,9 +70,7 @@ public function testLimitLessThanCountShouldReturnCountLimit()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testLimitIsCountShouldNotDeleteAnything()
     {
         $offset = 0;
@@ -88,9 +85,7 @@ public function testLimitIsCountShouldNotDeleteAnything()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testLimitGreaterThanCountShouldReturnCountUntilCount()
     {
         $offset = 5;
@@ -105,9 +100,7 @@ public function testLimitGreaterThanCountShouldReturnCountUntilCount()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testLimitIsNullShouldReturnCountIsOffset()
     {
         $offset = 1;
@@ -120,9 +113,7 @@ public function testLimitIsNullShouldReturnCountIsOffset()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testOffsetJustBeforeSummaryRowShouldJustReturnSummaryRow()
     {
         $offset = 9;
@@ -136,9 +127,7 @@ public function testOffsetJustBeforeSummaryRowShouldJustReturnSummaryRow()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testOffsetJustBeforeSummaryRowWithBigLimitShouldJustReturnSummaryRow()
     {
         $offset = 9;
@@ -152,9 +141,7 @@ public function testOffsetJustBeforeSummaryRowWithBigLimitShouldJustReturnSummar
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testOffsetBeforeSummaryRowShouldJustReturnRowAndSummaryRow()
     {
         $offset = 8;
@@ -168,9 +155,7 @@ public function testOffsetBeforeSummaryRowShouldJustReturnRowAndSummaryRow()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testOffsetGreaterThanCountShouldReturnEmptyTable()
     {
         $offset = 10;
@@ -182,9 +167,7 @@ public function testOffsetGreaterThanCountShouldReturnEmptyTable()
         $this->assertEquals(10, $table->getMetadata(DataTable::TOTAL_ROWS_BEFORE_LIMIT_METADATA_NAME));
     }
 
-    /**
-     * @group Core
-     */
+
     public function testLimitIsZeroShouldReturnEmptyTable()
     {
         $offset = 0;

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/PatternRecursiveTest.php b/tests/PHPUnit/Unit/DataTable/Filter/PatternRecursiveTest.php
@@ -11,6 +11,9 @@
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
 
+/**
+ * @group DataTableTest
+ */
 class DataTable_Filter_PatternRecursiveTest extends \PHPUnit_Framework_TestCase
 {
     /**

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/PatternTest.php b/tests/PHPUnit/Unit/DataTable/Filter/PatternTest.php
@@ -11,6 +11,9 @@
 use Piwik\DataTable;
 use Piwik\DataTable\Row;
 
+/**
+ * @group DataTableTest
+ */
 class DataTable_Filter_PatternTest extends \PHPUnit_Framework_TestCase
 {
     /**

diff --git a/tests/PHPUnit/Unit/DataTable/Filter/PivotByDimensionTest.php b/tests/PHPUnit/Unit/DataTable/Filter/PivotByDimensionTest.php
@@ -17,7 +17,7 @@
 use Exception;
 
 /**
- * @group Core
+ * @group DataTableTest
  */
 class PivotByDimensionTest extends PHPUnit_Framework_TestCase
 {