Merge pull request #5530 from dhensby/pulls/empty-from-clause

FIX Empty from clause when joining to same base table

model/DataQuery.php

@@ -657,9 +657,11 @@ class DataQuery {
 				$componentBaseClass = ClassInfo::baseDataClass($componentClass);
 				$this->query->addInnerJoin($relationTable,
 					"\"$relationTable\".\"$parentField\" = \"$parentBaseClass\".\"ID\"");
-				$this->query->addLeftJoin($componentBaseClass,
+				if (!$this->query->isJoinedTo($componentBaseClass)) {
-					"\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\"");
+					$this->query->addLeftJoin($componentBaseClass,
-				if(ClassInfo::hasTable($componentClass)) {
+						"\"$relationTable\".\"$componentField\" = \"$componentBaseClass\".\"ID\"");
+				}
+				if(ClassInfo::hasTable($componentClass)	&& !$this->query->isJoinedTo($componentClass)) {
 					$this->query->addLeftJoin($componentClass,
 						"\"$relationTable\".\"$componentField\" = \"$componentClass\".\"ID\"");
 				}

tests/model/DataListTest.php

@@ -20,6 +20,8 @@ class DataListTest extends SapphireTest {
 		'DataObjectTest_Player',
 		'DataObjectTest_TeamComment',
 		'DataObjectTest\NamespacedClass',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 
 	public function testFilterDataObjectByCreatedDate() {

tests/model/DataObjectLazyLoadingTest.php

@@ -23,7 +23,9 @@ class DataObjectLazyLoadingTest extends SapphireTest {
 		'DataObjectTest_Player',
 		'DataObjectTest_TeamComment',
 		'VersionedTest_DataObject',
-		'VersionedTest_Subclass'
+		'VersionedTest_Subclass',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 
 	public function testQueriedColumnsID() {

tests/model/DataObjectTest.php

@@ -17,7 +17,9 @@ class DataObjectTest extends SapphireTest {
 		'DataObjectTest_ValidatedObject',
 		'DataObjectTest_Player',
 		'DataObjectTest_TeamComment',
-		'DataObjectTest_ExtendedTeamComment'
+		'DataObjectTest_ExtendedTeamComment',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 
 	public function testDb() {

tests/model/DataObjectTest.yml

@@ -1,47 +1,57 @@
 DataObjectTest_Team:
-	team1:
+  team1:
-		Title: Team 1
+    Title: Team 1
-	team2:
+  team2:
-		Title: Team 2
+    Title: Team 2
-	team3:
+  team3:
-		Title: Team 3
+    Title: Team 3
 DataObjectTest_Player:
-	captain1:
+  captain1:
-		FirstName: Captain
+    FirstName: Captain
-		ShirtNumber: 007
+    ShirtNumber: 007
-		FavouriteTeam: =>DataObjectTest_Team.team1
+    FavouriteTeam: =>DataObjectTest_Team.team1
-		Teams: =>DataObjectTest_Team.team1
+    Teams: =>DataObjectTest_Team.team1
-		IsRetired: 1
+    IsRetired: 1
-	captain2:
+  captain2:
-		FirstName: Captain 2
+    FirstName: Captain 2
-		Teams: =>DataObjectTest_Team.team2
+    Teams: =>DataObjectTest_Team.team2
-	player1:
+  player1:
-		FirstName: Player 1
+    FirstName: Player 1
-	player2:
+  player2:
-		FirstName: Player 2
+    FirstName: Player 2
-		Teams: =>DataObjectTest_Team.team1,=>DataObjectTest_Team.team2
+    Teams: =>DataObjectTest_Team.team1,=>DataObjectTest_Team.team2
 DataObjectTest_SubTeam:
-	subteam1:
+  subteam1:
-		Title: Subteam 1
+    Title: Subteam 1
-		SubclassDatabaseField: Subclassed 1
+    SubclassDatabaseField: Subclassed 1
-		ExtendedDatabaseField: Extended 1
+    ExtendedDatabaseField: Extended 1
-		ParentTeam: =>DataObjectTest_Team.team1
+    ParentTeam: =>DataObjectTest_Team.team1
-	subteam2_with_player_relation:
+  subteam2_with_player_relation:
-		Title: Subteam 2
+    Title: Subteam 2
-		SubclassDatabaseField: Subclassed 2
+    SubclassDatabaseField: Subclassed 2
-		ExtendedHasOneRelationship: =>DataObjectTest_Player.player1
+    ExtendedHasOneRelationship: =>DataObjectTest_Player.player1
-	subteam3_with_empty_fields:
+  subteam3_with_empty_fields:
-		Title: Subteam 3
+    Title: Subteam 3
 DataObjectTest_TeamComment:
-	comment1:
+  comment1:
-		Name: Joe
+    Name: Joe
-		Comment: This is a team comment by Joe
+    Comment: This is a team comment by Joe
-		Team: =>DataObjectTest_Team.team1
+    Team: =>DataObjectTest_Team.team1
-	comment2:
+  comment2:
-		Name: Bob
+    Name: Bob
-		Comment: This is a team comment by Bob
+    Comment: This is a team comment by Bob
-		Team: =>DataObjectTest_Team.team1
+    Team: =>DataObjectTest_Team.team1
-	comment3:
+  comment3:
-		Name: Phil
+    Name: Phil
-		Comment: Phil is a unique guy, and comments on team2
+    Comment: Phil is a unique guy, and comments on team2
-		Team: =>DataObjectTest_Team.team2
+    Team: =>DataObjectTest_Team.team2
+ManyManyListTest_Product:
+  producta:
+    Title: 'Product A'
+  productb:
+    Title: 'Product B'
+    RelatedProducts: =>ManyManyListTest_Product.producta
+ManyManyListTest_Category:
+  categorya:
+    Title: 'Category A'
+    Products: =>ManyManyListTest_Product.producta,=>ManyManyListTest_Product.productb

tests/model/DataQueryTest.php

@@ -11,6 +11,7 @@ class DataQueryTest extends SapphireTest {
 		'DataQueryTest_D',
 		'DataQueryTest_E',
 		'DataQueryTest_F',
+		'DataQueryTest_G',
 	);
 
 
@@ -63,11 +64,39 @@ class DataQueryTest extends SapphireTest {
 	}
 
 	public function testApplyReplationDeepInheretence() {
+		//test has_one relation
 		$newDQ = new DataQuery('DataQueryTest_E');
 		//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());
+
+		//test many_many relation
+
+		//test many_many with separate inheritance
+		$newDQ = new DataQuery('DataQueryTest_C');
+		$baseDBTable = ClassInfo::baseDataClass('DataQueryTest_C');
+		$newDQ->applyRelation('ManyTestAs');
+		//check we are "joined" to the DataObject's table (there is no distinction between FROM or JOIN clauses)
+		$this->assertTrue($newDQ->query()->isJoinedTo($baseDBTable));
+		//check we are explicitly selecting "FROM" the DO's table
+		$this->assertContains("FROM \"$baseDBTable\"", $newDQ->sql());
+
+		//test many_many with shared inheritance
+		$newDQ = new DataQuery('DataQueryTest_E');
+		$baseDBTable = ClassInfo::baseDataClass('DataQueryTest_E');
+		//check we are "joined" to the DataObject's table (there is no distinction between FROM or JOIN clauses)
+		$this->assertTrue($newDQ->query()->isJoinedTo($baseDBTable));
+		//check we are explicitly selecting "FROM" the DO's table
+		$this->assertContains("FROM \"$baseDBTable\"", $newDQ->sql(), 'The FROM clause is missing from the query');
+		$newDQ->applyRelation('ManyTestGs');
+		//confirm we are still joined to the base table
+		$this->assertTrue($newDQ->query()->isJoinedTo($baseDBTable));
+		//double check it is the "FROM" clause
+		$this->assertContains("FROM \"$baseDBTable\"", $newDQ->sql(), 'The FROM clause has been removed from the query');
+		//another (potentially less crude check) for checking "FROM" clause
+		$fromTables = $newDQ->query()->getFrom();
+		$this->assertEquals('"' . $baseDBTable . '"', $fromTables[$baseDBTable]);
 	}
 
 	public function testRelationReturn() {
@@ -306,6 +335,10 @@ class DataQueryTest_E extends DataQueryTest_C implements TestOnly {
 		'SortOrder' => 'Int'
 	);
 
+	private static $many_many = array(
+		'ManyTestGs' => 'DataQueryTest_G',
+	);
+
 	private static $default_sort = '"DataQueryTest_E"."SortOrder" ASC';
 }
 
@@ -317,3 +350,11 @@ class DataQueryTest_F extends DataObject implements TestOnly {
 		'MyString' => 'Text'
 	);
 }
+
+class DataQueryTest_G extends DataQueryTest_C implements TestOnly {
+
+	private static $belongs_many_many = array(
+		'ManyTestEs' => 'DataQueryTest_E',
+	);
+
+}

tests/model/HasManyListTest.php

@@ -9,6 +9,8 @@ class HasManyListTest extends SapphireTest {
 		'DataObjectTest_Team',
 		'DataObjectTest_SubTeam',
 		'DataObjectTest_Player',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 	
 	public function testRelationshipEmptyOnNewRecords() {

tests/model/ManyManyListTest.php

@@ -15,6 +15,8 @@ class ManyManyListTest extends SapphireTest {
 		'DataObjectTest_Company',
 		'DataObjectTest_TeamComment',
 		'ManyManyListTest_ExtraFields',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 
 
@@ -267,6 +269,17 @@ class ManyManyListTest extends SapphireTest {
 		$this->assertEquals($expected, $list->sql());
 	}
 
+	public function testFilteringOnPreviouslyJoinedTable() {
+
+		/** @var ManyManyListTest_Category $category */
+		$category = $this->objFromFixture('ManyManyListTest_Category', 'categorya');
+
+		/** @var ManyManyList $productsRelatedToProductB */
+		$productsRelatedToProductB = $category->Products()->filter('RelatedProducts.Title', 'Product B');
+
+		$this->assertEquals(1, $productsRelatedToProductB->count());
+	}
+
 
 }
 
@@ -291,3 +304,33 @@ class ManyManyListTest_ExtraFields extends DataObject implements TestOnly {
 		)
 	);
 }
+
+class ManyManyListTest_Product extends DataObject implements TestOnly {
+
+	private static $db = array(
+		'Title' => 'Varchar'
+	);
+
+	private static $many_many = array(
+		'RelatedProducts' => 'ManyManyListTest_Product'
+	);
+
+	private static $belongs_many_many = array(
+		'RelatedTo' => 'ManyManyListTest_Product',
+		'Categories' => 'ManyManyListTest_Category'
+	);
+
+}
+
+class ManyManyListTest_Category extends DataObject implements TestOnly {
+
+	private static $db = array(
+		'Title' => 'Varchar'
+	);
+
+	private static $many_many = array(
+		'Products' => 'ManyManyListTest_Product'
+	);
+
+}
+

tests/model/MapTest.php

@@ -18,7 +18,9 @@ class SS_MapTest extends SapphireTest {
 		'DataObjectTest_FieldlessSubTable',
 		'DataObjectTest_ValidatedObject',
 		'DataObjectTest_Player',
-		'DataObjectTest_TeamComment'
+		'DataObjectTest_TeamComment',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 	
 

tests/model/PaginatedListTest.php

@@ -12,7 +12,9 @@ class PaginatedListTest extends SapphireTest {
 	protected $extraDataObjects = array(
 		'DataObjectTest_Team',		
 		'DataObjectTest_SubTeam',
-		'DataObjectTest_Player'
+		'DataObjectTest_Player',
+		'ManyManyListTest_Product',
+		'ManyManyListTest_Category',
 	);
 
 	public function testPageStart() {