Merge pull request #6883 from kenjis/fix-Model-update-null

feat: disallow `Model::update()` without WHERE clause

Author: kenjis

system/BaseModel.php

@@ -899,6 +899,10 @@ public function insertBatch(?array $set = null, ?bool $escape = null, int $batch
      */
     public function update($id = null, $data = null): bool
     {
+        if (is_bool($id)) {
+            throw new InvalidArgumentException('update(): argument #1 ($id) should not be boolean.');
+        }
+
         if (is_numeric($id) || is_string($id)) {
             $id = [$id];
         }
@@ -910,12 +914,12 @@ public function update($id = null, $data = null): bool
             return false;
         }
 
-        // Must be called first so we don't
+        // Must be called first, so we don't
         // strip out updated_at values.
         $data = $this->doProtectFields($data);
 
         // doProtectFields() can further remove elements from
-        // $data so we need to check for empty dataset again
+        // $data, so we need to check for empty dataset again
         if (empty($data)) {
             throw DataException::forEmptyDataset('update');
         }
@@ -1037,6 +1041,10 @@ public function updateBatch(?array $set = null, ?string $index = null, int $batc
      */
     public function delete($id = null, bool $purge = false)
     {
+        if (is_bool($id)) {
+            throw new InvalidArgumentException('delete(): argument #1 ($id) should not be boolean.');
+        }
+
         if ($id && (is_numeric($id) || is_string($id))) {
             $id = [$id];
         }

system/Config/Services.php

@@ -332,6 +332,8 @@ public static function image(?string $handler = null, ?Images $config = null, bo
         }
 
         $config ??= config('Images');
+        assert($config instanceof Images);
+
         $handler = $handler ?: $config->defaultHandler;
         $class   = $config->handlers[$handler];
 
@@ -639,6 +641,8 @@ public static function session(?App $config = null, bool $getShared = true)
         }
 
         $config ??= config('App');
+        assert($config instanceof App);
+
         $logger = AppServices::logger();
 
         $driverName = $config->sessionDriver;

system/Model.php

@@ -386,6 +386,12 @@ protected function doUpdate($id = null, $data = null): bool
             $builder->set($key, $val, $escape[$key] ?? null);
         }
 
+        if ($builder->getCompiledQBWhere() === []) {
+            throw new DatabaseException(
+                'Updates are not allowed unless they contain a "where" or "like" clause.'
+            );
+        }
+
         return $builder->update();
     }
 
@@ -589,7 +595,7 @@ public function countAllResults(bool $reset = true, bool $test = false)
         }
 
         // When $reset === false, the $tempUseSoftDeletes will be
-        // dependant on $useSoftDeletes value because we don't
+        // dependent on $useSoftDeletes value because we don't
         // want to add the same "where" condition for the second time
         $this->tempUseSoftDeletes = $reset
             ? $this->useSoftDeletes

tests/system/Models/DeleteModelTest.php

@@ -239,4 +239,42 @@ public function testPurgeDeletedWithSoftDeleteFalse(): void
         $jobs = $this->model->findAll();
         $this->assertCount(4, $jobs);
     }
+
+    /**
+     * @dataProvider emptyPkValues
+     *
+     * @param int|string|null $id
+     */
+    public function testDeleteThrowDatabaseExceptionWithoutWhereClause($id): void
+    {
+        // BaseBuilder throws Exception.
+        $this->expectException(DatabaseException::class);
+        $this->expectExceptionMessage(
+            'Deletes are not allowed unless they contain a "where" or "like" clause.'
+        );
+
+        // $useSoftDeletes = false
+        $this->createModel(JobModel::class);
+
+        $this->model->delete($id);
+    }
+
+    /**
+     * @dataProvider emptyPkValues
+     *
+     * @param int|string|null $id
+     */
+    public function testDeleteWithSoftDeleteThrowDatabaseExceptionWithoutWhereClause($id): void
+    {
+        // Model throws Exception.
+        $this->expectException(DatabaseException::class);
+        $this->expectExceptionMessage(
+            'Deletes are not allowed unless they contain a "where" or "like" clause.'
+        );
+
+        // $useSoftDeletes = true
+        $this->createModel(UserModel::class);
+
+        $this->model->delete($id);
+    }
 }

tests/system/Models/UpdateModelTest.php

@@ -11,8 +11,11 @@
 
 namespace CodeIgniter\Models;
 
+use CodeIgniter\Database\Exceptions\DatabaseException;
 use CodeIgniter\Database\Exceptions\DataException;
 use CodeIgniter\Entity\Entity;
+use Generator;
+use InvalidArgumentException;
 use stdClass;
 use Tests\Support\Models\EventModel;
 use Tests\Support\Models\JobModel;
@@ -378,4 +381,36 @@ public function testUpdateWithSetAndEscape(): void
             'email'   => '1+1',
         ]);
     }
+
+    /**
+     * @dataProvider provideInvalidIds
+     *
+     * @param false|null $id
+     */
+    public function testUpdateThrowDatabaseExceptionWithoutWhereClause($id, string $exception, string $exceptionMessage): void
+    {
+        $this->expectException($exception);
+        $this->expectExceptionMessage($exceptionMessage);
+
+        // $useSoftDeletes = false
+        $this->createModel(JobModel::class);
+
+        $this->model->update($id, ['name' => 'Foo Bar']);
+    }
+
+    public function provideInvalidIds(): Generator
+    {
+        yield from [
+            [
+                null,
+                DatabaseException::class,
+                'Updates are not allowed unless they contain a "where" or "like" clause.',
+            ],
+            [
+                false,
+                InvalidArgumentException::class,
+                'update(): argument #1 ($id) should not be boolean.',
+            ],
+        ];
+    }
 }

user_guide_src/source/changelogs/v4.3.0.rst

@@ -81,6 +81,8 @@ Others
 - **Database:** ``InvalidArgumentException`` that is a kind of ``LogicException`` in ``BaseBuilder::_whereIn()`` is not suppressed by the configuration. Previously if ``CI_DEBUG`` was false, the exception was suppressed.
 - **Database:** The data structure returned by :ref:`BaseConnection::getForeignKeyData() <metadata-getforeignkeydata>` has been changed.
 - **Database:** ``CodeIgniter\Database\BasePreparedQuery`` class returns now a bool value for write-type queries instead of the ``Result`` class object.
+- **Model:** ``Model::update()`` method now raises a ``DatabaseException`` if it generates an SQL
+  statement without a WHERE clause; Model does not support operations that update all records.
 - **Routing:** ``RouteCollection::resetRoutes()`` resets Auto-Discovery of Routes. Previously once discovered, RouteCollection never discover Routes files again even if ``RouteCollection::resetRoutes()`` is called.
 
 .. _v430-interface-changes:

user_guide_src/source/installation/upgrade_430.rst

@@ -210,6 +210,8 @@ Database
 - The second parameter ``$index`` of ``BaseBuilder::updateBatch()`` has changed to ``$constraints``. It now accepts types array, string, or ``RawSql``. Extending classes should likewise change types.
 - The ``$set`` parameter of ``BaseBuilder::insertBatch()`` and ``BaseBuilder::updateBatch()`` now accepts an object of a single row of data. Extending classes should likewise change the type.
 - The third parameter ``$index`` of ``BaseBuilder::_updateBatch()`` has changed to ``$values``, and the parameter type has changed to ``array``. Extending classes should likewise change the type.
+- The ``Model::update()`` method now raises a ``DatabaseException`` if it generates an SQL
+  statement without a WHERE clause. If you need to update all records in a table, use Query Builder instead. E.g., ``$model->builder()->update($data)``.
 
 Others
 ======

user_guide_src/source/models/model.rst

@@ -353,7 +353,11 @@ of the columns in a ``$table``, while the array's values are the values to save
 
 .. literalinclude:: model/016.php
 
-.. important:: If the ``$primaryKey`` field is set to ``null`` then the update will affect all records in the table.
+.. important:: Since v4.3.0, this method raises a ``DatabaseException``
+    if it generates an SQL statement without a WHERE clause.
+    In previous versions, if it is called without ``$primaryKey`` specified and
+    an SQL statement was generated without a WHERE clause, the query would still
+    execute and all records in the table would be updated.
 
 Multiple records may be updated with a single call by passing an array of primary keys as the first parameter: