Merge pull request #7024 from kinglozzer/hasmanylist-ambiguity

FIX: Ensure HasManyList foreign ID filter includes table name (fixes #7023)

model/HasManyList.php

@@ -44,8 +44,14 @@ class HasManyList extends RelationList {
 	protected function foreignIDFilter($id = null) {
 		if ($id === null) $id = $this->getForeignID();
 
-		// Apply relation filter
+		// Try to include the table name for the given foreign key
+		if ($table = ClassInfo::table_for_object_field($this->dataClass, $this->foreignKey)) {
+			$key = "\"$table\".\"$this->foreignKey\"";
+		} else {
 			$key = "\"$this->foreignKey\"";
+		}
+
+		// Apply relation filter
 		if(is_array($id)) {
 			return array("$key IN (".DB::placeholders($id).")"  => $id);
 		} else if($id !== null){

tests/model/HasManyListTest.php

@@ -2,16 +2,26 @@
 
 class HasManyListTest extends SapphireTest {
 
-	// Borrow the model from DataObjectTest
+	protected static $fixture_file = array(
-	protected static $fixture_file = 'DataObjectTest.yml';
+		'DataObjectTest.yml', // Borrow the model from DataObjectTest
+		'HasManyListTest.yml'
+	);
 
 	protected $extraDataObjects = array(
 		'DataObjectTest_Team',
 		'DataObjectTest_SubTeam',
 		'DataObjectTest_Player',
 		'DataObjectTest_TeamComment',
+		'DataObjectTest_Sortable',
+		'DataObjectTest_Company',
+		'DataObjectTest_EquipmentCompany',
+		'DataObjectTest_SubEquipmentCompany',
+		'DataObjectTest_Fan',
 		'ManyManyListTest_Product',
 		'ManyManyListTest_Category',
+		'HasManyListTest_Company',
+		'HasManyListTest_Employee',
+		'HasManyListTest_CompanyCar',
 	);
 
 	public function testRelationshipEmptyOnNewRecords() {
@@ -59,4 +69,61 @@ class HasManyListTest extends SapphireTest {
 		$this->assertEmpty($team2comment->TeamID);
 	}
 
+	/**
+	 * Test that multiple models with the same "has_one" relation name (and therefore the same "<hasone>ID"
+	 * column name) do not trigger a "Column '<hasone>ID' in where clause is ambiguous" error
+	 */
+	public function testAmbiguousRelationshipNames() {
+		$company = $this->objFromFixture('HasManyListTest_Company', 'silverstripe');
+
+		$johnsCars = $company->CompanyCars()->filter(array('User.Name' => 'John Smith'));
+		$this->assertCount(1, $johnsCars, 'John Smith has one company car');
+
+		$jennysCars = $company->CompanyCars()->filter(array('User.Name' => 'Jenny Smith'));
+		$this->assertCount(2, $jennysCars, 'Jenny Smith has two company cars');
+	}
+
+}
+
+class HasManyListTest_Company extends DataObject implements TestOnly {
+
+	private static $db = array(
+		'Name' => 'Varchar(100)'
+	);
+
+	private static $has_many = array(
+		'Employees' => 'HasManyListTest_Employee',
+		'CompanyCars' => 'HasManyListTest_CompanyCar'
+	);
+
+}
+
+class HasManyListTest_Employee extends DataObject implements TestOnly {
+
+	private static $db = array(
+		'Name' => 'Varchar(100)'
+	);
+
+	private static $has_one = array(
+		'Company' => 'HasManyListTest_Company'
+	);
+
+	private static $has_many = array(
+		'CompanyCars' => 'HasManyListTest_CompanyCar'
+	);
+
+}
+
+class HasManyListTest_CompanyCar extends DataObject implements TestOnly {
+
+	private static $db = array(
+		'Make' => 'Varchar(100)',
+		'Model' => 'Varchar(100)'
+	);
+
+	private static $has_one = array(
+		'User' => 'HasManyListTest_Employee',
+		'Company' => 'HasManyListTest_Company'
+	);
+
 }

tests/model/HasManyListTest.yml

@@ -0,0 +1,26 @@
+HasManyListTest_Company:
+  silverstripe:
+    Name: 'SilverStripe Ltd'
+HasManyListTest_Employee:
+  john:
+    Name: 'John Smith'
+    Company: =>HasManyListTest_Company.silverstripe
+  jenny:
+    Name: 'Jenny Smith'
+    Company: =>HasManyListTest_Company.silverstripe
+HasManyListTest_CompanyCar:
+  jaguar:
+    Make: 'Jaguar'
+    Model: 'E Type'
+    User: =>HasManyListTest_Employee.john
+    Company: =>HasManyListTest_Company.silverstripe
+  ferrari:
+    Make: 'Ferrari'
+    Model: 'F40'
+    User: =>HasManyListTest_Employee.jenny
+    Company: =>HasManyListTest_Company.silverstripe
+  lamborghini:
+    Make: 'Lamborghini'
+    Model: 'Countach'
+    User: =>HasManyListTest_Employee.jenny
+    Company: =>HasManyListTest_Company.silverstripe