Merge pull request #1963 from mringler/bugfix/avoid_cross_ref_relatio…

…n_name_collisions avoid name collision in cross ref relations
propelorm · Nov 28, 2023 · 8117e54 · 8117e54
2 parents 4f36eea + 03ff801
commit 8117e54
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 22 deletions.
diff --git a/src/Propel/Generator/Builder/Om/AbstractOMBuilder.php b/src/Propel/Generator/Builder/Om/AbstractOMBuilder.php
@@ -686,34 +686,43 @@ public function getFKPhpNameAffix(ForeignKey $fk, bool $plural = false): string
      * @return string
      */
     protected function getCrossFKsPhpNameAffix(CrossForeignKeys $crossFKs, bool $plural = true): string
+    {
+        $baseName = $this->buildCombineCrossFKsPhpNameAffix($crossFKs, false);
+
+        $existingTable = $this->getDatabase()->getTableByPhpName($baseName);
+        $isNameCollision = $existingTable && $this->getTable()->isConnectedWithTable($existingTable);
+
+        return ($plural || $isNameCollision) ? $this->buildCombineCrossFKsPhpNameAffix($crossFKs, $plural, $isNameCollision) : $baseName;
+    }
+
+    /**
+     * @param \Propel\Generator\Model\CrossForeignKeys $crossFKs
+     * @param bool $plural
+     * @param bool $withPrefix
+     *
+     * @return string
+     */
+    protected function buildCombineCrossFKsPhpNameAffix(CrossForeignKeys $crossFKs, bool $plural = true, bool $withPrefix = false): string
     {
         $names = [];
+        if ($withPrefix) {
+            $names[] = 'Cross';
+        }
+        $fks = $crossFKs->getCrossForeignKeys();
+        $lastCrossFk = array_pop($fks);
+        $unclassifiedPrimaryKeys = $crossFKs->getUnclassifiedPrimaryKeys();
+        $lastIsPlural = $plural && !$unclassifiedPrimaryKeys;
 
-        if ($plural) {
-            if ($crossFKs->getUnclassifiedPrimaryKeys()) {
-                //we have a non fk as pk as well, so we need to make pluralisation on our own and can't
-                //rely on getFKPhpNameAffix's pluralisation
-                foreach ($crossFKs->getCrossForeignKeys() as $fk) {
-                    $names[] = $this->getFKPhpNameAffix($fk, false);
-                }
-            } else {
-                //we have only fks, so give us names with plural and return those
-                $lastIdx = count($crossFKs->getCrossForeignKeys()) - 1;
-                foreach ($crossFKs->getCrossForeignKeys() as $idx => $fk) {
-                    $needPlural = $idx === $lastIdx; //only last fk should be plural
-                    $names[] = $this->getFKPhpNameAffix($fk, $needPlural);
-                }
+        foreach ($fks as $fk) {
+            $names[] = $this->getFKPhpNameAffix($fk, false);
+        }
+        $names[] = $this->getFKPhpNameAffix($lastCrossFk, $lastIsPlural);
 
-                return implode('', $names);
-            }
-        } else {
-            // no plural, so $plural=false
-            foreach ($crossFKs->getCrossForeignKeys() as $fk) {
-                $names[] = $this->getFKPhpNameAffix($fk, false);
-            }
+        if (!$unclassifiedPrimaryKeys) {
+            return implode('', $names);
         }
 
-        foreach ($crossFKs->getUnclassifiedPrimaryKeys() as $pk) {
+        foreach ($unclassifiedPrimaryKeys as $pk) {
             $names[] = $pk->getPhpName();
         }
 

diff --git a/src/Propel/Generator/Model/Table.php b/src/Propel/Generator/Model/Table.php
@@ -2288,4 +2288,18 @@ public function getAdditionalModelClassImports(): ?array
 
         return null;
     }
+
+    /**
+     * Check if there is a FK rellation between the current table and the given
+     * table in either direction.
+     *
+     * @param \Propel\Generator\Model\Table $table
+     *
+     * @return bool
+     */
+    public function isConnectedWithTable(Table $table): bool
+    {
+        return $this->getForeignKeysReferencingTable($table->getName()) ||
+            $table->getForeignKeysReferencingTable($this->getName());
+    }
 }
diff --git a/tests/Propel/Tests/Issues/Issue941Test.php b/tests/Propel/Tests/Issues/Issue941Test.php
@@ -0,0 +1,104 @@
+<?php
+
+/**
+ * MIT License. This file is part of the Propel package.
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Propel\Tests\Issues;
+
+use Nature;
+use Propel\Generator\Util\QuickBuilder;
+use Propel\Runtime\Collection\ObjectCollection;
+use Propel\Tests\TestCase;
+use Recherche;
+use Category;
+use RechercheNature;
+
+/**
+ * This test proves the bug described in https://github.com/propelorm/Propel2/issues/941.
+ *
+ * @group database
+ */
+class Issue941Test extends TestCase
+{
+    /**
+     * @return void
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+        if (!class_exists('\Nature')) {
+            $schema = '
+            <database>
+                <table name="recherche" phpName="Recherche">
+                    <column name="id" type="integer" primaryKey="true" autoIncrement="true"/>
+                </table>
+
+                <table name="category" phpName="Category">
+                    <column name="id" type="integer" primaryKey="true" autoIncrement="true"/>
+                </table>
+
+                <table name="recherche_nature" phpName="RechercheNature" isCrossRef="true">
+                    <column name="recherche_id" type="integer" primaryKey="true"/>
+                    <column name="nature_id" type="integer" primaryKey="true"/>
+                    <column name="category_id" type="integer"/>
+
+                    <foreign-key foreignTable="recherche" onDelete="cascade">
+                        <reference local="recherche_id" foreign="id"/>
+                    </foreign-key>
+                    <foreign-key foreignTable="nature">
+                        <reference local="nature_id" foreign="id"/>
+                    </foreign-key>
+
+                    <foreign-key foreignTable="category">
+                        <reference local="category_id" foreign="id"/>
+                    </foreign-key>
+
+                </table>
+
+                <table name="nature" phpName="Nature">
+                    <column name="id" type="integer" primaryKey="true" autoIncrement="true"/>
+                </table>
+
+            </database>
+            ';
+            QuickBuilder::buildSchema($schema);
+        }
+    }
+
+    /**
+     * @return void
+     */
+    public function testIssue941()
+    {
+        $nature = new Nature();
+        $nature->save();
+
+        $category = new Category();
+        $category->save();
+
+        // RechercheNature
+        $rechercheNature = new RechercheNature();
+        $rechercheNature->setNatureId($nature->getId());
+        $rechercheNature->setCategoryId($category->getId());
+
+        // Collection
+        $collection = new ObjectCollection();
+        $collection->setModel('\RechercheNature');
+        $collection->setData([$rechercheNature]);
+
+        // Recherche
+        $recherche = new Recherche();
+        $recherche->setRechercheNatures($collection);
+
+        $countBeforeSave = $recherche->countRechercheNatures();
+
+        $recherche->save();
+
+        $countAfterSave = $recherche->countRechercheNatures();
+
+        $this->assertEquals($countBeforeSave, $countAfterSave);
+    }
+}
diff --git a/tests/Propel/Tests/Issues/Issue989Test.php b/tests/Propel/Tests/Issues/Issue989Test.php
@@ -38,6 +38,7 @@ protected function setUp(): void
                 <table name="recherche_nature" phpName="RechercheNature" isCrossRef="true">
                     <column name="recherche_id" type="integer" primaryKey="true"/>
                     <column name="nature_id" type="integer" primaryKey="true"/>
+                    <column name="category_id" type="integer"/>
                     <foreign-key foreignTable="recherche" onDelete="cascade">
                         <reference local="recherche_id" foreign="id"/>
                     </foreign-key>