Browse Source

Fix saving belongstomany with complex key values

Fix incorrectly persisted data when a belongsToMany association
foreignKey/bindingKey fields were modeled as complex types like datetime
instances.

The strict comparison on the entire key data needed to be replaced
with a more nuanced solution. At worst this code will operate in linear
time, but I think that N will be <2 for the vast majority of
applications. I've attempted to make the solution as efficient as
possible and failures should only make one loop iteration.

Fixes #16562
Mark Story 3 years ago
parent
commit
f980cf8ff7

+ 20 - 1
src/ORM/Association/BelongsToMany.php

@@ -36,6 +36,7 @@ use SplObjectStorage;
  * that contains the association fields between the source and the target table.
  *
  * An example of a BelongsToMany association would be Article belongs to many Tags.
+ * In this example 'Article' is the source table and 'Tags' is the target table.
  */
 class BelongsToMany extends Association
 {
@@ -1277,7 +1278,25 @@ class BelongsToMany extends Association
             $fields = $result->extract($keys);
             $found = false;
             foreach ($indexed as $i => $data) {
-                if ($fields === $data) {
+                $matched = false;
+                foreach ($keys as $key) {
+                    if (!array_key_exists($key, $data) || !array_key_exists($key, $fields)) {
+                        // Either side missing is no match.
+                        $matched = false;
+                    } elseif (is_object($data[$key]) && is_object($fields[$key])) {
+                        // If both sides are an object then use == so that value objects
+                        // are seen as equivalent.
+                        $matched = $fields[$key] == $data[$key];
+                    } else {
+                        // Use strict equality for all other values.
+                        $matched = $fields[$key] === $data[$key];
+                    }
+                    // Stop checks on first failure.
+                    if (!$matched) {
+                        break;
+                    }
+                }
+                if ($matched) {
                     unset($indexed[$i]);
                     $found = true;
                     break;

+ 22 - 0
tests/Fixture/CompositeKeyArticlesFixture.php

@@ -0,0 +1,22 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (https://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ * @link          https://cakephp.org CakePHP(tm) Project
+ * @since         4.4.1
+ * @license       https://opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\Test\Fixture;
+
+use Cake\TestSuite\Fixture\TestFixture;
+
+class CompositeKeyArticlesFixture extends TestFixture
+{
+    public $records = [];
+}

+ 22 - 0
tests/Fixture/CompositeKeyArticlesTagsFixture.php

@@ -0,0 +1,22 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (https://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
+ * @link          https://cakephp.org CakePHP(tm) Project
+ * @since         4.4.1
+ * @license       https://opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\Test\Fixture;
+
+use Cake\TestSuite\Fixture\TestFixture;
+
+class CompositeKeyArticlesTagsFixture extends TestFixture
+{
+    public $records = [];
+}

+ 58 - 0
tests/TestCase/ORM/Association/BelongsToManyTest.php

@@ -21,6 +21,7 @@ use Cake\Database\Expression\QueryExpression;
 use Cake\Datasource\ConnectionManager;
 use Cake\Datasource\EntityInterface;
 use Cake\Event\EventInterface;
+use Cake\I18n\FrozenTime;
 use Cake\ORM\Association\BelongsTo;
 use Cake\ORM\Association\BelongsToMany;
 use Cake\ORM\Association\HasMany;
@@ -54,6 +55,8 @@ class BelongsToManyTest extends TestCase
         'core.BinaryUuidItems',
         'core.BinaryUuidTags',
         'core.BinaryUuidItemsBinaryUuidTags',
+        'core.CompositeKeyArticles',
+        'core.CompositeKeyArticlesTags',
     ];
 
     /**
@@ -1071,6 +1074,61 @@ class BelongsToManyTest extends TestCase
         $this->assertCount(1, $refresh->binary_uuid_tags, 'One tag should remain');
     }
 
+    public function testReplaceLinksComplexTypeForeignKey()
+    {
+        $articles = $this->fetchTable('CompositeKeyArticles');
+        $tags = $this->fetchTable('Tags');
+
+        $articles->belongsToMany('Tags', [
+            'foreignKey' => ['author_id', 'created'],
+        ]);
+
+        $article = $articles->newEntity([
+            'author_id' => 1,
+            'body' => 'First post',
+            'created' => new FrozenTime(),
+        ]);
+        $articles->saveOrFail($article);
+        $tag1 = $tags->find()->where(['Tags.name' => 'tag1'])->firstOrFail();
+        $tag2 = $tags->find()->where(['Tags.name' => 'tag2'])->firstOrFail();
+
+        $findArticle = function ($article) use ($articles) {
+            return $articles->find()
+                ->where(['CompositeKeyArticles.author_id' => $article->author_id])
+                ->contain('Tags')
+                ->firstOrFail();
+        };
+
+        $article = $findArticle($article);
+        $this->assertEmpty($article->tags);
+
+        // Create the first link
+        $article = $articles->patchEntity($article, ['tags' => ['_ids' => [$tag1->id]]]);
+        $result = $articles->save($article, ['associated' => 'Tags']);
+        $this->assertNotEmpty($result);
+        $this->assertCount(1, $result->tags);
+        $this->assertEquals($tag1->id, $result->tags[0]->id);
+
+        // Add second tag. Reload tag objects so created fields have different
+        // instances.
+        $article = $findArticle($article);
+        $article = $articles->patchEntity($article, ['tags' => ['_ids' => [$tag1->id, $tag2->id]]]);
+        $result = $articles->save($article, ['associated' => 'Tags']);
+
+        // Check in memory entity.
+        $this->assertNotEmpty($result);
+        $this->assertCount(2, $result->tags);
+        $this->assertEquals('tag1', $result->tags[0]->name);
+        $this->assertEquals('tag2', $result->tags[1]->name);
+
+        // Reload to check persisted state.
+        $result = $findArticle($article);
+        $this->assertNotEmpty($result);
+        $this->assertCount(2, $result->tags);
+        $this->assertEquals('tag1', $result->tags[0]->name);
+        $this->assertEquals('tag2', $result->tags[1]->name);
+    }
+
     /**
      * Provider for empty values
      *

+ 52 - 0
tests/schema.php

@@ -922,6 +922,58 @@ return [
         ],
     ],
     [
+        'table' => 'composite_key_articles',
+        'columns' => [
+            'author_id' => [
+                'type' => 'integer',
+                'null' => false,
+            ],
+            'created' => [
+                'type' => 'datetime',
+                'null' => false,
+            ],
+            'body' => [
+                'type' => 'text',
+            ],
+        ],
+        'constraints' => [
+            'composite_article_pk' => [
+                'type' => 'primary',
+                'columns' => [
+                    'author_id',
+                    'created',
+                ],
+            ],
+        ],
+    ],
+    [
+        'table' => 'composite_key_articles_tags',
+        'columns' => [
+            'author_id' => [
+                'type' => 'integer',
+                'null' => false,
+            ],
+            'created' => [
+                'type' => 'datetime',
+                'null' => false,
+            ],
+            'tag_id' => [
+                'type' => 'integer',
+                'null' => false,
+            ],
+        ],
+        'constraints' => [
+            'composite_article_tags_pk' => [
+                'type' => 'primary',
+                'columns' => [
+                    'author_id',
+                    'created',
+                    'tag_id',
+                ],
+            ],
+        ],
+    ],
+    [
         'table' => 'profiles',
         'columns' => [
             'id' => [