diff --git a/library/Notifications/Common/EntityManager.php b/library/Notifications/Common/EntityManager.php index 5b35bc485..0d9b04d1d 100644 --- a/library/Notifications/Common/EntityManager.php +++ b/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 $data + * + * @return array + */ + 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 * diff --git a/test/php/Lib/EntityManager/Pairing.php b/test/php/Lib/EntityManager/Pairing.php index bec33a3bf..3c4e45c4b 100644 --- a/test/php/Lib/EntityManager/Pairing.php +++ b/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']; } } diff --git a/test/php/Lib/EntityManager/RecordingConnection.php b/test/php/Lib/EntityManager/RecordingConnection.php index de2e929d4..5796fe784 100644 --- a/test/php/Lib/EntityManager/RecordingConnection.php +++ b/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 $data + * + * @return array + */ + 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 * diff --git a/test/php/library/Notifications/Common/EntityManagerTest.php b/test/php/library/Notifications/Common/EntityManagerTest.php index b5fc4c2f5..2e9c6b7c5 100644 --- a/test/php/library/Notifications/Common/EntityManagerTest.php +++ b/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' ); }