Opened 21 months ago

Closed 20 months ago

Last modified 20 months ago

#3300 closed New feature (fixed)

New API parameter: hideColumns= to specify a list of columns not to return

Reported by: matt Owned by: capedfuzz
Priority: normal Milestone: 1.12.x - Piwik 1.12.x
Component: Core Keywords:
Cc: Sensitive: no

Description

In:
http://demo.piwik.org/?module=API&method=API.getReportMetadata&idSites=&period=day&date=today&format=xml&token_auth=anonymous&hideColumns=metricsDocumentation

or http://demo.piwik.org/?module=API&method=API.getProcessedReport&idSite=7&period=day&date=today&apiModule=UserCountry&apiAction=getCountry&showTimer=1&format=xml&token_auth=anonymous

It would be very useful to be able to NOT return the <metricsDocumentation> field since it takes much space. For Piwik Mobile transfert speed is key and extra data such as this makes the app much slower than necessary, esp. since Mobile phone internet trasnafer speed is often slow.

The new parameter would at least work for these API above, but it would be nice to have a generic parameter hideColumns that would work in all APIs automatically.

Attachments (3)

3300.diff.tar.gz (2.0 KB) - added by capedfuzz 21 months ago.
Patch for this issue (implements filter_hide_columns/filter_show_columns).
3300.diff.tar.2.gz (3.8 KB) - added by capedfuzz 21 months ago.
New patch for hide/show columns.
3300.diff.tar.3.gz (3.8 KB) - added by capedfuzz 20 months ago.
Another patch.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 21 months ago by capedfuzz (diosmosis)

What about a parameter that lists columns to show (ie, filter_show_columns)? This way new columns won't result in performance degradations.

For metric documentation, since it's always the same, perhaps it could be moved to another API function 'getMetricDocumentation'? Doesn't make sense to return it w/ every report.

What do you think of these ideas?

comment:2 Changed 21 months ago by matt (mattab)

That could be a simpler solution indeed, to return metrics documentation in a different API...

listing columns to show might be more tricky. OK to move the <metricsDocumentation> to a separate API call :)

I'll ask Thomas if that'd work

comment:3 Changed 21 months ago by capedfuzz (diosmosis)

I've been thinking about this some more, and I've revised my solution. Using one getMetricsDocumentation function won't work since the docs can differ based on plugin (for example, the Actions plugin defines a different documentation for nb_visits than the default docs). Instead I think we should do the following:

  • Expose getDefaultMetricsDocumentation as an API function.
  • Modify getProcessedReport to return no metrics documentation.
  • Modify getMetadata & getReportMetadata to return only custom metrics documentation (so they won't return the default docs).

Do you think this will work?

comment:4 Changed 21 months ago by capedfuzz (diosmosis)

Actually, just came up w/ a better idea, I'll upload a patch in a bit.

comment:5 Changed 21 months ago by matt (mattab)

I think it's simpler to simply have a hideMetricsDoc=1 parameter?
the goal is really to make Piwik Mobile app faster and this is the onyl piece of data unused, so it's better / OK to do the quick fix here :)

comment:6 Changed 21 months ago by capedfuzz (diosmosis)

Yes that was my idea :)

comment:7 Changed 21 months ago by capedfuzz (diosmosis)

(In [6682]) Refs #3300, added hideMetricsDoc parameter to metadata API actions that can return metrics documentation (such as getProcessedReport, getReportMetadata & getMetadata).

comment:8 Changed 21 months ago by capedfuzz (diosmosis)

(In [6684]) Refs #3300, test hideMetricsDoc w/ getMetadata instead of getReportMetadata.

Changed 21 months ago by capedfuzz (diosmosis)

Patch for this issue (implements filter_hide_columns/filter_show_columns).

comment:9 Changed 21 months ago by capedfuzz (diosmosis)

I've put up a patch that implements the other part of this issue. It will allow you to use two new filter parameters: filter_hide_columns/filter_show_columns that will remove columns from the API result.

One thing to note: This patch allows filter_hide_columns or filter_show_columns to be specified. The way I've implemented this in the patch will result in the RemoveColumns filter running even if the parameters aren't supplied. It shouldn't result in any added overhead though, since nothing will happen when there are no parameters. But if it should be changed, I can fix it by creating two small sub-classes (RemoveColumnsWhitelist/RemoveColumnsBlacklist) in RemoveColumns.php.

Let me know what you think.

comment:10 Changed 21 months ago by matt (mattab)

  • Owner set to capedfuzz

comment:11 follow-up: Changed 21 months ago by matt (mattab)

Cool feature.

  • DataTable->deleteColumn(s) should be used to delete columns
  • at some point we should think of renaming filters, I dislike the name filter_xx for existing ones... for example this could be exposed via hideColumns showColumns
  • Tests: can you add a test of filtering out columns in processed reports: should it remove first level "entities" or simply remove columsn from the reportData ? It should remove the columns from the report itself (so expected that hideColumns=metricsDocumentation does not work)
  • Tests: add a test of expanded=1 and showing column is removed recursively (done via deleteColumn())

Changed 21 months ago by capedfuzz (diosmosis)

New patch for hide/show columns.

comment:12 in reply to: ↑ 11 Changed 21 months ago by capedfuzz (diosmosis)

I uploaded a new patch.

Replying to matt:

Cool feature.

  • DataTable->deleteColumn(s) should be used to delete columns

This didn't work too well w/ showColumns so I took a different approach to make sure subtables got filtered.

  • at some point we should think of renaming filters, I dislike the name filter_xx for existing ones... for example this could be exposed via hideColumns showColumns
  • Tests: can you add a test of filtering out columns in processed reports: should it remove first level "entities" or simply remove columsn from the reportData ? It should remove the columns from the report itself (so expected that hideColumns=metricsDocumentation does not work)

I got this to work, but I had to add a bit of a hack to API/API.php.

  • Tests: add a test of expanded=1 and showing column is removed recursively (done via deleteColumn())

Added :). Let me know what you think of this patch.

One other thing to note: Right now, this filter won't work w/ some columns, since they are added via DataTable::queueFilter. At the moment, queued filters are run after generic filters, so it won't affect them. Should this change?

comment:13 follow-up: Changed 21 months ago by matt (mattab)

Thanks for the new patch, the code is much better indeed :-)

Review:

  • What happens if all columns removed? or if column specified is not showing? throw exception?
  • hack in API.php OK :) we don't have much choice indeed!
  • Tests look good!
  • The parameters must work with processed metrics indeed. Maybe the filter should not be generic but be applied manually here
    Index: core/API/ResponseBuilder.php
    ===================================================================
    --- core/API/ResponseBuilder.php	(revision 6707)
    +++ core/API/ResponseBuilder.php	(working copy)
    @@ -301,6 +301,8 @@
     			$datatable->applyQueuedFilters();
     		}
     		
    +		// HERE call RemoveColumns filter manually?
    +		
     		// if requested, flatten nested tables
     		if (Piwik_Common::getRequestVar('flat', '0', 'string', $this->request) == '1')
     		{
    
  • in tests can you please add more excluded columns, so the output is more simple even? (it's better that tests fail less often but would still fail whenever needed)
  • Can you add a test when all columns excluded or column showColumns= does not exist?
  • Also add processed metrics in the tests :)

Changed 20 months ago by capedfuzz (diosmosis)

Another patch.

comment:14 in reply to: ↑ 13 Changed 20 months ago by capedfuzz (diosmosis)

Replying to matt:

Thanks for the new patch, the code is much better indeed :-)

Review:

  • What happens if all columns removed? or if column specified is not showing? throw exception?

If hideColumns specifies every column that is to be returned, an empty result is returned. If showColumns is empty, nothing happens. Do you think these are appropriate behaviors?

  • hack in API.php OK :) we don't have much choice indeed!
  • Tests look good!
  • The parameters must work with processed metrics indeed. Maybe the filter should not be generic but be applied manually here
    Index: core/API/ResponseBuilder.php
    ===================================================================
    --- core/API/ResponseBuilder.php	(revision 6707)
    +++ core/API/ResponseBuilder.php	(working copy)
    @@ -301,6 +301,8 @@
     			$datatable->applyQueuedFilters();
     		}
     		
    +		// HERE call RemoveColumns filter manually?
    +		
     		// if requested, flatten nested tables
     		if (Piwik_Common::getRequestVar('flat', '0', 'string', $this->request) == '1')
     		{
    

This is what I ended up doing. It won't work for ViewDataTable, but I'm not sure that's an issue.

  • in tests can you please add more excluded columns, so the output is more simple even? (it's better that tests fail less often but would still fail whenever needed)
  • Can you add a test when all columns excluded or column showColumns= does not exist?
  • Also add processed metrics in the tests :)

I've added a new patch w/ these tests. Let me know if it's ok to commit.

comment:15 follow-up: Changed 20 months ago by matt (mattab)

Feedback:

  • in handleTableReport(), refactor the code dealing with columns in a new private method to keep things tidy
  • in one of the tests, can you add for both hide/show a column name that does not exist, to check the code doesn't trigger errors/notices?

Looks good to commit!

comment:16 Changed 20 months ago by capedfuzz (diosmosis)

  • Resolution set to fixed
  • Status changed from new to closed

(In [6836]) Fixes #3300, added hideColumns/showColumns query parameters to customize API output & modified ColumnDelete filter to be more robust.

comment:17 in reply to: ↑ 15 Changed 20 months ago by capedfuzz (diosmosis)

Replying to matt:

Feedback:

  • in handleTableReport(), refactor the code dealing with columns in a new private method to keep things tidy
  • in one of the tests, can you add for both hide/show a column name that does not exist, to check the code doesn't trigger errors/notices?

I already do this (see 'nb_hits' in hide/showColumns in the VisitsSummary.get tests).

Looks good to commit!

I noticed that there was already a ColumnDelete filter, so I removed my RemoveColumns filter and augmented that one before I committed.

comment:18 Changed 20 months ago by capedfuzz (diosmosis)

(In [6837]) Refs #3300, tweak to code comment

Note: See TracTickets for help on using tickets.