Fix HasMany concat with non-physical field (#1085)

atk4 · Jan 7, 2023 · aacc247 · aacc247
1 parent 24e5a96
commit aacc247
Show file tree

Hide file tree

Showing 14 changed files with 88 additions and 77 deletions.
diff --git a/src/Field/SqlExpressionField.php b/src/Field/SqlExpressionField.php
@@ -26,32 +26,15 @@ class SqlExpressionField extends Field
     /** @var string Specifies how to aggregate this. */
     public $aggregate;
 
-    /** @var string Aggregation by concatenation. */
-    public $concat;
+    /** @var string */
+    public $concatSeparator;
 
     /** @var Reference\HasMany|null When defining as aggregate, this will point to relation object. */
     public $aggregateRelation;
 
     /** @var string Specifies which field to use. */
     public $field;
 
-    protected function init(): void
-    {
-        $this->_init();
-
-        if ($this->concat) {
-            $this->onHookToOwnerEntity(Model::HOOK_AFTER_SAVE, \Closure::fromCallable([$this, 'afterSave']));
-        }
-    }
-
-    /**
-     * Possibly that user will attempt to insert values here. If that is the case, then
-     * we would need to inject it into related hasMany relationship.
-     */
-    public function afterSave(Model $entity): void
-    {
-    }
-
     /**
      * Should this field use alias?
      * Expression fields always need alias.

diff --git a/src/Model.php b/src/Model.php
@@ -863,9 +863,7 @@ public function setId($value)
      */
     public function getModelCaption(): string
     {
-        return $this->caption ?? $this->readableCaption(
-            (new \ReflectionClass(static::class))->isAnonymous() ? get_parent_class(static::class) : static::class
-        );
+        return $this->caption ?? $this->readableCaption(get_debug_type($this));
     }
 
     /**

diff --git a/src/Persistence/Sql.php b/src/Persistence/Sql.php
@@ -415,20 +415,25 @@ public function action(Model $model, string $type, array $args = [])
                 [$fx, $field] = $args;
                 $field = is_string($field) ? $model->getField($field) : $field;
 
-                if ($type === 'fx') {
-                    $expr = $fx . '([])';
+                $query = $this->action($model, 'select', [[]]);
+
+                if ($fx === 'concat') {
+                    $expr = $query->groupConcat($field, $args['concatSeparator']);
                 } else {
-                    $expr = 'coalesce(' . $fx . '([]), 0)';
+                    $expr = $query->expr(
+                        $type === 'fx'
+                            ? $fx . '([])'
+                            : 'coalesce(' . $fx . '([]), 0)',
+                        [$field]
+                    );
                 }
 
-                $query = $this->action($model, 'select', [[]]);
-
                 if (isset($args['alias'])) {
-                    $query->reset('field')->field($query->expr($expr, [$field]), $args['alias']);
+                    $query->reset('field')->field($expr, $args['alias']);
                 } elseif ($field instanceof SqlExpressionField) {
-                    $query->reset('field')->field($query->expr($expr, [$field]), $fx . '_' . $field->shortName);
+                    $query->reset('field')->field($expr, $fx . '_' . $field->shortName);
                 } else {
-                    $query->reset('field')->field($query->expr($expr, [$field]));
+                    $query->reset('field')->field($expr);
                 }
                 $this->fixMssqlOracleMissingFieldsInGroup($model, $query);
 

diff --git a/src/Persistence/Sql/Mssql/Query.php b/src/Persistence/Sql/Mssql/Query.php
@@ -51,9 +51,9 @@ protected function _renderLimit(): ?string
             . ' fetch next ' . $cnt . ' rows only';
     }
 
-    public function groupConcat($field, string $delimiter = ',')
+    public function groupConcat($field, string $separator = ',')
     {
-        return $this->expr('string_agg({}, ' . $this->escapeStringLiteral($delimiter) . ')', [$field]);
+        return $this->expr('string_agg({}, ' . $this->escapeStringLiteral($separator) . ')', [$field]);
     }
 
     public function exists()

diff --git a/src/Persistence/Sql/Mysql/Query.php b/src/Persistence/Sql/Mysql/Query.php
@@ -17,8 +17,8 @@ class Query extends BaseQuery
 
     protected string $templateUpdate = 'update [table][join] set [set] [where]';
 
-    public function groupConcat($field, string $delimiter = ',')
+    public function groupConcat($field, string $separator = ',')
     {
-        return $this->expr('group_concat({} separator ' . $this->escapeStringLiteral($delimiter) . ')', [$field]);
+        return $this->expr('group_concat({} separator ' . $this->escapeStringLiteral($separator) . ')', [$field]);
     }
 }
diff --git a/src/Persistence/Sql/Oracle/Query.php b/src/Persistence/Sql/Oracle/Query.php
@@ -79,9 +79,9 @@ protected function _renderLimit(): ?string
             . ' fetch next ' . $cnt . ' rows only';
     }
 
-    public function groupConcat($field, string $delimiter = ',')
+    public function groupConcat($field, string $separator = ',')
     {
-        return $this->expr('listagg({field}, []) within group (order by {field})', ['field' => $field, $delimiter]);
+        return $this->expr('listagg({field}, []) within group (order by {field})', ['field' => $field, $separator]);
     }
 
     public function exists()

diff --git a/src/Persistence/Sql/Postgresql/Query.php b/src/Persistence/Sql/Postgresql/Query.php
@@ -38,8 +38,8 @@ protected function _renderLimit(): ?string
             . ' offset ' . (int) $this->args['limit']['shift'];
     }
 
-    public function groupConcat($field, string $delimiter = ','): BaseExpression
+    public function groupConcat($field, string $separator = ','): BaseExpression
     {
-        return $this->expr('string_agg({}, [])', [$field, $delimiter]);
+        return $this->expr('string_agg({}, [])', [$field, $separator]);
     }
 }
diff --git a/src/Persistence/Sql/Query.php b/src/Persistence/Sql/Query.php
@@ -1036,7 +1036,7 @@ public function andExpr()
      *
      * @return Expression
      */
-    public function groupConcat($field, string $delimiter = ',')
+    public function groupConcat($field, string $separator = ',')
     {
         throw new Exception('groupConcat() is SQL-dependent, so use a correct class');
     }

diff --git a/src/Persistence/Sql/Sqlite/Query.php b/src/Persistence/Sql/Sqlite/Query.php
@@ -13,8 +13,8 @@ class Query extends BaseQuery
 
     protected string $templateTruncate = 'delete [from] [tableNoalias]';
 
-    public function groupConcat($field, string $delimiter = ',')
+    public function groupConcat($field, string $separator = ',')
     {
-        return $this->expr('group_concat({}, [])', [$field, $delimiter]);
+        return $this->expr('group_concat({}, [])', [$field, $separator]);
     }
 }
diff --git a/src/Reference/HasMany.php b/src/Reference/HasMany.php
@@ -125,7 +125,9 @@ public function addField(string $fieldName, array $defaults = []): Field
         $field = $alias ?? $fieldName;
 
         if (isset($defaults['concat'])) {
-            $defaults['aggregate'] = $this->getOurModel(null)->dsql()->groupConcat($field, $defaults['concat']); // TODO better to concat by NUL byte or very unlikely string
+            $defaults['aggregate'] = 'concat';
+            $defaults['concatSeparator'] = $defaults['concat'];
+            unset($defaults['concat']);
         }
 
         if (isset($defaults['expr'])) {
@@ -152,7 +154,12 @@ public function addField(string $fieldName, array $defaults = []): Field
             };
         } else {
             $fx = function () use ($defaults, $field) {
-                return $this->refLink(null)->action('fx', [$defaults['aggregate'], $field]);
+                $args = [$defaults['aggregate'], $field];
+                if ($defaults['aggregate'] === 'concat') {
+                    $args['concatSeparator'] = $defaults['concatSeparator'];
+                }
+
+                return $this->refLink(null)->action('fx', $args);
             };
         }
 

diff --git a/tests/LookupSqlTest.php b/tests/LookupSqlTest.php
@@ -6,6 +6,7 @@
 
 use Atk4\Data\Model;
 use Atk4\Data\Schema\TestCase;
+use Doctrine\DBAL\Platforms\SQLServerPlatform;
 
 /**
  * You can lookup country by name or by code. We will also try looking up country by
@@ -32,13 +33,11 @@ protected function init(): void
         $this->addField('is_eu', ['type' => 'boolean', 'default' => false]);
 
         $this->hasMany('Users', ['model' => [LUser::class]])
-            ->addField('user_names', ['field' => 'name', 'concat' => ',']);
+            ->addField('user_names', ['field' => 'name', 'concat' => ', ']);
     }
 }
 
 /**
- * User.
- *
  * User has one country and may have friends. Friend is a many-to-many relationship between users.
  *
  * When importing users, you should be able to specify country using 'country_id' or using some of the
@@ -70,7 +69,7 @@ protected function init(): void
             ->addTitle();
 
         $this->hasMany('Friends', ['model' => [LFriend::class]])
-            ->addField('friend_names', ['field' => 'friend_name', 'concat' => ',']);
+            ->addField('friend_names', ['field' => 'friend_name', 'concat' => '; ']);
     }
 }
 
@@ -91,8 +90,7 @@ class LFriend extends Model
     public $table = 'friend';
     public ?string $titleField = 'friend_name';
 
-    /** @var bool */
-    public $skipReverse = false;
+    protected bool $skipReverse = false;
 
     protected function init(): void
     {
@@ -103,35 +101,32 @@ protected function init(): void
         $this->hasOne('friend_id', ['model' => [LUser::class]])
             ->addField('friend_name', 'name');
 
-        // add or remove reverse friendships
-        /*
+        // add/remove reverse friendships
         $this->onHookShort(self::HOOK_AFTER_INSERT, function () {
             if ($this->skipReverse) {
                 return;
             }
 
-            $c = clone $this;
+            $c = $this->getModel()->createEntity();
             $c->skipReverse = true;
-            $this->insert([
-                'user_id' => $this->get('friend_id'),
-                'friend_id' => $this->get('user_id'),
-            ]);
+            // $c->insert([
+            //     'user_id' => $this->get('friend_id'),
+            //     'friend_id' => $this->get('user_id'),
+            // ]);
         });
 
         $this->onHookShort(Model::HOOK_BEFORE_DELETE, function () {
             if ($this->skipReverse) {
                 return;
             }
 
-            $c = clone $this;
-            $c->skipReverse = true;
-
-            $c = $c->loadBy([
-                'user_id' => $this->get('friend_id'),
-                'friend_id' => $this->get('user_id'),
-            ])->delete();
+            // $c = $this->getModel()->loadBy([
+            //     'user_id' => $this->get('friend_id'),
+            //     'friend_id' => $this->get('user_id'),
+            // ]);
+            // $c->skipReverse = true;
+            // $c->delete();
         });
-        */
     }
 }
 
@@ -231,10 +226,7 @@ public function testImportInternationalUsers(): void
     {
         $c = new LCountry($this->db);
 
-        // Specifying hasMany here will perform input
         $c->insert(['name' => 'Canada', 'Users' => [['name' => 'Alain'], ['name' => 'Duncan', 'is_vip' => true]]]);
-
-        // Both lines will work quite similar
         $c->insert(['name' => 'Latvia', 'Users' => [['name' => 'imants'], ['name' => 'juris']]]);
 
         static::assertSameExportUnordered([
@@ -285,7 +277,6 @@ public function testImportByLookup(): void
     {
         $c = new LCountry($this->db);
 
-        // Specifying hasMany here will perform input
         $c->import([
             ['name' => 'Canada', 'code' => 'CA'],
             ['name' => 'Latvia', 'code' => 'LV', 'is_eu' => true],
@@ -303,7 +294,7 @@ public function testImportByLookup(): void
         $u->import([
             ['name' => 'Alain', 'country_code' => 'CA'],
             ['name' => 'Imants', 'country_code' => 'LV'],
-            // 'name' => 'Romans', 'country_code' => 'UK'], // does not exist
+            // ['name' => 'Romans', 'country_code' => 'UK'], // country code does not exist
         ]);
 
         static::assertSameExportUnordered([
@@ -356,11 +347,39 @@ public function testImportByLookup(): void
         ], $this->getDb(['country', 'user']));
     }
 
-    /* TODO - that's left for hasMTM implementation..., to be coming later
     public function testImportInternationalFriends(): void
     {
         $c = new LCountry($this->db);
 
+        $c->insert(['name' => 'Canada', 'Users' => [['name' => 'Alain'], ['name' => 'Duncan', 'is_vip' => true]]]);
+        $c->insert(['name' => 'Latvia', 'Users' => [['name' => 'imants'], ['name' => 'juris']]]);
+
+        static::assertSame('imants, juris', $c->loadBy('name', 'Latvia')->get('user_names'));
+
+        if ($this->getDatabasePlatform() instanceof SQLServerPlatform) {
+            static::markTestIncomplete('TODO MSSQL: Cannot perform an aggregate function on an expression containing an aggregate or a subquery');
+        }
+
+        $user1 = $c->ref('Users')->loadBy('name', 'Duncan');
+        $user2 = $c->loadBy('name', 'Latvia')->ref('Users')->loadBy('name', 'imants');
+        $user3 = $user2->getModel()->loadBy('name', 'juris');
+
+        $user2->ref('Friends')->import([
+            ['friend_id' => $user1->getId()],
+            ['friend_id' => $user3->getId()],
+        ]);
+
+        static::assertNull($user1->get('friend_names'));
+        static::assertNull($user2->get('friend_names'));
+        static::assertNull($user3->get('friend_names'));
+        $user1->reload();
+        $user2->reload();
+        $user3->reload();
+        static::assertNull($user1->get('friend_names'));
+        static::assertSame('Duncan; juris', $user2->get('friend_names'));
+        static::assertNull($user3->get('friend_names'));
+
+        /* TODO - that's left for hasMTM implementation..., to be coming later
         // Specifying hasMany here will perform input
         $c->insert(['Canada', 'Users' => ['Alain', ['Duncan', 'is_vip' => true]]]);
 
@@ -377,6 +396,6 @@ public function testImportInternationalFriends(): void
         ]]);
 
         // BTW - Alain should have 3 friends here
+        */
     }
-    */
 }
diff --git a/tests/Model/Smbo/Transfer.php b/tests/Model/Smbo/Transfer.php
@@ -35,8 +35,6 @@ protected function init(): void
 
                 $m2->_unset('destination_account_id');
 
-                $m2->getModel()->reloadAfterSave = false; // avoid check
-
                 $this->set('transfer_document_id', $m2->save()->getId());
             }
         });

diff --git a/tests/Persistence/SqlTest.php b/tests/Persistence/SqlTest.php
@@ -153,8 +153,7 @@ public function testModelSaveNoReload(): void
         $m->save(['name' => 'Jane', 'surname' => 'Doe']);
         static::assertSame('Jane', $m->get('name'));
         static::assertSame('Doe', $m->get('surname'));
-        static::assertSame(3, $m->getId());
-        // id field value is set with new id value even if reloadAfterSave = false
+        // ID field is set with new value even if reloadAfterSave = false
         static::assertSame(3, $m->getId());
     }
 

diff --git a/tests/RandomTest.php b/tests/RandomTest.php
@@ -334,11 +334,13 @@ public function testIssue220(): void
 
     public function testModelCaption(): void
     {
-        $m = new Model($this->db, ['table' => 'user']);
-
         // caption is not set, so generate it from class name Model
+        $m = new Model($this->db, ['table' => 'user']);
         static::assertSame('Atk 4 Data Model', $m->getModelCaption());
 
+        $m = new class($this->db, ['table' => 'user']) extends Model {};
+        static::assertSame('Atk 4 Data Model Anonymous', $m->getModelCaption());
+
         // caption is set
         $m->caption = 'test';
         static::assertSame('test', $m->getModelCaption());