Bug: getCompiledUpdate() & getCompiledInsert() throws DatabaseException

[demirkaric]

PHP Version

7.4

CodeIgniter4 Version

4.1.5

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

apache

Database

MySQL 8.0.27-0ubuntu0.20.04.1

What happened?

Calling methods getCompiledInsert() & getCompiledUpdate() while using MySql adapter does not work.

Steps to Reproduce

The below is the code that tries to retreive compiled update and compiled insert, in both cases it produces the same error:
[CodeIgniter\Database\Exceptions\DatabaseException 8]
You must use the "set" method to update an entry.

$model = new Model();
$compiledUpdate =  $model->set("test", "1")->where("id", 1)->getCompiledUpdate();
$compiledInsert =  $model->set("test", "1")->getCompiledInsert();

Expected Output

In case of getCompiledUpdate() the expected string is:
UPDATE testTable SET id= 1 WHERE id = 1;

In case of getCompiledInsert() the expected string is:
INSERT INTO testTable (test) VALUES (1);

Anything else?

No response

[kenjis]

I don't know this is a bug or not.

You call set() in the CodeIgniter\Model, so you don't call set() in the Query Builder.
So you got the error You must use the "set" method to update an entry.

[demirkaric]

For example In the model I can normally do getCompiledSelect() and it works perfectly. Update and Insert throws error,

[kenjis]

I see. It is no wonder somebody thinks it is a bug.

Workaroud:

        $model = new Model();
        $builder = $model->builder();
        $compiledUpdate =  $builder->set("test", "1")->where("id", 1)->getCompiledUpdate();
        $compiledInsert =  $builder->set("test", "1")->getCompiledInsert();

[demirkaric]

Check this code:

        $test = new Builder("test", $this->db);
        $aa = $test->set("test", 1)->where("test2", 2)->getCompiledUpdate();
        try {
            $model = new Model();
            $model->setAllowedFields(["test", "test2"]);
            $bb = $model->set("test", 1)->where("test2", 2)->getCompiledUpdate();
        } catch (DatabaseException $exception) {
            $bb = $exception->getMessage();
        }

        dd($aa, $bb);

The method should have the same output
Result:

│ $aa
string (46) "UPDATE test SET test = 1
WHERE test2 = 2"

│ $bb string (49) "You must use the "set" method to update an entry."

[iRedds]

When using the Model::set() method, the model temporarily stores data for validation and filter available fields. This data is passed to the builder only after checks and before the request (update / insert).

The getCompiledUpdate() and getCompiledInsert() methods are called directly on the builder instance.
This causes the temporary data to hang in the model.

[iRedds]

It's not really a bug, it's a design flaw.

[demirkaric]

So how can I report design flaw 😄

[kenjis]

@iRedds I don't know the difference, but I think this should be fixed.
It is unreasonable to call this a specification.

[paulbalandan]

This is why we should never mixin a class's functionalities (BaseBuilder) with another class (Model). Too much complications down the road.

Although, Model's set has the same function signature as BaseBuilder's set, they don't do the same thing as @iRedds pointed out.

CodeIgniter4/system/Model.php

Lines 566 to 577 in 3570214

	public function set($key, $value = '', ?bool $escape = null)
	{
	$data = is_array($key) ? $key : [$key => $value];
	foreach (array_keys($data) as $k) {
	$this->tempData['escape'][$k] = $escape;
	}
	$this->tempData['data'] = array_merge($this->tempData['data'] ?? [], $data);
	return $this;
	}

CodeIgniter4/system/Database/BaseBuilder.php

Lines 1374 to 1395 in 3570214

	public function set($key, $value = '', ?bool $escape = null)
	{
	$key = $this->objectToArray($key);
	if (! is_array($key)) {
	$key = [$key => $value];
	}
	$escape = is_bool($escape) ? $escape : $this->db->protectIdentifiers;
	foreach ($key as $k => $v) {
	if ($escape) {
	$bind = $this->setBind($k, $v, $escape);
	$this->QBSet[$this->db->protectIdentifiers($k, false)] = ":{$bind}:";
	} else {
	$this->QBSet[$this->db->protectIdentifiers($k, false)] = $v;
	}
	}
	return $this;
	}

Since Model calls on the Builder's methods through the magic __call method and set is defined by the Model, calling Model::set won't call Builder::set.

[kenjis]

I sent a PR #5561.

[lonnieezell]

Is this maybe a better job to be fixed by documentation?

The model provides convenience features and additional functionality that people commonly use to make working with a single table more convenient. It's modeled after all of the MY_Model classes that people would build for CI3. As such, it does not provide a perfect interface to the Query Builder. And I don't think it needs to as they are separate classes with different purposes. I don't think it unreasonable to just provide guidance in the user guide saying you need to access the builder directly for this, though if the data hasn't been set, because it hasn't been validated yet, then they would need to do that first.

[kenjis]

@lonnieezell How do we validate the data?
And if this is not a bug, could you show us the correct code?

$compiledUpdate =  $model->set("test", "1")->where("id", 1)->getCompiledUpdate();

[lonnieezell]

What I'm saying is they should not be expected to return the same data. They are separate classes with different purposes. I get that there is confusion since you can access the builder methods in the calling chain, and perhaps I was wrong to include that convenience feature in to begin with, since it keeps being met with confusion.

If you need to get the compiledInsert or Update you should do so directly on the builder instance. Through normal model usage I don't think this is something that is an issue. I don't know the usage of the OP, but I can only see it being used if someone is wanting to either compile their own queries from fragments they use these methods with, or get a whole query string to use it as a subquery. In either of those cases, I think it's not unreasonable to expect to have to use a new builder instance to compile the queries.

Maybe I'm completely missing a use case here? If I am I don't know of a good, non-hacky, fix if we want to keep validation in the model, which I'm assuming we do. The hacky fix would be to handle it within the __call() method by checking the method being called and, if it's one of those two (or any others that might require data set first), to bypass validation(?) and set the data on the builder at that point.

[kenjis]

@lonnieezell

What I'm saying is they should not be expected to return the same data. They are separate classes with different purposes.

If you need to get the compiledInsert or Update you should do so directly on the builder instance.

This is very difficult to understand. Because they are mixed.
And the documentation does not say about it well.

I think the mix-in is not good design, but it is there and used by many users.
If this is not a bug, I think we need to be clear it is not a bug.

How about #5562 ?

[sfadschm]

I feel #5562 is a good solution. Can exceptions contain links to the docs? That might be helpful, too.
I also feel that the docs should clarify more specific which builder functions can be used in the model as a mixin and which should not.

I agree with Lonnie for the rest. The intention was godd but it is now causing confusion so this should be fixed by documentation.

[demirkaric]

In a such case #5562 getCompiledSelect() should be limited on the model since it is an instance of QueryBuilder class. It is a bit strange to have some methods working and some don't.

[kenjis]

getCompiledSelect() is related to soft deletion.
In this moment getCompiledSelect() can be used with no errors, but it does not take care of soft deletion.

Soft deletion is functionality in the Model. Shouldn't the Model provide a way to get compiled SQL?

It is a bit strange to have some methods working and some don't.

Yes.

[kenjis]

@lonnieezell I thought about this Model/QB issue. But I can't tell the correct usage of the Model.

Is this the correct usage of the Model?

$this->model->where('name', 'Senior Developer')->get()->getFirstRow();

[lonnieezell]

@lonnieezell I thought about this Model/QB issue. But I can't tell the correct usage of the Model.

Is this the correct usage of the Model?

$this->model->where('name', 'Senior Developer')->get()->getFirstRow();

Here's how I think about it: use the Model methods where available, but the QB methods are there to supplement. The primary reason it got mixed it at all is so you didn't have to write custom methods in the model whenever you wanted to filter something, not just get all results. Without mixing the QB in you would only have the most basic CRUD methods, pagination, and validation. That's much better than CI3, but still requires lots of custom methods when you want only the students that went to a certain school. With it mixed in, you can now do where() and orderBy() and limit() without making a custom method, if you don't want to.

For your example I'd do it like:

$model->where('name', 'Senior Developer')->first();

[lonnieezell]

And yes, I like #5562, and approved it with one tiny comment.

I do feel like a bit of additions to the user guide would be helpful around this.

[kenjis]

I'm not sure this is enough, but tried to improve the documentation: #5694