From ec5e4f458179689700e83940d6c74d3413f9c274 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 15 Apr 2016 15:46:19 +1200 Subject: [PATCH] BUG Fix versioned writes where subtables have no fields key BUG Remove unnecessary config nesting in tests which are now handled via core Fixes some regressions in recent framework fixes to versioned --- code/search/SearchUpdater.php | 13 +++++++--- tests/BatchedProcessorTest.php | 31 +++++++++++------------ tests/SearchUpdaterTest.php | 11 +-------- tests/SearchVariantVersionedTest.php | 20 +++++++-------- tests/SolrIndexTest.php | 6 ++--- tests/SolrIndexVersionedTest.php | 37 +++++++++++++--------------- 6 files changed, 54 insertions(+), 64 deletions(-) diff --git a/code/search/SearchUpdater.php b/code/search/SearchUpdater.php index 4a9dcde..4acd075 100644 --- a/code/search/SearchUpdater.php +++ b/code/search/SearchUpdater.php @@ -90,14 +90,14 @@ class SearchUpdater extends Object $writes = array(); foreach ($manipulation as $table => $details) { - if (!isset($details['id']) || !isset($details['fields'])) { + if (!isset($details['id'])) { continue; } $id = $details['id']; $state = $details['state']; $class = $details['class']; - $fields = $details['fields']; + $fields = isset($details['fields']) ? $details['fields'] : array(); $base = ClassInfo::baseDataClass($class); $key = "$id:$base:".serialize($state); @@ -125,6 +125,13 @@ class SearchUpdater extends Object } } + // Trim records without fields + foreach(array_keys($writes) as $key) { + if(empty($writes[$key]['fields'])) { + unset($writes[$key]); + } + } + // Then extract any state that is needed for the writes SearchVariant::call('extractManipulationWriteState', $writes); @@ -136,7 +143,7 @@ class SearchUpdater extends Object /** * Send updates to the current search processor for execution - * + * * @param array $writes */ public static function process_writes($writes) diff --git a/tests/BatchedProcessorTest.php b/tests/BatchedProcessorTest.php index 65d7c55..6978d3e 100644 --- a/tests/BatchedProcessorTest.php +++ b/tests/BatchedProcessorTest.php @@ -42,7 +42,7 @@ class BatchedProcessor_QueuedJobService class BatchedProcessorTest extends SapphireTest { protected $oldProcessor; - + protected $extraDataObjects = array( 'BatchedProcessorTest_Object' ); @@ -66,8 +66,7 @@ class BatchedProcessorTest extends SapphireTest public function setUp() { parent::setUp(); - Config::nest(); - + if (!interface_exists('QueuedJob')) { $this->skipTest = true; $this->markTestSkipped("These tests need the QueuedJobs module installed to run"); @@ -79,11 +78,11 @@ class BatchedProcessorTest extends SapphireTest } SS_Datetime::set_mock_now('2015-05-07 06:00:00'); - + Config::inst()->update('SearchUpdateBatchedProcessor', 'batch_size', 5); Config::inst()->update('SearchUpdateBatchedProcessor', 'batch_soft_cap', 0); Config::inst()->update('SearchUpdateCommitJobProcessor', 'cooldown', 600); - + Versioned::reading_stage("Stage"); Injector::inst()->registerService(new BatchedProcessor_QueuedJobService(), 'QueuedJobService'); @@ -92,7 +91,7 @@ class BatchedProcessorTest extends SapphireTest SearchUpdateCommitJobProcessor::$dirty_indexes = array(); SearchUpdateCommitJobProcessor::$has_run = false; - + $this->oldProcessor = SearchUpdater::$processor; SearchUpdater::$processor = new SearchUpdateQueuedJobProcessor(); } @@ -102,8 +101,6 @@ class BatchedProcessorTest extends SapphireTest if ($this->oldProcessor) { SearchUpdater::$processor = $this->oldProcessor; } - Config::unnest(); - Injector::inst()->unregisterNamedObject('QueuedJobService'); FullTextSearch::force_index_list(); parent::tearDown(); } @@ -132,7 +129,7 @@ class BatchedProcessorTest extends SapphireTest $processor->batchData(); return $processor; } - + /** * Tests that large jobs are broken up into a suitable number of batches */ @@ -141,14 +138,14 @@ class BatchedProcessorTest extends SapphireTest $index = singleton('BatchedProcessorTest_Index'); $index->reset(); $processor = $this->generateDirtyIds(); - + // Check initial state $data = $processor->getJobData(); $this->assertEquals(9, $data->totalSteps); $this->assertEquals(0, $data->currentStep); $this->assertEmpty($data->isComplete); $this->assertEquals(0, count($index->getAdded())); - + // Advance state for ($pass = 1; $pass <= 8; $pass++) { $processor->process(); @@ -156,7 +153,7 @@ class BatchedProcessorTest extends SapphireTest $this->assertEquals($pass, $data->currentStep); $this->assertEquals($pass * 5, count($index->getAdded())); } - + // Last run should have two hanging items $processor->process(); $data = $processor->getJobData(); @@ -218,7 +215,7 @@ class BatchedProcessorTest extends SapphireTest ); } - + /** * Tests that the batch_soft_cap setting is properly respected */ @@ -227,26 +224,26 @@ class BatchedProcessorTest extends SapphireTest $index = singleton('BatchedProcessorTest_Index'); $index->reset(); $processor = $this->generateDirtyIds(); - + // Test that increasing the soft cap to 2 will reduce the number of batches Config::inst()->update('SearchUpdateBatchedProcessor', 'batch_soft_cap', 2); $processor->batchData(); $data = $processor->getJobData(); //Debug::dump($data);die; $this->assertEquals(8, $data->totalSteps); - + // A soft cap of 1 should not fit in the hanging two items Config::inst()->update('SearchUpdateBatchedProcessor', 'batch_soft_cap', 1); $processor->batchData(); $data = $processor->getJobData(); $this->assertEquals(9, $data->totalSteps); - + // Extra large soft cap should fit both items Config::inst()->update('SearchUpdateBatchedProcessor', 'batch_soft_cap', 4); $processor->batchData(); $data = $processor->getJobData(); $this->assertEquals(8, $data->totalSteps); - + // Process all data and ensure that all are processed adequately for ($pass = 1; $pass <= 8; $pass++) { $processor->process(); diff --git a/tests/SearchUpdaterTest.php b/tests/SearchUpdaterTest.php index dd0e006..d2211c0 100644 --- a/tests/SearchUpdaterTest.php +++ b/tests/SearchUpdaterTest.php @@ -74,7 +74,7 @@ class SearchUpdaterTest extends SapphireTest protected $usesDatabase = true; private static $index = null; - + public function setUp() { parent::setUp(); @@ -87,8 +87,6 @@ class SearchUpdaterTest extends SapphireTest SearchUpdater::bind_manipulation_capture(); - Config::nest(); - Config::inst()->update('Injector', 'SearchUpdateProcessor', array( 'class' => 'SearchUpdateImmediateProcessor' )); @@ -97,13 +95,6 @@ class SearchUpdaterTest extends SapphireTest SearchUpdater::clear_dirty_indexes(); } - public function tearDown() - { - Config::unnest(); - - parent::tearDown(); - } - public function testBasic() { $item = new SearchUpdaterTest_Container(); diff --git a/tests/SearchVariantVersionedTest.php b/tests/SearchVariantVersionedTest.php index 2774901..76747f5 100644 --- a/tests/SearchVariantVersionedTest.php +++ b/tests/SearchVariantVersionedTest.php @@ -2,6 +2,9 @@ class SearchVariantVersionedTest extends SapphireTest { + /** + * @var SearchVariantVersionedTest_Index + */ private static $index = null; protected $extraDataObjects = array( @@ -23,8 +26,6 @@ class SearchVariantVersionedTest extends SapphireTest SearchUpdater::bind_manipulation_capture(); - Config::nest(); - Config::inst()->update('Injector', 'SearchUpdateProcessor', array( 'class' => 'SearchUpdateImmediateProcessor' )); @@ -33,13 +34,6 @@ class SearchVariantVersionedTest extends SapphireTest SearchUpdater::clear_dirty_indexes(); } - public function tearDown() - { - Config::unnest(); - - parent::tearDown(); - } - public function testPublishing() { // Check that write updates Stage @@ -71,9 +65,13 @@ class SearchVariantVersionedTest extends SapphireTest $item->write(); SearchUpdater::flush_dirty_indexes(); - $this->assertEquals(self::$index->getAdded(array('ID', '_versionedstage')), array( - array('ID' => $item->ID, '_versionedstage' => 'Stage') + + $expected = array(array( + 'ID' => $item->ID, + '_versionedstage' => 'Stage' )); + $added = self::$index->getAdded(array('ID', '_versionedstage')); + $this->assertEquals($expected, $added); } public function testExcludeVariantState() diff --git a/tests/SolrIndexTest.php b/tests/SolrIndexTest.php index 3864275..c710113 100644 --- a/tests/SolrIndexTest.php +++ b/tests/SolrIndexTest.php @@ -12,12 +12,12 @@ class SolrIndexTest extends SapphireTest public function setUp() { + parent::setUp(); + if (!class_exists('Phockito')) { $this->markTestSkipped("These tests need the Phockito module installed to run"); $this->skipTest = true; } - - parent::setUp(); } public function testFieldDataHasOne() @@ -88,7 +88,7 @@ class SolrIndexTest extends SapphireTest \Hamcrest_Matchers::anything() ); } - + /** * Test boosting on field schema (via queried fields parameter) */ diff --git a/tests/SolrIndexVersionedTest.php b/tests/SolrIndexVersionedTest.php index d7c3bc5..b9f7de2 100644 --- a/tests/SolrIndexVersionedTest.php +++ b/tests/SolrIndexVersionedTest.php @@ -7,9 +7,9 @@ if (class_exists('Phockito')) { class SolrIndexVersionedTest extends SapphireTest { protected $oldMode = null; - + protected static $index = null; - + protected $extraDataObjects = array( 'SearchVariantVersionedTest_Item' ); @@ -17,7 +17,7 @@ class SolrIndexVersionedTest extends SapphireTest public function setUp() { parent::setUp(); - + if (!class_exists('Phockito')) { $this->skipTest = true; return $this->markTestSkipped("These tests need the Phockito module installed to run"); @@ -35,23 +35,20 @@ class SolrIndexVersionedTest extends SapphireTest SearchUpdater::bind_manipulation_capture(); - Config::nest(); - Config::inst()->update('Injector', 'SearchUpdateProcessor', array( 'class' => 'SearchUpdateImmediateProcessor' )); FullTextSearch::force_index_list(self::$index); SearchUpdater::clear_dirty_indexes(); - + $this->oldMode = Versioned::get_reading_mode(); Versioned::reading_stage('Stage'); } - + public function tearDown() { Versioned::set_reading_mode($this->oldMode); - Config::unnest(); parent::tearDown(); } @@ -59,21 +56,21 @@ class SolrIndexVersionedTest extends SapphireTest { return Phockito::mock('Solr3Service'); } - + protected function getExpectedDocumentId($id, $stage) { // Prevent subsites from breaking tests $subsites = class_exists('Subsite') ? '"SearchVariantSubsites":"0",' : ''; return $id.'-SiteTree-{'.$subsites.'"SearchVariantVersioned":"'.$stage.'"}'; } - + public function testPublishing() { - + // Setup mocks $serviceMock = $this->getServiceMock(); self::$index->setService($serviceMock); - + // Check that write updates Stage Versioned::reading_stage('Stage'); Phockito::reset($serviceMock); @@ -85,7 +82,7 @@ class SolrIndexVersionedTest extends SapphireTest 'ClassName' => 'SearchVariantVersionedTest_Item' )); Phockito::verify($serviceMock)->addDocument($doc); - + // Check that write updates Live Versioned::reading_stage('Stage'); Phockito::reset($serviceMock); @@ -99,14 +96,14 @@ class SolrIndexVersionedTest extends SapphireTest )); Phockito::verify($serviceMock)->addDocument($doc); } - + public function testDelete() { - + // Setup mocks $serviceMock = $this->getServiceMock(); self::$index->setService($serviceMock); - + // Delete the live record (not the stage) Versioned::reading_stage('Stage'); Phockito::reset($serviceMock); @@ -121,7 +118,7 @@ class SolrIndexVersionedTest extends SapphireTest ->deleteById($this->getExpectedDocumentId($id, 'Live')); Phockito::verify($serviceMock, 0) ->deleteById($this->getExpectedDocumentId($id, 'Stage')); - + // Delete the stage record Versioned::reading_stage('Stage'); Phockito::reset($serviceMock); @@ -155,7 +152,7 @@ if (!class_exists('Phockito')) { class SolrDocumentMatcher extends Hamcrest_BaseMatcher { protected $properties; - + public function __construct($properties) { $this->properties = $properties; @@ -171,13 +168,13 @@ class SolrDocumentMatcher extends Hamcrest_BaseMatcher if (! ($item instanceof Apache_Solr_Document)) { return false; } - + foreach ($this->properties as $key => $value) { if ($item->{$key} != $value) { return false; } } - + return true; } }