From d0c6cb770082d6e6aba7b59da28383a2a2cfb230 Mon Sep 17 00:00:00 2001 From: Carsten Brandt Date: Sun, 14 Jun 2015 18:39:12 +0200 Subject: [PATCH] abort removing duplicate records when pk is not in result set in this cases it does not make sense to remove duplicates as the result is not on record level anymore. This could be the case after GROUP BY has been applied. fixes #8772 --- framework/CHANGELOG.md | 1 + framework/db/ActiveQuery.php | 10 +++ tests/data/ar/Customer.php | 7 +++ tests/framework/db/ActiveRecordTest.php | 84 +++++++++++++++++++++++++ 4 files changed, 102 insertions(+) diff --git a/framework/CHANGELOG.md b/framework/CHANGELOG.md index 44c79a8c71..5210953fa6 100644 --- a/framework/CHANGELOG.md +++ b/framework/CHANGELOG.md @@ -20,6 +20,7 @@ Yii Framework 2 Change Log - Bug #8606: Fixed `yii\web\Response::xSendFile()` does not reset format (vyants) - Bug #8627: Fixed `yii\db\Migration` produces incorrect results due to table schema caching (klimov-paul) - Bug #8661: Fixed `yii.activeForm.js` scrolling to top (nkovacs) +- Bug #8772: ActiveQuery failed removing duplicate records after join when the resultset did not contain the pk values e.g. after grouping (cebe) - Bug: Fixed string comparison in `BaseActiveRecord::unlink()` which may result in wrong comparison result for hash valued primary keys starting with `0e` (cebe) - Bug: Pass correct action name to `yii\console\Controller::options()` when default action was requested (cebe) - Bug: Automatic garbage collection in `yii\caching\FileCache` was not triggered (kidol) diff --git a/framework/db/ActiveQuery.php b/framework/db/ActiveQuery.php index 97ed4d6e2e..5088547c0d 100644 --- a/framework/db/ActiveQuery.php +++ b/framework/db/ActiveQuery.php @@ -248,9 +248,14 @@ class ActiveQuery extends Query implements ActiveQueryInterface $pks = $class::primaryKey(); if (count($pks) > 1) { + // composite primary key foreach ($models as $i => $model) { $key = []; foreach ($pks as $pk) { + if (!isset($model[$pk])) { + // do not continue if the primary key is not part of the result set + break 2; + } $key[] = $model[$pk]; } $key = serialize($key); @@ -263,8 +268,13 @@ class ActiveQuery extends Query implements ActiveQueryInterface } elseif (empty($pks)) { throw new InvalidConfigException("Primary key of '{$class}' can not be empty."); } else { + // single column primary key $pk = reset($pks); foreach ($models as $i => $model) { + if (!isset($model[$pk])) { + // do not continue if the primary key is not part of the result set + break; + } $key = $model[$pk]; if (isset($hash[$key])) { unset($models[$i]); diff --git a/tests/data/ar/Customer.php b/tests/data/ar/Customer.php index 65125c35c5..9f51da6c8c 100644 --- a/tests/data/ar/Customer.php +++ b/tests/data/ar/Customer.php @@ -22,6 +22,8 @@ class Customer extends ActiveRecord public $status2; + public $sumTotal; + public static function tableName() { return 'customer'; @@ -32,6 +34,11 @@ class Customer extends ActiveRecord return $this->hasOne(Profile::className(), ['id' => 'profile_id']); } + public function getOrdersPlain() + { + return $this->hasMany(Order::className(), ['customer_id' => 'id']); + } + public function getOrders() { return $this->hasMany(Order::className(), ['customer_id' => 'id'])->orderBy('id'); diff --git a/tests/framework/db/ActiveRecordTest.php b/tests/framework/db/ActiveRecordTest.php index dec38eddb9..78b45d9dd0 100644 --- a/tests/framework/db/ActiveRecordTest.php +++ b/tests/framework/db/ActiveRecordTest.php @@ -736,4 +736,88 @@ class ActiveRecordTest extends DatabaseTestCase $this->setExpectedException('yii\db\StaleObjectException'); $record->save(false); } + + public function testPopulateWithoutPk() + { + // tests with single pk asArray + $aggregation = Customer::find() + ->select(['{{customer}}.[[status]]', 'SUM({{order}}.[[total]]) AS [[sumtotal]]']) + ->joinWith('ordersPlain', false) + ->groupBy('{{customer}}.[[status]]') + ->orderBy('status') + ->asArray() + ->all(); + + $expected = [ + [ + 'status' => 1, + 'sumtotal' => 183, + ], + [ + 'status' => 2, + 'sumtotal' => 0, + ], + ]; + $this->assertEquals($expected, $aggregation); + + // tests with single pk with Models + $aggregation = Customer::find() + ->select(['{{customer}}.[[status]]', 'SUM({{order}}.[[total]]) AS [[sumTotal]]']) + ->joinWith('ordersPlain', false) + ->groupBy('{{customer}}.[[status]]') + ->orderBy('status') + ->all(); + $this->assertCount(2, $aggregation); + $this->assertContainsOnlyInstancesOf(Customer::className(), $aggregation); + foreach($aggregation as $item) { + if ($item->status == 1) { + $this->assertEquals(183, $item->sumTotal); + } elseif ($item->status == 2) { + $this->assertEquals(0, $item->sumTotal); + } + } + + // tests with composite pk asArray + $aggregation = OrderItem::find() + ->select(['[[order_id]]', 'SUM([[subtotal]]) AS [[subtotal]]']) + ->joinWith('order', false) + ->groupBy('[[order_id]]') + ->orderBy('[[order_id]]') + ->asArray() + ->all(); + $expected = [ + [ + 'order_id' => 1, + 'subtotal' => 70, + ], + [ + 'order_id' => 2, + 'subtotal' => 33, + ], + [ + 'order_id' => 3, + 'subtotal' => 40, + ], + ]; + $this->assertEquals($expected, $aggregation); + + // tests with composite pk with Models + $aggregation = OrderItem::find() + ->select(['[[order_id]]', 'SUM([[subtotal]]) AS [[subtotal]]']) + ->joinWith('order', false) + ->groupBy('[[order_id]]') + ->orderBy('[[order_id]]') + ->all(); + $this->assertCount(3, $aggregation); + $this->assertContainsOnlyInstancesOf(OrderItem::className(), $aggregation); + foreach($aggregation as $item) { + if ($item->order_id == 1) { + $this->assertEquals(70, $item->subtotal); + } elseif ($item->order_id == 2) { + $this->assertEquals(33, $item->subtotal); + } elseif ($item->order_id == 3) { + $this->assertEquals(40, $item->subtotal); + } + } + } }