From 163039c32853c3462282a8e6a3c097f6b5e6924e Mon Sep 17 00:00:00 2001 From: rrigby <16025441+rrigby@users.noreply.github.com> Date: Sat, 15 Nov 2025 09:12:06 +1000 Subject: [PATCH 1/2] Throw exceptions instead of returning false. This fixes the previous behaviour where most issues resulted in an SQLError exception, however a handful of things were handled and simply returned false, which wasn't clear when using the library and lead to bugs. --- src/Mapping.php | 62 ++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/src/Mapping.php b/src/Mapping.php index dcadd27..727664a 100644 --- a/src/Mapping.php +++ b/src/Mapping.php @@ -92,7 +92,7 @@ public function count(string $column = '*') if ($this->definition->getDeletionTimestamp()) { $this->isNull($this->prefixTableNameTo($this->definition->getDeletionTimestamp())); } - + return parent::count($column); } @@ -124,10 +124,7 @@ public function insert(array $data) unset($base[$this->definition->getPrimaryKey()[0]]); } - if (!parent::insert($base)) { - // Transaction already cancelled by the statement handler - return false; - } + parent::insert($base); if ($this->definition->isAutoIncrement()) { $this->lastId = $this->db->getLastId(); @@ -154,10 +151,7 @@ public function insert(array $data) foreach ($items as $item) { $item[$property->getForeignColumn()] = $data[$property->getLocalColumn()]; - if (!$mapping->insert($item)) { - // Transaction already cancelled by the statement handler - return false; - } + $mapping->insert($item); } } @@ -177,6 +171,7 @@ public function insert(array $data) * * @param array $data * @return boolean + * @throws \Exception */ public function update(array $data = array()) { @@ -188,7 +183,7 @@ public function update(array $data = array()) foreach ($primaryKey as $column) { if (!array_key_exists($column, $data)) { - return false; + throw new \Exception('Failed to update record. Missing primary key column: ' . $column); } if (is_null($data[$column])) { @@ -199,7 +194,7 @@ public function update(array $data = array()) } if (!$original = $this->findOne()) { - return false; + throw new \Exception('Failed to update record. Original not found.'); } $useTransaction = !$this->db->getConnection()->inTransaction(); @@ -222,12 +217,12 @@ public function update(array $data = array()) ]); return true; - } catch (\Exception $e) { + } catch (\Exception $exception) { if ($useTransaction) { $this->db->cancelTransaction(); } - return false; + throw $exception; } } @@ -236,6 +231,7 @@ public function update(array $data = array()) * * @param array $data * @return bool + * @throws \Exception */ public function save(array $data) { @@ -243,7 +239,7 @@ public function save(array $data) foreach ($primaryKey as $column) { if (!array_key_exists($column, $data)) { - return false; + throw new \Exception('Failed to save record. Missing primary key column: ' . $column); } if (is_null($data[$column])) { @@ -261,6 +257,7 @@ public function save(array $data) * Removes data matching the condition. * * @return bool + * @throws \Exception */ public function remove() { @@ -271,11 +268,18 @@ public function remove() $ids = $this->collectPrimary($original, $ids); } - try { + $useTransaction = !$this->db->getConnection()->inTransaction(); + + if ($useTransaction) { $this->db->startTransaction(); + } + + try { $this->delete($ids); - $this->db->closeTransaction(); + if ($useTransaction) { + $this->db->closeTransaction(); + } foreach ($data as $item) { $this->dispatch('removed', $item); @@ -283,8 +287,11 @@ public function remove() return true; } catch (\Exception $exception) { - $this->db->cancelTransaction(); - return false; + if ($useTransaction) { + $this->db->cancelTransaction(); + } + + throw $exception; } } @@ -331,9 +338,7 @@ private function replace(array $data, array $original, array $deleteIds = []) $this->definition->getModificationData() ); - if (!$query->update($base)) { - throw new \Exception('Failed to update record.'); - } + $query->update($base); } foreach ($this->definition->getProperties() as $property) { @@ -361,9 +366,7 @@ private function replace(array $data, array $original, array $deleteIds = []) $item[$property->getForeignColumn()] = $data[$property->getLocalColumn()]; } - if (!$mapping->insert($item)) { - throw new \Exception('Failed to insert record.'); - } + $mapping->insert($item); } foreach ($delete as $item) { @@ -380,7 +383,7 @@ private function replace(array $data, array $original, array $deleteIds = []) continue; } - $originalItem = Collection::first($propertyOriginal, function($original) use ($item, $propertyPrimary) { + $originalItem = Collection::first($propertyOriginal, function ($original) use ($item, $propertyPrimary) { foreach ($propertyPrimary as $column) { if ($original[$column] != $item[$column]) { return false; @@ -409,7 +412,7 @@ private function delete($ids = []) foreach ($ids as $table => $deleteColumns) { foreach ($deleteColumns as $deletion => $deletionContext) { // Arrange values into groups based on all but the last key - $grouped = Collection::group($deletionContext['keys'], function(array $keys) { + $grouped = Collection::group($deletionContext['keys'], function (array $keys) { array_pop($keys); return implode(':', $keys); }); @@ -549,7 +552,7 @@ private function map(array $data) $results = $mapping ->findAll(); - $properties[$property->getName()] = Collection::group($results, function($result) use ($groupColumn) { + $properties[$property->getName()] = Collection::group($results, function ($result) use ($groupColumn) { return $result[$groupColumn]; }); } @@ -657,7 +660,7 @@ private function dispatch(string $event, array $data, $args = []) private function requiresPrefix(string $column): bool { $pattern = '/^[a-zA-Z_][a-zA-Z0-9_]*$/'; - return (bool) preg_match($pattern, $column); + return (bool)preg_match($pattern, $column); } /** @@ -674,7 +677,8 @@ private function requiresPrefix(string $column): bool * * @return string | array */ - private function prefixTableNameTo($input) { + private function prefixTableNameTo($input) + { $table = $this->definition->getTable(); if (is_string($input)) { From a6c3dc6be18aaa429a6243f532170f2b4971a56e Mon Sep 17 00:00:00 2001 From: rrigby <16025441+rrigby@users.noreply.github.com> Date: Sat, 15 Nov 2025 09:14:52 +1000 Subject: [PATCH 2/2] Add tests confirming change and some existing quirks --- tests/MappingTest.php | 134 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 3 deletions(-) diff --git a/tests/MappingTest.php b/tests/MappingTest.php index 3c54bbb..8100736 100644 --- a/tests/MappingTest.php +++ b/tests/MappingTest.php @@ -192,7 +192,7 @@ public function testRemove() ); } - public function testReadOnlyInsert() + public function testReadOnlyInsert() { $customer = [ 'id' => 10, @@ -288,22 +288,150 @@ public function testInsertRollback() } catch (SQLException $exception) { } + $this->assertFalse($this->db->getConnection()->inTransaction()); + $inserted = $this->getMapping()->eq('id', 3)->findOne(); $this->assertNull($inserted); } + public function testInsertDuplicateThrowsException() + { + $this->expectException(SQLException::class); + $this->expectExceptionMessage('UNIQUE constraint failed'); + + $customer = [ + 'id' => 1, + 'name' => 'Dave Matthews', + 'orders' => [] + ]; + + $this->getMapping()->insert($customer); + } + + public function testInsertDuplicateChildrenThrowsException() + { + $this->expectException(SQLException::class); + $this->expectExceptionMessage('UNIQUE constraint failed'); + + $customer = [ + 'id' => 3, + 'name' => 'Dave Matthews', + 'orders' => [ + [ + 'id' => 4, + 'date_created' => '2018-02-01', + 'discount' => [ + 'description' => 'Dessert Discount', + 'amount' => 20 + ], + 'items' => [ + [ + 'id' => 7, + 'description' => 'Ice Cream', + 'amount' => 400 + ], + [ + 'id' => 7, + 'description' => 'Cookies', + 'amount' => '230' + ] + ] + ] + ] + ]; + + $this->getMapping()->insert($customer); + } + public function testUpdateRollback() { $customer = $this->getMapping()->eq('id', 1)->findOne(); $original = $customer; - $customer['orders'][0]['items'][] = ['id' => '3']; - $this->getMapping()->update($customer); + $customer['orders'][0]['items'] = array_merge( + $customer['orders'][0]['items'], + [ + [ + 'id' => 7, + 'description' => 'Ice Cream', + 'amount' => 400 + ], + [ + 'id' => 7, + 'description' => 'Cookies', + 'amount' => '230' + ] + ] + ); + + try { + $this->getMapping()->update($customer); + } catch (SQLException $exception) { + } $updated = $this->getMapping()->eq('id', 1)->findOne(); $this->assertEquals($original, $updated); } + public function testUpdateThrowsExceptionWhenInsertingDuplicateChildrenRecords() + { + $this->expectException(SQLException::class); + $this->expectExceptionMessage('UNIQUE constraint failed'); + + $customer = $this->getMapping()->eq('id', 1)->findOne(); + + $customer['orders'][0]['items'] = array_merge( + $customer['orders'][0]['items'], + [ + [ + 'id' => 7, + 'description' => 'Ice Cream', + 'amount' => 400 + ], + [ + 'id' => 7, + 'description' => 'Cookies', + 'amount' => '230' + ] + ] + ); + + $this->getMapping()->update($customer); + } + + public function testUpdateDoesNotErrorWhenUpdatingDuplicateChildrenRecords() + { + $customer = $this->getMapping()->eq('id', 1)->findOne(); + $original = $customer; + + // You can provide the same item multiple times to update. + $customer['orders'][0]['items'] = array_merge( + $customer['orders'][0]['items'], + [ + [ + 'id' => 3, + 'description' => 'Ice Cream', + 'amount' => 400 + ], + [ + 'id' => 3, + 'description' => 'Chocolate Bar', + 'amount' => 600 + ] + ] + ); + + $this->getMapping()->update($customer); + + $updated = $this->getMapping()->eq('id', 1)->findOne(); + $this->assertNotEquals($original, $updated); + + // The last array entry for that item will be the one reflected in the database. + $this->assertEquals($updated['orders'][0]['items'][2]['id'], 3); + $this->assertEquals($updated['orders'][0]['items'][2]['description'], 'Chocolate Bar'); + $this->assertEquals($updated['orders'][0]['items'][2]['amount'], 600); + } + public function testPrefixAndRemoveTableName() { $method = new \ReflectionMethod('PicoMapper\Mapping', 'prefixTableNameTo');