From f931b8d3264e4b62d8502757eba644f915066123 Mon Sep 17 00:00:00 2001 From: Julian Seidenberg Date: Tue, 12 Feb 2013 16:44:32 +1300 Subject: [PATCH] API Data corruption on Versioned due to lazy loading Lazy loading no longer loads fields from the versions table when querying. This could lead to incorrect data being displayed if the data on the object and the version it pointed to did not match. API methods to allow setting of the context of the query that generated the DataObject on that object (used by the lazy loading mechanism to correctly query the Stage, Live, or Versions tables) See https://github.com/silverstripe/sapphire/pull/1178 for context. --- model/DataList.php | 5 +- model/DataObject.php | 45 +++++++- model/DataQuery.php | 8 ++ model/Versioned.php | 22 +++- tests/model/DataObjectLazyLoadingTest.php | 133 ++++++++++++++++++++++ 5 files changed, 205 insertions(+), 8 deletions(-) diff --git a/model/DataList.php b/model/DataList.php index 25fafb3d6..d998537c8 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -665,7 +665,10 @@ class DataList extends ViewableData implements SS_List, SS_Filterable, SS_Sortab } else { $item = Injector::inst()->create($defaultClass, $row, false, $this->model); } - + + //set query params on the DataObject to tell the lazy loading mechanism the context the object creation context + $item->setSourceQueryParams($this->dataQuery()->getQueryParams()); + return $item; } diff --git a/model/DataObject.php b/model/DataObject.php index afa4e4dfd..e8a025d59 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -2119,9 +2119,10 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity // Limit query to the current record, unless it has the Versioned extension, // in which case it requires special handling through augmentLoadLazyFields() - if (!isset($this->record['Version'])){ + if(!$this->hasExtension('Versioned')) { $dataQuery->where("\"$tableClass\".\"ID\" = {$this->record['ID']}")->limit(1); } + $columns = array(); // Add SQL for fields, both simple & multi-value @@ -2135,7 +2136,7 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity if ($columns) { $query = $dataQuery->query(); - $this->extend('augmentLoadLazyFields', $query, $dataQuery, $this->record); + $this->extend('augmentLoadLazyFields', $query, $dataQuery, $this); $this->extend('augmentSQL', $query, $dataQuery); $dataQuery->setQueriedColumns($columns); @@ -2931,6 +2932,46 @@ class DataObject extends ViewableData implements DataObjectInterface, i18nEntity return array_shift($tableClasses); } + /** + * @var Array Parameters used in the query that built this object. + * This can be used by decorators (e.g. lazy loading) to + * run additional queries using the same context. + */ + protected $sourceQueryParams; + + /** + * @see $sourceQueryParams + * @return array + */ + public function getSourceQueryParams() { + return $this->sourceQueryParams; + } + + /** + * @see $sourceQueryParams + * @param array + */ + public function setSourceQueryParams($array) { + $this->sourceQueryParams = $array; + } + + /** + * @see $sourceQueryParams + * @param array + */ + public function setSourceQueryParam($key, $value) { + $this->sourceQueryParams[$key] = $value; + } + + /** + * @see $sourceQueryParams + * @return Mixed + */ + public function getSourceQueryParam($key) { + if(isset($this->sourceQueryParams[$key])) return $this->sourceQueryParams[$key]; + else return null; + } + //-------------------------------------------------------------------------------------------// /** diff --git a/model/DataQuery.php b/model/DataQuery.php index 7a0ecc446..361515820 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -743,6 +743,14 @@ class DataQuery { if(isset($this->queryParams[$key])) return $this->queryParams[$key]; else return null; } + + /** + * Returns all query parameters + * @return array query parameters array + */ + public function getQueryParams() { + return $this->queryParams; + } } /** diff --git a/model/Versioned.php b/model/Versioned.php index 9e48987c5..aedcb32d6 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -277,14 +277,26 @@ class Versioned extends DataExtension { * For lazy loaded fields requiring extra sql manipulation, ie versioning * @param SQLQuery $query * @param DataQuery $dataQuery - * @param array $record + * @param DataObject $dataObject */ - function augmentLoadLazyFields(SQLQuery &$query, DataQuery &$dataQuery = null, $record) { + function augmentLoadLazyFields(SQLQuery &$query, DataQuery &$dataQuery = null, $dataObject) { + // The VersionedMode local variable ensures that this decorator only applies to + // queries that have originated from the Versioned object, and have the Versioned + // metadata set on the query object. This prevents regular queries from + // accidentally querying the *_versions tables. + $versionedMode = $dataObject->getSourceQueryParam('Versioned.mode'); $dataClass = $dataQuery->dataClass(); - if (isset($record['Version'])){ - $dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']); - $dataQuery->where("\"$dataClass\".\"Version\" = " . $record['Version']); + $modesToAllowVersioning = array('all_versions', 'latest_versions', 'archive'); + if( + !empty($dataObject->Version) && + (!empty($versionedMode) && in_array($versionedMode,$modesToAllowVersioning)) + ) { + $dataQuery->where("\"$dataClass\".\"RecordID\" = " . $dataObject->ID); + $dataQuery->where("\"$dataClass\".\"Version\" = " . $dataObject->Version); $dataQuery->setQueryParam('Versioned.mode', 'all_versions'); + } else { + // Same behaviour as in DataObject->loadLazyFields + $dataQuery->where("\"$dataClass\".\"ID\" = {$dataObject->ID}")->limit(1); } } diff --git a/tests/model/DataObjectLazyLoadingTest.php b/tests/model/DataObjectLazyLoadingTest.php index 7549b565f..ceb073f9d 100644 --- a/tests/model/DataObjectLazyLoadingTest.php +++ b/tests/model/DataObjectLazyLoadingTest.php @@ -268,6 +268,7 @@ class DataObjectLazyLoadingTest extends SapphireTest { $obj1->write(); $version2 = $obj1->Version; + $reloaded = Versioned::get_version('VersionedTest_Subclass', $obj1->ID, $version1); $this->assertEquals($reloaded->Name, 'test'); $this->assertEquals($reloaded->ExtraField, 'foo'); @@ -275,6 +276,138 @@ class DataObjectLazyLoadingTest extends SapphireTest { $reloaded = Versioned::get_version('VersionedTest_Subclass', $obj1->ID, $version2); $this->assertEquals($reloaded->Name, 'test2'); $this->assertEquals($reloaded->ExtraField, 'baz'); + + $reloaded = Versioned::get_latest_version('VersionedTest_Subclass', $obj1->ID); + $this->assertEquals($reloaded->Version, $version2); + $this->assertEquals($reloaded->Name, 'test2'); + $this->assertEquals($reloaded->ExtraField, 'baz'); + + $allVersions = Versioned::get_all_versions('VersionedTest_Subclass', $obj1->ID); + $this->assertEquals(2, $allVersions->Count()); + $this->assertEquals($allVersions->First()->Version, $version1); + $this->assertEquals($allVersions->First()->Name, 'test'); + $this->assertEquals($allVersions->First()->ExtraField, 'foo'); + $this->assertEquals($allVersions->Last()->Version, $version2); + $this->assertEquals($allVersions->Last()->Name, 'test2'); + $this->assertEquals($allVersions->Last()->ExtraField, 'baz'); + $obj1->delete(); } + + public function testLazyLoadedFieldsDoNotReferenceVersionsTable() { + // Save another record, sanity check that we're getting the right one + $obj2 = new VersionedTest_Subclass(); + $obj2->Name = "test2"; + $obj2->ExtraField = "foo2"; + $obj2->write(); + + $obj1 = new VersionedLazySub_DataObject(); + $obj1->PageName = "old-value"; + $obj1->ExtraField = "old-value"; + $obj1ID = $obj1->write(); + $obj1->publish('Stage', 'Live'); + + $obj1 = VersionedLazySub_DataObject::get()->byID($obj1ID); + $this->assertEquals( + 'old-value', + $obj1->PageName, + "Correct value on base table when fetching base class" + ); + $this->assertEquals( + 'old-value', + $obj1->ExtraField, + "Correct value on sub table when fetching base class" + ); + + $obj1 = VersionedLazy_DataObject::get()->byID($obj1ID); + $this->assertEquals( + 'old-value', + $obj1->PageName, + "Correct value on base table when fetching sub class" + ); + $this->assertEquals( + 'old-value', + $obj1->ExtraField, + "Correct value on sub table when fetching sub class" + ); + + // Force inconsistent state to test behaviour (shouldn't select from *_versions) + DB::query(sprintf( + "UPDATE \"VersionedLazy_DataObject_versions\" SET \"PageName\" = 'versioned-value' " . + "WHERE \"RecordID\" = %d", + $obj1ID + )); + DB::query(sprintf( + "UPDATE \"VersionedLazySub_DataObject_versions\" SET \"ExtraField\" = 'versioned-value' " . + "WHERE \"RecordID\" = %d", + $obj1ID + )); + + $obj1 = VersionedLazySub_DataObject::get()->byID($obj1ID); + $this->assertEquals( + 'old-value', + $obj1->PageName, + "Correct value on base table when fetching base class" + ); + $this->assertEquals( + 'old-value', + $obj1->ExtraField, + "Correct value on sub table when fetching base class" + ); + $obj1 = VersionedLazy_DataObject::get()->byID($obj1ID); + $this->assertEquals( + 'old-value', + $obj1->PageName, + "Correct value on base table when fetching sub class" + ); + $this->assertEquals( + 'old-value', + $obj1->ExtraField, + "Correct value on sub table when fetching sub class" + ); + + // Update live table only to test behaviour (shouldn't select from *_versions or stage) + DB::query(sprintf( + 'UPDATE "VersionedLazy_DataObject_Live" SET "PageName" = \'live-value\' WHERE "ID" = %d', + $obj1ID + )); + DB::query(sprintf( + 'UPDATE "VersionedLazySub_DataObject_Live" SET "ExtraField" = \'live-value\' WHERE "ID" = %d', + $obj1ID + )); + + Versioned::reading_stage('Live'); + $obj1 = VersionedLazy_DataObject::get()->byID($obj1ID); + $this->assertEquals( + 'live-value', + $obj1->PageName, + "Correct value from base table when fetching base class on live stage" + ); + $this->assertEquals( + 'live-value', + $obj1->ExtraField, + "Correct value from sub table when fetching base class on live stage" + ); + } + } + + +/** Additional classes for versioned lazy loading testing */ +class VersionedLazy_DataObject extends DataObject { + static $db = array( + "PageName" => "Varchar" + ); + static $extensions = array( + "Versioned('Stage', 'Live')" + ); +} + +class VersionedLazySub_DataObject extends VersionedLazy_DataObject { + static $db = array( + "ExtraField" => "Varchar", + ); + static $extensions = array( + "Versioned('Stage', 'Live')" + ); +} \ No newline at end of file