From 1d81ecb2938f9967d33daba97a7bd88164286d06 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Thu, 20 Sep 2018 15:35:06 +1200 Subject: [PATCH 1/4] NEW: Add DB::replace_parameter() This helper is necessary when you want to replace a parameter with a non-literal such as a column name. This arises when you are transforming queries such as we do in the prepopulation query for Hierarchy::numChildren() --- src/ORM/DB.php | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/ORM/DB.php b/src/ORM/DB.php index 18c595e98e6..85b4e86c761 100644 --- a/src/ORM/DB.php +++ b/src/ORM/DB.php @@ -430,6 +430,69 @@ public static function inline_parameters($sql, $parameters) return $joined; } + + /** + * Replace 1 parameter with the given value + * For example, you can use this to replace a parameter with a column name instead of a literal + * @param string $sql The parameterised query + * @param int $paramIdx The 0-based position of the parameter + * @param array $replacement The value to insert into the queyr + * @param bool $skipEscaping Set to true to insert the value as-is, with no escaping. Use with caution! + * + * @return string + */ + public static function replace_parameter($sql, $paramIdx, $replacement, $skipEscaping = false) + { + $segments = preg_split('/\?/', $sql); + $joined = ''; + $inString = false; + $numSegments = count($segments); + $currentParamIdx = 0; + + for ($i = 0; $i < $numSegments; $i++) { + $input = $segments[$i]; + // Append next segment + $joined .= $segments[$i]; + // Don't add placeholder after last segment + if ($i === $numSegments - 1) { + break; + } + // check string escape on previous fragment + // Remove escaped backslashes, count them! + $input = preg_replace('/\\\\\\\\/', '', $input); + // Count quotes + $totalQuotes = substr_count($input, "'"); // Includes double quote escaped quotes + $escapedQuotes = substr_count($input, "\\'"); + if ((($totalQuotes - $escapedQuotes) % 2) !== 0) { + $inString = !$inString; + } + + // Append placeholder replacement + if ($inString) { + // Literal question mark + $joined .= '?'; + continue; + } + + // If we've found the right parameter, replace it + if ($currentParamIdx == $paramIdx) { + if ($skipEscaping) { + $value = $replacement; + } elseif (is_bool($replacement)) { + $value = $replacement ? '1' : '0'; + } elseif (is_int($replacement)) { + $value = $replacement; + } else { + $value = (DB::get_conn() !== null) ? Convert::raw2sql($replacement, true) : $replacement; + } + $joined .= $value; + } + + $currentParamIdx++; + } + return $joined; + } + /** * Execute the given SQL parameterised query with the specified arguments * From ac433765919c27ad00b92ebc25f6cfec3bb437ca Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Tue, 18 Sep 2018 13:18:48 +1200 Subject: [PATCH 2/4] NEW: Add Hierarchy::prepopulate_numchildren_cache() API: Hierarchy::stageChildren() customisations must be applied to the base class and not include record-specific behaviour. Adds the ability to prepopulate the cache for Hierarchy::numChildren() in a batch. Note that this optimisation means that stageChildren() is not called on each record in order to calculate numChildren(). This means that the structure of the stageChildren() query must be the same for all records and the behaviour cannot be customised only for a subclass of the base data class. For example, apply your customisations to SiteTree and not a subclass. This is an useful part of optimising the query count on tree generation. See https://github.com/silverstripe/silverstripe-framework/issues/8379 --- docs/en/04_Changelogs/4.3.0.md | 1 + src/ORM/Hierarchy/Hierarchy.php | 122 +++++++++++++++++++++++++++++--- 2 files changed, 114 insertions(+), 9 deletions(-) diff --git a/docs/en/04_Changelogs/4.3.0.md b/docs/en/04_Changelogs/4.3.0.md index fcaad6995a4..7068f2eada4 100644 --- a/docs/en/04_Changelogs/4.3.0.md +++ b/docs/en/04_Changelogs/4.3.0.md @@ -4,6 +4,7 @@ - `DataList::column()` now returns all values and not just "distinct" values from a column as per the API docs - `DataList`, `ArrayList` and `UnsavedRalationList` all have `columnUnique()` method for fetching distinct column values + - Take care with `stageChildren()` overrides. `Hierarchy::numChildren() ` results will only make use of `stageChildren()` customisations that are applied to the base class and don't include record-specific behaviour. ## Upgrading {#upgrading} diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index b4777508507..e69f8b67f30 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -10,7 +10,10 @@ use SilverStripe\ORM\ArrayList; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataExtension; +use SilverStripe\ORM\DB; use SilverStripe\Versioned\Versioned; +use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Convert; use Exception; /** @@ -71,6 +74,15 @@ class Hierarchy extends DataExtension */ private static $hide_from_cms_tree = array(); + /** + * Used to enable or disable the prepopulation of the numchildren cache. + * Defaults to true. + * + * @config + * @var boolean + */ + private static $prepopulate_numchildren_cache = true; + /** * Prevent virtual page virtualising these fields * @@ -79,9 +91,17 @@ class Hierarchy extends DataExtension */ private static $non_virtual_fields = [ '_cache_children', - '_cache_numChildren', ]; + /** + * A cache used by numChildren(). + * Clear through {@link flushCache()}. + * version (int)0 means not on this stage. + * + * @var array + */ + protected static $cache_numChildren = []; + public static function get_extra_config($class, $extension, $args) { return array( @@ -271,12 +291,14 @@ public function numHistoricalChildren() */ public function numChildren($cache = true) { - // Load if caching - if ($cache) { - $numChildren = $this->owner->_cache_numChildren; - if (isset($numChildren)) { - return $numChildren; - } + + $baseClass = $this->owner->baseClass(); + $stage = 'Stage'; + $id = $this->owner->ID; + + // cached call + if ($cache && isset(self::$cache_numChildren[$baseClass][$stage][$id])) { + return self::$cache_numChildren[$baseClass][$stage][$id]; } // We call stageChildren(), because Children() has canView() filtering @@ -284,11 +306,93 @@ public function numChildren($cache = true) // Save if caching if ($cache) { - $this->owner->_cache_numChildren = $numChildren; + self::$cache_numChildren[$baseClass][$stage][$id] = $numChildren; } + return $numChildren; } + /** + * Pre-populate the cache for Versioned::get_versionnumber_by_stage() for + * a list of record IDs, for more efficient database querying. If $idList + * is null, then every record will be pre-cached. + * + * @param string $class + * @param string $stage + * @param array $idList + */ + public static function prepopulate_numchildren_cache($baseClass, $stage, $idList = null) + { + if (!Config::inst()->get(static::class, 'prepopulate_numchildren_cache')) { + return; + } + + /** @var Versioned|DataObject $singleton */ + $dummyObject = new $baseClass(); + $dummyObject->ID = -23478234; // going to use this for search & replace + $baseTable = $dummyObject->baseTable(); + + $idColumn = Convert::symbol2sql("{$baseTable}.ID"); + $parentIdColumn = Convert::symbol2sql("ParentID"); + + + // Get the stageChildren() result of a dummy object and break down into a generic query + $query = $dummyObject->stageChildren(true)->dataQuery()->query(); + $where = $query->getWhere(); + + $newWhere = []; + + foreach ($where as $i => $compoundClause) { + foreach ($compoundClause as $clause => $params) { + // Skip any "WHERE ParentID = [marker]" clauses as this will be replaced with a GROUP BY + if (!(preg_match('/^"[^"]+"\."ParentID" = \?$/', $clause) && $clause[1] == $dummyObject->ID)) { + // Replace any marker IDs with ""."ID" + $paramNum = 0; + foreach ($params as $j => $param) { + if ($param == $dummyObject->ID) { + unset($params[$j]); + $clause = DB::replace_parameter($clause, $paramNum, $parentIdColumn, true); + } else { + $paramNum++; + } + } + + $newWhere[] = [ $clause => $params ]; + } + } + } + + // optional ID-list filter + if ($idList) { + // Validate the ID list + foreach ($idList as $id) { + if (!is_numeric($id)) { + user_error( + "Bad ID passed to Versioned::prepopulate_numchildren_cache() in \$idList: " . $id, + E_USER_ERROR + ); + } + } + $newWhere[] = ['"ParentID" IN (' . DB::placeholders($idList) . ')' => $idList]; + } + + $query->setWhere($newWhere); + $query->setOrderBy(null); + + $query->setSelect([ + '"ParentID"', + "COUNT(DISTINCT $idColumn) AS \"NumChildren\"", + ]); + $query->setGroupBy([ + "ParentID + "]); + + $numChildrens = $query->execute()->map(); + foreach ($numChildrens as $id => $numChildren) { + self::$cache_numChildren[$baseClass][$stage][$id] = $numChildren; + } + } + /** * Checks if we're on a controller where we should filter. ie. Are we loading the SiteTree? * @@ -439,6 +543,6 @@ public function getBreadcrumbs($separator = ' » ') public function flushCache() { $this->owner->_cache_children = null; - $this->owner->_cache_numChildren = null; + self::$cache_numChildren = []; } } From 574918c054b8ef68a2d3ad7520c48d7a374db0f4 Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Thu, 20 Sep 2018 20:04:40 +1200 Subject: [PATCH 3/4] Add a `_complete` flag prepopulate_numchildren_cache This will avoid trying to count the children of childless object. --- src/ORM/Hierarchy/Hierarchy.php | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index e69f8b67f30..b19f4eb163d 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -297,8 +297,13 @@ public function numChildren($cache = true) $id = $this->owner->ID; // cached call - if ($cache && isset(self::$cache_numChildren[$baseClass][$stage][$id])) { - return self::$cache_numChildren[$baseClass][$stage][$id]; + if ($cache) { + if (isset(self::$cache_numChildren[$baseClass][$stage][$id])) { + return self::$cache_numChildren[$baseClass][$stage][$id]; + } elseif (isset(self::$cache_numChildren[$baseClass][$stage]['_complete'])) { + // If the cache is complete and we didn't find our ID in the cache, it means this object is childless. + return 0; + } } // We call stageChildren(), because Children() has canView() filtering @@ -387,9 +392,12 @@ public static function prepopulate_numchildren_cache($baseClass, $stage, $idList "ParentID "]); - $numChildrens = $query->execute()->map(); - foreach ($numChildrens as $id => $numChildren) { - self::$cache_numChildren[$baseClass][$stage][$id] = $numChildren; + $numChildren = $query->execute()->map(); + self::$cache_numChildren[$baseClass][$stage] = $numChildren; + if (!$idList) { + // If all objects are being cached, mark this cache as complete + // to avoid counting children of childless object. + self::$cache_numChildren[$baseClass][$stage]['_complete'] = true; } } From eb4d73c8135629bf1ccf0879437ada16a1445547 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Fri, 21 Sep 2018 18:31:12 +1200 Subject: [PATCH 4/4] NEW: Add Hierarchy::prepopulateTreeDataCache() This provides a more extensible way of preopulating caches for optimised tree generation. Fixes https://github.com/silverstripe/silverstripe-framework/issues/8391 --- src/ORM/Hierarchy/Hierarchy.php | 37 ++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/src/ORM/Hierarchy/Hierarchy.php b/src/ORM/Hierarchy/Hierarchy.php index b19f4eb163d..423b967e8fc 100644 --- a/src/ORM/Hierarchy/Hierarchy.php +++ b/src/ORM/Hierarchy/Hierarchy.php @@ -293,14 +293,14 @@ public function numChildren($cache = true) { $baseClass = $this->owner->baseClass(); - $stage = 'Stage'; + $cacheType = 'numChildren'; $id = $this->owner->ID; // cached call if ($cache) { - if (isset(self::$cache_numChildren[$baseClass][$stage][$id])) { - return self::$cache_numChildren[$baseClass][$stage][$id]; - } elseif (isset(self::$cache_numChildren[$baseClass][$stage]['_complete'])) { + if (isset(self::$cache_numChildren[$baseClass][$cacheType][$id])) { + return self::$cache_numChildren[$baseClass][$cacheType][$id]; + } elseif (isset(self::$cache_numChildren[$baseClass][$cacheType]['_complete'])) { // If the cache is complete and we didn't find our ID in the cache, it means this object is childless. return 0; } @@ -311,12 +311,33 @@ public function numChildren($cache = true) // Save if caching if ($cache) { - self::$cache_numChildren[$baseClass][$stage][$id] = $numChildren; + self::$cache_numChildren[$baseClass][$cacheType][$id] = $numChildren; } return $numChildren; } + /** + * Pre-populate any appropriate caches prior to rendering a tree. + * This is used to allow for the efficient rendering of tree views, notably in the CMS. + * In the cace of Hierarchy, it caches numChildren values. Other extensions can provide an + * onPrepopulateTreeDataCache(DataList $recordList = null, array $options) methods to hook + * into this event as well. + * + * @param DataList $recordList The list of records to prepopulate caches for. Null for all records. + * @param array $options A map of hints about what should be cached. "numChildrenMethod" and + * "childrenMethod" are allowed keys. + */ + public function prepopulateTreeDataCache(DataList $recordList = null, array $options = []) + { + if (empty($options['numChildrenMethod']) || $options['numChildrenMethod'] === 'numChildren') { + $idList = $recordList ? $recordList->column('ID') : null; + self::prepopulate_numchildren_cache($this->owner->baseClass(), $idList); + } + + $this->owner->extend('onPrepopulateTreeDataCache', $recordList, $options); + } + /** * Pre-populate the cache for Versioned::get_versionnumber_by_stage() for * a list of record IDs, for more efficient database querying. If $idList @@ -326,7 +347,7 @@ public function numChildren($cache = true) * @param string $stage * @param array $idList */ - public static function prepopulate_numchildren_cache($baseClass, $stage, $idList = null) + public static function prepopulate_numchildren_cache($baseClass, $idList = null) { if (!Config::inst()->get(static::class, 'prepopulate_numchildren_cache')) { return; @@ -393,11 +414,11 @@ public static function prepopulate_numchildren_cache($baseClass, $stage, $idList "]); $numChildren = $query->execute()->map(); - self::$cache_numChildren[$baseClass][$stage] = $numChildren; + self::$cache_numChildren[$baseClass]['numChildren'] = $numChildren; if (!$idList) { // If all objects are being cached, mark this cache as complete // to avoid counting children of childless object. - self::$cache_numChildren[$baseClass][$stage]['_complete'] = true; + self::$cache_numChildren[$baseClass]['numChildren']['_complete'] = true; } }