Merge pull request propelorm#1421 from prgTW/fix/issue/1420

Fixes invalid hydration when using mergeWith of criteria with "with" models

src/Propel/Generator/Builder/Om/TableMapBuilder.php

@@ -191,6 +191,7 @@ protected function addClassBody(&$script)
     protected function addSelectMethods(&$script)
     {
         $this->addAddSelectColumns($script);
+        $this->addRemoveSelectColumns($script);
     }
 
     /**
@@ -1330,6 +1331,53 @@ public static function addSelectColumns(Criteria \$criteria, \$alias = null)
 
  // addAddSelectColumns()
 
+    /**
+     * Adds the removeSelectColumns() method.
+     *
+     * @param string $script The script will be modified in this method.
+     *
+     * @return void
+     */
+    protected function addRemoveSelectColumns(&$script)
+    {
+        $script .= "
+    /**
+     * Remove all the columns needed to create a new object.
+     *
+     * Note: any columns that were marked with lazyLoad=\"true\" in the
+     * XML schema will not be removed as they are only loaded on demand.
+     *
+     * @param Criteria \$criteria object containing the columns to remove.
+     * @param string   \$alias    optional table alias
+     * @throws PropelException Any exceptions caught during processing will be
+     *                         rethrown wrapped into a PropelException.
+     */
+    public static function removeSelectColumns(Criteria \$criteria, \$alias = null)
+    {
+        if (null === \$alias) {";
+        foreach ($this->getTable()->getColumns() as $col) {
+            if (!$col->isLazyLoad()) {
+                $script .= "
+            \$criteria->removeSelectColumn({$col->getFQConstantName()});";
+            } // if !col->isLazyLoad
+        } // foreach
+        $script .= "
+        } else {";
+        foreach ($this->getTable()->getColumns() as $col) {
+            if (!$col->isLazyLoad()) {
+                $script .= "
+            \$criteria->removeSelectColumn(\$alias . '." . $col->getName() . "');";
+            } // if !col->isLazyLoad
+        } // foreach
+        $script .= "
+        }";
+        $script .= "
+    }
+";
+    }
+
+ // addRemoveSelectColumns()
+
     /**
      * Adds the getTableMap() method which is a convenience method for apps to get DB metadata.
      *

src/Propel/Runtime/ActiveQuery/Criteria.php

@@ -1440,6 +1440,22 @@ public function addSelectColumn($name)
         return $this;
     }
 
+    /**
+     * Remove select column.
+     *
+     * @param string $name Name of the select column.
+     *
+     * @return $this Modified Criteria object (for fluent API)
+     */
+    public function removeSelectColumn($name)
+    {
+        while (false !== ($key = array_search($name, $this->selectColumns, true))) {
+            unset($this->selectColumns[$key]);
+        }
+
+        return $this;
+    }
+
     /**
      * Set the query comment, that appears after the first verb in the SQL query
      *

src/Propel/Runtime/ActiveQuery/ModelCriteria.php

@@ -843,6 +843,18 @@ public function endUse()
      */
     public function mergeWith(Criteria $criteria, $operator = null)
     {
+        if (
+            $criteria instanceof ModelCriteria
+            && !$criteria->getPrimaryCriteria()
+            && $criteria->isSelfColumnsSelected()
+            && $criteria->getWith()
+        ) {
+            if (!$this->isSelfColumnsSelected()) {
+                $this->addSelfSelectColumns();
+            }
+            $criteria->removeSelfSelectColumns();
+        }
+
         parent::mergeWith($criteria, $operator);
 
         // merge with
@@ -948,6 +960,36 @@ public function addSelfSelectColumns($force = false)
         return $this;
     }
 
+    /**
+     * Removes the select columns for the current table
+     *
+     * @param bool $force To enforce removing columns for changed alias, set it to true (f.e. with sub selects)
+     *
+     * @return $this The current object, for fluid interface
+     */
+    public function removeSelfSelectColumns($force = false)
+    {
+        if (!$this->isSelfSelected && !$force) {
+            return $this;
+        }
+
+        $tableMap = $this->modelTableMapName;
+        $tableMap::removeSelectColumns($this, $this->useAliasInSQL ? $this->modelAlias : null);
+        $this->isSelfSelected = false;
+
+        return $this;
+    }
+
+    /**
+     * Returns whether select columns for the current table are included
+     *
+     * @return bool
+     */
+    public function isSelfColumnsSelected()
+    {
+        return $this->isSelfSelected;
+    }
+
     /**
      * Adds the select columns for a relation
      *

tests/Propel/Tests/Issues/Issue1420Test.php

@@ -0,0 +1,105 @@
+<?php
+/**
+ * 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.
+ *
+ * @license MIT License
+ */
+
+namespace Propel\Tests\Issues;
+
+use Propel\Generator\Util\QuickBuilder;
+use Propel\Tests\TestCase;
+
+/**
+ * Regression test for https://github.com/propelorm/Propel2/issues/1420
+ *
+ * @group database
+ */
+class Issue1420Test extends TestCase
+{
+    protected function setUp()
+    {
+        parent::setUp();
+        if (!class_exists('\Table1420A')) {
+            $schema = <<<XML
+            <!DOCTYPE database SYSTEM "../../../../resources/dtd/database.dtd">
+            <database name="issue_1420" defaultIdMethod="native">
+                <table name="table_1420_a">
+                    <column name="id" primaryKey="true" type="INTEGER" />
+                    <column name="a_field" type="VARCHAR" size="10" />
+                </table>
+                <table name="table_1420_b">
+                    <column name="id" primaryKey="true" type="INTEGER" />
+                    <column name="table_1420_a_id" type="INTEGER" />
+                    <column name="b_field" type="VARCHAR" size="10" />
+                    
+                    <foreign-key foreignTable="table_1420_a" name="table_1420_b_fk1">
+                        <reference local="table_1420_a_id" foreign="id"/>
+                    </foreign-key>
+                </table>
+                <table name="table_1420_c">
+                    <column name="id" primaryKey="true" type="INTEGER" />
+                    <column name="table_1420_a_id" type="INTEGER" />
+                    <column name="c_field" type="VARCHAR" size="10" />
+                    
+                    <foreign-key foreignTable="table_1420_a" name="table_1420_c_fk1">
+                        <reference local="table_1420_a_id" foreign="id"/>
+                    </foreign-key>
+                </table>
+            </database>
+XML;
+            QuickBuilder::buildSchema($schema);
+        }
+    }
+
+    protected function tearDown()
+    {
+        parent::tearDown();
+        \Table1420CQuery::create()->deleteAll();
+        \Table1420BQuery::create()->deleteAll();
+        \Table1420AQuery::create()->deleteAll();
+    }
+
+    /*
+     * Test whether hydration works properly for all the models
+     */
+    public function testValidHydration()
+    {
+        // Set up 3 models in relations A hasMany B, A hasMany C
+        // A: id=1, a_field=a_value
+        // B: id=2, b_field=b_value (referenced to A:1)
+        // C: id=3, c_field=c_value (referenced to A:1)
+        (new \Table1420A)->setId(1)->setAField('a_value')->save();
+        (new \Table1420B)->setId(2)->setTable1420AId(1)->setBField('b_value')->save();
+        (new \Table1420C)->setId(3)->setTable1420AId(1)->setCField('c_value')->save();
+
+        // querying for A models together with B models hydrated
+        $aQuery = (new \Table1420AQuery)->leftJoinWith('Table1420B');
+
+        // merged criteria has with model and adds self columns (because it's primary criteria)
+        $mergeWith = (new \Table1420AQuery)->leftJoinWith('Table1420C');
+
+        // merging queries together results produces these columns in SELECT part:
+        // A columns, B columns (base criteria), A columns again, C columns
+        // "A columns again" causes the further models be hydrated with wrong data
+        // (here C is hydrated partially from A columns)
+        $aQuery->mergeWith($mergeWith);
+
+        $a = $aQuery->find()->getFirst();
+
+        $this->assertSame(1, $a->getId());
+        $this->assertSame('a_value', $a->getAField());
+
+        $b = $a->getTable1420Bs()->getFirst();
+        $this->assertSame(2, $b->getId());
+        $this->assertSame(1, $b->getTable1420AId());
+        $this->assertSame('b_value', $b->getBField());
+
+        $c = $a->getTable1420Cs()->getFirst();
+        $this->assertSame(3, $c->getId());
+        $this->assertSame(1, $c->getTable1420AId());
+        $this->assertSame('c_value', $c->getCField());
+    }
+}

tests/Propel/Tests/Runtime/ActiveQuery/ModelCriteriaTest.php

@@ -2991,6 +2991,7 @@ public function testMergeWithWiths()
         $c1->leftJoinWith('b.Author a');
         $c2 = new ModelCriteria('bookstore', 'Propel\Tests\Bookstore\Author');
         $c1->mergeWith($c2);
+        $this->assertCount(1, array_filter($c1->getSelectColumns(), function($v) { return BookTableMap::COL_ID == $v; }), '$c1 criteria has selected Book columns twice');
         $with = $c1->getWith();
         $this->assertEquals(1, count($with), 'mergeWith() does not remove an existing join');
         $this->assertEquals('modelName: Propel\Tests\Bookstore\Author, relationName: Author, relationMethod: setAuthor, leftPhpName: , rightPhpName: a', $with['a']->__toString(), 'mergeWith() does not remove an existing join');
@@ -2999,6 +3000,7 @@ public function testMergeWithWiths()
         $c1->leftJoinWith('b.Author a');
         $c2 = new ModelCriteria('bookstore', '\Propel\Tests\Bookstore\Author');
         $c1->mergeWith($c2);
+        $this->assertCount(1, array_filter($c1->getSelectColumns(), function($v) { return BookTableMap::COL_ID == $v; }), '$c1 criteria has selected Book columns twice');
         $with = $c1->getWith();
         $this->assertEquals(1, count($with), 'mergeWith() does not remove an existing join');
         $this->assertEquals('modelName: Propel\Tests\Bookstore\Author, relationName: Author, relationMethod: setAuthor, leftPhpName: , rightPhpName: a', $with['a']->__toString(), 'mergeWith() does not remove an existing join');
@@ -3007,6 +3009,7 @@ public function testMergeWithWiths()
         $c2 = new ModelCriteria('bookstore', 'Propel\Tests\Bookstore\Book', 'b');
         $c2->leftJoinWith('b.Author a');
         $c1->mergeWith($c2);
+        $this->assertCount(1, array_filter($c1->getSelectColumns(), function($v) { return BookTableMap::COL_ID == $v; }), '$c1 criteria has selected Book columns twice');
         $with = $c1->getWith();
         $this->assertEquals(1, count($with), 'mergeWith() merge joins to an empty join');
         $this->assertEquals('modelName: Propel\Tests\Bookstore\Author, relationName: Author, relationMethod: setAuthor, leftPhpName: , rightPhpName: a', $with['a']->__toString(), 'mergeWith() merge joins to an empty join');
@@ -3016,6 +3019,7 @@ public function testMergeWithWiths()
         $c2 = new ModelCriteria('bookstore', 'Propel\Tests\Bookstore\Book', 'b');
         $c2->innerJoinWith('b.Publisher p');
         $c1->mergeWith($c2);
+        $this->assertCount(1, array_filter($c1->getSelectColumns(), function($v) { return BookTableMap::COL_ID == $v; }), '$c1 criteria has selected Book columns twice');
         $with = $c1->getWith();
         $this->assertEquals(2, count($with), 'mergeWith() merge joins to an existing join');
         $this->assertEquals('modelName: Propel\Tests\Bookstore\Author, relationName: Author, relationMethod: setAuthor, leftPhpName: , rightPhpName: a', $with['a']->__toString(), 'mergeWith() merge joins to an empty join');