Reporting API: showColumns and hideColumns should be applied recursively #11100
Conversation
mattab
added
the
Enhancement
…age generation time in Visitor Profile (#11095) * Live API: New 'generationTimeMilliseconds' field used to process Average generation time in Visitor Profile fixes #11066 * more precision * Expected test files * Reporting API: showColumns and hideColumns should be applied recursively (#11100) * Recursively delete columns specified with hideColumns * Custom Dimensions tests * Invalid argument supplied for foreach() + Contents tests * New unit test for ColumnDelete filter * Submodules
|
If I get this PR right then this is quite a big breaking change I'd say in a 3.0.1 release. Definitely need to mention it in CHANGELOG and need to test all premium features again etc. It can be especially critical when there are different structures and suddenly eg rootTable or subtable doesn't contain any data anymore etc. Like subtables have different columns etc and showColumns/deleteColumns is used. |
|
Such changes should always go through a review |
| */ | ||
| protected function removeColumnsFromTable(&$table) | ||
| { | ||
| if(!$this->isArrayAccess($table)) { |
There was a problem hiding this comment.
without it there's a notice. it's quite weird how this filter is used on both DataTable (normal flow) and arrays (in ResponseBuilder)
There was a problem hiding this comment.
It's not weird, it's the advantage of "interfaces" etc being able to treat them the same. It is now throwing notices probably since you start iterating nested on possible scalar values etc maybe
|
Also we return sometimes arrays with different structures in APIs. Eg like "getAvailablePatterns" and then root level of the array contains much different columns than children arrays. Ideally we also test Piwik Mobile with this change as it makes use of showColumns etc a lot to save data and for faster loading |
| } | ||
| } | ||
|
|
||
| $this->removeColumnsFromTable($table); |
There was a problem hiding this comment.
Shouldn't enableRecursive() take care of this by calling filtersubtable? The nested part
There was a problem hiding this comment.
The enableRecursive does not function when the filter is applied to an array.
| return; | ||
| } | ||
| foreach ($table as $index => &$row) { | ||
| if(!$this->isArrayAccess($row)) { |
There was a problem hiding this comment.
I presume you are doing this since it can now be a scalar etc. I'd definitely recommend testing performance for this. I remember from several performance profiles that showColumns/deleteColumns was always slow and one of the major contributors (like top 3 contributors) to slow performance in Piwik and now iterating over all array keys etc may make it much slower and we may want to find a different way for arrays vs dataTables etc.
|
Until performance is checked for this, and implications on breaking changes etc I'd rather revert this one |
|
Updated demo.piwik.org to run 3.0.1-b1 and just tested Piwik Mobile 2 which seems to work fine (reports, Visitor log, Real time map). Is there some feature in particular we could test? |
|
Need to test all features in Piwik Mobile and all premium features as it is a breaking API change and I can imagine that it may break something there. I can run the tests there later but should first also check re performance etc. Also it should be rather in a 3.1 release instead of 3.0.1. We didn't want to have any breaking changes in Piwik 3 and now we are starting to break in a patch release |
|
It shouldn't be a breaking change unless relying on showColumns / hideColumns to not be recursive... Just checked across premium plugins it seems we never use showColumns / hideColumns What are your concerns re: performance? it shouldn't have any performance impact as it wouldn't load new sub-tables on demand or so. Only apply filter to any in-memory report. Will add mention to Changelog and fix PSR. fyi: Reason this was added now is because I wanted to do this so many times when having to hide columns from API response (eg. in tests) wouldn't hide all columns... |
Yes this is the SLOW part. In some cases took like 15-20% of the time and now doing much more might make it even slower.
Calling the same API method the same way may return a different result. It is a breaking change by definition. Especially when having different data structures in root vs sub tables / arrays this can be a problem where it suddenly returns no data etc.
They are sometimes applied by visualizations in Piwik, export feature maybe etc and mobile app etc. |
|
see PR: #11106 - I did grep across whole codebase and concluded it appears to be safe and won't break our own plugins. Premium plugins should be especially safe unless I missed something... Would be good to re-run tests before release (due early Jan) |
|
Piwik is a platform that other people rely on. We shouldn't only check whether it is only safe for us ;) Will re-run tests end this or next week. |
mattab
added
the
not-in-changelog
|
Will be reverted in #11113 |
No description provided.