Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If no archives names are requested when querying archive data, do not initiate archiving. #17547

Merged
merged 2 commits into from May 11, 2021

Conversation

diosmosis
Copy link
Member

Description:

This solves a very convoluted bug involving CustomReports:

  • user creates a custom report with just unique visitors and places it in the visitors overview
  • user views a large range (eg, 2021-01-01,2021-03-31)
  • the first request on the page, for visits summary data succeeds creating plugin specific range archives with data
  • the custom report request is triggered
  • the custom report data is successfully requested
  • since its for a range, CustomReports decides we're not allowed to archive unique visitors (because of an INI config setting) and removes it from the columns_to_display
  • Visualization tries to get the site summary to build totals data
  • this issues an API request to API.get for a range, using columns_to_display, which is empty
  • API.get issues a request to VisitsSummary.get w/ 0 columns
  • VisitsSummary.get creates an Archive instance and calls getDataTableFromNumeric() with an empty array for $names
  • since it's a range, Archive.php tries to launch archiving. since no data is requested, there is no specific plugin to load, so we assume it's the all plugins archive. so we create a 'done' archive.
  • PluginsArchiver does nothing, since there is nothing being requested
  • then we finalize the 'done' archive w/ the DONE_OK flag
  • now on the next request, all archive requests to got the new 'done' archive for the range, even though it contains nothing
  • the user sees 0 for all metrics for the range

This is fixed in this PR by not launching archiving if nothing is requested in Archive.php.

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 diosmosis added the Needs Review PRs that need a code review label May 11, 2021
@diosmosis diosmosis added this to the 4.3.0 milestone May 11, 2021
core/Archive.php Outdated
@@ -493,6 +493,9 @@ protected function get($archiveNames, $archiveDataType, $idSubtable = null)

$result = new Archive\DataCollection(
$dataNames, $archiveDataType, $this->params->getIdSites(), $this->params->getPeriods(), $this->params->getSegment(), $defaultRow = null);
if (empty($archiveNames)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should that be dataNames? if not, could maybe exit right in the top? Also what if archiveNames is eg null or false would then this still be detected? or maybe not if it is [null]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be either. We need to return an empty DataCollection value so the result is properly created (eg, for DataTables we need correct metadata).

I'll add extra code for empty values.

@diosmosis diosmosis merged commit c0a78e0 into 4.x-dev May 11, 2021
@diosmosis diosmosis deleted the l3-83-empty-names-check branch May 11, 2021 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants