tonyair/silverstripe-framework
1
0
Fork 0
mirror of https://github.com/silverstripe/silverstripe-framework synced 2024-09-28 12:26:23 +02:00
Code Issues Projects Releases Wiki Activity

API Data corruption on Versioned due to lazy loading

Browse Source 
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.
This commit is contained in:
Julian Seidenberg 2013-02-12 16:44:32 +13:00 committed by Ingo Schommer
parent e2bf9649f3
commit 10199f908a
5 changed files with 223 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
model/DataList.php
View File

@ -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;
}

45
model/DataObject.php
View File

@ -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;
}
//-------------------------------------------------------------------------------------------//
/**

40
model/DataQuery.php
View File

@ -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;
}
}

26
model/Versioned.php
View File

@ -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)

133
tests/model/DataObjectLazyLoadingTest.php
View File

@ -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')"
);
}