Bug in index the label of DataTable_Row in file DataTable.php

[jetuelle]

The line "$this->rowsIndexByLabel[$label] = count($this->rows)-1;" in function

"public function addRow( Piwik_DataTable_Row $row )" doesn't work correctly. For example:

$rows ( 1=>"AA", 2=>"BB", 3=>"CC", 4=>"DD", 5=>"EE", 6=>"FF"); note:1=>"AA": "1" is the index id of label "AA"

delete $rows[3], we get:

$rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF");

addRow($newrow),the label of $newrow is "GG"

The result should be: $rows ( 1=>"AA", 2=>"BB", 4=>"DD", 5=>"EE", 6=>"FF", 7=>"GG");

while according to function addRow(), the index id of $newrow's label("GG") is

$this->rowsIndexByLabel[$label] = count($this->rows)-1 =5-1=4,

but the right index id of label "GG" is 7.

We find the function rebuildIndex() useless after we change the function addRow().

[jetuelle]

The code change contains the following three points:

1、（1）original code(4 functions):

public function addRow( Piwik_DataTable_Row $row )

{

$this->rows[] = $row;

if(!$this->indexNotUpToDate

&& $this->rebuildIndexContinuously)

{

$label = $row->getColumn('label'); if($label !== false) {

$this->rowsIndexByLabel[$label] = count($this->rows)-1;

} $this->indexNotUpToDate = false;

}

}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2) {

$rows1 = $table1->getRows(); $rows2 = $table2->getRows();

$table1->rebuildIndex(); $table2->rebuildIndex();

if($table1->getRowsCount() != $table2->getRowsCount()) {

return false;

}

foreach($rows1 as $row1) {

$row2 = $table2->getRowFromLabel($row1->getColumn('label')); if($row2 === false

!Piwik_DataTable_Row::isEqual($row1,$row2))

{

return false;

}

} return true;

}

public function getRowFromLabel( $label ) {

$this->rebuildIndexContinuously = true; if($this->indexNotUpToDate) {

$this->rebuildIndex();

}

if($label === self::LABEL_SUMMARY_ROW

&& !is_null($this->summaryRow))

{

return $this->summaryRow;

}

$label = (string)$label; if(!isset($this->rowsIndexByLabel[$label])) {

return false;

} return $this->rows[$this->rowsIndexByLabel[$label]];

}

public function sort( $functionCallback, $columnSortedBy )

{

$this->indexNotUpToDate = true; $this->tableSortedBy = $columnSortedBy; usort( $this->rows, $functionCallback );

if($this->enableRecursiveSort === true) {

foreach($this->getRows() as $row) {

if(($idSubtable = $row->getIdSubDataTable()) !== null) {

$table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable); $table->enableRecursiveSort(); $table->sort($functionCallback, $columnSortedBy);

}

}

}

}

（2）changed code(4 functions): public function addRow( Piwik_DataTable_Row $row )

{

$this->rows[] = $row;

$label = $row->getColumn('label'); if($label !== false) {

$this->rowsIndexByLabel[$label] = $this->nextRowId; $this->nextRowId++;

}

}

static public function isEqual(Piwik_DataTable $table1, Piwik_DataTable $table2) {

$rows1 = $table1->getRows(); $rows2 = $table2->getRows();

if($table1->getRowsCount() != $table2->getRowsCount()) {

return false;

}

foreach($rows1 as $row1) {

$row2 = $table2->getRowFromLabel($row1->getColumn('label')); if($row2 === false

!Piwik_DataTable_Row::isEqual($row1,$row2))

{

return false;

}

} return true;

}

public function getRowFromLabel( $label ) {

if($label === self::LABEL_SUMMARY_ROW

&& !is_null($this->summaryRow))

{

return $this->summaryRow;

}

$label = (string)$label; if(!isset($this->rowsIndexByLabel[$label])) {

return false;

} return $this->rows[$this->rowsIndexByLabel[$label]];

}

public function sort( $functionCallback, $columnSortedBy )

{

$this->tableSortedBy = $columnSortedBy; usort( $this->rows, $functionCallback );

if($this->enableRecursiveSort === true) {

foreach($this->getRows() as $row) {

if(($idSubtable = $row->getIdSubDataTable()) !== null) {

$table = Piwik_DataTable_Manager::getInstance()->getTable($idSubtable); $table->enableRecursiveSort(); $table->sort($functionCallback, $columnSortedBy);

}

}

}

}

2、added codes:

protected $nextRowId = 0;

3、deleted code:

protected $indexNotUpToDate = true;

protected $rebuildIndexContinuously = false;

private function rebuildIndex() {

foreach($this->rows as $id => $row) {

$label = $row->getColumn('label'); if($label !== false) {

$this->rowsIndexByLabel[$label] = $id;

}

} $this->indexNotUpToDate = false;

}

[vipsoft]

Can you upload this as a patch? (use "svn diff")

[matt]

Also if you can please submit a unit test to show that there is a bug somewhere? the unit test for the dataTable is:  http://dev.piwik.org/trac/browser/trunk/tests/core/DataTable.test.php

[matt]

jetuelle, please post a patch, your code is very hard to read in its current form.

rebuildIndex is not useless, it is helpful to access quickly rows with a given label instead of looping through the hundreds of rows in the datatable.

[jetuelle]

The patch file for DataTable.php

[jetuelle]

The file with change by jetuelle

[jetuelle]

The diff file of DataTable.php

[jetuelle]

The patch file for DataTable.php

[matt]

Marking as invalid. Using delete $rows[3] is not supported, you should use $dataTable->deleteRow( $idRow ); instead.

If you wish to prove a bug, please provide a new unit test to  http://dev.piwik.org/trac/browser/trunk/tests/core/DataTable.test.php that would revel the bug. thx