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

[kenjis]

Description
It is not necessary that the update() method be able to update all records without WHERE clause. Updating all records is a risky operation and should not be mixed with update method with id(s).

The current API is not good design because it is easily to create a vulnerability that allows all records to be updated if you accidentally forget to validate the $id. See https://forum.codeigniter.com/showthread.php?tid=84833

• [BC] disallow Model::update() without WHERE clause (throws Exception).
• disallow $id as bool for update()/delete() (throws Exception).
• add Model::updateAll($data) for UPDATE without WHERE clause.
• add tests for Model::detete(). DELETE without WHERE clause is already not allowed.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[michalsn]

I would prefer to have something less invasive. updateAll() method is probably used pretty commonly, and I don't want to push developers to rename methods in their code.

Maybe something like allowEmptyWhere() method, which sets the proper flag inside the model and resets it after the query is executed?

That could serve both methods: update() and delete() - when useSoftDeletes option is enabled.

$model->set($data)->allowEmptyWhere()->update();
$model->allowEmptyWhere()->delete();

[kenjis]

I would prefer to have something less invasive. updateAll() method is probably used pretty commonly, and I don't want to push developers to rename methods in their code.

What do you mean?
Even if we add allowEmptyWhere(), developers need to add the method when they update all records.
After all they need to change.

I would like to deprecate the usage update(null, $data) if possible.
You can write updateAll($data), and it never happens that null as id is passed accidentally.

[michalsn]

What do you mean?
Even if we add allowEmptyWhere(), developers need to add the method when they update all records.
After all they need to change.

Yes, that's true. But there is a big difference, at least from my point of view.

We all have our habits. If I already have updateAll() method implemented in my code, I will get used to using it on multiple occasions in many models. The proposed change will make this method "restricted" to use, and I will have to change a code in my project (hopefully only one).

If instead of this, we will use allowEmptyWhere() for allowing updates with null (which we both think are very rare), then the amount of code that has to be changed is minimal.

Plus, as I mentioned, this also can be used with soft deletes. I don't think there is any straightforward way right now to soft delete all the records.

I would like to deprecate the usage update(null, $data) if possible.
You can write updateAll($data), and it never happens that null as id is passed accidentally.

I get your point, but again - with allowEmptyWhere(), the API could stay the same, and still, we would not have to worry about accidents.

I'm not pushing to do it my way. I got the idea after reading the code from this PR.

[MGatner]

This feels very heavy-handed. I agree this will probably prevent more mistakes than actual existing use cases, but it could really break current projects.

Alternatives: we could add it to our Features Config and encourage people to turn it on in the release notes / upgrade guides. We could trigger & log deprecations or warnings with the intent to make the hard change in a future version (4.4? 5.0?). Other ideas?

[sclubricants]

I don't quite follow what the problem is. Can you give a code example of a problematic situation?

[michalsn]

You can follow the thread on the forum: https://forum.codeigniter.com/showthread.php?tid=84833 - Kenjis gives some examples there.

[sclubricants]

I'm just looking into this problem now and still don't yet get everything that is going on here but I can say that using the new updateBatch() method would probably make more sense for model. It works in a completely different manner and would probably not have issues like this associated with it. updateBatch() performs an update based on each row of data and not based on a WHERE.

I'll look some more at this but those are my initial thoughts.

[kenjis]

@sclubricants update(null, $data) executes UPDATE without WHERE clause (if you don't set WHERE at all).
I don't think it is a good API because if a dev forgets to check the first parameter is not null, all records will be updated.

[sclubricants]

If when calling update() on model ..if model then calls $builder->onContraint(id)->updateBatch($dat) then it would throw an error if id is not present. If id is present but null then the data would match no rows and the rows with null ids wouldn't update anything.

Using model assumes there is a constraint or pk. So mapping update to updateBatch() should be feasible.

If your updating records in any other manner you really shouldn't use model. Any models without an id/pk should only be able to insert.

I need to dig in this a bit more though so I understand the problem better.

[sclubricants]

The method says it only updates a single record. Unfortunately it does not always deliver that promise.

As an alternative:

    protected function doUpdate($allOrId = false, $data = null): bool
    {
        $escape       = $this->escape;
        $this->escape = [];

        $builder = $this->builder();

        if ($allOrId !== true) {
            $builder = $builder->whereIn($this->table . '.' . $this->primaryKey, $allOrId ?? '');
        }

        // Must use the set() method to ensure to set the correct escape flag
        foreach ($data as $key => $val) {
            $builder->set($key, $val, $escape[$key] ?? null);
        }

        return $builder->update();
    }

This is still a breaking change but only a small change would be needed. Pass true instead of null to update all

[sclubricants]

I'd rather see something like this:

    public function update($data = null, $alternativeConstraint = null): bool
    {

    }

    protected function doUpdate($data = null, $alternativeConstraint = null): bool
    {
        $escape       = $this->escape;
        $this->escape = [];

        $builder = $this->builder()->setData($data, $escape)->onConstraint($alternativeConstraint ?? $this->primaryKey);

        return $builder->updateBatch();
    }

[sclubricants]

Maybe for 4.4 I can go though model and refactor some things to be more inline with builder.

[kenjis]

The method says it only updates a single record. Unfortunately it does not always deliver that promise.

Yes, the comment say so:
https://github.com/codeigniter4/CodeIgniter4/blob/4.3/system/BaseModel.php#L892-L893
But it is not true, because $id accepts an array:
https://codeigniter4.github.io/CodeIgniter4/models/model.html#update

[kenjis]

This feels very heavy-handed. I agree this will probably prevent more mistakes than actual existing use cases, but it could really break current projects.

I proposed this breaking change because I think it is a rare use case where all records are updated.
If there is actually code that accepts both specific ids and null, then the app will break.
But I can't imagine such code.

Adding Feature Config is fine, but I prefer the default is to disallow updates all.

[sclubricants]

I don't see why we need updateAll(). It seems you should use builder for thos types of operations.

For that matter make model method that returns an instance of builder with table set.

$model->builder()->update()

[kenjis]

update($id, $data) for updating a specific id or specific ids.
updateAll($data) for updating without ids.

$model->builder()->set()->update() works, but $model->set()->builder()->update() does not work.
The model is difficult to understand. When using a model, it seems better to be able to handle it with the model's methods.

However, if many people say that updateAll() is not necessary, I am fine without it.

[MGatner]

Let's make sure @lonnieezell @paulbalandan and @samsonasik have seen this and had a chance to weigh in.

[kenjis]

Rebased.

[paulbalandan]

This should likewise be changed.

[kenjis]

Changed.

[paulbalandan]

In what instances would this be a bool?

[kenjis]

When an attacker passes a bool and the dev forgets to validate the id value.

[michalsn]

• add Model::updateAll($data) for UPDATE without WHERE clause.

This also should be treated as a BC change - a new reserved method name.

[kenjis]

At least, @sclubricants and @iRedds say that updateAll() is not necessary.

@michalsn If it is a breaking change, adding any new method will be a breaking change?

[kenjis]

Rebased.

Okay, I will remove updateAll() in this PR.

In the future Id like to see us work toward the ability of composite primary keys. The new *Batch() methods will work well with composite keys.

Yes. I think it is the way to go.

[kenjis]

Removed updateAll() and added docs.

[kenjis]

I think that any DML operations in model should be performed on the basis of the primary key.

That may indeed be true.

[kenjis]

Updated the existing note in docs. 87a997d

[kenjis]

Run vendor/bin/psalm
...
ERROR: UndefinedClass - system/Config/Services.php:338:16 - Type null cannot be called as a class (see https://psalm.dev/019)
        return new $class($config);


ERROR: UndefinedClass - system/Config/Services.php:659:19 - Type null cannot be called as a class (see https://psalm.dev/019)
        $driver = new $driverName($config, AppServices::request()->getIPAddress());

https://github.com/codeigniter4/CodeIgniter4/actions/runs/3655630049/jobs/6177588634

[kenjis]

Error: Variable $config on left side of ??= always exists and is not nullable.
Error: Variable $config on left side of ??= always exists and is not nullable.
 ------ --------------------------------------------------------------- 
  Line   system/Config/Services.php                                     
 ------ --------------------------------------------------------------- 
  335    Variable $config on left side of ??= always exists and is not  
         nullable.                                                      
  643    Variable $config on left side of ??= always exists and is not  
         nullable.                                                      
 ------ --------------------------------------------------------------- 

https://github.com/codeigniter4/CodeIgniter4/actions/runs/3655902799/jobs/6177741790

[MGatner]

Ah the old static analysis standoff! 😂

Yes I'm thinking in the same direction as @sclubricants... The intent of Model is to work with identifiable entities (the concept, not the type). I know we aren't officially a repository pattern, but I think the concept stands - and anonymously updating persisted sets of data is not conceptually sound.