Fix inefficient list views which render object names (#155)

fixes #153 requires Icinga/ipl-orm#128, Icinga/ipl-orm#126 and Icinga/ipl-orm#124
Icinga · Apr 24, 2024 · 7c602fc · 7c602fc
2 parents 0a3b06c + 49c3608
commit 7c602fc
Show file tree

Hide file tree

Showing 10 changed files with 192 additions and 27 deletions.
diff --git a/application/controllers/EventController.php b/application/controllers/EventController.php
@@ -26,6 +26,7 @@ public function indexAction(): void
 
         $query = Event::on(Database::get())
             ->with(['object', 'object.source', 'incident', 'incident.object'])
+            ->withColumns(['object.id_tags', 'incident.object.id_tags'])
             ->filter(Filter::equal('event.id', $id));
 
         // ipl-orm doesn't detect dependent joins yet

diff --git a/application/controllers/EventsController.php b/application/controllers/EventsController.php
@@ -32,7 +32,8 @@ public function indexAction(): void
         $compact = $this->view->compact;
 
         $events = Event::on(Database::get())
-            ->with(['object', 'object.source', 'incident']);
+            ->with(['object', 'object.source', 'incident'])
+            ->withColumns('object.id_tags');
 
         $limitControl = $this->createLimitControl();
         $sortControl = $this->createSortControl(

diff --git a/application/controllers/IncidentController.php b/application/controllers/IncidentController.php
@@ -27,6 +27,7 @@ public function indexAction(): void
 
         $query = Incident::on(Database::get())
             ->with(['object', 'object.source'])
+            ->withColumns('object.id_tags')
             ->filter(Filter::equal('incident.id', $id));
 
         $this->applyRestrictions($query);

diff --git a/application/controllers/IncidentsController.php b/application/controllers/IncidentsController.php
@@ -29,7 +29,8 @@ public function indexAction(): void
         $this->addTitleTab(t('Incidents'));
 
         $incidents = Incident::on(Database::get())
-            ->with('object');
+            ->with('object')
+            ->withColumns('object.id_tags');
 
         $limitControl = $this->createLimitControl();
         $sortControl = $this->createSortControl(

diff --git a/library/Notifications/Common/Database.php b/library/Notifications/Common/Database.php
@@ -7,6 +7,7 @@
 use Icinga\Application\Config as AppConfig;
 use Icinga\Data\ResourceFactory;
 use Icinga\Exception\ConfigurationError;
+use ipl\Orm\Query;
 use ipl\Sql\Adapter\Pgsql;
 use ipl\Sql\Config as SqlConfig;
 use ipl\Sql\Connection;
@@ -105,4 +106,42 @@ private static function getConnection(): Connection
 
         return $db;
     }
+
+    /**
+     * Generate a group by expression and register it on the given select
+     *
+     * @param Query $query
+     * @param Select $select
+     *
+     * @return void
+     */
+    public static function registerGroupBy(Query $query, Select $select): void
+    {
+        $resolver = $query->getResolver();
+
+        $groupBy = [];
+        foreach ((array) $query->getModel()->getKeyName() as $key) {
+            $groupBy[] = $resolver->qualifyColumn($key, $resolver->getAlias($query->getModel()));
+        }
+
+        foreach ($query->getWith() as $relation) {
+            foreach ((array) $relation->getTarget()->getKeyName() as $key) {
+                $groupBy[] = $resolver->qualifyColumn($key, $resolver->getAlias($relation->getTarget()));
+            }
+        }
+
+        // For PostgreSQL, ALL non-aggregate SELECT columns must appear in the GROUP BY clause:
+        if ($query->getDb()->getAdapter() instanceof Pgsql) {
+            /**
+             * Ignore Expressions, i.e. aggregate functions {@see getColumns()},
+             * which do not need to be added to the GROUP BY.
+             */
+            $candidates = array_filter($select->getColumns(), 'is_string');
+            // Remove already considered columns for the GROUP BY
+            $candidates = array_diff($candidates, $groupBy);
+            $groupBy = array_merge($groupBy, $candidates);
+        }
+
+        $select->groupBy($groupBy);
+    }
 }
diff --git a/library/Notifications/Model/Behavior/IdTagAggregator.php b/library/Notifications/Model/Behavior/IdTagAggregator.php
@@ -0,0 +1,86 @@
+<?php
+
+/* Icinga Notifications Web | (c) 2024 Icinga GmbH | GPLv2 */
+
+namespace Icinga\Module\Notifications\Model\Behavior;
+
+use Icinga\Module\Notifications\Model\Objects;
+use ipl\Orm\AliasedExpression;
+use ipl\Orm\ColumnDefinition;
+use ipl\Orm\Contract\PropertyBehavior;
+use ipl\Orm\Contract\QueryAwareBehavior;
+use ipl\Orm\Contract\RewriteColumnBehavior;
+use ipl\Orm\Exception\InvalidColumnException;
+use ipl\Orm\Query;
+use ipl\Sql\Adapter\Pgsql;
+use ipl\Stdlib\Filter;
+
+class IdTagAggregator extends PropertyBehavior implements RewriteColumnBehavior, QueryAwareBehavior
+{
+    /** @var Query */
+    protected $query;
+
+    final public function __construct()
+    {
+        // TODO: Should not be based on PropertyBehavior. (https://github.com/Icinga/ipl-orm/issues/129)
+        parent::__construct(['id_tags']);
+    }
+
+    public function setQuery(Query $query)
+    {
+        $this->query = $query;
+    }
+
+    public function rewriteColumn($column, ?string $relation = null)
+    {
+        if ($column === 'id_tags') {
+            $path = ($relation ?? $this->query->getModel()->getTableAlias()) . '.object_id_tag';
+
+            $this->query->utilize($path);
+            $pathAlias = $this->query->getResolver()->getAlias(
+                $this->query->getResolver()->resolveRelation($path)->getTarget()
+            );
+
+            $myAlias = $this->query->getResolver()->getAlias(
+                $relation
+                    ? $this->query->getResolver()->resolveRelation($relation)->getTarget()
+                    : $this->query->getModel()
+            );
+
+            return new AliasedExpression("{$myAlias}_id_tags", sprintf(
+                $this->query->getDb()->getAdapter() instanceof Pgsql
+                    ? 'json_object_agg(%s, %s)'
+                    : 'json_objectagg(%s, %s)',
+                $this->query->getResolver()->qualifyColumn('tag', $pathAlias),
+                $this->query->getResolver()->qualifyColumn('value', $pathAlias)
+            ));
+        }
+    }
+
+    public function isSelectableColumn(string $name): bool
+    {
+        return $name === 'id_tags';
+    }
+
+    public function fromDb($value, $key, $context)
+    {
+        if (! is_string($value)) {
+            return [];
+        }
+
+        return json_decode($value, true) ?? [];
+    }
+
+    public function toDb($value, $key, $context)
+    {
+        throw new InvalidColumnException($key, new Objects());
+    }
+
+    public function rewriteColumnDefinition(ColumnDefinition $def, string $relation): void
+    {
+    }
+
+    public function rewriteCondition(Filter\Condition $condition, $relation = null)
+    {
+    }
+}
diff --git a/library/Notifications/Model/Event.php b/library/Notifications/Model/Event.php
@@ -4,12 +4,15 @@
 
 namespace Icinga\Module\Notifications\Model;
 
+use Icinga\Module\Notifications\Common\Database;
 use ipl\Orm\Behavior\Binary;
 use ipl\Orm\Behavior\MillisecondTimestamp;
 use ipl\Orm\Behaviors;
 use ipl\Orm\Model;
 use ipl\Orm\Query;
 use ipl\Orm\Relations;
+use ipl\Sql\Connection;
+use ipl\Sql\Select;
 
 /**
  * Event model
@@ -62,6 +65,19 @@ public function getDefaultSort()
         return 'event.time';
     }
 
+    public static function on(Connection $db)
+    {
+        $query = parent::on($db);
+
+        $query->on(Query::ON_SELECT_ASSEMBLED, function (Select $select) use ($query) {
+            if (isset($query->getUtilize()['event.object.object_id_tag'])) {
+                Database::registerGroupBy($query, $select);
+            }
+        });
+
+        return $query;
+    }
+
     public function createBehaviors(Behaviors $behaviors)
     {
         $behaviors->add(new MillisecondTimestamp(['time']));

diff --git a/library/Notifications/Model/Incident.php b/library/Notifications/Model/Incident.php
@@ -4,11 +4,15 @@
 
 namespace Icinga\Module\Notifications\Model;
 
+use Icinga\Module\Notifications\Common\Database;
 use ipl\Orm\Behavior\Binary;
 use ipl\Orm\Behavior\MillisecondTimestamp;
 use ipl\Orm\Behaviors;
 use ipl\Orm\Model;
+use ipl\Orm\Query;
 use ipl\Orm\Relations;
+use ipl\Sql\Connection;
+use ipl\Sql\Select;
 
 /**
  * Incident Model
@@ -57,6 +61,19 @@ public function getDefaultSort()
         return ['incident.severity desc, incident.started_at'];
     }
 
+    public static function on(Connection $db)
+    {
+        $query = parent::on($db);
+
+        $query->on(Query::ON_SELECT_ASSEMBLED, function (Select $select) use ($query) {
+            if (isset($query->getUtilize()['incident.object.object_id_tag'])) {
+                Database::registerGroupBy($query, $select);
+            }
+        });
+
+        return $query;
+    }
+
     public function createBehaviors(Behaviors $behaviors)
     {
         $behaviors->add(new Binary(['object_id']));

diff --git a/library/Notifications/Model/Objects.php b/library/Notifications/Model/Objects.php
@@ -4,14 +4,17 @@
 
 namespace Icinga\Module\Notifications\Model;
 
+use Icinga\Module\Notifications\Model\Behavior\IdTagAggregator;
 use ipl\Html\HtmlString;
 use ipl\Html\ValidHtml;
 use ipl\Orm\Behavior\Binary;
 use ipl\Orm\Behaviors;
 use ipl\Orm\Model;
-use ipl\Orm\Query;
 use ipl\Orm\Relations;
 
+/**
+ * @property array<string, string> $id_tags
+ */
 class Objects extends Model
 {
     public function getTableName()
@@ -52,6 +55,7 @@ public function getDefaultSort()
     public function createBehaviors(Behaviors $behaviors)
     {
         $behaviors->add(new Binary(['id']));
+        $behaviors->add(new IdTagAggregator());
     }
 
     public function createRelations(Relations $relations)
@@ -73,15 +77,8 @@ public function getName(): ValidHtml
     {
         //TODO: Once hooks are available, they should render the tags accordingly
         $objectTags = [];
-        /** @var Query $objectIdTagQuery */
-        $objectIdTagQuery = $this->object_id_tag;
-        /** @var ObjectIdTag $id_tag */
-        foreach ($objectIdTagQuery as $id_tag) {
-            /** @var string $tag */
-            $tag = $id_tag->tag;
-            /** @var string $value */
-            $value = $id_tag->value;
 
+        foreach ($this->id_tags as $tag => $value) {
             $objectTags[] = sprintf('%s=%s', $tag, $value);
         }
 

diff --git a/library/Notifications/Widget/Detail/IncidentDetail.php b/library/Notifications/Widget/Detail/IncidentDetail.php
@@ -7,7 +7,6 @@
 use Icinga\Module\Notifications\Common\Database;
 use Icinga\Module\Notifications\Model\Incident;
 use Icinga\Module\Notifications\Model\Objects;
-use Icinga\Module\Notifications\Model\Source;
 use Icinga\Module\Notifications\Widget\EventSourceBadge;
 use Icinga\Module\Notifications\Widget\ItemList\IncidentContactList;
 use Icinga\Module\Notifications\Widget\ItemList\IncidentHistoryList;
@@ -16,7 +15,8 @@
 use ipl\Html\Html;
 use ipl\Html\HtmlElement;
 use ipl\Html\Table;
-use ipl\Stdlib\Filter;
+use ipl\Orm\Query;
+use ipl\Sql\Select;
 use ipl\Web\Widget\Link;
 use ipl\Web\Widget\StateBall;
 
@@ -93,22 +93,28 @@ protected function createRelatedObject()
 
     protected function createHistory()
     {
+        $history = $this->incident->incident_history
+            ->with([
+                'event',
+                'event.object',
+                'event.object.source',
+                'contact',
+                'rule',
+                'rule_escalation',
+                'contactgroup',
+                'schedule',
+                'channel'
+            ]);
+
+        $history
+            ->withColumns('event.object.id_tags')
+            ->on(Query::ON_SELECT_ASSEMBLED, function (Select $select) use ($history) {
+                Database::registerGroupBy($history, $select);
+            });
+
         return [
             Html::tag('h2', t('Incident History')),
-            new IncidentHistoryList(
-                $this->incident->incident_history
-                    ->with([
-                        'event',
-                        'event.object',
-                        'event.object.source',
-                        'contact',
-                        'rule',
-                        'rule_escalation',
-                        'contactgroup',
-                        'schedule',
-                        'channel'
-                    ])
-            )
+            new IncidentHistoryList($history)
         ];
     }