CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

[Model] Upsert problem with data validation, allowed fields, dates defined

Open maniaba opened this issue 2 years ago • 6 comments

PHP Version

7.4

CodeIgniter4 Version

4.3.2

CodeIgniter4 Installation Method

Composer (as dependency to an existing project)

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

apache

Database

MySQL (ver. 8.0.28)

What happened?

Upsert does not work correctly with the model. Allowed fields, validation rules, dates (created_at, updated_at) are defined within the model, this does not apply to the query. The model should throw validation errors first!

Steps to Reproduce


class TestModel extends BaseModel
{
    protected $table         = 'table';
    protected $allowedFields = [
        'item_id', 'item_quantity'
    ];
    protected $validationRules = [
        'item_id'              => 'required|is_natural_no_zero',
        'item_quantity'    => 'required|numeric',
    ];

    protected $useSoftDeletes   = true;
    protected $protectFields      = true;
    protected $useTimestamps  = true;
}

$model = new TestModel();

$model->upsert([
    'item_id' => "string data", //validation is not correct
    'item_quantity' => 99, //everything ok
    'col1' => "string data", //not in allowed fields
]);

//check validation errors
$model->errors();

$model->db->getLastQuery()->getQuery();

Expected Output

INSERT INTO `table` (`item_quantity`, `created_at`, `updated_at`)
VALUES (99, unix_timestamp, unix_timestamp)
ON DUPLICATE KEY UPDATE
`table`.`item_quantity` = VALUES(`item_quantity`),
`table`.`updated_at` = VALUES(`updated_at`)

Anything else?

No response

maniaba avatar Feb 20 '23 17:02 maniaba

What are the current outputs?

$model->errors();
$model->db->getLastQuery()->getQuery();

kenjis avatar Feb 20 '23 23:02 kenjis

This is not a bug.

Important The Model does not provide a perfect interface to the Query Builder. The Model and the Query Builder are separate classes with different purposes. They should not be expected to return the same data.

If the Query Builder returns a result, it is returned as is. In that case, the result may be different from the one returned by the model’s method and may not be what was expected. The model’s events are not triggered.

To prevent unexpected behavior, do not use Query Builder methods that return results and specify the model’s method at the end of the method chaining. https://codeigniter4.github.io/CodeIgniter4/models/model.html#mixing-methods-of-query-builder-and-model

The model does not have the upsert() method. So it is a Query Builder method.

kenjis avatar Feb 20 '23 23:02 kenjis

By the way, why do you need to use upsert() in the Model?

kenjis avatar Feb 20 '23 23:02 kenjis

By the way, why do you need to use upsert() in the Model?

When we need to save something, we use insert(), update() or save() (model methods). When we want to use insert() or update() (upsert()), we don't have that method in the model. I understand that query builder and model are different classes and give different queries (example soft delete will not add in the query builder, but model will add, which is fine). I think it is necessary to have an upsert() in the model. Otherwise, what is the purpose of having it only in the query builder?

maniaba avatar Feb 21 '23 07:02 maniaba

What are the current outputs?

$model->errors();
$model->db->getLastQuery()->getQuery();

Output for the errors() method returns null, should be a validation error for the item_id field

$model->errors(); // returns `null`, should be a validation error for the item_id field

Output for the method getQuery() , col1 is not in allowed files and I get a database error for an unknown column, and item_id is string, must be natural no zero.

$model->db->getLastQuery()->getQuery();
INSERT INTO `table` (`col1`, `item_id`, `item_quantity`)
VALUES ('string data','string data',99)
ON DUPLICATE KEY UPDATE
`table`.`col1` = VALUES(`col1`),
`table`.`item_id` = VALUES(`item_id`),
`table`.`item_quantity` = VALUES(`item_quantity`)

maniaba avatar Feb 21 '23 07:02 maniaba

Otherwise, what is the purpose of having it only in the query builder?

No, no. It was just added in Query Builder. If you want to UPSERT, now you can use it with Query Builder.

As you know, not all Query Builder methods work with the Model. If you really think the upsert() should work with the Model, please make a feature request or send us a PR.

This is not a bug, just the current Model does not support the method.

kenjis avatar Feb 21 '23 07:02 kenjis