fixes #364, optimized the bulkDelete for composite primary keys, security...

fixes #364, optimized the bulkDelete for composite primary keys, security check for circular references in cascading saves

lib/Doctrine/Collection.php

@@ -635,6 +635,7 @@ class Doctrine_Collection extends Doctrine_Access implements Countable, Iterator
             $record->delete($conn);
         }
 
+        Doctrine::debug(true);
         $conn->commit();
 
         $this->data = array();

lib/Doctrine/Connection/UnitOfWork.php

@@ -139,9 +139,16 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module
      */
     public function saveGraph(Doctrine_Record $record)
     {
-    	$conn = $this->getConnection();
+    	$conn = $this->getConnection();  
+        
+        if ($conn->transaction->isSaved($record)) {
+            return false;
+        }
+
+        $conn->transaction->addSaved($record);
 
         $conn->beginTransaction();
+
         $saveLater = $this->saveRelated($record);
 
         if ($record->isValid()) {
@@ -163,6 +170,8 @@ class Doctrine_Connection_UnitOfWork extends Doctrine_Connection_Module
         $this->saveAssociations($record);
 
         $conn->commit();
+        
+        return true;
     }
     /**
      * saves the given record

lib/Doctrine/Transaction.php

@@ -119,7 +119,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      */
     public function isSaved(Doctrine_Record $record)
     {
-        return in_array($this->_saved, $record);
+        return in_array($record, $this->_saved, true);
     }
 
     /**
@@ -191,31 +191,38 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
      */
     public function bulkDelete()
     {
+
         foreach ($this->delete as $name => $deletes) {
             $record = false;
             $ids    = array();
 
     	    if (is_array($deletes[count($deletes)-1]->getTable()->getIdentifier())) {
-                foreach ($deletes as $k => $record) {
-                    $cond = array();
-                    $ids = $record->obtainIdentifier();
-                    $query = 'DELETE FROM ' 
-                           . $this->conn->quoteIdentifier($record->getTable()->getTableName()) 
+                if (count($deletes) > 0) {
+                    $query = 'DELETE FROM '
+                           . $this->conn->quoteIdentifier($deletes[0]->getTable()->getTableName())
                            . ' WHERE ';
-
-                    foreach (array_keys($ids) as $id){
-                        $cond[] = $id . ' = ? ';
+    
+                    $params = array();
+                    $cond = array();
+                    foreach ($deletes as $k => $record) {
+                        $ids = $record->obtainIdentifier();
+                        $tmp = array();
+                        foreach (array_keys($ids) as $id){
+                            $tmp[] = $id . ' = ? ';
+                        }
+                        $params = array_merge($params, array_values($ids));
+                        $cond[] = '(' . implode(' AND ', $tmp) . ')';
                     }
+                    $query .= implode(' OR ', $cond);
 
-                    $query = $query . implode(' AND ', $cond);
-                    $this->conn->execute($query, array_values($ids));
+                    $this->conn->execute($query, $params);
                 }
     	    } else {
     		    foreach ($deletes as $k => $record) {
                     $ids[] = $record->getIncremented();
     		    }
     		    if ($record instanceof Doctrine_Record) {
-        			$params = substr(str_repeat('?, ', count($ids)),0,-2);
+        			$params = substr(str_repeat('?, ', count($ids)), 0, -2);
     
         			$query = 'DELETE FROM '
         				   . $this->conn->quoteIdentifier($record->getTable()->getTableName())
@@ -340,6 +347,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
 
             $listener->postSavepointCommit($event);
         } else {
+
             if ($this->transactionLevel == 1) {
                 $event = new Doctrine_Event($this, Doctrine_Event::TX_COMMIT);
                 
@@ -348,7 +356,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
                 if ( ! $event->skipOperation) {
                     try {
                         $this->bulkDelete();
-    
+
                     } catch(Exception $e) {
                         $this->rollback();
     
@@ -368,7 +376,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
                         $coll->takeSnapshot();
                     }
                     $this->_collections = array();
-    
+                    $this->_saved = array();
                     $this->conn->getDbh()->commit();
     
                     //$this->conn->unitOfWork->reset();
@@ -434,7 +442,8 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
                     throw new Doctrine_Transaction_Exception($e->getMessage());
                 }
             }
-            
+            $this->_saved = array();
+
             $listener->postTransactionRollback($event);
         }
 
@@ -506,7 +515,7 @@ class Doctrine_Transaction extends Doctrine_Connection_Module
 
         return $i;
     }
-    
+
     /**
      * setIsolation
      *

tests/Query/CacheTestCase.php

@@ -32,6 +32,7 @@
  */
 class Doctrine_Query_Cache_TestCase extends Doctrine_UnitTestCase 
 {
+
     public function testResultSetCacheAddsResultSetsIntoCache()
     {
         $q = new Doctrine_Query();
@@ -51,6 +52,7 @@ class Doctrine_Query_Cache_TestCase extends Doctrine_UnitTestCase
         $this->assertTrue($coll instanceof Doctrine_Collection);
         $this->assertEqual($coll->count(), 8);
     }
+
     public function testResultSetCacheSupportsQueriesWithJoins()
     {
         $q = new Doctrine_Query();
@@ -70,6 +72,7 @@ class Doctrine_Query_Cache_TestCase extends Doctrine_UnitTestCase
         $this->assertTrue($coll instanceof Doctrine_Collection);
         $this->assertEqual($coll->count(), 8);
     }
+    
     public function testResultSetCacheSupportsPreparedStatements()
     {
         $q = new Doctrine_Query();

tests/Ticket364TestCase.php

@@ -41,7 +41,23 @@ class Doctrine_Ticket364_TestCase extends Doctrine_UnitTestCase {
         
         parent::prepareTables();
     }
-	
+
+	public function testMultiplePrimaryKeys()
+	{
+        $r = new Doctrine_Collection('NestReference');
+        $r[0]->parent_id = 1;
+        $r[0]->child_id = 2;
+        $r[1]->parent_id = 2;
+        $r[1]->child_id = 3;
+        $r->save();
+
+        $r->delete();
+        $this->conn->clear();
+        $q = new Doctrine_Query();
+        $coll = $q->from('NestReference')->execute();
+        $this->assertEqual(count($coll), 0);
+    }
+
     public function testCircularNonEqualSelfReferencingRelationSaving() {
         $n1 = new NestTest();
         $n1->set('name', 'node1');
@@ -54,6 +70,18 @@ class Doctrine_Ticket364_TestCase extends Doctrine_UnitTestCase {
         $n1->save();
         $n2->get('Children')->add($n1);
         $n2->save();
+
+        $q = new Doctrine_Query();
+        $coll = $q->from('NestReference')->execute();
+
+        $this->assertEqual(count($coll), 2);
+
+        $coll->delete();
+        $this->conn->clear();
+
+        $q = new Doctrine_Query();
+        $coll = $q->from('NestReference')->execute();
+        $this->assertEqual(count($coll), 0);
     }
 
     public function testCircularEqualSelfReferencingRelationSaving() {
@@ -68,6 +96,11 @@ class Doctrine_Ticket364_TestCase extends Doctrine_UnitTestCase {
         $n1->save();
         $n2->get('Relatives')->add($n1);
         $n2->save();
+
+        $q = new Doctrine_Query();
+        $coll = $q->from('NestReference')->execute(array(), Doctrine::FETCH_ARRAY);
+
+        $this->assertEqual(count($coll), 2);
     }
 
-}
\ No newline at end of file
+}

tests/classes.php

@@ -4,7 +4,7 @@ class Entity extends Doctrine_Record
     public function setUp() 
     {
         $this->ownsOne('Email', array('local' => 'email_id'));
-        $this->ownsMany('Phonenumber', array('foreign' => 'entity_id'));
+        $this->hasMany('Phonenumber', array('local' => 'id', 'foreign' => 'entity_id'));
         $this->ownsOne('Account', array('foreign' => 'entity_id'));
         $this->hasMany('Entity', array('local' => 'entity1', 
                                        'refClass' => 'EntityReference',
@@ -167,7 +167,9 @@ class Phonenumber extends Doctrine_Record
     }
     public function setUp() 
     {
-        $this->hasOne('Entity', array('local' => 'entity_id'));
+        $this->hasOne('Entity', array('local' => 'entity_id', 
+                                      'foreign' => 'id', 
+                                      'onDelete' => 'CASCADE'));
     }
 }
 
@@ -627,8 +629,10 @@ class PackageVersion extends Doctrine_Record {
         $this->hasMany('PackageVersionNotes as Note', 'PackageVersionNotes.package_version_id');
     }
 }
-class PackageVersionNotes extends Doctrine_Record {
-    public function setTableDefinition() {
+class PackageVersionNotes extends Doctrine_Record 
+{
+    public function setTableDefinition()
+    {
         $this->hasColumn('package_version_id', 'integer');
         $this->hasColumn('description', 'string', 255);
     }
@@ -639,7 +643,8 @@ class PackageVersionNotes extends Doctrine_Record {
 }
 class NestTest extends Doctrine_Record
 {
-    public function setTableDefinition() {
+    public function setTableDefinition() 
+    {
         $this->hasColumn('name', 'string');
     }
     public function setUp()
@@ -659,7 +664,8 @@ class NestTest extends Doctrine_Record
 }
 class NestReference extends Doctrine_Record 
 {
-    public function setTableDefinition() {
+    public function setTableDefinition() 
+    {
         $this->hasColumn('parent_id', 'integer', 4, 'primary');
         $this->hasColumn('child_id', 'integer', 4, 'primary');
     }

tests/run.php

@@ -72,7 +72,7 @@ $test = new GroupTest('Doctrine Framework Unit Tests');
 
 $test->addTestCase(new Doctrine_Ticket330_TestCase());
     */
-
+/**    */
 // Connection drivers (not yet fully tested)
 $test->addTestCase(new Doctrine_Connection_Pgsql_TestCase());
 $test->addTestCase(new Doctrine_Connection_Oracle_TestCase());
@@ -170,6 +170,9 @@ $test->addTestCase(new Doctrine_Relation_TestCase());
 //$test->addTestCase(new Doctrine_Relation_Access_TestCase());
 //$test->addTestCase(new Doctrine_Relation_ManyToMany_TestCase());
 
+
+$test->addTestCase(new Doctrine_Relation_OneToMany_TestCase());
+
 $test->addTestCase(new Doctrine_Relation_Nest_TestCase());
 
 $test->addTestCase(new Doctrine_Relation_OneToOne_TestCase());
@@ -298,6 +301,7 @@ $test->addTestCase(new Doctrine_Query_MultipleAggregateValue_TestCase());
 
 $test->addTestCase(new Doctrine_Query_TestCase());
 
+$test->addTestCase(new Doctrine_Ticket364_TestCase());
 //$test->addTestCase(new Doctrine_IntegrityAction_TestCase());
 
 //$test->addTestCase(new Doctrine_AuditLog_TestCase());