Browse Source

Don't allow allow one() to do queries.

The belongsToMany special casing should only apply to belongsToMany.
Having it in one() makes it too broad and potentially breaks the promise
that one() makes - which is to create new entities.

This change also fixes the N query issue the old code had.
Mark Story 11 years ago
parent
commit
6cdd95ad71
2 changed files with 76 additions and 49 deletions
  1. 20 18
      src/ORM/Marshaller.php
  2. 56 31
      tests/TestCase/ORM/MarshallerTest.php

+ 20 - 18
src/ORM/Marshaller.php

@@ -106,23 +106,9 @@ class Marshaller
 
         $schema = $this->_table->schema();
         $primaryKey = $schema->primaryKey();
-
-        $entity = null;
-        if (array_intersect($primaryKey, array_keys($data)) == $primaryKey) {
-            $tableName = $this->_table->alias();
-            $query = $this->_table->find('all');
-            foreach ($primaryKey as $pkey) {
-                $query->where(["$tableName.$pkey" => $data[$pkey]]);
-            }
-            $entity = $query->first();
-        }
-
-        if (!isset($entity)) {
-            $entityClass = $this->_table->entityClass();
-            $entity = new $entityClass();
-            $entity->source($this->_table->registryAlias());
-        }
-
+        $entityClass = $this->_table->entityClass();
+        $entity = new $entityClass();
+        $entity->source($this->_table->registryAlias());
 
         if (isset($options['accessibleFields'])) {
             foreach ((array)$options['accessibleFields'] as $key => $value) {
@@ -276,6 +262,7 @@ class Marshaller
      */
     protected function _belongsToMany(Association $assoc, array $data, $options = [])
     {
+        // Accept _ids = [1, 2]
         $associated = isset($options['associated']) ? $options['associated'] : [];
         $hasIds = array_key_exists('_ids', $data);
         if ($hasIds && is_array($data['_ids'])) {
@@ -285,7 +272,22 @@ class Marshaller
             return [];
         }
 
-        $records = $this->many($data, $options);
+        // Accept [ [id => 1], [id = 2] ] style.
+        $primaryKey = array_flip($assoc->target()->schema()->primaryKey());
+        if (array_intersect_key($primaryKey, current($data)) === $primaryKey) {
+            $primaryCount = count($primaryKey);
+            $query = $assoc->find();
+            foreach ($data as $row) {
+                $keys = array_intersect_key($row, $primaryKey);
+                if (count($keys) === $primaryCount) {
+                    $query->orWhere($keys);
+                }
+            }
+            $records = $query->toArray();
+        } else {
+            $records = $this->many($data, $options);
+        }
+
         $joint = $assoc->junction();
         $jointMarshaller = $joint->marshaller();
 

+ 56 - 31
tests/TestCase/ORM/MarshallerTest.php

@@ -590,37 +590,6 @@ class MarshallerTest extends TestCase
     }
 
     /**
-     * test one() to update associations.
-     *
-     * @return void
-     */
-    public function testOneAssociationPatchExisting()
-    {
-        $data = [
-            'title' => 'My title',
-            'body' => 'My content',
-            'author_id' => 1,
-            'user' => [
-                'id' => 1,
-                'username' => 'mark',
-                'password' => 'secret'
-            ]
-        ];
-        $marshall = new Marshaller($this->articles);
-        $result = $marshall->one($data, ['associated' => ['Users']]);
-
-        $this->assertEquals($data['title'], $result->title);
-        $this->assertEquals($data['body'], $result->body);
-        $this->assertEquals($data['author_id'], $result->author_id);
-
-        $this->assertInstanceOf('Cake\ORM\Entity', $result->user);
-        $this->assertFalse($result->user->isNew(), 'Existing association');
-        $this->assertEquals(1, $result->user->id);
-        $this->assertEquals($data['user']['username'], $result->user->username);
-        $this->assertEquals($data['user']['password'], $result->user->password);
-    }
-
-    /**
      * Test many() with a simple set of data.
      *
      * @return void
@@ -1187,6 +1156,62 @@ class MarshallerTest extends TestCase
     }
 
     /**
+     * Test merging belongsToMany data doesn't create 'new' entities.
+     *
+     * @return void
+     */
+    public function testMergeBelongsToManyJoinDataAssociatedWithIds()
+    {
+        $data = [
+            'title' => 'My title',
+            'tags' => [
+                [
+                    'id' => 1,
+                    '_joinData' => [
+                        'active' => 1,
+                        'user' => ['username' => 'MyLux'],
+                    ]
+                ],
+                [
+                    'id' => 2,
+                    '_joinData' => [
+                        'active' => 0,
+                        'user' => ['username' => 'IronFall'],
+                    ]
+                ],
+            ],
+        ];
+        $articlesTags = TableRegistry::get('ArticlesTags');
+        $articlesTags->belongsTo('Users');
+
+        $marshall = new Marshaller($this->articles);
+        $article = $this->articles->get(1, ['associated' => 'Tags']);
+        $result = $marshall->merge($article, $data, ['associated' => ['Tags._joinData.Users']]);
+
+        $this->assertInstanceOf('Cake\ORM\Entity', $result->tags[0]);
+        $this->assertInstanceOf('Cake\ORM\Entity', $result->tags[1]);
+        $this->assertInstanceOf('Cake\ORM\Entity', $result->tags[0]->_joinData->user);
+
+        $this->assertInstanceOf('Cake\ORM\Entity', $result->tags[1]->_joinData->user);
+        $this->assertFalse($result->tags[0]->isNew(), 'Should not be new, as id is in db.');
+        $this->assertFalse($result->tags[1]->isNew(), 'Should not be new, as id is in db.');
+        $this->assertEquals(1, $result->tags[0]->id);
+        $this->assertEquals(2, $result->tags[1]->id);
+
+        $this->assertEquals(1, $result->tags[0]->_joinData->active);
+        $this->assertEquals(0, $result->tags[1]->_joinData->active);
+
+        $this->assertEquals(
+            $data['tags'][0]['_joinData']['user']['username'],
+            $result->tags[0]->_joinData->user->username
+        );
+        $this->assertEquals(
+            $data['tags'][1]['_joinData']['user']['username'],
+            $result->tags[1]->_joinData->user->username
+        );
+    }
+
+    /**
      * Test merging the _joinData entity for belongstomany associations.
      *
      * @return void