EntityManager: Quote all identifiers

library/Notifications/Common/EntityManager.php

@@ -163,7 +163,7 @@ protected function delete(Model $model): void
         }
 
         $this->ensureOlderThanBaseline($model);
-        $this->db->delete($model->getTableName(), $condition);
+        $this->db->delete($this->db->quoteIdentifier($model->getTableName()), $condition);
         foreach ((array) $model->getKeyName() as $k) {
             unset($model->$k);
         }
@@ -376,8 +376,8 @@ private function reconcileChildren(Relation $relation, Model $model, array $keys
         }
 
         $select = (new Select())
-            ->from($childModel->getTableName())
-            ->columns($columns)
+            ->from($this->db->quoteIdentifier($childModel->getTableName()))
+            ->columns(array_map($this->db->quoteIdentifier(...), $columns))
             ->where($this->createCondition($condition));
 
         foreach ($this->db->select($select)->fetchAll(PDO::FETCH_ASSOC) as $row) {
@@ -534,7 +534,7 @@ protected function persist(Model $model, Behaviors $behaviors): void
         // Insert case
         if ($model->isNew()) {
             $this->stampChangedAt($model);
-            $this->db->insert($model->getTableName(), $this->extract($model, $behaviors));
+            $this->db->insert($this->db->quoteIdentifier($model->getTableName()), $this->extract($model, $behaviors));
 
             $keyName = $model->getKeyName();
             if (is_string($keyName) && ! $model->hasProperty($keyName)) {
@@ -583,7 +583,7 @@ protected function persist(Model $model, Behaviors $behaviors): void
         $this->stampChangedAt($model);
         $data = $this->extract($model, $behaviors, $model->getModifiedProperties());
 
-        $this->db->update($model->getTableName(), $data, $condition);
+        $this->db->update($this->db->quoteIdentifier($model->getTableName()), $data, $condition);
 
         $model->clearModifiedProperties();
     }
@@ -609,7 +609,7 @@ protected function stampChangedAt(Model $model): void
 
         $currentMax = $this->db->select(
             (new Select())
-                ->from($model->getTableName())
+                ->from($this->db->quoteIdentifier($model->getTableName()))
                 ->columns([$column => new Expression("MAX($column)")])
         )->fetchColumn();
 
@@ -687,7 +687,7 @@ protected function extract(Model $model, Behaviors $behaviors, ?array $only = nu
                 continue;
             }
 
-            $data[$property] = $behaviors->persistProperty($model->$property, $property);
+            $data[$this->db->quoteIdentifier($property)] = $behaviors->persistProperty($model->$property, $property);
         }
 
         return $data;
@@ -777,13 +777,19 @@ protected function reconcileJunction(
 
         foreach ($desired as $identity => $value) {
             if (! array_key_exists($identity, $stored)) {
-                $this->db->insert($table, array_merge($sourceColumns, [$junctionColumn => $value]));
+                $this->db->insert(
+                    $this->db->quoteIdentifier($table),
+                    $this->quoteColumns(array_merge($sourceColumns, [$junctionColumn => $value]))
+                );
             }
         }
 
         $removals = $replace ? array_diff_key($stored, $desired) : array_intersect_key($toDetach, $stored);
         foreach ($removals as $value) {
-            $this->db->delete($table, $this->createCondition(array_merge($sourceColumns, [$junctionColumn => $value])));
+            $this->db->delete(
+                $this->db->quoteIdentifier($table),
+                $this->createCondition(array_merge($sourceColumns, [$junctionColumn => $value]))
+            );
         }
     }
 
@@ -817,8 +823,8 @@ private function reconcileSoftDeleteJunction(
         }
 
         $select = (new Select())
-            ->from($junction->getTableName())
-            ->columns($columns)
+            ->from($this->db->quoteIdentifier($junction->getTableName()))
+            ->columns(array_map($this->db->quoteIdentifier(...), $columns))
             ->where($this->createCondition($sourceColumns));
 
         $stored = [];
@@ -909,8 +915,8 @@ private function reconcileSoftDeleteJunction(
     private function fetchJunctionRows(string $table, array $sourceColumns, string $junctionColumn): array
     {
         $select = (new Select())
-            ->from($table)
-            ->columns($junctionColumn)
+            ->from($this->db->quoteIdentifier($table))
+            ->columns($this->db->quoteIdentifier($junctionColumn))
             ->where($this->createCondition($sourceColumns));
 
         $stored = [];
@@ -986,12 +992,29 @@ private function createCondition(array $columns): array
     {
         $condition = [];
         foreach ($columns as $column => $value) {
-            $condition[$column . ' = ?'] = $value;
+            $condition[$this->db->quoteIdentifier($column) . ' = ?'] = $value;
         }
 
         return $condition;
     }
 
+    /**
+     * Copy a column => value map with its keys quoted with ({@see Connection::quoteIdentifier()})
+     *
+     * @param array<string, mixed> $data
+     *
+     * @return array<string, mixed>
+     */
+    private function quoteColumns(array $data): array
+    {
+        $quoted = [];
+        foreach ($data as $column => $value) {
+            $quoted[$this->db->quoteIdentifier($column)] = $value;
+        }
+
+        return $quoted;
+    }
+
     /**
      * Get the model's table columns and cache them per model class
      *

test/php/Lib/EntityManager/Pairing.php

@@ -7,20 +7,28 @@
 
 use Icinga\Module\Notifications\Common\Model;
 
+/**
+ * Compound-key fixture whose table and columns are deliberately SQL reserved keywords.
+ *
+ * `values` (table), `order`/`group` (the compound primary key) and `insert` (a plain column) are reserved in
+ * both sqlite and MySQL, so every identifier the {@see EntityManager} emits for this model must be quoted or the
+ * statement is a syntax error. This exercises the quoting of table names, INSERT/UPDATE data columns and the
+ * compound primary key WHERE across the insert/update/delete lifecycle.
+ */
 class Pairing extends Model
 {
     public function getTableName(): string
     {
-        return 'pairing';
+        return 'values';
     }
 
     public function getKeyName(): array
     {
-        return ['left_id', 'right_id'];
+        return ['order', 'group'];
     }
 
     public function getColumns(): array
     {
-        return ['left_id', 'right_id', 'label'];
+        return ['order', 'group', 'insert'];
     }
 }

test/php/Lib/EntityManager/RecordingConnection.php

@@ -28,7 +28,11 @@ class RecordingConnection extends Connection
     public function insert(string $table, iterable $data): PDOStatement
     {
         $data = is_array($data) ? $data : iterator_to_array($data);
-        $this->calls[] = ['method' => 'insert', 'table' => $table, 'data' => $data];
+        $this->calls[] = [
+            'method' => 'insert',
+            'table'  => $this->unquote($table),
+            'data'   => $this->unquoteKeys($data),
+        ];
 
         return parent::insert($table, $data);
     }
@@ -42,9 +46,9 @@ public function update(
         $data = is_array($data) ? $data : iterator_to_array($data);
         $this->calls[] = [
             'method'    => 'update',
-            'table'     => $table,
-            'data'      => $data,
-            'condition' => $condition,
+            'table'     => is_string($table) ? $this->unquote($table) : $table,
+            'data'      => $this->unquoteKeys($data),
+            'condition' => is_array($condition) ? $this->unquoteKeys($condition) : $condition,
         ];
 
         return parent::update($table, $data, $condition, $operator);
@@ -55,11 +59,51 @@ public function delete(
         string|array|null $condition = null,
         string $operator = Sql::ALL
     ): PDOStatement {
-        $this->calls[] = ['method' => 'delete', 'table' => $table, 'condition' => $condition];
+        $this->calls[] = [
+            'method'    => 'delete',
+            'table'     => is_string($table) ? $this->unquote($table) : $table,
+            'condition' => is_array($condition) ? $this->unquoteKeys($condition) : $condition,
+        ];
 
         return parent::delete($table, $condition, $operator);
     }
 
+    /**
+     * Strip the adapter's identifier quoting from a recorded string
+     *
+     * The EntityManager quotes every identifier it emits ({@see Connection::quoteIdentifier()}). The recorded
+     * calls are normalized back to their logical names so assertions can express the expected write set in plain
+     * table/column names, independent of the adapter's quote character. Only the real SQL forwarded to the parent
+     * keeps the quoting.
+     *
+     * @param string $identifier
+     *
+     * @return string
+     */
+    private function unquote(string $identifier): string
+    {
+        $quoted = $this->quoteIdentifier('x');
+
+        return str_replace([$quoted[0], $quoted[strlen($quoted) - 1]], '', $identifier);
+    }
+
+    /**
+     * Copy a map with its keys (quoted identifiers or `column = ?` expressions) unquoted via {@see self::unquote()}
+     *
+     * @param array<string, mixed> $data
+     *
+     * @return array<string, mixed>
+     */
+    private function unquoteKeys(array $data): array
+    {
+        $unquoted = [];
+        foreach ($data as $key => $value) {
+            $unquoted[$this->unquote((string) $key)] = $value;
+        }
+
+        return $unquoted;
+    }
+
     /**
      * Drop the recorded calls so subsequent assertions only see writes from the next action
      *

test/php/library/Notifications/Common/EntityManagerTest.php

@@ -89,9 +89,9 @@ protected function connect(array $pdoOptions = []): RecordingConnection
             . 'CREATE TABLE trinket (id BLOB PRIMARY KEY, name VARCHAR NOT NULL);'
             . 'CREATE TABLE charm (id INTEGER PRIMARY KEY AUTOINCREMENT,
                 trinket_id BLOB NOT NULL, label VARCHAR NOT NULL);'
-            . 'CREATE TABLE pairing ('
-            . 'left_id INTEGER NOT NULL, right_id INTEGER NOT NULL, label VARCHAR NOT NULL,'
-            . ' PRIMARY KEY (left_id, right_id));'
+            . 'CREATE TABLE "values" ('
+            . '"order" INTEGER NOT NULL, "group" INTEGER NOT NULL, "insert" VARCHAR NOT NULL,'
+            . ' PRIMARY KEY ("order", "group"));'
             . 'CREATE TABLE tag (id INTEGER PRIMARY KEY AUTOINCREMENT, name VARCHAR NOT NULL);'
             . 'CREATE TABLE gadget_tag ('
             . 'gadget_id INTEGER NOT NULL, tag_id INTEGER NOT NULL,'
@@ -302,40 +302,42 @@ public function testChangingMutabilityThrowsAnException()
 
     public function testCompoundKeyIsWrittenScopedAndClearedAcrossItsLifecycle()
     {
-        // Two rows sharing left_id but differing in right_id: every WHERE has to include *both* key
-        // columns to target exactly one of them.
+        // Two rows sharing `order` but differing in `group`: every WHERE has to include *both* key
+        // columns to target exactly one of them. The Pairing fixture's table (`values`), key columns
+        // (`order`/`group`) and plain column (`insert`) are all SQL reserved keywords, so this also
+        // proves the EntityManager quotes table names, INSERT/UPDATE columns and the compound key WHERE.
         $a = (new Pairing())->setNew();
-        $a->left_id = 1;
-        $a->right_id = 1;
-        $a->label = 'A';
+        $a->order = 1;
+        $a->group = 1;
+        $a->insert = 'A';
         $this->em()->save($a);
 
         $b = (new Pairing())->setNew();
-        $b->left_id = 1;
-        $b->right_id = 2;
-        $b->label = 'B';
+        $b->order = 1;
+        $b->group = 2;
+        $b->insert = 'B';
         $this->em()->save($b);
 
         $this->assertFalse($b->isNew(), 'A saved compound-key model should no longer be new');
         $this->assertFalse($b->isModified(), 'A saved compound-key model should carry no pending changes');
-        $this->assertSame(1, $b->left_id, 'left_id should not be overwritten by a lastInsertId fetch');
-        $this->assertSame(2, $b->right_id, 'right_id should not be overwritten by a lastInsertId fetch');
+        $this->assertSame(1, $b->order, 'order should not be overwritten by a lastInsertId fetch');
+        $this->assertSame(2, $b->group, 'group should not be overwritten by a lastInsertId fetch');
 
         $this->db->resetCalls();
-        $b->label = 'B2';
+        $b->insert = 'B2';
         $this->em()->save($b);
         $this->assertSame(
-            ['left_id = ?' => 1, 'right_id = ?' => 2],
+            ['order = ?' => 1, 'group = ?' => 2],
             $this->db->calls[0]['condition'],
             'Both key columns should scope the UPDATE'
         );
 
         $this->em()->save($b->delete());
-        $this->assertFalse($b->hasProperty('left_id'), 'Each part of a compound key should be cleared on delete');
-        $this->assertFalse($b->hasProperty('right_id'), 'Each part of a compound key should be cleared on delete');
+        $this->assertFalse($b->hasProperty('order'), 'Each part of a compound key should be cleared on delete');
+        $this->assertFalse($b->hasProperty('group'), 'Each part of a compound key should be cleared on delete');
         $this->assertSame(
-            [['left_id' => 1, 'right_id' => 1, 'label' => 'A']],
-            $this->rows('SELECT left_id, right_id, label FROM pairing'),
+            [['order' => 1, 'group' => 1, 'insert' => 'A']],
+            $this->rows('SELECT "order", "group", "insert" FROM "values"'),
             'Only the row matching all key columns should be updated then deleted; the sibling stays intact'
         );
     }