From 10199f908a07907f8675dacd40e5f32885000d51 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 | 40 ++++--- model/Versioned.php | 26 +++-- tests/model/DataObjectLazyLoadingTest.php | 133 ++++++++++++++++++++++ 5 files changed, 223 insertions(+), 26 deletions(-) diff --git a/model/DataList.php b/model/DataList.php index 86195c2c2..7e613a86c 100644 --- a/model/DataList.php +++ b/model/DataList.php @@ -643,7 +643,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 3ae4c7186..421ba8b4a 100644 --- a/model/DataObject.php +++ b/model/DataObject.php @@ -2077,9 +2077,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 @@ -2093,7 +2094,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); @@ -2978,6 +2979,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 baeff5e17..5628826c7 100644 --- a/model/DataQuery.php +++ b/model/DataQuery.php @@ -426,8 +426,8 @@ class DataQuery { $clone->query->addHaving($having); return $clone; } else { - return $this; - } + return $this; + } } /** @@ -450,8 +450,8 @@ class DataQuery { $clone->query->addWhere($filter); return $clone; } else { - return $this; - } + return $this; + } } /** @@ -469,8 +469,8 @@ class DataQuery { $clone->query->addWhereAny($filter); return $clone; } else { - return $this; - } + return $this; + } } /** @@ -535,10 +535,10 @@ class DataQuery { } return $clone; } else { - return $this; - } + return $this; } - + } + /** * Add an INNER JOIN clause to this query. * @@ -552,8 +552,8 @@ class DataQuery { $clone->query->addInnerJoin($table, $onClause, $alias); return $clone; } else { - return $this; - } + return $this; + } } /** @@ -569,8 +569,8 @@ class DataQuery { $clone->query->addLeftJoin($table, $onClause, $alias); return $clone; } else { - return $this; - } + return $this; + } } /** @@ -662,7 +662,7 @@ class DataQuery { * @param string $field */ public function subtract(DataQuery $subtractQuery, $field='ID') { - $subSelect= $subtractQuery->getFinalisedQuery(); + $subSelect = $subtractQuery->getFinalisedQuery(); $fieldExpression = $this->expressionForField($field, $subSelect); $subSelect->setSelect(array()); $subSelect->selectField($fieldExpression, $field); @@ -714,7 +714,7 @@ class DataQuery { // Special case for ID if($field == 'ID') { $baseClass = ClassInfo::baseDataClass($this->dataClass); - return "\"$baseClass\".\"ID\""; + return "\"$baseClass\".\"ID\""; } else { return $query->expressionForField($field); @@ -754,5 +754,13 @@ 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 bacaecb11..76525444c 100644 --- a/model/Versioned.php +++ b/model/Versioned.php @@ -137,7 +137,7 @@ class Versioned extends DataExtension { */ public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) { $baseTable = ClassInfo::baseDataClass($dataQuery->dataClass()); - + switch($dataQuery->getQueryParam('Versioned.mode')) { // Noop case '': @@ -262,14 +262,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); } } @@ -745,7 +757,7 @@ class Versioned extends DataExtension { /** * Return a list of all the versions available. - * @param string $filter + * @param string $filter */ public function allVersions($filter = "", $sort = "", $limit = "", $join = "", $having = "") { // Make sure the table names are not postfixed (e.g. _Live) 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