From 9fef0bd88f996dc95a09f93e5fdac427de088163 Mon Sep 17 00:00:00 2001 From: jekkos Date: Thu, 8 Oct 2026 22:30:16 +0200 Subject: [PATCH] fix(models): handle query() returning false in result-consuming methods (#4734) * fix(models): handle query() returning false in result-consuming methods As of CI 4.1.2, Connection::query() returns false when a query fails (previously an empty result object). Guard the model/helper methods that consume the result so a failed query degrades to an empty result instead of throwing "Call to member function getResultArray() on false". - Tax::get_taxes() - Sale::get_all_suspended() - Reports\Inventory_low::getData() - migration_helper: dropAllForeignKeyConstraints(), indexExists(), primaryKeyExists() Adds QueryFalseGuardTest forcing query() to return false. Closes #3634. * test(query-false): mock concrete driver instead of abstract BaseConnection onlyMethods() does not stub the 16 abstract methods on BaseConnection, so mocking it produced an invalid double. Mock the concrete driver class (resolved from the live test connection) instead. * fix(3634): fail loudly when migration metadata queries return false Model read methods degrade to an empty result on a failed query, but the migration metadata helpers (indexExists, primaryKeyExists, dropAllForeignKeyConstraints) must not mistake a failed query for 'not found' and proceed with schema changes. They now throw a DatabaseException so the migration aborts loudly. - Add an optional injectable BaseConnection param for testability. - Add missing docstrings (docstring-coverage check). - Add regression tests asserting the helpers throw on a failed query. * fix(3634): use supplied connection for FK drop; stub error() in tests Address CodeRabbit review: dropAllForeignKeyConstraints now builds its Forge from the supplied connection (consistent with the lookup) instead of always the default one. Test mock also stubs error() to avoid a warning on the constructor-disabled connection. * fix(3634): revert FK drop to Database::forge() (driver-specific forge) new Forge($db) creates the base Forge which does not implement dropForeignKey for the driver ('This feature is not available'), breaking migrations. Restore Database::forge(), which loads the driver-specific Forge via the factory. The optional connection param is retained solely for testing the fail-loud behavior on the lookup path. * test(3634): add regression test for Inventory_low::getData query-failure guard Covers the low-stock report's degrade-to-empty behavior on a failed query, addressing the CodeRabbit note that it lacked a focused test. * test(3634): adopt test{Method}_{Purpose} naming convention Align QueryFalseGuardTest method names with the convention from #4730 (testGetTaxes_ReturnsEmptyArrayWhenQueryFails, etc.), per review. * test: split QueryFalseGuardTest into per-component test files Apply CodeRabbit review: extract the shared failing-connection helpers into tests/Support/FailedQueryConnectionTrait.php and move each test to its component's test file (TaxTest, SaleTest, Inventory_lowTest, migration_helperTest). Use fresh model instances instead of the shared model() singleton so the injected failing connection does not leak into other tests. * style(tests): remove redundant docblocks from query-failure regression tests --------- Co-authored-by: objecttothis <17935339+objecttothis@users.noreply.github.com> --- app/Helpers/migration_helper.php | 94 ++++++++++++++++++-- app/Models/Reports/Inventory_low.php | 2 +- app/Models/Sale.php | 2 +- app/Models/Tax.php | 2 +- tests/Models/Reports/Inventory_lowTest.php | 34 +++++++ tests/Models/SaleTest.php | 12 +++ tests/Models/TaxTest.php | 27 ++++++ tests/Support/FailedQueryConnectionTrait.php | 41 +++++++++ tests/helpers/migration_helperTest.php | 53 +++++++++++ 9 files changed, 255 insertions(+), 12 deletions(-) create mode 100644 tests/Models/Reports/Inventory_lowTest.php create mode 100644 tests/Models/TaxTest.php create mode 100644 tests/Support/FailedQueryConnectionTrait.php create mode 100644 tests/helpers/migration_helperTest.php diff --git a/app/Helpers/migration_helper.php b/app/Helpers/migration_helper.php index 0e86143c6..d7da83922 100644 --- a/app/Helpers/migration_helper.php +++ b/app/Helpers/migration_helper.php @@ -1,5 +1,7 @@ query(" SELECT DISTINCT kcu.CONSTRAINT_NAME, @@ -144,6 +153,23 @@ function dropAllForeignKeyConstraints(string $table, string $column): array { AND rc.CONSTRAINT_NAME IS NOT NULL "); + if ($result === false) { + $detail = 'unknown error'; + try { + $error = $db->error(); + if (is_array($error) && !empty($error['message'])) { + $detail = (string) $error['message']; + } + } catch (\Throwable $e) { + // A disconnected or mocked connection may not support error(); keep the default. + } + + throw new DatabaseException(sprintf( + 'Failed to look up foreign key constraints referencing table "%s" column "%s": %s', + $table, $column, $detail + )); + } + $deletedConstraints = []; foreach ($result->getResultArray() as $constraint) { @@ -187,22 +213,72 @@ function deleteIndex(string $table, string $index): void { * * @param string $table * @param string $index + * @param BaseConnection|null $db Connection to use (defaults to the default connection). + * * @return bool + * + * @throws DatabaseException If the metadata query fails. */ -function indexExists(string $table, string $index): bool { - $db = Database::connect(); +function indexExists(string $table, string $index, ?BaseConnection $db = null): bool { + $db ??= Database::connect(); $result = $db->query('SELECT COUNT(*) FROM information_schema.statistics WHERE table_schema = DATABASE() AND table_name = \'' . $db->getPrefix() . "$table' AND index_name = '$index'"); + + if ($result === false) { + $detail = 'unknown error'; + try { + $error = $db->error(); + if (is_array($error) && !empty($error['message'])) { + $detail = (string) $error['message']; + } + } catch (\Throwable $e) { + // A disconnected or mocked connection may not support error(); keep the default. + } + + throw new DatabaseException(sprintf( + 'Failed to check whether index "%s" exists on table "%s": %s', + $index, $table, $detail + )); + } + $row_array = $result->getRowArray(); - return $row_array && $row_array['COUNT(*)'] > 0; + return $row_array !== null && $row_array['COUNT(*)'] > 0; } -function primaryKeyExists(string $table): bool { - $db = Database::connect(); +/** + * Checks if the specified table has a primary key. + * + * @param string $table + * @param BaseConnection|null $db Connection to use (defaults to the default connection). + * + * @return bool + * + * @throws DatabaseException If the metadata query fails. + */ +function primaryKeyExists(string $table, ?BaseConnection $db = null): bool { + $db ??= Database::connect(); $result = $db->query('SELECT COUNT(*) FROM information_schema.table_constraints WHERE table_schema = DATABASE() AND table_name = \'' . $db->getPrefix() . "$table' AND constraint_type = 'PRIMARY KEY'"); + + if ($result === false) { + $detail = 'unknown error'; + try { + $error = $db->error(); + if (is_array($error) && !empty($error['message'])) { + $detail = (string) $error['message']; + } + } catch (\Throwable $e) { + // A disconnected or mocked connection may not support error(); keep the default. + } + + throw new DatabaseException(sprintf( + 'Failed to check whether table "%s" has a primary key: %s', + $table, $detail + )); + } + $row_array = $result->getRowArray(); - return $row_array && $row_array['COUNT(*)'] > 0; + return $row_array !== null && $row_array['COUNT(*)'] > 0; } function recreateForeignKeyConstraints(array $constraints): void { diff --git a/app/Models/Reports/Inventory_low.php b/app/Models/Reports/Inventory_low.php index 65f7a1b14..7dfb16d8a 100644 --- a/app/Models/Reports/Inventory_low.php +++ b/app/Models/Reports/Inventory_low.php @@ -47,7 +47,7 @@ class Inventory_low extends Report AND stock_locations.deleted = 0 ORDER BY items.name"); - return $query->getResultArray() ?: []; + return $query ? $query->getResultArray() : []; } /** diff --git a/app/Models/Sale.php b/app/Models/Sale.php index e1a0da097..45f84ab49 100644 --- a/app/Models/Sale.php +++ b/app/Models/Sale.php @@ -1193,7 +1193,7 @@ class Sale extends Model . $this->db->prefixTable('sales') . ' where sale_status = ' . SUSPENDED . ' AND customer_id = ' . $customer_id); } - return $query->getResultArray() ?: []; + return $query ? $query->getResultArray() : []; } /** diff --git a/app/Models/Tax.php b/app/Models/Tax.php index 61e6e9361..dfa7783e3 100644 --- a/app/Models/Tax.php +++ b/app/Models/Tax.php @@ -127,7 +127,7 @@ class Tax extends Model $query = $this->db->query($sql); - return $query->getResultArray() ?: []; + return $query ? $query->getResultArray() : []; } /** diff --git a/tests/Models/Reports/Inventory_lowTest.php b/tests/Models/Reports/Inventory_lowTest.php new file mode 100644 index 000000000..92840294f --- /dev/null +++ b/tests/Models/Reports/Inventory_lowTest.php @@ -0,0 +1,34 @@ +modelWithFailingQuery(Inventory_low::class); + + $this->assertSame([], $report->getData([])); + } +} diff --git a/tests/Models/SaleTest.php b/tests/Models/SaleTest.php index aff3b79c5..e6372d754 100644 --- a/tests/Models/SaleTest.php +++ b/tests/Models/SaleTest.php @@ -7,6 +7,7 @@ use CodeIgniter\Database\Config; use CodeIgniter\Test\CIUnitTestCase; use CodeIgniter\Test\DatabaseTestTrait; use Tests\Support\EmployeeFixtureTrait; +use Tests\Support\FailedQueryConnectionTrait; use Tests\Support\ItemFixtureTrait; /** @@ -18,6 +19,7 @@ class SaleTest extends CIUnitTestCase { use DatabaseTestTrait; use EmployeeFixtureTrait; + use FailedQueryConnectionTrait; use ItemFixtureTrait; protected $migrate = true; @@ -434,4 +436,14 @@ class SaleTest extends CIUnitTestCase $this->assertTrue($deleteResult); $this->assertEqualsWithDelta(10.0, $this->getItemQuantity($itemId, self::LOCATION_ID), 0.001); } + + public function testGetAllSuspended_ReturnsEmptyArrayWhenQueryFails(): void + { + $model = $this->modelWithFailingQuery(Sale::class); + + // Exercises both the NEW_ENTRY branch and the customer-id branch. + $this->assertSame([], $model->get_all_suspended()); + $this->assertSame([], $model->get_all_suspended(NEW_ENTRY)); + $this->assertSame([], $model->get_all_suspended(123)); + } } diff --git a/tests/Models/TaxTest.php b/tests/Models/TaxTest.php new file mode 100644 index 000000000..9075cf70c --- /dev/null +++ b/tests/Models/TaxTest.php @@ -0,0 +1,27 @@ +modelWithFailingQuery(Tax::class); + + $this->assertSame([], $model->get_taxes(1, 1)); + } +} diff --git a/tests/Support/FailedQueryConnectionTrait.php b/tests/Support/FailedQueryConnectionTrait.php new file mode 100644 index 000000000..6906e703e --- /dev/null +++ b/tests/Support/FailedQueryConnectionTrait.php @@ -0,0 +1,41 @@ +DBGroup)); + + $db = $this->getMockBuilder($driverClass) + ->disableOriginalConstructor() + ->onlyMethods(['query', 'error']) + ->getMock(); + $db->method('query')->willReturn(false); + $db->method('error')->willReturn(['code' => 1, 'message' => 'simulated query failure']); + + return $db; + } + + /** + * @template T of object + * + * @param class-string $class + * + * @return T + */ + protected function modelWithFailingQuery(string $class) + { + $model = new $class(); + + $prop = new \ReflectionProperty(BaseModel::class, 'db'); + $prop->setAccessible(true); + $prop->setValue($model, $this->failingConnection()); + + return $model; + } +} diff --git a/tests/helpers/migration_helperTest.php b/tests/helpers/migration_helperTest.php new file mode 100644 index 000000000..5fe2e5555 --- /dev/null +++ b/tests/helpers/migration_helperTest.php @@ -0,0 +1,53 @@ +requireMigrationHelper(); + $this->expectException(DatabaseException::class); + + indexExists('some_table', 'some_index', $this->failingConnection()); + } + + public function testPrimaryKeyExists_ThrowsWhenQueryFails(): void + { + $this->requireMigrationHelper(); + $this->expectException(DatabaseException::class); + + primaryKeyExists('some_table', $this->failingConnection()); + } + + public function testDropAllForeignKeyConstraints_ThrowsWhenQueryFails(): void + { + $this->requireMigrationHelper(); + $this->expectException(DatabaseException::class); + + dropAllForeignKeyConstraints('some_table', 'some_column', $this->failingConnection()); + } + + /** + * Ensure the migration helper functions are loaded (they are global). + */ + private function requireMigrationHelper(): void + { + if (! function_exists('indexExists')) { + require APPPATH . 'Helpers/migration_helper.php'; + } + } +}