Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
maniaba opened this issue Feb 20, 2023 · 6 comments
Open
Labels
enhancement PRs that improve existing functionalities

Comments

@maniaba
Copy link
Contributor

maniaba commented Feb 20, 2023

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 maniaba added the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 20, 2023
@kenjis
Copy link
Member

kenjis commented Feb 20, 2023

What are the current outputs?

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

@kenjis
Copy link
Member

kenjis commented Feb 20, 2023

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 kenjis removed the bug Verified issues on the current code behavior or pull requests that will fix them label Feb 20, 2023
@kenjis
Copy link
Member

kenjis commented Feb 20, 2023

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

@kenjis kenjis changed the title Bug: Upsert problem with data validation, allowed fields, dates defined within the model. Bug: [Model] Upsert problem with data validation, allowed fields, dates defined Feb 20, 2023
@maniaba
Copy link
Contributor Author

maniaba commented Feb 21, 2023

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
Copy link
Contributor Author

maniaba commented Feb 21, 2023

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`)

@kenjis
Copy link
Member

kenjis commented Feb 21, 2023

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 kenjis added the enhancement PRs that improve existing functionalities label Feb 21, 2023
@kenjis kenjis changed the title Bug: [Model] Upsert problem with data validation, allowed fields, dates defined [Model] Upsert problem with data validation, allowed fields, dates defined Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

No branches or pull requests

2 participants