@diosmosis opened this Pull Request on May 3rd 2021 Member

Description:

In several places, matomo will look for the summary row if the label == -1. This causes problems (sometimes fatal) if data is tracked w/ the value -1. In this case there will be a normal row w/ the same label as the summary row, causing strange things to happen.

This PR fixes this by relying less on using the label of a row to identify if it is a summary row. Changes include:

  • do not index the summary row in the internal row label => row id index in Row
  • when getting the row from the label, check the row index first, and if it is not found there, and the label is -1, then return the summary row (this is mainly for BC)
  • when aggregating tables, aggregate the summary row specifically w/o searching for a row w/ a label in either table
  • add a flag to Row, isSummaryRow, which is set by DataTable. this is now returned from Row::isSummaryRow() instead of checking the label

Review

  • [ ] Functional review done
  • [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] Code review done
  • [ ] Tests were added if useful/possible
  • [ ] Reviewed for breaking changes
  • [ ] Developer changelog updated if needed
  • [ ] Documentation added if needed
  • [ ] Existing documentation updated if needed
@diosmosis commented on May 6th 2021 Member

@tsteur note: I didn't fix every place where the error could occur, since there would be a lot to look for, but I can if needed. Or I can create a new issue to make sure we don't use getRowFromLabel/getRowIdFromLabel w/ LABEL_SUMMARY_ROW.

@tsteur commented on May 6th 2021 Member

@diosmosis seems a lot of uses of the label constant are in other plugins anyway so that might be fine for now. I'm thinking though it might be useful to call setIsSummaryRow in https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/Actions/ArchivingHelper.php#L573 just to be safe and so isSummaryRow will work correctly?

Thinking https://github.com/matomo-org/matomo/blob/4.x-dev/core/DataTable.php#L1319-L1321 might need to be tweaked too to not break anything. Be good to check. Eg in truncate check there might be a setIsSummaryRow missing.

Also noticed there is a class DataTableSummaryRow could we set isSummaryRow=true there automatically?

I guess the other uses of the LABEL constant won't really cause any issues but be good to look over it.

I'm thinking a different issue might not be needed then.

@diosmosis commented on May 6th 2021 Member

@tsteur

I'm thinking though it might be useful to call setIsSummaryRow in https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/Actions/ArchivingHelper.php#L573 just to be safe and so isSummaryRow will work correctly?

We could do this, though it's added as the summary row here: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/Actions/ArchivingHelper.php#L388

If we do this for a row that isn't added with that method, however, it could cause other unforeseen problems, I think...

Thinking https://github.com/matomo-org/matomo/blob/4.x-dev/core/DataTable.php#L1319-L1321 might need to be tweaked too to not break anything. Be good to check. Eg in truncate check there might be a setIsSummaryRow missing.

Already saw this. This is fine since the Truncate filter just sets the label of a new summary row which it adds. It doesn't search for a row by that label.

Also noticed there is a class DataTableSummaryRow could we set isSummaryRow=true there automatically?

We could... however if the row is used as a normal row (for whatever reason) then it wouldn't be accurate and would lead to problems.

I guess the other uses of the LABEL constant won't really cause any issues but be good to look over it.

I did look at those, BUT I didn't look at every use of getRowFromLabel. It's possible code, for example, loops through every row and uses getRowFromLabel (in fact I think I found one place looking quickly a bit before):

foreach ($dataTable1->getRows() as $row) {
    $dataTable2->getRowFromLabel($row->getColumn('label'));
}

getRows() includes the summary row so this would have the same issue, but we wouldn't know since the label isn't explicitly used here. We can look for these too, but it would take more time.

@sgiehl commented on May 6th 2021 Member

Just tried local archiving with those changes, and the warning that was triggered before, isn't triggered anymore. But not yet sure if those changes might have other implications.

@diosmosis commented on May 7th 2021 Member

@tsteur @sgiehl I updated the PR, can you take another look and let me know if it's ok to merge or apply the fix on cloud?

@sgiehl commented on May 7th 2021 Member

The changes look good for me now. Not sure if we might have any issues with archives that have been built before, with mixed up row with -1 and summary row.

@diosmosis commented on May 7th 2021 Member

@sgiehl I think the tables themselves will be handled fine, they'll just have been summed incorrectly (eg, adding the -1 row to the summary row or the summary row to the -1 row). The data would be off but we can't fix that w/o invalidating.

@tsteur commented on May 10th 2021 Member

Looks good @diosmosis

We could do this, though it's added as the summary row here: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/Actions/ArchivingHelper.php#L388

I would maybe make that change just in case it'll be used differently in the future. Otherwise once tests pass all good 👍

This Pull Request was closed on May 11th 2021
Powered by GitHub Issue Mirror