diff --git a/_config/versionedownership.yml b/_config/versionedownership.yml index 9f973967..a6b43386 100644 --- a/_config/versionedownership.yml +++ b/_config/versionedownership.yml @@ -4,6 +4,10 @@ Name: versionedownership SilverStripe\ORM\DataObject: extensions: RecursivePublishable: SilverStripe\Versioned\RecursivePublishable + +SilverStripe\Core\Injector\Injector: + SilverStripe\Versioned\RecursiveStagesInterface: + class: SilverStripe\Versioned\RecursiveStagesService --- Name: versionedownership-admin OnlyIf: diff --git a/src/RecursivePublishable.php b/src/RecursivePublishable.php index e02a19d0..79d636c8 100644 --- a/src/RecursivePublishable.php +++ b/src/RecursivePublishable.php @@ -12,7 +12,6 @@ use SilverStripe\ORM\FieldType\DBDatetime; use SilverStripe\ORM\Queries\SQLUpdate; use SilverStripe\ORM\SS_List; -use SilverStripe\ORM\Tests\MySQLDatabaseTest\Data; /** * Provides owns / owned_by and recursive publishing API for all objects. @@ -448,4 +447,22 @@ public function onBeforeDuplicate($original, &$doWrite, &$relations) $owns = $this->owner->config()->get('owns'); $relations = array_intersect($allowed ?? [], $owns); } + + /** + * Make sure to flush cached data in case the data changes + * Extension point in @see DataObject::onAfterWrite() + */ + public function onAfterWrite(): void + { + RecursiveStagesService::reset(); + } + + /** + * Make sure to flush cached data in case the data changes + * Extension point in @see DataObject::onAfterDelete() + */ + public function onAfterDelete(): void + { + RecursiveStagesService::reset(); + } } diff --git a/src/RecursiveStagesInterface.php b/src/RecursiveStagesInterface.php new file mode 100644 index 00000000..41c588a3 --- /dev/null +++ b/src/RecursiveStagesInterface.php @@ -0,0 +1,19 @@ +stagesDifferCache = []; + $this->ownedObjectsCache = []; + } + + public static function reset(): void + { + /** @var RecursiveStagesInterface $service */ + $service = Injector::inst()->get(RecursiveStagesInterface::class); + + if (!$service instanceof RecursiveStagesService) { + // This covers the case where the service is overridden + return; + } + + $service->flushCachedData(); + } + + /** + * Determine if content differs on stages including nested objects + * This method also supports non-versioned models to allow traversal of hierarchy + * which includes both versioned and non-versioned models + * In-memory cache is included and optimised for the most likely lookup profile: + * Non-shared models can have deep ownership hierarchy (i.e. content blocks) + * Shared models are unlikely to have deep ownership hierarchy (i.e Assets) + * This means that we provide in-memory cache only for top level models as we do not expect to query + * the nested models multiple times + */ + public function stagesDifferRecursive(DataObject $object): bool + { + $cacheKey = $object->getUniqueKey(); + + if (!array_key_exists($cacheKey, $this->stagesDifferCache)) { + $this->stagesDifferCache[$cacheKey] = $this->determineStagesDifferRecursive($object); + } + + return $this->stagesDifferCache[$cacheKey]; + } + + /** + * Execution ownership hierarchy traversal and inspect individual models + * This method use "stack based" recursive traversal as opposed to "true" recursive traversal + * for performance reasons (avoid memory spikes and long execution times due to deeper stack) + */ + protected function determineStagesDifferRecursive(DataObject $object): bool + { + if (!$object->exists()) { + // Model hasn't been saved to DB, so we can just bail out as there is nothing to inspect + return false; + } + + // Compare existing content (perform full ownership traversal) + $models = [$object]; + + // We will keep track of inspected models so we don;t inspect the same model multiple times + // This prevents some edge cases like infinite loops + $identifiers = []; + + /** @var DataObject|Versioned $model */ + while ($model = array_shift($models)) { + $identifier = $this->getObjectIdentifier($model); + + if (in_array($identifier, $identifiers)) { + // We've already inspected this model, so we can skip processing it + // This is to prevent potential infinite loops + continue; + } + + // Mark model as inspected + $identifiers[] = $identifier; + + if ($model->hasExtension(Versioned::class) && $model->isModifiedOnDraft()) { + // Model is dirty, + // we can return here as there is no need to check the rest of the hierarchy + return true; + } + + // Discover and add owned objects for inspection + $relatedObjects = $this->getOwnedObjects($model); + $models = array_merge($models, $relatedObjects); + } + + // Compare deleted content, + // this wouldn't be covered in hierarchy traversal as deleted models are no longer present + $draftIdentifiers = $this->getOwnedIdentifiers($object, Versioned::DRAFT); + $liveIdentifiers = $this->getOwnedIdentifiers($object, Versioned::LIVE); + + return $draftIdentifiers !== $liveIdentifiers; + } + + /** + * Get unique identifiers for all owned objects, so we can easily compare them + */ + protected function getOwnedIdentifiers(DataObject $object, string $stage): array + { + $identifiers = Versioned::withVersionedMode(function () use ($object, $stage): array { + Versioned::set_stage($stage); + + /** @var DataObject $stagedObject */ + $stagedObject = DataObject::get_by_id($object->ClassName, $object->ID); + + if ($stagedObject === null) { + return []; + } + + $models = [$stagedObject]; + $identifiers = []; + + while ($model = array_shift($models)) { + $identifier = $this->getObjectIdentifier($model); + + if (in_array($identifier, $identifiers)) { + // We've already inspected this model, so we can skip processing it + // This is to prevent potential infinite loops + continue; + } + + $identifiers[] = $identifier; + $relatedObjects = $this->getOwnedObjects($model); + $models = array_merge($models, $relatedObjects); + } + + return $identifiers; + }); + + sort($identifiers, SORT_STRING); + + return array_values($identifiers); + } + + /** + * This lookup will attempt to find "owned" objects + * This method uses the 'owns' relation, same as @see RecursivePublishable::publishRecursive() + */ + protected function getOwnedObjects(DataObject $object): array + { + /** @var DataObject|Versioned $object */ + if (!$object->hasExtension(RecursivePublishable::class)) { + return []; + } + + // Add versioned stage to cache key to cover the case where non-versioned model owns versioned models + // In this situation the versioned models can have different cached state which we need to cover + $cacheKey = $object->getUniqueKey() . '-' . Versioned::get_stage(); + + if (!array_key_exists($cacheKey, $this->ownedObjectsCache)) { + $this->ownedObjectsCache[$cacheKey] = $object + // We intentionally avoid "true" recursive traversal here as it's not performant + // (often the cause of memory usage spikes and longer exeuction time due to deeper stack depth) + // Instead we use "stack based" recursive traversal approach for better performance + // which avoids the nested method calls + ->findOwned(false) + ->toArray(); + } + + return $this->ownedObjectsCache[$cacheKey]; + } + + /** + * This method covers cases where @see DataObject::getUniqueKey() can't be used + * For example when we want to compare models across stages we can't use getUniqueKey() + * as that one contains stage fragments which prevents us from making cross-stage comparison + */ + private function getObjectIdentifier(DataObject $object): string + { + return $object->ClassName . '-' . $object->ID; + } +} diff --git a/src/Versioned.php b/src/Versioned.php index 6f28c515..8e917cf0 100644 --- a/src/Versioned.php +++ b/src/Versioned.php @@ -11,6 +11,7 @@ use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; use SilverStripe\Core\Extension; +use SilverStripe\Core\Injector\Injector; use SilverStripe\Core\Resettable; use SilverStripe\Forms\FieldList; use SilverStripe\ORM\ArrayList; @@ -1997,6 +1998,18 @@ public function stagesDiffer() return (bool) $stagesDiffer; } + /** + * Determine if content differs on stages including nested objects + * 'owns' configuration drives the relationship traversal + */ + public function stagesDifferRecursive(): bool + { + /** @var RecursiveStagesInterface $service */ + $service = Injector::inst()->get(RecursiveStagesInterface::class); + + return $service->stagesDifferRecursive($this->owner); + } + /** * @param string $filter * @param string $sort diff --git a/tests/php/RecursiveStagesServiceTest.php b/tests/php/RecursiveStagesServiceTest.php new file mode 100644 index 00000000..6458ce8f --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest.php @@ -0,0 +1,131 @@ + [ + Versioned::class, + ], + GroupObject::class => [ + Versioned::class, + ], + ChildObject::class => [ + Versioned::class, + ], + ]; + + public function testStageDiffersRecursiveWithInvalidObject(): void + { + Versioned::withVersionedMode(function (): void { + Versioned::set_stage(Versioned::DRAFT); + + /** @var PrimaryObject|Versioned $primaryItem */ + $primaryItem = PrimaryObject::create(); + $primaryItem->Title = 'This registers as a change in DataObject::isChanged()'; + + $this->assertFalse($primaryItem->stagesDifferRecursive(), 'We expect to see no changes on invalid object'); + }); + } + + /** + * @dataProvider objectsProvider + */ + public function testStageDiffersRecursive(string $class, string $identifier, bool $delete, bool $expected): void + { + Versioned::withVersionedMode(function () use ($class, $identifier, $delete, $expected): void { + Versioned::set_stage(Versioned::DRAFT); + + /** @var PrimaryObject|Versioned $primaryObject */ + $primaryObject = $this->objFromFixture(PrimaryObject::class, 'primary-object-1'); + $primaryObject->publishRecursive(); + + $this->assertFalse($primaryObject->stagesDifferRecursive(), 'We expect no changes to be present initially'); + + // Fetch a specific record and make an edit + $record = $this->objFromFixture($class, $identifier); + + if ($delete) { + // Delete the record + $record->delete(); + } else { + // Update the record + $record->Title .= '-updated'; + $record->write(); + } + + $this->assertEquals($expected, $primaryObject->stagesDifferRecursive(), 'We expect to see changes depending on the case'); + }); + } + + public function objectsProvider(): array + { + return [ + 'primary object (versioned, update)' => [ + PrimaryObject::class, + 'primary-object-1', + false, + true, + ], + 'column (non-versioned, update)' => [ + ColumnObject::class, + 'column-1', + false, + false, + ], + 'column (non-versioned, delete)' => [ + ColumnObject::class, + 'column-1', + true, + false, + ], + 'group (versioned, update)' => [ + GroupObject::class, + 'group-1', + false, + true, + ], + 'group (versioned, delete)' => [ + GroupObject::class, + 'group-1', + true, + true, + ], + 'child (versioned, update)' => [ + ChildObject::class, + 'child-object-1', + false, + true, + ], + 'child (versioned, delete)' => [ + ChildObject::class, + 'child-object-1', + true, + true, + ], + ]; + } +} diff --git a/tests/php/RecursiveStagesServiceTest.yml b/tests/php/RecursiveStagesServiceTest.yml new file mode 100644 index 00000000..727a1f71 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest.yml @@ -0,0 +1,24 @@ +# Fixture hierarchy +# -> primary-object-1 (top level publish object, versioned) +# --> column-1 (non-versioned) +# ---> group-1 (versioned) +# ----> child-object-1 (versioned) + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject: + primary-object-1: + Title: 'PrimaryObject1' + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject: + column-1: + Title: 'Column1' + PrimaryObject: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\PrimaryObject.primary-object-1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject: + group-1: + Title: 'Group1' + Column: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ColumnObject.column-1 + +SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\ChildObject: + child-object-1: + Title: 'Item1' + Group: =>SilverStripe\Versioned\Tests\RecursiveStagesServiceTest\GroupObject.group-1 diff --git a/tests/php/RecursiveStagesServiceTest/ChildObject.php b/tests/php/RecursiveStagesServiceTest/ChildObject.php new file mode 100644 index 00000000..2c8af507 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/ChildObject.php @@ -0,0 +1,24 @@ + 'Varchar(255)', + ]; + + private static array $has_one = [ + 'Group' => GroupObject::class, + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/ColumnObject.php b/tests/php/RecursiveStagesServiceTest/ColumnObject.php new file mode 100644 index 00000000..c7f4e7d8 --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/ColumnObject.php @@ -0,0 +1,42 @@ + 'Varchar(255)', + ]; + + private static array $has_one = [ + 'PrimaryObject' => PrimaryObject::class, + ]; + + private static array $has_many = [ + 'Groups' => GroupObject::class, + ]; + + private static array $owns = [ + 'Groups', + ]; + + private static array $cascade_duplicates = [ + 'Groups', + ]; + + private static array $cascade_deletes = [ + 'Groups', + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/GroupObject.php b/tests/php/RecursiveStagesServiceTest/GroupObject.php new file mode 100644 index 00000000..0b624c5d --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/GroupObject.php @@ -0,0 +1,42 @@ + 'Varchar(255)', + ]; + + private static array $has_one = [ + 'Column' => ColumnObject::class, + ]; + + private static array $has_many = [ + 'Children' => ChildObject::class, + ]; + + private static array $owns = [ + 'Children', + ]; + + private static array $cascade_duplicates = [ + 'Children', + ]; + + private static array $cascade_deletes = [ + 'Children', + ]; +} diff --git a/tests/php/RecursiveStagesServiceTest/PrimaryObject.php b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php new file mode 100644 index 00000000..2520f22d --- /dev/null +++ b/tests/php/RecursiveStagesServiceTest/PrimaryObject.php @@ -0,0 +1,35 @@ + 'Varchar(255)', + ]; + + private static array $has_many = [ + 'Columns' => ColumnObject::class, + ]; + + private static array $owns = [ + 'Columns', + ]; + + private static array $cascade_duplicates = [ + 'Columns', + ]; + + private static array $cascade_deletes = [ + 'Columns', + ]; +}