diff options
author | Kate Butler <kate@innocraft.com> | 2019-09-16 23:50:03 +0300 |
---|---|---|
committer | Thomas Steur <tsteur@users.noreply.github.com> | 2019-09-16 23:50:03 +0300 |
commit | 9f767b4f069adafbcddef2f4cf6c5a4a8333f68b (patch) | |
tree | 86c3c1cfedd89858b3d102f8f59cb14d19614d82 /plugins | |
parent | 5bb2cdaab831831f0e927de7857937230ca15b30 (diff) |
Tweak behaviour of orphaned segment archive purge (#14857)
Diffstat (limited to 'plugins')
-rw-r--r-- | plugins/CoreAdminHome/Tasks.php | 33 | ||||
-rw-r--r-- | plugins/CoreAdminHome/tests/Integration/TasksTest.php | 127 | ||||
-rw-r--r-- | plugins/SegmentEditor/Model.php | 81 | ||||
-rw-r--r-- | plugins/SegmentEditor/tests/Integration/ModelTest.php | 186 |
4 files changed, 274 insertions, 153 deletions
diff --git a/plugins/CoreAdminHome/Tasks.php b/plugins/CoreAdminHome/Tasks.php index ef9b0bd215..d94979c9c3 100644 --- a/plugins/CoreAdminHome/Tasks.php +++ b/plugins/CoreAdminHome/Tasks.php @@ -23,9 +23,9 @@ use Piwik\Piwik; use Piwik\Plugins\CoreAdminHome\Emails\JsTrackingCodeMissingEmail; use Piwik\Plugins\CoreAdminHome\Emails\TrackingFailuresEmail; use Piwik\Plugins\CoreAdminHome\Tasks\ArchivesToPurgeDistributedList; +use Piwik\Plugins\SegmentEditor\Model; use Piwik\Plugins\SitesManager\SitesManager; use Piwik\Scheduler\Schedule\SpecificTime; -use Piwik\Segment; use Piwik\Settings\Storage\Backend\MeasurableSettingsTable; use Piwik\Tracker\Failures; use Piwik\Site; @@ -275,7 +275,10 @@ class Tasks extends \Piwik\Plugin\Tasks */ public function purgeOrphanedArchives() { - $segmentHashesByIdSite = $this->getSegmentHashesByIdSite(); + $eightDaysAgo = Date::factory('now')->subDay(8); + $model = new Model(); + $deletedSegments = $model->getSegmentsDeletedSince($eightDaysAgo); + $archiveTables = ArchiveTableCreator::getTablesArchivesInstalled('numeric'); $datesPurged = array(); @@ -286,35 +289,15 @@ class Tasks extends \Piwik\Plugin\Tasks $dateObj = Date::factory("$year-$month-15"); $this->archivePurger->purgeDeletedSiteArchives($dateObj); - $this->archivePurger->purgeDeletedSegmentArchives($dateObj, $segmentHashesByIdSite); + if (count($deletedSegments)) { + $this->archivePurger->purgeDeletedSegmentArchives($dateObj, $deletedSegments); + } $datesPurged[$date] = true; } } /** - * Get a list of all segment hashes that currently exist, indexed by idSite. - * @return array - */ - public function getSegmentHashesByIdSite() - { - //Get a list of hashes of all segments that exist now - $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment') - . " WHERE deleted = 0"; - $rows = Db::fetchAll($sql); - $segmentHashes = array(); - foreach ($rows as $row) { - $idSite = (int)$row['enable_only_idsite']; - if (! isset($segmentHashes[$idSite])) { - $segmentHashes[$idSite] = array(); - } - $segmentHashes[$idSite][] = Segment::getSegmentHash($row['definition']); - } - - return $segmentHashes; - } - - /** * we should only purge outdated & custom range archives if we know cron archiving has just run, * or if browser triggered archiving is enabled. if cron archiving has run, then we know the latest * archives are in the database, and we can remove temporary ones. if browser triggered archiving is diff --git a/plugins/CoreAdminHome/tests/Integration/TasksTest.php b/plugins/CoreAdminHome/tests/Integration/TasksTest.php index 830da7ef97..a23d368de4 100644 --- a/plugins/CoreAdminHome/tests/Integration/TasksTest.php +++ b/plugins/CoreAdminHome/tests/Integration/TasksTest.php @@ -219,133 +219,6 @@ class TasksTest extends IntegrationTestCase $this->assertEquals(2, $mail->getNumFailures()); } - public function test_getSegmentHashesByIdSite_emptyWhenNoSegments() - { - $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite(); - $this->assertEquals(array(), $segmentsByIdSite); - } - - public function test_getSegmentHashesByIdSite_allWebsiteAndSiteSpecificSegments() - { - $model = new Model(); - $model->createSegment(array( - 'name' => 'Test Segment 1', - 'definition' => 'continentCode==eur', - 'enable_only_idsite' => 0, - 'deleted' => 0 - )); - $model->createSegment(array( - 'name' => 'Test Segment 2', - 'definition' => 'countryCode==nz', - 'enable_only_idsite' => 0, - 'deleted' => 0 - )); - $model->createSegment(array( - 'name' => 'Test Segment 3', - 'definition' => 'countryCode==au', - 'enable_only_idsite' => 2, - 'deleted' => 0 - )); - - $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite(); - $expected = array( - 0 => array('be90051048558489e1d62f4245a6dc65', 'b92fbb3009b32cf632965802de2fb760'), - 2 => array('cffd4336c22c6782211f853495076b1a') - ); - $this->assertEquals($expected, $segmentsByIdSite); - } - - public function test_getSegmentHashesByIdSite_deletedSegment() - { - $model = new Model(); - $model->createSegment(array( - 'name' => 'Test Segment 1', - 'definition' => 'continentCode==eur', - 'enable_only_idsite' => 0, - 'deleted' => 0 - )); - $model->createSegment(array( - 'name' => 'Test Segment 2', - 'definition' => 'countryCode==nz', - 'enable_only_idsite' => 0, - 'deleted' => 1 - )); - $model->createSegment(array( - 'name' => 'Test Segment 3', - 'definition' => 'countryCode==au', - 'enable_only_idsite' => 2, - 'deleted' => 0 - )); - - $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite(); - $expected = array( - 0 => array('be90051048558489e1d62f4245a6dc65'), - 2 => array('cffd4336c22c6782211f853495076b1a') - ); - $this->assertEquals($expected, $segmentsByIdSite); - } - - public function test_getSegmentHashesByIdSite_invalidSegment() - { - $model = new Model(); - $model->createSegment(array( - 'name' => 'Test Segment 4', - 'definition' => 'countryCode=nz', //The single "=" is invalid - we should generate a hash anyway - 'enable_only_idsite' => 0, - 'deleted' => 0 - )); - $model->createSegment(array( - 'name' => 'Test Segment 5', - 'definition' => 'countryCode==au', - 'enable_only_idsite' => 0, - 'deleted' => 0 - )); - - $expected = array( - 0 => array('5ffe7e116fae7576c047b1fb811584a5', 'cffd4336c22c6782211f853495076b1a'), - ); - - $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite(); - $this->assertEquals($expected, $segmentsByIdSite); - } - - public function test_getSegmentHashesByIdSite_siteSpecificCustomDimension() - { - // Insert a custom dimension for idsite = 1 - $configuration = new Configuration(); - $configuration->configureNewDimension( - 1, - 'mydimension', - CustomDimensions::SCOPE_VISIT, - 1, - 1, - array(), - true - ); - - $model = new Model(); - $model->createSegment(array( - 'name' => 'Test Segment 6', - 'definition' => 'mydimension==red', - 'enable_only_idsite' => 1, - 'deleted' => 0 - )); - $model->createSegment(array( - 'name' => 'Test Segment 7', - 'definition' => 'countryCode==au', - 'enable_only_idsite' => 2, - 'deleted' => 0 - )); - - $expected = array( - 1 => array('240d2a84a309debd26bdbaa8eb3d363c'), - 2 => array('cffd4336c22c6782211f853495076b1a') - ); - - $segmentsByIdSite = $this->tasks->getSegmentHashesByIdSite(); - $this->assertEquals($expected, $segmentsByIdSite); - } - /** * @param Date[] $dates */ diff --git a/plugins/SegmentEditor/Model.php b/plugins/SegmentEditor/Model.php index 0025c7c15c..c505f7a4f2 100644 --- a/plugins/SegmentEditor/Model.php +++ b/plugins/SegmentEditor/Model.php @@ -129,6 +129,87 @@ class Model return $segment; } + /** + * Gets a list of segments that have been deleted in the last week and therefore may have orphaned archives. + * @param Date $date Segments deleted on or after this date will be returned. + * @return array of segments. The segments are only populated with the fields needed for archive invalidation + * (e.g. definition, enable_only_idsite). + * @throws \Exception + */ + public function getSegmentsDeletedSince(Date $date) + { + $dateStr = $date->getDatetime(); + $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment') + . " WHERE deleted = 1 AND ts_last_edit >= ?"; + $deletedSegments = Db::fetchAll($sql, array($dateStr)); + + if (empty($deletedSegments)) { + return array(); + } + + $existingSegments = $this->getExistingSegmentsLike($deletedSegments); + + foreach ($deletedSegments as $i => $deleted) { + $deletedSegments[$i]['idsites_to_preserve'] = array(); + foreach ($existingSegments as $existing) { + if ($existing['definition'] != $deleted['definition'] && + $existing['definition'] != urlencode($deleted['definition']) && + $existing['definition'] != urldecode($deleted['definition']) + ) { + continue; + } + + if ( + $existing['enable_only_idsite'] == $deleted['enable_only_idsite'] + || $existing['enable_only_idsite'] == 0 + ) { + // There is an identical segment (for either the specific site or for all sites) that is active + // The archives for this segment will therefore still be needed + unset($deletedSegments[$i]); + break; + } elseif ($deleted['enable_only_idsite'] == 0) { + // It is an all-sites segment that got deleted, but there is a single-site segment that is active + // Need to make sure we don't erase the segment's archives for that particular site + $deletedSegments[$i]['idsites_to_preserve'][] = $existing['enable_only_idsite']; + } + } + } + + return $deletedSegments; + } + + private function getExistingSegmentsLike(array $segments) + { + if (empty($segments)) { + return array(); + } + + $whereClauses = array(); + $bind = array(); + $definitionWhereClauseTemplate = '(definition = ? OR definition = ? OR definition = ?)'; + foreach ($segments as $segment) { + // Sometimes they are stored encoded and sometimes they aren't + $bind[] = $segment['definition']; + $bind[] = urlencode($segment['definition']); + $bind[] = urldecode($segment['definition']); + + if ($segment['enable_only_idsite'] == 0) { + // They deleted an all-sites segment, but there is a single-site segment with same definition? + // Need to handle this carefully so that the archives for the single-site segment are preserved + $whereClauses[] = "$definitionWhereClauseTemplate"; + } else { + $whereClauses[] = "($definitionWhereClauseTemplate AND (enable_only_idsite = ? OR enable_only_idsite = 0))"; + $bind[] = $segment['enable_only_idsite']; + } + } + $whereClauses = implode(' OR ', $whereClauses); + + // Check for any non-deleted segments with the same definition + $sql = "SELECT DISTINCT definition, enable_only_idsite FROM " . Common::prefixTable('segment') + . " WHERE deleted = 0 AND (" . $whereClauses . ")"; + return Db::fetchAll($sql, $bind); + } + public function deleteSegment($idSegment) { $fieldsToSet = array( diff --git a/plugins/SegmentEditor/tests/Integration/ModelTest.php b/plugins/SegmentEditor/tests/Integration/ModelTest.php index 92034f3efc..f309221c8f 100644 --- a/plugins/SegmentEditor/tests/Integration/ModelTest.php +++ b/plugins/SegmentEditor/tests/Integration/ModelTest.php @@ -25,6 +25,8 @@ class ModelTest extends IntegrationTestCase private $idSegment3; + private $idSegment4; + public function setUp() { parent::setUp(); @@ -55,7 +57,11 @@ class ModelTest extends IntegrationTestCase { parent::tearDown(); // Force a hard delete of segment - $idsToDelete = $this->idSegment1 . ', ' . $this->idSegment2 . ', ' . $this->idSegment3; + $idsToDelete = array($this->idSegment1, $this->idSegment2, $this->idSegment3); + if ($this->idSegment4) { + $idsToDelete[] = $this->idSegment4; + } + $idsToDelete = implode(',', $idsToDelete); Db::query( "DELETE FROM " . Common::prefixTable('segment') . " WHERE idsegment IN ($idsToDelete)" ); @@ -146,6 +152,184 @@ class ModelTest extends IntegrationTestCase $this->assertEmpty($segment); } + public function test_getSegmentsDeletedSince_noDeletedSegments() + { + $date = Date::factory('now'); + $segments = $this->model->getSegmentsDeletedSince($date); + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_oneDeletedSegment() + { + $this->model->deleteSegment($this->idSegment3); + + $date = Date::factory('now')->subDay(1); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertCount(1, $segments); + $this->assertEquals('country==Hobbiton', $segments[0]['definition']); + } + + public function test_getSegmentsDeletedSince_segmentDeletedTooLongAgo() + { + // Manually delete it to set timestamp 9 days in past + $deletedAt = Date::factory('now')->subDay(9)->toString('Y-m-d H:i:s'); + $this->model->updateSegment($this->idSegment1, array( + 'deleted' => 1, + 'ts_last_edit' => $deletedAt + )); + + // The segment deleted above should not be included as it was more than 8 days ago + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_duplicateSegment() + { + // Turn segment1 into a duplicate of segment2, except it's also deleted + $this->model->updateSegment($this->idSegment1, array( + 'definition' => 'country==Genovia', + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_duplicateSegmentDifferentIdSite() + { + // Turn segment2 into a duplicate of segment3, except for a different idsite and also deleted + $this->model->updateSegment($this->idSegment2, array( + 'definition' => 'country==Hobbiton', + 'enable_only_idsite' => 2, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertCount(1, $segments); + $this->assertEquals('country==Hobbiton', $segments[0]['definition']); + $this->assertEquals(2, $segments[0]['enable_only_idsite']); + } + + public function test_getSegmentsDeletedSince_duplicateSegmentAllSitesAndSingleSite() + { + // Turn segment2 into a duplicate of segment3, except for all sites and also deleted + $this->model->updateSegment($this->idSegment2, array( + 'definition' => 'country==Hobbiton', + 'enable_only_idsite' => 0, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertCount(1, $segments); + $this->assertEquals('country==Hobbiton', $segments[0]['definition']); + $this->assertEquals(0, $segments[0]['enable_only_idsite']); + $this->assertEquals(array(1), $segments[0]['idsites_to_preserve']); + } + + public function test_getSegmentsDeletedSince_duplicateSegmentSingleSiteAndAllSites() + { + // Turn segment3 into a duplicate of segment1, except for a single site and deleted + $this->model->updateSegment($this->idSegment3, array( + 'definition' => 'country==Narnia', + 'enable_only_idsite' => 1, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + // There is still a live segment for all sites, so the deleted site-specific one is ignored + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_ExistingSiteSpecificAndAllSitesMatch() + { + // A deleted all-sites segment, with both an all-sites and a site-specific segment still present + $this->model->updateSegment($this->idSegment1, array( + 'definition' => 'actions >= 1', + 'enable_only_idsite' => 0, + 'deleted' => 0, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + $this->model->updateSegment($this->idSegment2, array( + 'definition' => 'actions >= 1', + 'enable_only_idsite' => 0, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + $this->model->updateSegment($this->idSegment3, array( + 'definition' => 'actions >= 1', + 'enable_only_idsite' => 3, + 'deleted' => 0, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + $this->idSegment4 = $this->model->createSegment(array( + 'definition' => 'actions >= 1', + 'enable_only_idsite' => 1, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_urlDecodedVersionOfSegment() + { + // Turn segment2 into a duplicate of segment3, except a urlencoded version + $this->model->updateSegment($this->idSegment2, array( + 'definition' => 'country%3D%3DHobbiton', + 'enable_only_idsite' => 1, + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + // The two encoded and decoded version of the segments should be treated as duplicates + // This means there segment has a non-deleted version so it's not returned + $this->assertEmpty($segments); + } + + public function test_getSegmentsDeletedSince_urlEncodedVersionOfSegment() + { + // segment1 => url decoded version, deleted + $this->model->updateSegment($this->idSegment1, array( + 'definition' => 'country==Narnia', + 'deleted' => 1, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + // segment2 => url encoded version, not deleted + $this->model->updateSegment($this->idSegment2, array( + 'definition' => 'country%3D%3DNarnia', + 'deleted' => 0, + 'ts_last_edit' => Date::factory('now')->toString('Y-m-d H:i:s') + )); + + $date = Date::factory('now')->subDay(8); + $segments = $this->model->getSegmentsDeletedSince($date); + + // The two encoded and decoded version of the segments should be treated as duplicates + // This means there segment has a non-deleted version so it's not returned + $this->assertEmpty($segments); + } + private function assertReturnedIdsMatch(array $expectedIds, array $resultSet) { $this->assertEquals(count($expectedIds), count($resultSet)); |