From 963d9197d3e714e4f137ae538b86c4bd6288c0e2 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 19 May 2017 14:07:45 +1200 Subject: [PATCH] API Ensure that all DataQuery joins are aliased based on relationship name --- src/ORM/DataList.php | 6 +- src/ORM/DataObjectSchema.php | 5 +- src/ORM/DataQuery.php | 263 ++++++++++++------ src/ORM/Filters/FulltextFilter.php | 6 +- src/ORM/Filters/SearchFilter.php | 35 ++- .../GridField/GridFieldSortableHeaderTest.php | 31 ++- tests/php/ORM/DataListTest.php | 73 ++++- .../ORM/DataObjectDuplicationTest/Class4.php | 2 + tests/php/ORM/DataObjectTest.php | 4 + tests/php/ORM/DataObjectTest.yml | 34 +++ tests/php/ORM/DataObjectTest/Bracket.php | 32 +++ tests/php/ORM/DataObjectTest/CEO.php | 6 +- tests/php/ORM/DataObjectTest/Company.php | 4 +- tests/php/ORM/DataObjectTest/Player.php | 8 +- .../ORM/DataObjectTest/RelationChildFirst.php | 12 + .../DataObjectTest/RelationChildSecond.php | 12 + .../php/ORM/DataObjectTest/RelationParent.php | 15 + tests/php/ORM/DataObjectTest/Team.php | 8 +- tests/php/ORM/DataQueryTest.php | 10 +- tests/php/ORM/ManyManyListTest.php | 12 +- tests/php/ORM/ManyManyListTest/Category.php | 4 + 21 files changed, 443 insertions(+), 139 deletions(-) create mode 100644 tests/php/ORM/DataObjectTest/Bracket.php create mode 100644 tests/php/ORM/DataObjectTest/RelationChildFirst.php create mode 100644 tests/php/ORM/DataObjectTest/RelationChildSecond.php create mode 100644 tests/php/ORM/DataObjectTest/RelationParent.php diff --git a/src/ORM/DataList.php b/src/ORM/DataList.php index ce16ed483..d546454b3 100644 --- a/src/ORM/DataList.php +++ b/src/ORM/DataList.php @@ -543,11 +543,13 @@ class DataList extends ViewableData implements SS_List, Filterable, Sortable, Li $relations = explode('.', $field); $fieldName = array_pop($relations); - // Apply + // Apply relation $relationModelName = $query->applyRelation($relations, $linearOnly); + $relationPrefix = $query->applyRelationPrefix($relations); // Find the db field the relation belongs to - $columnName = DataObject::getSchema()->sqlColumnForField($relationModelName, $fieldName); + $columnName = DataObject::getSchema() + ->sqlColumnForField($relationModelName, $fieldName, $relationPrefix); } ); } diff --git a/src/ORM/DataObjectSchema.php b/src/ORM/DataObjectSchema.php index ad6cb5a03..f1e8f32b6 100644 --- a/src/ORM/DataObjectSchema.php +++ b/src/ORM/DataObjectSchema.php @@ -88,15 +88,16 @@ class DataObjectSchema * * @param string $class Class name (not a table). * @param string $field Name of field that belongs to this class (or a parent class) + * @param string $tablePrefix Optional prefix for table (alias) * @return string The SQL identifier string for the corresponding column for this field */ - public function sqlColumnForField($class, $field) + public function sqlColumnForField($class, $field, $tablePrefix = null) { $table = $this->tableForField($class, $field); if (!$table) { throw new InvalidArgumentException("\"{$field}\" is not a field on class \"{$class}\""); } - return "\"{$table}\".\"{$field}\""; + return "\"{$tablePrefix}{$table}\".\"{$field}\""; } /** diff --git a/src/ORM/DataQuery.php b/src/ORM/DataQuery.php index 3c711eec1..4a4be50c6 100644 --- a/src/ORM/DataQuery.php +++ b/src/ORM/DataQuery.php @@ -756,11 +756,38 @@ class DataQuery return $this; } + /** + * Prefix of all joined table aliases. E.g. ->filter('Banner.Image.Title)' + * Will join the Banner, and then Image relations + * `$relationPrefx` will be `banner_image_` + * Each table in the Image chain will be suffixed to this prefix. E.g. + * `banner_image_File` and `banner_image_Image` + * + * This will be null if no relation is joined. + * E.g. `->filter('Title')` + * + * @param string|array $relation Relation in '.' delimited string, or array of parts + * @return string Table prefix + */ + public static function applyRelationPrefix($relation) + { + if (!$relation) { + return null; + } + if (is_string($relation)) { + $relation = explode(".", $relation); + } + return strtolower(implode('_', $relation)) . '_'; + } + /** * Traverse the relationship fields, and add the table * mappings to the query object state. This has to be called * in any overloaded {@link SearchFilter->apply()} methods manually. * + * Note, that in order to filter against the joined relation user code must + * use {@see tablePrefix()} to get the table alias used for this relation. + * * @param string|array $relation The array/dot-syntax relation to follow * @param bool $linearOnly Set to true to restrict to linear relations only. Set this * if this relation will be used for sorting, and should not include duplicate rows. @@ -780,20 +807,43 @@ class DataQuery $modelClass = $this->dataClass; $schema = DataObject::getSchema(); + $currentRelation = []; foreach ($relation as $rel) { + // Get prefix for join for this table (and parent to join on) + $parentPrefix = $this->applyRelationPrefix($currentRelation); + $currentRelation[] = $rel; + $tablePrefix = $this->applyRelationPrefix($currentRelation); + + // Check has_one if ($component = $schema->hasOneComponent($modelClass, $rel)) { // Join via has_one - $this->joinHasOneRelation($modelClass, $rel, $component); + $this->joinHasOneRelation($modelClass, $rel, $component, $parentPrefix, $tablePrefix); $modelClass = $component; - } elseif ($component = $schema->hasManyComponent($modelClass, $rel)) { + continue; + } + + // Check has_many + if ($component = $schema->hasManyComponent($modelClass, $rel)) { // Fail on non-linear relations if ($linearOnly) { throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass"); } // Join via has_many - $this->joinHasManyRelation($modelClass, $rel, $component); + $this->joinHasManyRelation($modelClass, $rel, $component, $parentPrefix, $tablePrefix, 'has_many'); $modelClass = $component; - } elseif ($component = $schema->manyManyComponent($modelClass, $rel)) { + continue; + } + + // check belongs_to (like has_many but linear safe) + if ($component = $schema->belongsToComponent($modelClass, $rel)) { + // Piggy back off has_many logic + $this->joinHasManyRelation($modelClass, $rel, $component, $parentPrefix, $tablePrefix, 'belongs_to'); + $modelClass = $component; + continue; + } + + // Check many_many + if ($component = $schema->manyManyComponent($modelClass, $rel)) { // Fail on non-linear relations if ($linearOnly) { throw new InvalidArgumentException("$rel is not a linear relation on model $modelClass"); @@ -804,31 +854,113 @@ class DataQuery $component['childClass'], $component['parentField'], $component['childField'], - $component['join'] + $component['join'], + $parentPrefix, + $tablePrefix ); $modelClass = $component['childClass']; - } else { - throw new InvalidArgumentException("$rel is not a relation on model $modelClass"); + continue; } + + // no relation + throw new InvalidArgumentException("$rel is not a relation on model $modelClass"); } return $modelClass; } + /** + * Join the given has_many relation to this query. + * Also works with belongs_to + * + * Doesn't work with polymorphic relationships + * + * @param string $localClass Name of class that has the has_many to the joined class + * @param string $localField Name of the has_many relationship to join + * @param string $foreignClass Class to join + * @param string $localPrefix Table prefix for parent class + * @param string $foreignPrefix Table prefix to use + * @param string $type 'has_many' or 'belongs_to' + */ + protected function joinHasManyRelation( + $localClass, + $localField, + $foreignClass, + $localPrefix = null, + $foreignPrefix = null, + $type = 'has_many' + ) { + if (!$foreignClass || $foreignClass === DataObject::class) { + throw new InvalidArgumentException("Could not find a has_many relationship {$localField} on {$localClass}"); + } + $schema = DataObject::getSchema(); + + // Skip if already joined + // Note: don't just check base class, since we need to join on the table with the actual relation key + $foreignTable = $schema->tableName($foreignClass); + $foreignTableAliased = $foreignPrefix . $foreignTable; + if ($this->query->isJoinedTo($foreignTableAliased)) { + return; + } + + // Join table with associated has_one + /** @var DataObject $model */ + $foreignKey = $schema->getRemoteJoinField($localClass, $localField, $type, $polymorphic); + $localIDColumn = $schema->sqlColumnForField($localClass, 'ID', $localPrefix); + if ($polymorphic) { + $foreignKeyIDColumn = $schema->sqlColumnForField($foreignClass, "{$foreignKey}ID", $foreignPrefix); + $foreignKeyClassColumn = $schema->sqlColumnForField($foreignClass, "{$foreignKey}Class", $foreignPrefix); + $localClassColumn = $schema->sqlColumnForField($localClass, 'ClassName', $localPrefix); + $joinExpression = + "{$foreignKeyIDColumn} = {$localIDColumn} AND {$foreignKeyClassColumn} = {$localClassColumn}"; + } else { + $foreignKeyIDColumn = $schema->sqlColumnForField($foreignClass, $foreignKey, $foreignPrefix); + $joinExpression = "{$foreignKeyIDColumn} = {$localIDColumn}"; + } + $this->query->addLeftJoin( + $foreignTable, + $joinExpression, + $foreignTableAliased + ); + + // Add join clause to the component's ancestry classes so that the search filter could search on + // its ancestor fields. + $ancestry = ClassInfo::ancestry($foreignClass, true); + $ancestry = array_reverse($ancestry); + foreach ($ancestry as $ancestor) { + $ancestorTable = $schema->tableName($ancestor); + if ($ancestorTable !== $foreignTable) { + $ancestorTableAliased = $foreignPrefix.$ancestorTable; + $this->query->addLeftJoin( + $ancestorTable, + "\"{$foreignTableAliased}\".\"ID\" = \"{$ancestorTableAliased}\".\"ID\"", + $ancestorTableAliased + ); + } + } + } + /** * Join the given class to this query with the given key * * @param string $localClass Name of class that has the has_one to the joined class * @param string $localField Name of the has_one relationship to joi * @param string $foreignClass Class to join + * @param string $localPrefix Table prefix to use for local class + * @param string $foreignPrefix Table prefix to use for joined table */ - protected function joinHasOneRelation($localClass, $localField, $foreignClass) - { + protected function joinHasOneRelation( + $localClass, + $localField, + $foreignClass, + $localPrefix = null, + $foreignPrefix = null + ) { if (!$foreignClass) { throw new InvalidArgumentException("Could not find a has_one relationship {$localField} on {$localClass}"); } - if ($foreignClass === 'SilverStripe\ORM\DataObject') { + if ($foreignClass === DataObject::class) { throw new InvalidArgumentException( "Could not join polymorphic has_one relationship {$localField} on {$localClass}" ); @@ -838,14 +970,18 @@ class DataQuery // Skip if already joined $foreignBaseClass = $schema->baseDataClass($foreignClass); $foreignBaseTable = $schema->tableName($foreignBaseClass); - if ($this->query->isJoinedTo($foreignBaseTable)) { + if ($this->query->isJoinedTo($foreignPrefix.$foreignBaseTable)) { return; } // Join base table - $foreignIDColumn = $schema->sqlColumnForField($foreignBaseClass, 'ID'); - $localColumn = $schema->sqlColumnForField($localClass, "{$localField}ID"); - $this->query->addLeftJoin($foreignBaseTable, "{$foreignIDColumn} = {$localColumn}"); + $foreignIDColumn = $schema->sqlColumnForField($foreignBaseClass, 'ID', $foreignPrefix); + $localColumn = $schema->sqlColumnForField($localClass, "{$localField}ID", $localPrefix); + $this->query->addLeftJoin( + $foreignBaseTable, + "{$foreignIDColumn} = {$localColumn}", + $foreignPrefix.$foreignBaseTable + ); // Add join clause to the component's ancestry classes so that the search filter could search on // its ancestor fields. @@ -855,63 +991,17 @@ class DataQuery foreach ($ancestry as $ancestor) { $ancestorTable = $schema->tableName($ancestor); if ($ancestorTable !== $foreignBaseTable) { - $this->query->addLeftJoin($ancestorTable, "{$foreignIDColumn} = \"{$ancestorTable}\".\"ID\""); + $ancestorTableAliased = $foreignPrefix.$ancestorTable; + $this->query->addLeftJoin( + $ancestorTable, + "{$foreignIDColumn} = \"{$ancestorTableAliased}\".\"ID\"", + $ancestorTableAliased + ); } } } } - /** - * Join the given has_many relation to this query. - * - * Doesn't work with polymorphic relationships - * - * @param string $localClass Name of class that has the has_many to the joined class - * @param string $localField Name of the has_many relationship to join - * @param string $foreignClass Class to join - */ - protected function joinHasManyRelation($localClass, $localField, $foreignClass) - { - if (!$foreignClass || $foreignClass === 'SilverStripe\ORM\DataObject') { - throw new InvalidArgumentException("Could not find a has_many relationship {$localField} on {$localClass}"); - } - $schema = DataObject::getSchema(); - - // Skip if already joined - $foreignTable = $schema->tableName($foreignClass); - if ($this->query->isJoinedTo($foreignTable)) { - return; - } - - // Join table with associated has_one - /** @var DataObject $model */ - $foreignKey = $schema->getRemoteJoinField($localClass, $localField, 'has_many', $polymorphic); - $localIDColumn = $schema->sqlColumnForField($localClass, 'ID'); - if ($polymorphic) { - $foreignKeyIDColumn = $schema->sqlColumnForField($foreignClass, "{$foreignKey}ID"); - $foreignKeyClassColumn = $schema->sqlColumnForField($foreignClass, "{$foreignKey}Class"); - $localClassColumn = $schema->sqlColumnForField($localClass, 'ClassName'); - $this->query->addLeftJoin( - $foreignTable, - "{$foreignKeyIDColumn} = {$localIDColumn} AND {$foreignKeyClassColumn} = {$localClassColumn}" - ); - } else { - $foreignKeyIDColumn = $schema->sqlColumnForField($foreignClass, $foreignKey); - $this->query->addLeftJoin($foreignTable, "{$foreignKeyIDColumn} = {$localIDColumn}"); - } - - // Add join clause to the component's ancestry classes so that the search filter could search on - // its ancestor fields. - $ancestry = ClassInfo::ancestry($foreignClass, true); - $ancestry = array_reverse($ancestry); - foreach ($ancestry as $ancestor) { - $ancestorTable = $schema->tableName($ancestor); - if ($ancestorTable !== $foreignTable) { - $this->query->addInnerJoin($ancestorTable, "\"{$foreignTable}\".\"ID\" = \"{$ancestorTable}\".\"ID\""); - } - } - } - /** * Join table via many_many relationship * @@ -921,6 +1011,8 @@ class DataQuery * @param string $parentField * @param string $componentField * @param string $relationClassOrTable Name of relation table + * @param string $parentPrefix Table prefix for parent class + * @param string $componentPrefix Table prefix to use for both joined and mapping table */ protected function joinManyManyRelationship( $relationClass, @@ -928,7 +1020,9 @@ class DataQuery $componentClass, $parentField, $componentField, - $relationClassOrTable + $relationClassOrTable, + $parentPrefix = null, + $componentPrefix = null ) { $schema = DataObject::getSchema(); @@ -936,23 +1030,30 @@ class DataQuery $relationClassOrTable = $schema->tableName($relationClassOrTable); } - // Join on parent table - $parentIDColumn = $schema->sqlColumnForField($parentClass, 'ID'); + // Check if already joined to component alias (skip join table for the check) + $componentBaseClass = $schema->baseDataClass($componentClass); + $componentBaseTable = $schema->tableName($componentBaseClass); + $componentAliasedTable = $componentPrefix . $componentBaseTable; + if ($this->query->isJoinedTo($componentAliasedTable)) { + return; + } + + // Join parent class to join table + $relationAliasedTable = $componentPrefix.$relationClassOrTable; + $parentIDColumn = $schema->sqlColumnForField($parentClass, 'ID', $parentPrefix); $this->query->addLeftJoin( $relationClassOrTable, - "\"$relationClassOrTable\".\"$parentField\" = {$parentIDColumn}" + "\"{$relationAliasedTable}\".\"{$parentField}\" = {$parentIDColumn}", + $relationAliasedTable ); // Join on base table of component class - $componentBaseClass = $schema->baseDataClass($componentClass); - $componentBaseTable = $schema->tableName($componentBaseClass); - $componentIDColumn = $schema->sqlColumnForField($componentBaseClass, 'ID'); - if (!$this->query->isJoinedTo($componentBaseTable)) { + $componentIDColumn = $schema->sqlColumnForField($componentBaseClass, 'ID', $componentPrefix); $this->query->addLeftJoin( $componentBaseTable, - "\"$relationClassOrTable\".\"$componentField\" = {$componentIDColumn}" + "\"{$relationAliasedTable}\".\"{$componentField}\" = {$componentIDColumn}", + $componentAliasedTable ); - } // Add join clause to the component's ancestry classes so that the search filter could search on // its ancestor fields. @@ -960,8 +1061,13 @@ class DataQuery $ancestry = array_reverse($ancestry); foreach ($ancestry as $ancestor) { $ancestorTable = $schema->tableName($ancestor); - if ($ancestorTable != $componentBaseTable && !$this->query->isJoinedTo($ancestorTable)) { - $this->query->addLeftJoin($ancestorTable, "{$componentIDColumn} = \"{$ancestorTable}\".\"ID\""); + if ($ancestorTable !== $componentBaseTable) { + $ancestorTableAliased = $componentPrefix.$ancestorTable; + $this->query->addLeftJoin( + $ancestorTable, + "{$componentIDColumn} = \"{$ancestorTableAliased}\".\"ID\"", + $ancestorTableAliased + ); } } } @@ -1060,7 +1166,6 @@ class DataQuery /** * An arbitrary store of query parameters that can be used by decorators. - * @todo This will probably be made obsolete if we have subclasses of DataList and/or DataQuery. */ private $queryParams; diff --git a/src/ORM/Filters/FulltextFilter.php b/src/ORM/Filters/FulltextFilter.php index 6336ab943..ff47362ff 100755 --- a/src/ORM/Filters/FulltextFilter.php +++ b/src/ORM/Filters/FulltextFilter.php @@ -91,9 +91,11 @@ class FulltextFilter extends SearchFilter */ protected function prepareColumns($columns) { + $prefix = DataQuery::applyRelationPrefix($this->relation); $table = DataObject::getSchema()->tableForField($this->model, current($columns)); - $columns = array_map(function ($column) use ($table) { - return Convert::symbol2sql("$table.$column"); + $fullTable = $prefix . $table; + $columns = array_map(function ($col) use ($fullTable) { + return "\"{$fullTable}\".\"{$col}\""; }, $columns); return implode(',', $columns); } diff --git a/src/ORM/Filters/SearchFilter.php b/src/ORM/Filters/SearchFilter.php index e7a077026..30392643b 100644 --- a/src/ORM/Filters/SearchFilter.php +++ b/src/ORM/Filters/SearchFilter.php @@ -2,6 +2,7 @@ namespace SilverStripe\ORM\Filters; +use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Injector\Injectable; use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataQuery; @@ -28,7 +29,11 @@ abstract class SearchFilter use Injectable; /** - * @var string Classname of the inspected {@link DataObject} + * Classname of the inspected {@link DataObject}. + * If pointing to a relation, this will be the classname of the leaf + * class in the relation + * + * @var string */ protected $model; @@ -53,11 +58,13 @@ abstract class SearchFilter protected $modifiers; /** - * @var string Name of a has-one, has-many or many-many relation (not the classname). + * @var array Parts of a has-one, has-many or many-many relation (not the classname). * Set in the constructor as part of the name in dot-notation, and used in * {@link applyRelation()}. + * + * Also used to build table prefix (see getRelationTablePrefix) */ - protected $relation; + protected $relation = []; /** * An array of data about an aggregate column being used @@ -137,11 +144,11 @@ abstract class SearchFilter * Set the root model class to be selected by this * search query. * - * @param string $className + * @param string|DataObject $className */ public function setModel($className) { - $this->model = $className; + $this->model = ClassInfo::class_name($className); } /** @@ -261,6 +268,7 @@ abstract class SearchFilter "Model supplied to " . static::class . " should be an instance of DataObject." ); } + $tablePrefix = DataQuery::applyRelationPrefix($this->relation); $schema = DataObject::getSchema(); if ($this->aggregate) { @@ -280,24 +288,25 @@ abstract class SearchFilter )); } return sprintf( - '%s("%s".%s)', + '%s("%s%s".%s)', $function, + $tablePrefix, $table, $column ? "\"$column\"" : '"ID"' ); } - // Find table this field belongs to + // Check if this column is a table on the current model $table = $schema->tableForField($this->model, $this->name); - if (!$table) { - // fallback to the provided name in the event of a joined column - // name (as the candidate class doesn't check joined records) - $parts = explode('.', $this->fullName); - return '"' . implode('"."', $parts) . '"'; + if ($table) { + return $schema->sqlColumnForField($this->model, $this->name, $tablePrefix); } - return sprintf('"%s"."%s"', $table, $this->name); + // fallback to the provided name in the event of a joined column + // name (as the candidate class doesn't check joined records) + $parts = explode('.', $this->fullName); + return '"' . implode('"."', $parts) . '"'; } /** diff --git a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php index 255ed269c..c39ee1788 100644 --- a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php +++ b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php @@ -136,12 +136,12 @@ class GridFieldSortableHeaderTest extends SapphireTest $config = new GridFieldConfig_RecordEditor(); $gridField = new GridField('testfield', 'testfield', $list, $config); $state = $gridField->State->GridFieldSortableHeader; - $compontent = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); + $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); // Test that inherited dataobjects will work correctly $state->SortColumn = 'Cheerleader.Hat.Colour'; $state->SortDirection = 'asc'; - $relationListA = $compontent->getManipulatedData($gridField, $list); + $relationListA = $component->getManipulatedData($gridField, $list); $relationListAsql = Convert::nl2os($relationListA->sql(), ' '); // Assert that all tables are joined properly @@ -153,12 +153,16 @@ class GridFieldSortableHeaderTest extends SapphireTest ); $this->assertContains( 'LEFT JOIN "GridFieldSortableHeaderTest_Cheerleader" ' - . 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Team"."CheerleaderID"', + . 'AS "cheerleader_GridFieldSortableHeaderTest_Cheerleader" ' + . 'ON "cheerleader_GridFieldSortableHeaderTest_Cheerleader"."ID" = ' + . '"GridFieldSortableHeaderTest_Team"."CheerleaderID"', $relationListAsql ); $this->assertContains( 'LEFT JOIN "GridFieldSortableHeaderTest_CheerleaderHat" ' - . 'ON "GridFieldSortableHeaderTest_CheerleaderHat"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."HatID"', + . 'AS "cheerleader_hat_GridFieldSortableHeaderTest_CheerleaderHat" ' + . 'ON "cheerleader_hat_GridFieldSortableHeaderTest_CheerleaderHat"."ID" = ' + . '"cheerleader_GridFieldSortableHeaderTest_Cheerleader"."HatID"', $relationListAsql ); @@ -168,7 +172,7 @@ class GridFieldSortableHeaderTest extends SapphireTest $relationListA->column('City') ); $state->SortDirection = 'desc'; - $relationListAdesc = $compontent->getManipulatedData($gridField, $list); + $relationListAdesc = $component->getManipulatedData($gridField, $list); $this->assertEquals( array('Melbourne', 'Wellington', 'Auckland', 'Cologne'), $relationListAdesc->column('City') @@ -177,7 +181,7 @@ class GridFieldSortableHeaderTest extends SapphireTest // Test subclasses of tables $state->SortColumn = 'CheerleadersMom.Hat.Colour'; $state->SortDirection = 'asc'; - $relationListB = $compontent->getManipulatedData($gridField, $list); + $relationListB = $component->getManipulatedData($gridField, $list); $relationListBsql = $relationListB->sql(); // Assert that subclasses are included in the query @@ -188,20 +192,27 @@ class GridFieldSortableHeaderTest extends SapphireTest $relationListBsql ); // Joined tables are joined basetable first + // Note: CheerLeader is base of Mom table, hence the alias $this->assertContains( 'LEFT JOIN "GridFieldSortableHeaderTest_Cheerleader" ' - . 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Team"."CheerleadersMomID"', + . 'AS "cheerleadersmom_GridFieldSortableHeaderTest_Cheerleader" ' + . 'ON "cheerleadersmom_GridFieldSortableHeaderTest_Cheerleader"."ID" = ' + . '"GridFieldSortableHeaderTest_Team"."CheerleadersMomID"', $relationListBsql ); // Then the basetable of the joined record is joined to the specific subtable $this->assertContains( 'LEFT JOIN "GridFieldSortableHeaderTest_Mom" ' - . 'ON "GridFieldSortableHeaderTest_Cheerleader"."ID" = "GridFieldSortableHeaderTest_Mom"."ID"', + . 'AS "cheerleadersmom_GridFieldSortableHeaderTest_Mom" ' + . 'ON "cheerleadersmom_GridFieldSortableHeaderTest_Cheerleader"."ID" = ' + . '"cheerleadersmom_GridFieldSortableHeaderTest_Mom"."ID"', $relationListBsql ); $this->assertContains( 'LEFT JOIN "GridFieldSortableHeaderTest_CheerleaderHat" ' - . 'ON "GridFieldSortableHeaderTest_CheerleaderHat"."ID" = "GridFieldSortableHeaderTest_Cheerleader"."HatID"', + . 'AS "cheerleadersmom_hat_GridFieldSortableHeaderTest_CheerleaderHat" ' + . 'ON "cheerleadersmom_hat_GridFieldSortableHeaderTest_CheerleaderHat"."ID" = ' + . '"cheerleadersmom_GridFieldSortableHeaderTest_Cheerleader"."HatID"', $relationListBsql ); @@ -212,7 +223,7 @@ class GridFieldSortableHeaderTest extends SapphireTest $relationListB->column('City') ); $state->SortDirection = 'desc'; - $relationListBdesc = $compontent->getManipulatedData($gridField, $list); + $relationListBdesc = $component->getManipulatedData($gridField, $list); $this->assertEquals( array('Melbourne', 'Wellington', 'Auckland', 'Cologne'), $relationListBdesc->column('City') diff --git a/tests/php/ORM/DataListTest.php b/tests/php/ORM/DataListTest.php index 703454b57..6b55bff21 100755 --- a/tests/php/ORM/DataListTest.php +++ b/tests/php/ORM/DataListTest.php @@ -5,10 +5,12 @@ namespace SilverStripe\ORM\Tests; use SilverStripe\Core\Convert; use SilverStripe\Core\Injector\InjectorNotFoundException; use SilverStripe\ORM\DataList; +use SilverStripe\ORM\DataQuery; use SilverStripe\ORM\DB; use SilverStripe\ORM\Filterable; use SilverStripe\ORM\Filters\ExactMatchFilter; use SilverStripe\Dev\SapphireTest; +use SilverStripe\ORM\Tests\DataObjectTest\Bracket; use SilverStripe\ORM\Tests\DataObjectTest\EquipmentCompany; use SilverStripe\ORM\Tests\DataObjectTest\Fan; use SilverStripe\ORM\Tests\DataObjectTest\Player; @@ -851,6 +853,69 @@ class DataListTest extends SapphireTest $this->assertEquals(2, $gtList->count()); } + /** + * Test that a filter correctly aliases relationships that share common classes + */ + public function testFilterSharedRelationalClasses() + { + /** @var Bracket $final1 */ + $final1 = $this->objFromFixture(Bracket::class, 'final'); + $prefinal1 = $this->objFromFixture(Bracket::class, 'prefinal1'); + $prefinal2 = $this->objFromFixture(Bracket::class, 'prefinal2'); + $semifinal1 = $this->objFromFixture(Bracket::class, 'semifinal1'); + $team2 = $this->objFromFixture(Team::class, 'team2'); + + // grand child can be found from parent + $found = Bracket::get()->filter('Next.Next.Title', $final1->Title); + $this->assertDOSEquals( + [['Title' => $semifinal1->Title]], + $found + ); + + // grand child can be found from child + $found = Bracket::get()->filter('Next.Title', $prefinal1->Title); + $this->assertDOSEquals( + [['Title' => $semifinal1->Title]], + $found + ); + + // child can be found from parent + $found = Bracket::get()->filter('Next.Title', $final1->Title); + $this->assertDOSEquals( + [ + ['Title' => $prefinal1->Title], + ['Title' => $prefinal2->Title] + ], + $found + ); + + // Complex filter, get brackets where the following bracket was won by team 1 + // Note: Includes results from multiple levels + $found = Bracket::get()->filter('Next.Winner.Title', $team2->Title); + $this->assertDOSEquals( + [ + ['Title' => $prefinal1->Title], + ['Title' => $prefinal2->Title], + ['Title' => $semifinal1->Title] + ], + $found + ); + } + + public function testFilterOnImplicitJoinWithSharedInheritance() + { + $list = DataObjectTest\RelationChildFirst::get()->filter(array( + 'ManyNext.ID' => array( + $this->idFromFixture(DataObjectTest\RelationChildSecond::class, 'test1'), + $this->idFromFixture(DataObjectTest\RelationChildSecond::class, 'test2'), + ), + )); + $this->assertEquals(2, $list->count()); + $ids = $list->column('ID'); + $this->assertContains($this->idFromFixture(DataObjectTest\RelationChildFirst::class, 'test1'), $ids); + $this->assertContains($this->idFromFixture(DataObjectTest\RelationChildFirst::class, 'test2'), $ids); + } + public function testFilterAny() { $list = TeamComment::get(); @@ -1229,13 +1294,13 @@ class DataListTest extends SapphireTest $filter = new ExactMatchFilter( 'Comments.Count()' ); - $filter->setModel(new DataObjectTest\Team()); - $this->assertEquals('COUNT("DataObjectTest_Team"."ID")', $filter->getDBName()); + $filter->apply(new DataQuery(DataObjectTest\Team::class)); + $this->assertEquals('COUNT("comments_DataObjectTest_TeamComment"."ID")', $filter->getDBName()); foreach (['Comments.Max(ID)', 'Comments.Max( ID )', 'Comments.Max( ID)'] as $name) { $filter = new ExactMatchFilter($name); - $filter->setModel(new DataObjectTest\Team()); - $this->assertEquals('MAX("DataObjectTest_Team"."ID")', $filter->getDBName()); + $filter->apply(new DataQuery(DataObjectTest\Team::class)); + $this->assertEquals('MAX("comments_DataObjectTest_TeamComment"."ID")', $filter->getDBName()); } } diff --git a/tests/php/ORM/DataObjectDuplicationTest/Class4.php b/tests/php/ORM/DataObjectDuplicationTest/Class4.php index 2bef1b24f..8d5aad1e4 100644 --- a/tests/php/ORM/DataObjectDuplicationTest/Class4.php +++ b/tests/php/ORM/DataObjectDuplicationTest/Class4.php @@ -13,6 +13,8 @@ use SilverStripe\ORM\ManyManyList; */ class Class4 extends DataObject implements TestOnly { + private static $table_name = 'DataObjectDuplicateTest_Class4'; + private static $db = [ 'Title' => 'Varchar', ]; diff --git a/tests/php/ORM/DataObjectTest.php b/tests/php/ORM/DataObjectTest.php index 3bce211a1..8af3bed35 100644 --- a/tests/php/ORM/DataObjectTest.php +++ b/tests/php/ORM/DataObjectTest.php @@ -51,6 +51,10 @@ class DataObjectTest extends SapphireTest DataObjectTest\Ploy::class, DataObjectTest\Bogey::class, DataObjectTest\Sortable::class, + DataObjectTest\Bracket::class, + DataObjectTest\RelationParent::class, + DataObjectTest\RelationChildFirst::class, + DataObjectTest\RelationChildSecond::class, ); protected static function getExtraDataObjects() diff --git a/tests/php/ORM/DataObjectTest.yml b/tests/php/ORM/DataObjectTest.yml index 57ea497bc..139f654c2 100644 --- a/tests/php/ORM/DataObjectTest.yml +++ b/tests/php/ORM/DataObjectTest.yml @@ -145,3 +145,37 @@ SilverStripe\ORM\Tests\DataObjectTest\Company: company1: Name: 'Team co.' Owner: =>SilverStripe\ORM\Tests\DataObjectTest\Player.player2 +SilverStripe\ORM\Tests\DataObjectTest\Bracket: + final: + Title: 'Final' + Winner: =>SilverStripe\ORM\Tests\DataObjectTest\Team.team2 + prefinal1: + Title: 'Prelim final 1' + Next: =>SilverStripe\ORM\Tests\DataObjectTest\Bracket.final + Winner: =>SilverStripe\ORM\Tests\DataObjectTest\Team.team2 + prefinal2: + Title: 'Prelim final 2' + Next: =>SilverStripe\ORM\Tests\DataObjectTest\Bracket.final + Winner: =>SilverStripe\ORM\Tests\DataObjectTest\Team.team1 + semifinal1: + Title: 'Semi final 1' + Next: =>SilverStripe\ORM\Tests\DataObjectTest\Bracket.prefinal1 + Winner: =>SilverStripe\ORM\Tests\DataObjectTest\Team.team2 +SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond: + test1: + Title: 'Test 1' + test2: + Title: 'Test 2' + test3: + Title: 'Test 3' +SilverStripe\ORM\Tests\DataObjectTest\RelationChildFirst: + test1: + Title: 'Test1' + ManyNext: + - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test1 + - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test2 + test2: + Title: 'Test2' + ManyNext: + - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test1 + - =>SilverStripe\ORM\Tests\DataObjectTest\RelationChildSecond.test3 diff --git a/tests/php/ORM/DataObjectTest/Bracket.php b/tests/php/ORM/DataObjectTest/Bracket.php new file mode 100644 index 000000000..8dc7249c2 --- /dev/null +++ b/tests/php/ORM/DataObjectTest/Bracket.php @@ -0,0 +1,32 @@ + 'Varchar(100)', + ]; + + private static $has_one = [ + 'Next' => Bracket::class, + 'First' => Team::class, + 'Second' => Team::class, + 'Winner' => Team::class, + ]; + + private static $has_many = [ + 'Previous' => Bracket::class, + ]; +} diff --git a/tests/php/ORM/DataObjectTest/CEO.php b/tests/php/ORM/DataObjectTest/CEO.php index 909aa97ae..99274fffc 100644 --- a/tests/php/ORM/DataObjectTest/CEO.php +++ b/tests/php/ORM/DataObjectTest/CEO.php @@ -7,8 +7,8 @@ class CEO extends Staff private static $table_name = 'DataObjectTest_CEO'; private static $belongs_to = array( - 'Company' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Company.CEO', - 'PreviousCompany' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Company.PreviousCEO', - 'CompanyOwned' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Company.Owner' + 'Company' => Company::class.'.CEO', + 'PreviousCompany' => Company::class.'.PreviousCEO', + 'CompanyOwned' => Company::class.'.Owner' ); } diff --git a/tests/php/ORM/DataObjectTest/Company.php b/tests/php/ORM/DataObjectTest/Company.php index 6ef791dbf..40ea9e562 100644 --- a/tests/php/ORM/DataObjectTest/Company.php +++ b/tests/php/ORM/DataObjectTest/Company.php @@ -20,7 +20,7 @@ class Company extends DataObject implements TestOnly ]; private static $has_many = array( - 'CurrentStaff' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Staff.CurrentCompany', - 'PreviousStaff' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Staff.PreviousCompany' + 'CurrentStaff' => Staff::class.'.CurrentCompany', + 'PreviousStaff' => Staff::class.'.PreviousCompany' ); } diff --git a/tests/php/ORM/DataObjectTest/Player.php b/tests/php/ORM/DataObjectTest/Player.php index 045fc68c7..ef49ce58f 100644 --- a/tests/php/ORM/DataObjectTest/Player.php +++ b/tests/php/ORM/DataObjectTest/Player.php @@ -24,13 +24,13 @@ class Player extends Member implements TestOnly ); private static $has_many = array( - 'Fans' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Fan.Favourite', // Polymorphic - Player fans - 'CaptainTeams' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Team.Captain', - 'FoundingTeams' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Team.Founder' + 'Fans' => Fan::class.'.Favourite', // Polymorphic - Player fans + 'CaptainTeams' => Team::class.'.Captain', + 'FoundingTeams' => Team::class.'.Founder' ); private static $belongs_to = array( - 'CompanyOwned' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Company.Owner' + 'CompanyOwned' => Company::class.'.Owner' ); private static $searchable_fields = array( diff --git a/tests/php/ORM/DataObjectTest/RelationChildFirst.php b/tests/php/ORM/DataObjectTest/RelationChildFirst.php new file mode 100644 index 000000000..2a86a30af --- /dev/null +++ b/tests/php/ORM/DataObjectTest/RelationChildFirst.php @@ -0,0 +1,12 @@ + RelationChildSecond::class, + ]; +} diff --git a/tests/php/ORM/DataObjectTest/RelationChildSecond.php b/tests/php/ORM/DataObjectTest/RelationChildSecond.php new file mode 100644 index 000000000..83687635d --- /dev/null +++ b/tests/php/ORM/DataObjectTest/RelationChildSecond.php @@ -0,0 +1,12 @@ + RelationChildFirst::class, + ]; +} diff --git a/tests/php/ORM/DataObjectTest/RelationParent.php b/tests/php/ORM/DataObjectTest/RelationParent.php new file mode 100644 index 000000000..62c528d09 --- /dev/null +++ b/tests/php/ORM/DataObjectTest/RelationParent.php @@ -0,0 +1,15 @@ + 'Varchar(255)', + ]; +} diff --git a/tests/php/ORM/DataObjectTest/Team.php b/tests/php/ORM/DataObjectTest/Team.php index dbe4350ad..b44317d11 100644 --- a/tests/php/ORM/DataObjectTest/Team.php +++ b/tests/php/ORM/DataObjectTest/Team.php @@ -39,8 +39,8 @@ class Team extends DataObject implements TestOnly private static $has_many = array( 'SubTeams' => SubTeam::class, 'Comments' => TeamComment::class, - 'Fans' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Fan.Favourite', // Polymorphic - Team fans - 'PlayerFans' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\Player.FavouriteTeam' + 'Fans' => Fan::class.'.Favourite', // Polymorphic - Team fans + 'PlayerFans' => Player::class.'.FavouriteTeam' ); private static $many_many = array( @@ -54,8 +54,8 @@ class Team extends DataObject implements TestOnly ); private static $belongs_many_many = array( - 'Sponsors' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\EquipmentCompany.SponsoredTeams', - 'EquipmentSuppliers' => 'SilverStripe\\ORM\\Tests\\DataObjectTest\\EquipmentCompany.EquipmentCustomers' + 'Sponsors' => EquipmentCompany::class.'.SponsoredTeams', + 'EquipmentSuppliers' => EquipmentCompany::class.'.EquipmentCustomers' ); private static $summary_fields = array( diff --git a/tests/php/ORM/DataQueryTest.php b/tests/php/ORM/DataQueryTest.php index 398f39c25..92fa58591 100644 --- a/tests/php/ORM/DataQueryTest.php +++ b/tests/php/ORM/DataQueryTest.php @@ -75,13 +75,13 @@ class DataQueryTest extends SapphireTest // Test applyRelation with two has_ones pointing to the same class $dq = new DataQuery(DataQueryTest\ObjectB::class); $dq->applyRelation('TestC'); - $this->assertTrue($dq->query()->isJoinedTo('DataQueryTest_C')); - $this->assertContains('"DataQueryTest_C"."ID" = "DataQueryTest_B"."TestCID"', $dq->sql()); + $this->assertTrue($dq->query()->isJoinedTo('testc_DataQueryTest_C')); + $this->assertContains('"testc_DataQueryTest_C"."ID" = "DataQueryTest_B"."TestCID"', $dq->sql()); $dq = new DataQuery(DataQueryTest\ObjectB::class); $dq->applyRelation('TestCTwo'); - $this->assertTrue($dq->query()->isJoinedTo('DataQueryTest_C')); - $this->assertContains('"DataQueryTest_C"."ID" = "DataQueryTest_B"."TestCTwoID"', $dq->sql()); + $this->assertTrue($dq->query()->isJoinedTo('testctwo_DataQueryTest_C')); + $this->assertContains('"testctwo_DataQueryTest_C"."ID" = "DataQueryTest_B"."TestCTwoID"', $dq->sql()); } public function testApplyReplationDeepInheretence() @@ -91,7 +91,7 @@ class DataQueryTest extends SapphireTest //apply a relation to a relation from an ancestor class $newDQ->applyRelation('TestA'); $this->assertTrue($newDQ->query()->isJoinedTo('DataQueryTest_C')); - $this->assertContains('"DataQueryTest_A"."ID" = "DataQueryTest_C"."TestAID"', $newDQ->sql($params)); + $this->assertContains('"testa_DataQueryTest_A"."ID" = "DataQueryTest_C"."TestAID"', $newDQ->sql($params)); //test many_many relation diff --git a/tests/php/ORM/ManyManyListTest.php b/tests/php/ORM/ManyManyListTest.php index eecbdc1d2..dc1ec8a6d 100644 --- a/tests/php/ORM/ManyManyListTest.php +++ b/tests/php/ORM/ManyManyListTest.php @@ -346,17 +346,11 @@ class ManyManyListTest extends SapphireTest public function testFilteringOnPreviouslyJoinedTable() { - - /** - * @var ManyManyListTest\Category $category -*/ + /** @var ManyManyListTest\Category $category */ $category = $this->objFromFixture(ManyManyListTest\Category::class, 'categorya'); - /** - * @var ManyManyList $productsRelatedToProductB -*/ - $productsRelatedToProductB = $category->Products()->filter('RelatedProducts.Title', 'Product B'); - + /** @var ManyManyList $productsRelatedToProductB */ + $productsRelatedToProductB = $category->Products()->filter('RelatedProducts.Title', 'Product A'); $this->assertEquals(1, $productsRelatedToProductB->count()); } } diff --git a/tests/php/ORM/ManyManyListTest/Category.php b/tests/php/ORM/ManyManyListTest/Category.php index b7d7bf28c..8ab7c67ba 100644 --- a/tests/php/ORM/ManyManyListTest/Category.php +++ b/tests/php/ORM/ManyManyListTest/Category.php @@ -4,7 +4,11 @@ namespace SilverStripe\ORM\Tests\ManyManyListTest; use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\ManyManyList; +/** + * @method ManyManyList Products() + */ class Category extends DataObject implements TestOnly { private static $table_name = 'ManyManyListTest_Category';