Merge pull request #273 from rchavik/#275

Fixes #275 (lighthouse) Old HABTM links should not be deleted when new HABTM links contain these
cakephp · Dec 13, 2011 · 8444911 · 8444911
2 parents ab5bfdc + 8b4e79c
commit 8444911
Show file tree

Hide file tree

Showing 9 changed files with 394 additions and 12 deletions.
diff --git a/lib/Cake/Console/Templates/default/classes/model.ctp b/lib/Cake/Console/Templates/default/classes/model.ctp
@@ -154,7 +154,7 @@ if (!empty($associations['hasAndBelongsToMany'])):
 		$out .= "\t\t\t'joinTable' => '{$relation['joinTable']}',\n";
 		$out .= "\t\t\t'foreignKey' => '{$relation['foreignKey']}',\n";
 		$out .= "\t\t\t'associationForeignKey' => '{$relation['associationForeignKey']}',\n";
-		$out .= "\t\t\t'unique' => true,\n";
+		$out .= "\t\t\t'unique' => 'keepExisting',\n";
 		$out .= "\t\t\t'conditions' => '',\n";
 		$out .= "\t\t\t'fields' => '',\n";
 		$out .= "\t\t\t'order' => '',\n";

diff --git a/lib/Cake/Model/Datasource/DboSource.php b/lib/Cake/Model/Datasource/DboSource.php
@@ -2782,15 +2782,15 @@ public function insertMulti($table, $fields, $values) {
 		$statement = $this->_connection->prepare($sql);
 		$this->begin();
 
-		foreach ($values[0] as $key => $val) {
+		foreach ($values[key($values)] as $key => $val) {
 			$type = $this->introspectType($val);
 			$columnMap[$key] = $pdoMap[$type];
 		}
 
-		for ($x = 0; $x < $count; $x++) {
+		foreach ($values as $row => $value) {
 			$i = 1;
-			foreach ($values[$x] as $key => $val) {
-				$statement->bindValue($i, $val, $columnMap[$key]);
+			foreach ($value as $col => $val) {
+				$statement->bindValue($i, $val, $columnMap[$col]);
 				$i += 1;
 			}
 			$statement->execute();

diff --git a/lib/Cake/Model/Model.php b/lib/Cake/Model/Model.php
@@ -1715,7 +1715,7 @@ protected function _saveMulti($joined, $id, $db) {
 					)
 				);
 
-				$newData = $newValues = array();
+				$newData = $newValues = $newJoins = array();
 				$primaryAdded = false;
 
 				$fields =  array(
@@ -1731,11 +1731,12 @@ protected function _saveMulti($joined, $id, $db) {
 
 				foreach ((array)$data as $row) {
 					if ((is_string($row) && (strlen($row) == 36 || strlen($row) == 16)) || is_numeric($row)) {
+						$newJoins[] = $row;
 						$values = array($id, $row);
 						if ($isUUID && $primaryAdded) {
 							$values[] = String::uuid();
 						}
-						$newValues[] = $values;
+						$newValues[$row] = $values;
 						unset($values);
 					} elseif (isset($row[$this->hasAndBelongsToMany[$assoc]['associationForeignKey']])) {
 						$newData[] = $row;
@@ -1744,23 +1745,28 @@ protected function _saveMulti($joined, $id, $db) {
 					}
 				}
 
+				$keepExisting = $this->hasAndBelongsToMany[$assoc]['unique'] === 'keepExisting';
 				if ($this->hasAndBelongsToMany[$assoc]['unique']) {
 					$conditions = array(
 						$join . '.' . $this->hasAndBelongsToMany[$assoc]['foreignKey'] => $id
 					);
 					if (!empty($this->hasAndBelongsToMany[$assoc]['conditions'])) {
 						$conditions = array_merge($conditions, (array)$this->hasAndBelongsToMany[$assoc]['conditions']);
 					}
+					$associationForeignKey = $this->{$join}->alias .'.'. $this->hasAndBelongsToMany[$assoc]['associationForeignKey'];
 					$links = $this->{$join}->find('all', array(
 						'conditions' => $conditions,
 						'recursive' => empty($this->hasAndBelongsToMany[$assoc]['conditions']) ? -1 : 0,
-						'fields' => $this->hasAndBelongsToMany[$assoc]['associationForeignKey']
+						'fields' => $associationForeignKey,
 					));
 
-					$associationForeignKey = "{$join}." . $this->hasAndBelongsToMany[$assoc]['associationForeignKey'];
 					$oldLinks = Set::extract($links, "{n}.{$associationForeignKey}");
 					if (!empty($oldLinks)) {
- 						$conditions[$associationForeignKey] = $oldLinks;
+						if ($keepExisting && !empty($newJoins)) {
+							$conditions[$associationForeignKey] = array_diff($oldLinks, $newJoins);
+						} else {
+							$conditions[$associationForeignKey] = $oldLinks;
+						}
 						$dbMulti->delete($this->{$join}, $conditions);
 					}
 				}
@@ -1774,7 +1780,21 @@ protected function _saveMulti($joined, $id, $db) {
 				}
 
 				if (!empty($newValues)) {
-					$dbMulti->insertMulti($this->{$join}, $fields, $newValues);
+					if ($keepExisting && !empty($links)) {
+						foreach ($links as $link) {
+							$oldJoin = $link[$join][$this->hasAndBelongsToMany[$assoc]['associationForeignKey']];
+							if (! in_array($oldJoin, $newJoins) ) {
+								$conditions[$associationForeignKey] = $oldJoin;
+								$db->delete($this->{$join}, $conditions);
+							} else {
+								unset($newValues[$oldJoin]);
+							}
+						}
+						$newValues = array_values($newValues);
+					}
+					if (!empty($newValues)) {
+						$dbMulti->insertMulti($this->{$join}, $fields, $newValues);
+					}
 				}
 			}
 		}

diff --git a/lib/Cake/Test/Case/Model/ModelIntegrationTest.php b/lib/Cake/Test/Case/Model/ModelIntegrationTest.php
@@ -623,6 +623,158 @@ public function testCrossDatabaseJoins() {
 		$this->assertEquals($TestModel->Comment->find('all'), $expected);
 	}
 
+/**
+ * test HABM operations without clobbering existing records #275
+ *
+ * @return void
+ */
+	function testHABTMKeepExisting() {
+		$this->loadFixtures('Site', 'Domain', 'DomainsSite');
+
+		$Site = new Site();
+		$results = $Site->find('count');
+		$expected = 3;
+		$this->assertEquals($results, $expected);
+
+		$data = $Site->findById(1);
+
+		// include api.cakephp.org
+		$data['Domain'] = array('Domain' => array(1, 2, 3));
+		$Site->save($data);
+
+		$Site->id = 1;
+		$results = $Site->read();
+		$expected = 3; // 3 domains belonging to cakephp
+		$this->assertEquals($expected, count($results['Domain']));
+
+
+		$Site->id = 2;
+		$results = $Site->read();
+		$expected = 2; // 2 domains belonging to markstory
+		$this->assertEquals($expected, count($results['Domain']));
+
+		$Site->id = 3;
+		$results = $Site->read();
+		$expected = 2;
+		$this->assertEquals($expected, count($results['Domain']));
+		$results['Domain'] = array('Domain' => array(7));
+		$Site->save($results); // remove association from domain 6
+		$results = $Site->read();
+		$expected = 1; // only 1 domain left belonging to rchavik
+		$this->assertEquals($expected, count($results['Domain']));
+
+		// add deleted domain back
+		$results['Domain'] = array('Domain' => array(6, 7));
+		$Site->save($results);
+		$results = $Site->read();
+		$expected = 2; // 2 domains belonging to rchavik
+		$this->assertEquals($expected, count($results['Domain']));
+
+		$Site->DomainsSite->id = $results['Domain'][0]['DomainsSite']['id'];
+		$Site->DomainsSite->saveField('active', true);
+
+		$results = $Site->Domain->DomainsSite->find('count', array(
+			'conditions' => array(
+				'DomainsSite.active' => true,
+				),
+			));
+		$expected = 5;
+		$this->assertEquals($expected, $results);
+
+		// activate api.cakephp.org
+		$activated = $Site->DomainsSite->findByDomainId(3);
+		$activated['DomainsSite']['active'] = true;
+		$Site->DomainsSite->save($activated);
+
+		$results = $Site->DomainsSite->find('count', array(
+			'conditions' => array(
+				'DomainsSite.active' => true,
+				),
+			));
+		$expected = 6;
+		$this->assertEquals($expected, $results);
+
+		// remove 2 previously active domains, and leave $activated alone
+		$data = array(
+			'Site' => array('id' => 1, 'name' => 'cakephp (modified)'),
+			'Domain' => array(
+				'Domain' => array(3),
+				)
+			);
+		$Site->create($data);
+		$Site->save($data);
+
+		// tests that record is still identical prior to removal
+		$Site->id = 1;
+		$results = $Site->read();
+		unset($results['Domain'][0]['DomainsSite']['updated']);
+		unset($activated['DomainsSite']['updated']);
+		$this->assertEquals($activated['DomainsSite'], $results['Domain'][0]['DomainsSite']);
+	}
+
+/**
+ * test HABM operations without clobbering existing records #275
+ *
+ * @return void
+ */
+	function testHABTMKeepExistingWithThreeDbs() {
+		$config = ConnectionManager::enumConnectionObjects();
+		$this->skipIf(
+			!isset($config['test']) || !isset($config['test2']) || !isset($config['test_database_three']),
+			'Primary, secondary, and tertiary test databases not configured, skipping test.  To run this test define $test, $test2, and $test_database_three in your database configuration.'
+			);
+
+		$this->loadFixtures('Player', 'Guild', 'GuildsPlayer', 'Armor', 'ArmorsPlayer');
+		$Player = ClassRegistry::init('Player');
+		$Player->bindModel(array(
+			'hasAndBelongsToMany' => array(
+				'Armor' => array(
+					'with' => 'ArmorsPlayer',
+					'unique' => 'keepExisting',
+					),
+				),
+			), false);
+		$this->assertEquals('test', $Player->useDbConfig);
+		$this->assertEquals('test', $Player->Guild->useDbConfig);
+		$this->assertEquals('test2', $Player->Guild->GuildsPlayer->useDbConfig);
+		$this->assertEquals('test2', $Player->Armor->useDbConfig);
+		$this->assertEquals('test_database_three', $Player->ArmorsPlayer->useDbConfig);
+
+		$players = $Player->find('all');
+		$this->assertEquals(4 , count($players));
+		$playersGuilds = Set::extract('/Guild/GuildsPlayer', $players);
+		$this->assertEquals(3 , count($playersGuilds));
+		$playersArmors = Set::extract('/Armor/ArmorsPlayer', $players);
+		$this->assertEquals(3 , count($playersArmors));
+		unset($players);
+
+		$larry = $Player->findByName('larry');
+		$larrysArmor = Set::extract('/Armor/ArmorsPlayer', $larry);
+		$this->assertEquals(1 , count($larrysArmor));
+
+		$larry['Guild']['Guild'] = array(1, 3); // larry joins another guild
+		$larry['Armor']['Armor'] = array(2, 3); // purchases chainmail
+		$Player->save($larry);
+		unset($larry);
+
+		$larry = $Player->findByName('larry');
+		$larrysGuild = Set::extract('/Guild/GuildsPlayer', $larry);
+		$this->assertEquals(2 , count($larrysGuild));
+		$larrysArmor = Set::extract('/Armor/ArmorsPlayer', $larry);
+		$this->assertEquals(2 , count($larrysArmor));
+
+		$larrysArmorsPlayersIds = Set::extract('/Armor/ArmorsPlayer/id', $larry);
+
+		$Player->ArmorsPlayer->id = 3;
+		$Player->ArmorsPlayer->saveField('broken', true); // larry's cloak broke
+
+		$larry = $Player->findByName('larry');
+		$larrysArmor = Set::extract('/Armor/ArmorsPlayer', $larry);
+		$larrysCloak = Set::extract('/ArmorsPlayer[armor_id=3]', $larrysArmor);
+		$this->assertNotEmpty($larrysCloak);
+		$this->assertTrue($larrysCloak[0]['ArmorsPlayer']['broken']); // still broken
+	}
+
 /**
  * testDisplayField method
  *

diff --git a/lib/Cake/Test/Case/Model/ModelTestBase.php b/lib/Cake/Test/Case/Model/ModelTestBase.php
@@ -70,7 +70,7 @@ abstract class BaseModelTest extends CakeTestCase {
 		'core.uuiditems_uuidportfolio', 'core.uuiditems_uuidportfolio_numericid', 'core.fruit',
 		'core.fruits_uuid_tag', 'core.uuid_tag', 'core.product_update_all', 'core.group_update_all',
 		'core.player', 'core.guild', 'core.guilds_player', 'core.armor', 'core.armors_player',
-		'core.bidding', 'core.bidding_message'
+		'core.bidding', 'core.bidding_message', 'core.site', 'core.domain', 'core.domains_site',
 	);
 
 /**

diff --git a/lib/Cake/Test/Case/Model/models.php b/lib/Cake/Test/Case/Model/models.php
@@ -3279,6 +3279,24 @@ public function afterSave($created) {
 	}
 }
 
+class Site extends CakeTestModel {
+	var $name = 'Site';
+	var $useTable = 'sites';
+
+	var $hasAndBelongsToMany = array(
+		'Domain' => array('unique' => 'keepExisting'),
+		);
+}
+
+class Domain extends CakeTestModel {
+	var $name = 'Domain';
+	var $useTable = 'domains';
+
+	var $hasAndBelongsToMany = array(
+		'Site' => array('unique' => 'keepExisting'),
+		);
+}
+
 /**
  * TestModel class
  *

diff --git a/lib/Cake/Test/Fixture/DomainFixture.php b/lib/Cake/Test/Fixture/DomainFixture.php
@@ -0,0 +1,65 @@
+<?php
+/**
+ * Short description for file.
+ *
+ * PHP versions 5
+ *
+ * CakePHP(tm) Tests <http://book.cakephp.org/view/1196/Testing>
+ * Copyright 2005-2010, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ *  Licensed under The Open Group Test Suite License
+ *  Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright 2005-2010, Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link          http://book.cakephp.org/view/1196/Testing CakePHP(tm) Tests
+ * @package       cake
+ * @subpackage    cake.tests.fixtures
+ * @since         CakePHP(tm) v 2.1
+ * @license       http://www.opensource.org/licenses/opengroup.php The Open Group Test Suite License
+ */
+
+/**
+ * Short description for class.
+ *
+ * @package       cake
+ * @subpackage    cake.tests.fixtures
+ */
+class DomainFixture extends CakeTestFixture {
+
+/**
+ * name property
+ *
+ * @var string 'Domain'
+ * @access public
+ */
+	var $name = 'Domain';
+
+/**
+ * fields property
+ *
+ * @var array
+ * @access public
+ */
+	var $fields = array(
+		'id' => array('type' => 'integer', 'key' => 'primary'),
+		'domain' => array('type' => 'string', 'null' => false),
+		'created' => 'datetime',
+		'updated' => 'datetime'
+	);
+
+/**
+ * records property
+ *
+ * @var array
+ * @access public
+ */
+	var $records = array(
+		array('domain' => 'cakephp.org', 'created' => '2007-03-17 01:16:23', 'updated' => '2007-03-17 01:18:31'),
+		array('domain' => 'book.cakephp.org', 'created' => '2007-03-17 01:16:23', 'updated' => '2007-03-17 01:18:31'),
+		array('domain' => 'api.cakephp.org', 'created' => '2007-03-17 01:16:23', 'updated' => '2007-03-17 01:18:31'),
+		array('domain' => 'mark-story.com', 'created' => '2007-03-17 01:18:23', 'updated' => '2007-03-17 01:20:31'),
+		array('domain' => 'tinadurocher.com', 'created' => '2007-03-17 01:18:23', 'updated' => '2007-03-17 01:20:31'),
+		array('domain' => 'chavik.com', 'created' => '2001-02-03 00:01:02', 'updated' => '2007-03-17 01:22:31'),
+		array('domain' => 'xintesa.com', 'created' => '2001-02-03 00:01:02', 'updated' => '2007-03-17 01:22:31'),
+	);
+}