Merge pull request #17676 from cakephp/5.1-duplicate-association-alias

Throw exception when setting association with duplicate alias.

.github/workflows/ci.yml

@@ -46,9 +46,11 @@ jobs:
           - 11211/tcp
 
     steps:
-    - name: Setup MySQL latest
+    - name: Setup MySQL
       if: matrix.db-type == 'mysql'
-      run: docker run --rm --name=mysqld -e MYSQL_ROOT_PASSWORD=root -e MYSQL_DATABASE=cakephp -p 3306:3306 -d mysql --default-authentication-plugin=mysql_native_password --disable-log-bin
+      run: |
+        sudo service mysql start
+        mysql -h 127.0.0.1 -u root -proot -e 'CREATE DATABASE cakephp;'
 
     - name: Setup PostgreSQL latest
       if: matrix.db-type == 'pgsql'
@@ -101,10 +103,6 @@ jobs:
       if: matrix.php-version == '8.1' && matrix.db-type == 'mysql'
       run: echo "::add-matcher::${{ runner.tool_cache }}/phpunit.json"
 
-    - name: Wait for MySQL
-      if: matrix.db-type == 'mysql' || matrix.db-type == 'mariadb'
-      run: while ! `mysqladmin ping -h 127.0.0.1 --silent`; do printf 'Waiting for MySQL...\n'; sleep 2; done;
-
     - name: Run PHPUnit
       env:
         REDIS_PORT: ${{ job.services.redis.ports['6379'] }}

src/ORM/AssociationCollection.php

@@ -17,6 +17,7 @@
 namespace Cake\ORM;
 
 use ArrayIterator;
+use Cake\Core\Exception\CakeException;
 use Cake\Datasource\EntityInterface;
 use Cake\ORM\Locator\LocatorAwareTrait;
 use Cake\ORM\Locator\LocatorInterface;
@@ -70,6 +71,7 @@ public function __construct(?LocatorInterface $tableLocator = null)
      * @param string $alias The association alias
      * @param \Cake\ORM\Association $association The association to add.
      * @return \Cake\ORM\Association The association object being added.
+     * @throws \Cake\Core\Exception\CakeException If the alias is already added.
      * @template T of \Cake\ORM\Association
      * @psalm-param T $association
      * @psalm-return T
@@ -78,6 +80,10 @@ public function add(string $alias, Association $association): Association
     {
         [, $alias] = pluginSplit($alias);
 
+        if (isset($this->_items[$alias])) {
+            throw new CakeException(sprintf('Association alias `%s` is already set.', $alias));
+        }
+
         return $this->_items[$alias] = $association;
     }
 

src/ORM/Behavior/Translate/EavStrategy.php

@@ -131,6 +131,10 @@ protected function setupAssociations(): void
                 $conditions[$name . '.content !='] = '';
             }
 
+            if ($this->table->associations()->has($name)) {
+                $this->table->associations()->remove($name);
+            }
+
             $this->table->hasOne($name, [
                 'targetTable' => $fieldTable,
                 'foreignKey' => 'foreign_key',
@@ -145,6 +149,9 @@ protected function setupAssociations(): void
             $conditions["$targetAlias.content !="] = '';
         }
 
+        if ($this->table->associations()->has($targetAlias)) {
+            $this->table->associations()->remove($targetAlias);
+        }
         $this->table->hasMany($targetAlias, [
             'className' => $table,
             'foreignKey' => 'foreign_key',

src/ORM/Behavior/Translate/ShadowTableStrategy.php

@@ -105,6 +105,11 @@ protected function setupAssociations(): void
         $config = $this->getConfig();
 
         $targetAlias = $this->translationTable->getAlias();
+
+        if ($this->table->associations()->has($targetAlias)) {
+            $this->table->associations()->remove($targetAlias);
+        }
+
         $this->table->hasMany($targetAlias, [
             'className' => $config['translationTable'],
             'foreignKey' => 'id',
@@ -184,6 +189,10 @@ protected function setupHasOneAssociation(string $locale, ArrayObject $options):
             $joinType = $config['onlyTranslated'] ? 'INNER' : 'LEFT';
         }
 
+        if ($this->table->associations()->has($config['hasOneAlias'])) {
+            $this->table->associations()->remove($config['hasOneAlias']);
+        }
+
         $this->table->hasOne($config['hasOneAlias'], [
             'foreignKey' => ['id'],
             'joinType' => $joinType,

tests/TestCase/Datasource/Paging/PaginatorTestTrait.php

@@ -163,11 +163,11 @@ public function testPaginateCustomFinderOptions(): void
     public function testPaginateNestedEagerLoader(): void
     {
         $articles = $this->getTableLocator()->get('Articles');
-        $articles->belongsToMany('Tags');
         $tags = $this->getTableLocator()->get('Tags');
+        $tags->associations()->remove('Authors');
         $tags->belongsToMany('Authors');
         $articles->getEventManager()->on('Model.beforeFind', function ($event, $query): void {
-            $query ->matching('Tags', function ($q) {
+            $query->matching('Tags', function ($q) {
                 return $q->matching('Authors', function ($q) {
                     return $q->where(['Authors.name' => 'larry']);
                 });

tests/TestCase/ORM/Association/BelongsToManyTest.php

@@ -399,11 +399,7 @@ public function testFinderOption(): void
         $articles = $this->getTableLocator()->get('Articles');
         $tags = $this->getTableLocator()->get('Tags');
 
-        $tags->belongsToMany('Articles', [
-            'sourceTable' => $tags,
-            'targetTable' => $articles,
-            'finder' => 'published',
-        ]);
+        $tags->associations()->get('Articles')->setFinder('published');
         $articles->updateAll(['published' => 'N'], ['id' => 1]);
         $entity = $tags->get(1, ...['contain' => 'Articles']);
         $this->assertCount(1, $entity->articles, 'only one article should load');
@@ -926,12 +922,9 @@ public function testReplaceLinkWithFinderInJunctionAssociations(): void
 
         // Update an article to not match the association finder.
         $articles->updateAll(['published' => 'N'], ['id' => 1]);
-        $assoc = $tags->belongsToMany('Articles', [
-            'sourceTable' => $tags,
-            'targetTable' => $articles,
-            'through' => $joint,
-            'finder' => 'published',
-        ]);
+        $assoc = $tags->associations()->get('Articles')
+            ->setFinder('published')
+            ->setThrough($joint);
         $entity = $tags->get(1, ...['contain' => 'Articles']);
         $this->assertCount(1, $entity->articles);
 
@@ -984,15 +977,11 @@ public function testReplaceLinksFinderCondition(): void
         $this->setAppNamespace('TestApp');
 
         $joint = $this->getTableLocator()->get('ArticlesTags');
-        $articles = $this->getTableLocator()->get('Articles');
         $tags = $this->getTableLocator()->get('Tags');
 
-        $assoc = $tags->belongsToMany('Articles', [
-            'sourceTable' => $tags,
-            'targetTable' => $articles,
-            'through' => $joint,
-            'finder' => ['published' => ['title' => 'First Article']],
-        ]);
+        $assoc = $tags->associations()->get('Articles')
+            ->setFinder(['published' => ['title' => 'First Article']])
+            ->setThrough($joint);
         $entity = $tags->get(1, ...['contain' => 'Articles']);
         $this->assertCount(1, $entity->articles);
 
@@ -1018,12 +1007,9 @@ public function testReplaceLinksFinderContain(): void
         $articles = $this->getTableLocator()->get('Articles');
         $tags = $this->getTableLocator()->get('Tags');
 
-        $assoc = $tags->belongsToMany('Articles', [
-            'sourceTable' => $tags,
-            'targetTable' => $articles,
-            'through' => $joint,
-            'finder' => 'withAuthors',
-        ]);
+        $assoc = $tags->associations()->get('Articles')
+            ->setFinder('withAuthors')
+            ->setThrough($joint);
         $tag = $tags->get(1);
         $article = $articles->get(1);
 

tests/TestCase/ORM/Association/BelongsToTest.php

@@ -461,7 +461,7 @@ public function testAttachToFormatResultsNoDirtyResults(): void
     {
         $this->setAppNamespace('TestApp');
         $articles = $this->getTableLocator()->get('Articles');
-        $articles->belongsTo('Authors')
+        $articles->associations()->get('Authors')
             ->setFinder('formatted');
 
         $query = $articles->find()

tests/TestCase/ORM/Association/HasManyTest.php

@@ -181,7 +181,7 @@ public function testSetSort(): void
     public function testSorting(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $assoc = $authors->hasMany('Articles');
+        $assoc = $authors->Articles;
 
         $field = 'Articles.id';
         $driver = $authors->getConnection()->getDriver();
@@ -502,7 +502,6 @@ public function testEagerLoaderMultipleKeys(): void
     public function testEagerloaderNoForeignKeys(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles');
 
         $this->expectException(InvalidArgumentException::class);
         $this->expectExceptionMessage('Unable to load `Articles` association. Ensure foreign key in `Authors`');
@@ -666,9 +665,7 @@ public function testPropertyOption(): void
     public function testPropertyOptionMarshalAndValidation(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', [
-            'propertyName' => 'blogs',
-        ]);
+        $authors->Articles->setProperty('blogs');
         $authors->getValidator()
             ->requirePresence('blogs', true, 'blogs must be set');
 
@@ -703,9 +700,7 @@ public function testPropertyNoPlugin(): void
     public function testValueBinderUpdateOnSubQueryStrategy(): void
     {
         $Authors = $this->getTableLocator()->get('Authors');
-        $Authors->hasMany('Articles', [
-            'strategy' => Association::STRATEGY_SUBQUERY,
-        ]);
+        $Authors->Articles->setStrategy(Association::STRATEGY_SUBQUERY);
 
         $query = $Authors->find();
         $authorsAndArticles = $query
@@ -730,9 +725,7 @@ public function testValueBinderUpdateOnSubQueryStrategy(): void
     public function testSubqueryWithLimit()
     {
         $Authors = $this->getTableLocator()->get('Authors');
-        $Authors->hasMany('Articles', [
-            'strategy' => Association::STRATEGY_SUBQUERY,
-        ]);
+        $Authors->Articles->setStrategy(Association::STRATEGY_SUBQUERY);
 
         $query = $Authors->find();
         $result = $query
@@ -755,9 +748,7 @@ public function testSubqueryWithLimitAndOrder()
         $this->skipIf(ConnectionManager::get('test')->getDriver() instanceof Sqlserver, 'Sql Server does not support ORDER BY on field not in GROUP BY');
 
         $Authors = $this->getTableLocator()->get('Authors');
-        $Authors->hasMany('Articles', [
-            'strategy' => Association::STRATEGY_SUBQUERY,
-        ]);
+        $Authors->Articles->setStrategy(Association::STRATEGY_SUBQUERY);
 
         $query = $Authors->find();
         $result = $query
@@ -842,10 +833,7 @@ protected function assertSelectClause($expected, $query): void
     public function testUnlinkSuccess(): void
     {
         $articles = $this->getTableLocator()->get('Articles');
-        $assoc = $this->author->hasMany('Articles', [
-            'sourceTable' => $this->author,
-            'targetTable' => $articles,
-        ]);
+        $assoc = $this->author->Articles;
 
         $entity = $this->author->get(1, ...['contain' => 'Articles']);
         $initial = $entity->articles;
@@ -866,10 +854,7 @@ public function testUnlinkSuccess(): void
     public function testUnlinkWithEmptyArray(): void
     {
         $articles = $this->getTableLocator()->get('Articles');
-        $assoc = $this->author->hasMany('Articles', [
-            'sourceTable' => $this->author,
-            'targetTable' => $articles,
-        ]);
+        $assoc = $this->author->Articles;
 
         $entity = $this->author->get(1, ...['contain' => 'Articles']);
         $initial = $entity->articles;
@@ -889,10 +874,7 @@ public function testUnlinkWithEmptyArray(): void
     public function testLinkUsesSingleTransaction(): void
     {
         $articles = $this->getTableLocator()->get('Articles');
-        $assoc = $this->author->hasMany('Articles', [
-            'sourceTable' => $this->author,
-            'targetTable' => $articles,
-        ]);
+        $assoc = $this->author->Articles;
 
         // Ensure author in fixture has zero associated articles
         $entity = $this->author->get(2, ...['contain' => 'Articles']);
@@ -1129,7 +1111,7 @@ public function testSetSaveStrategy(): void
     public function testSaveReplaceSaveStrategy(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => HasMany::SAVE_REPLACE]);
+        $authors->Articles->setSaveStrategy(HasMany::SAVE_REPLACE);
 
         $entity = $authors->newEntity([
             'name' => 'mylux',
@@ -1160,7 +1142,7 @@ public function testSaveReplaceSaveStrategy(): void
     public function testSaveReplaceSaveStrategyClosureConditions(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles')
+        $authors->Articles
             ->setDependent(true)
             ->setSaveStrategy('replace')
             ->setConditions(function () {
@@ -1201,7 +1183,7 @@ public function testSaveReplaceSaveStrategyClosureConditions(): void
     public function testSaveReplaceSaveStrategyNotAdding(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => 'replace']);
+        $authors->Articles->setSaveStrategy('replace');
 
         $entity = $authors->newEntity([
             'name' => 'mylux',
@@ -1229,7 +1211,7 @@ public function testSaveReplaceSaveStrategyNotAdding(): void
     public function testSaveAppendSaveStrategy(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => 'append']);
+        $authors->Articles->setSaveStrategy('append');
 
         $entity = $authors->newEntity([
             'name' => 'mylux',
@@ -1261,8 +1243,8 @@ public function testSaveAppendSaveStrategy(): void
     public function testSaveDefaultSaveStrategy(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => HasMany::SAVE_APPEND]);
-        $this->assertSame(HasMany::SAVE_APPEND, $authors->getAssociation('articles')->getSaveStrategy());
+        $authors->Articles->setSaveStrategy(HasMany::SAVE_APPEND);
+        $this->assertSame(HasMany::SAVE_APPEND, $authors->getAssociation('Articles')->getSaveStrategy());
     }
 
     /**
@@ -1271,7 +1253,8 @@ public function testSaveDefaultSaveStrategy(): void
     public function testSaveReplaceSaveStrategyDependent(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => HasMany::SAVE_REPLACE, 'dependent' => true]);
+        $authors->Articles->setSaveStrategy(HasMany::SAVE_REPLACE)
+            ->setDependent(true);
 
         $entity = $authors->newEntity([
             'name' => 'mylux',
@@ -1303,7 +1286,8 @@ public function testSaveReplaceSaveStrategyDependent(): void
     public function testSaveReplaceSaveStrategyDependentWithStringKeys(): void
     {
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', ['saveStrategy' => HasMany::SAVE_REPLACE, 'dependent' => true]);
+        $authors->Articles->setSaveStrategy(HasMany::SAVE_REPLACE)
+            ->setDependent(true);
 
         $entity = $authors->newEntity([
             'name' => 'mylux',
@@ -1341,11 +1325,9 @@ public function testSaveReplaceSaveStrategyDependentWithConditions(): void
         $this->setAppNamespace('TestApp');
 
         $authors = $this->getTableLocator()->get('Authors');
-        $authors->hasMany('Articles', [
-            'finder' => 'published',
-            'saveStrategy' => HasMany::SAVE_REPLACE,
-            'dependent' => true,
-        ]);
+        $authors->Articles->setSaveStrategy(HasMany::SAVE_REPLACE)
+            ->setDependent(true)
+            ->setFinder('published');
         $articles = $authors->Articles->getTarget();
 
         // Remove an article from the association finder scope

tests/TestCase/ORM/AssociationCollectionTest.php

@@ -16,6 +16,7 @@
  */
 namespace Cake\Test\TestCase\ORM;
 
+use Cake\Core\Exception\CakeException;
 use Cake\ORM\Association\BelongsTo;
 use Cake\ORM\Association\BelongsToMany;
 use Cake\ORM\AssociationCollection;
@@ -436,4 +437,14 @@ public function testAssociationsCanBeIterated(): void
         $result = iterator_to_array($this->associations, true);
         $this->assertSame($expected, $result);
     }
+
+    public function testExceptionOnDuplicateAlias(): void
+    {
+        $this->expectException(CakeException::class);
+        $this->expectExceptionMessage('Association alias `Users` is already set.');
+
+        $belongsTo = new BelongsTo('');
+        $this->associations->add('Users', $belongsTo);
+        $this->associations->add('Users', $belongsTo);
+    }
 }