Skip to content

Commit

Permalink
BUG Test case for versioned now correctly checks IDs returned from Ve…
Browse files Browse the repository at this point in the history 
…rsioned::get_including_deleted

BUG Issue with deleted records not being queried properly.
API DataQuery::expressionForField no longer requires a second parameter. Rather the query object is inferred from the DataQuery itself. This should improve consistency of use of this function.
  • Loading branch information
@tractorcow
tractorcow committed Dec 17, 2012
1 parent 75b0c3e commit 6aa16e1
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 20 deletions.
30 changes: 17 additions & 13 deletions model/DataQuery.php
View file
Expand Up @@ -647,12 +647,12 @@ public function applyRelation($relation) {
* @param string $field
*/
public function subtract(DataQuery $subtractQuery, $field='ID') {
$subSelect= $subtractQuery->getFinalisedQuery();
$fieldExpression = $this->expressionForField($field, $subSelect);
$fieldExpression = $subtractQuery->expressionForField($field);
$subSelect = $subtractQuery->getFinalisedQuery();
$subSelect->setSelect(array());
$subSelect->selectField($fieldExpression, $field);
$subSelect->setOrderBy(null);
$this->where($this->expressionForField($field, $this).' NOT IN ('.$subSelect->sql().')');
$this->where($this->expressionForField($field).' NOT IN ('.$subSelect->sql().')');

return $this;
}
Expand All @@ -679,9 +679,9 @@ public function selectFromTable($table, $fields) {
* @param String $field See {@link expressionForField()}.
*/
public function column($field = 'ID') {
$fieldExpression = $this->expressionForField($field);
$query = $this->getFinalisedQuery(array($field));
$originalSelect = $query->getSelect();
$fieldExpression = $this->expressionForField($field, $query);
$query->setSelect(array());
$query->selectField($fieldExpression, $field);
$this->ensureSelectContainsOrderbyColumns($query, $originalSelect);
Expand All @@ -692,17 +692,21 @@ public function column($field = 'ID') {
/**
* @param String $field Select statement identifier, either the unquoted column name,
* the full composite SQL statement, or the alias set through {@link SQLQuery->selectField()}.
* @param SQLQuery $query
* @return String
* @return String The expression used to query this field via this DataQuery
*/
protected function expressionForField($field, $query) {
// Special case for ID
if($field == 'ID') {
protected function expressionForField($field) {

// Prepare query object for selecting this field
$query = $this->getFinalisedQuery(array($field));

// Allow query to define the expression for this field
$expression = $query->expressionForField($field);
if(!empty($expression)) return $expression;

// Special case for ID, if not provided
if($field === 'ID') {
$baseClass = ClassInfo::baseDataClass($this->dataClass);
return "\"$baseClass\".\"ID\"";

} else {
return $query->expressionForField($field);
return "\"$baseClass\".\"ID\"";
}
}

Expand Down
24 changes: 19 additions & 5 deletions model/Versioned.php
View file
Expand Up @@ -136,8 +136,8 @@ public function augmentDataQueryCreation(SQLQuery &$query, DataQuery &$dataQuery
* @todo Should this all go into VersionedDataQuery?
*/
public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) {
$baseTable = ClassInfo::baseDataClass($dataQuery->dataClass());

$baseTable = ClassInfo::baseDataClass($dataQuery->dataClass());
switch($dataQuery->getQueryParam('Versioned.mode')) {
// Noop
case '':
Expand Down Expand Up @@ -232,8 +232,19 @@ public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) {
foreach(self::$db_for_versions_table as $name => $type) {
$query->selectField(sprintf('"%s_versions"."%s"', $baseTable, $name), $name);
}

// Alias the record ID as the row ID
$query->selectField(sprintf('"%s_versions"."%s"', $baseTable, 'RecordID'), "ID");
$query->addOrderBy(sprintf('"%s_versions"."%s"', $baseTable, 'Version'));

// Ensure that any sort order referring to this ID is correctly aliased
$orders = $query->getOrderBy();
foreach($orders as $order => $dir) {
if($order === "\"$baseTable\".\"ID\"") {
unset($orders[$order]);
$orders["\"{$baseTable}_versions\".\"RecordID\""] = $dir;
}
}
$query->setOrderBy($orders);

// latest_version has one more step
// Return latest version instances, regardless of whether they are on a particular stage
Expand All @@ -250,6 +261,9 @@ public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) {
) AS \"{$alias}_versions_latest\"
WHERE \"{$alias}_versions_latest\".\"RecordID\" = \"{$alias}_versions\".\"RecordID\"
)");
} else {
// If all versions are requested, ensure that records are sorted by this field
$query->addOrderBy(sprintf('"%s_versions"."%s"', $baseTable, 'Version'));
}
break;
default:
Expand All @@ -266,8 +280,8 @@ public function augmentSQL(SQLQuery &$query, DataQuery &$dataQuery = null) {
*/
function augmentLoadLazyFields(SQLQuery &$query, DataQuery &$dataQuery = null, $record) {
$dataClass = $dataQuery->dataClass();
if (isset($record['Version'])){
$dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']);
if (isset($record['Version'])){
$dataQuery->where("\"$dataClass\".\"RecordID\" = " . $record['ID']);
$dataQuery->where("\"$dataClass\".\"Version\" = " . $record['Version']);
$dataQuery->setQueryParam('Versioned.mode', 'all_versions');
}
Expand Down
16 changes: 14 additions & 2 deletions tests/model/VersionedTest.php
View file
Expand Up @@ -74,8 +74,15 @@ public function testForceChangeUpdatesVersion() {
* Test Versioned::get_including_deleted()
*/
public function testGetIncludingDeleted() {
// Delete a page
$this->objFromFixture('VersionedTest_DataObject', 'page3')->delete();
// Get all ids of pages
$allPageIDs = DataObject::get('VersionedTest_DataObject', "\"ParentID\" = 0", "\"VersionedTest_DataObject\".\"ID\" ASC")->column('ID');

// Modify a page, ensuring that the Version ID and Record ID will differ,
// and then subsequently delete it
$targetPage = $this->objFromFixture('VersionedTest_DataObject', 'page3');
$targetPage->Content = 'To be deleted';
$targetPage->write();
$targetPage->delete();

// Get all items, ignoring deleted
$remainingPages = DataObject::get("VersionedTest_DataObject", "\"ParentID\" = 0",
Expand All @@ -90,12 +97,17 @@ public function testGetIncludingDeleted() {
// Check that page 3 is still there
$this->assertEquals(array("Page 1", "Page 2", "Page 3"), $allPages->column('Title'));

// Check that the returned pages have the correct IDs
$this->assertEquals($allPageIDs, $allPages->column('ID'));

// Check that this still works if we switch to reading the other stage
Versioned::reading_stage("Live");
$allPages = Versioned::get_including_deleted("VersionedTest_DataObject", "\"ParentID\" = 0",
"\"VersionedTest_DataObject\".\"ID\" ASC");
$this->assertEquals(array("Page 1", "Page 2", "Page 3"), $allPages->column('Title'));

// Check that the returned pages still have the correct IDs
$this->assertEquals($allPageIDs, $allPages->column('ID'));
}

public function testVersionedFieldsAdded() {
Expand Down

0 comments on commit 6aa16e1

Please sign in to comment.