Merge pull request #181 from sminnee/faster-draftpage-treeview

FIX: Performance optimisation for draft pages in treeview

src/Versioned.php

@@ -2278,8 +2278,14 @@ public static function get_versionnumber_by_stage($class, $stage, $id, $cache =
         }
 
         // cached call
-        if ($cache && isset(self::$cache_versionnumber[$baseClass][$stage][$id])) {
-            return self::$cache_versionnumber[$baseClass][$stage][$id] ?: null;
+        if ($cache) {
+            if (isset(self::$cache_versionnumber[$baseClass][$stage][$id])) {
+                return self::$cache_versionnumber[$baseClass][$stage][$id] ?: null;
+            } elseif (isset(self::$cache_versionnumber[$baseClass][$stage]['_complete'])) {
+                // if the cache was marked as "complete" then we know the record is missing, just return null
+                // this is used for treeview optimisation to avoid unnecessary re-requests for draft pages
+                return null;
+            }
         }
 
         // get version as performance-optimized SQL query (gets called for each record in the sitetree)
@@ -2320,6 +2326,13 @@ public static function prepopulate_versionnumber_cache($class, $stage, $idList =
         if (!Config::inst()->get(static::class, 'prepopulate_versionnumber_cache')) {
             return;
         }
+
+        /** @var Versioned|DataObject $singleton */
+        $singleton = DataObject::singleton($class);
+        $baseClass = $singleton->baseClass();
+        $baseTable = $singleton->baseTable();
+        $stageTable = $singleton->stageTable($baseTable, $stage);
+
         $filter = "";
         $parameters = [];
         if ($idList) {
@@ -2334,13 +2347,12 @@ public static function prepopulate_versionnumber_cache($class, $stage, $idList =
             }
             $filter = 'WHERE "ID" IN (' . DB::placeholders($idList) . ')';
             $parameters = $idList;
-        }
 
-        /** @var Versioned|DataObject $singleton */
-        $singleton = DataObject::singleton($class);
-        $baseClass = $singleton->baseClass();
-        $baseTable = $singleton->baseTable();
-        $stageTable = $singleton->stageTable($baseTable, $stage);
+        // If we are caching IDs for _all_ records then we can mark this cache as "complete" and in the case of a cache-miss
+        // no subsequent call is necessary
+        } else {
+            self::$cache_versionnumber[$baseClass][$stage] = [ '_complete' => true ];
+        }
 
         $versions = DB::prepared_query("SELECT \"ID\", \"Version\" FROM \"$stageTable\" $filter", $parameters)->map();
 

tests/php/VersionedNumberCacheTest.php

@@ -0,0 +1,107 @@
+<?php
+
+namespace SilverStripe\Versioned\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Versioned\Versioned;
+use SilverStripe\Versioned\Tests\VersionedTest\TestObject;
+
+/**
+ * @internal Only test the right values are returned, not that the cache is actually used.
+ */
+class VersionedNumberCacheTest extends SapphireTest
+{
+
+    public static $extra_dataobjects = [
+        VersionedTest\TestObject::class
+    ];
+
+    /**
+     * @var int
+     */
+    private static $publishedID;
+
+    /**
+     * @var int
+     */
+    private static $draftOnlyID;
+
+    private static $expectedVersions = [ ];
+
+
+    public static function setUpBeforeClass()
+    {
+        parent::setUpBeforeClass();
+
+        // Initialise our dummy object
+        $obj = TestObject::create(['Title' => 'Initial version']);
+        $obj->write();
+        self::$publishedID = $obj->ID;
+
+        // Create our live version
+        $obj->Title = 'This will be our live version';
+        $obj->write();
+        $obj->publishSingle();
+        $liveVersion = $obj->Version;
+
+        // Create our draft version
+        $obj->Title = 'This will be our draft version';
+        $obj->write();
+        $draftVersion = $obj->Version;
+
+        // This object won't ne publish
+        $draftOnly = TestObject::create(['Title' => 'Draft Only object']);
+        $draftOnly->write();
+        self::$draftOnlyID = $draftOnly->ID;
+
+        self::$expectedVersions = [
+            'liveVersion' => $liveVersion,
+            'draftVersion' => $draftVersion,
+            'null' => null
+        ];
+    }
+
+    public function setUp()
+    {
+        parent::setUp();
+        TestObject::singleton()->flushCache();
+    }
+
+    public function cacheDataProvider()
+    {
+        return [
+            [Versioned::DRAFT, 'publishedID', false, 'draftVersion'],
+            [Versioned::DRAFT, 'publishedID', true, 'draftVersion'],
+            [Versioned::LIVE, 'publishedID', false, 'liveVersion'],
+            [Versioned::LIVE, 'publishedID', true, 'liveVersion'],
+            [Versioned::LIVE, 'draftOnlyID', false, 'null'],
+            [Versioned::LIVE, 'draftOnlyID', true, 'null'],
+        ];
+    }
+
+
+    /**
+     * @dataProvider cacheDataProvider
+     */
+    public function testVersionNumberCache($stage, $ID, $cache, $expected)
+    {
+        $actual = Versioned::get_versionnumber_by_stage(TestObject::class, $stage, self::${$ID}, $cache);
+        $this->assertEquals(self::$expectedVersions[$expected], $actual);
+
+        if ($cache) {
+            // When cahing is eanbled, try re-accessing version number to make sure the cache returns the same value
+            $actual = Versioned::get_versionnumber_by_stage(TestObject::class, $stage, self::${$ID}, $cache);
+            $this->assertEquals(self::$expectedVersions[$expected], $actual);
+        }
+    }
+
+    /**
+     * @dataProvider cacheDataProvider
+     */
+    public function testPrepopulatedVersionNumberCache($stage, $ID, $cache, $expected)
+    {
+        Versioned::prepopulate_versionnumber_cache(TestObject::class, $stage);
+        $actual = Versioned::get_versionnumber_by_stage(TestObject::class, $stage, self::${$ID}, $cache);
+        $this->assertEquals(self::$expectedVersions[$expected], $actual);
+    }
+}