Merge pull request #114 from tractorcow/pulls/fix-versioned-writes

BUG Fix versioned writes where subtables have no fields key

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)

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();

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();

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()

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)
      */

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;
     }
 }