Dev: upsert()/setData() and binds

[kenjis]

See https://github.com/codeigniter4/CodeIgniter4/pull/6600#discussion_r992735103

The following test (MySQL) fails now. Should upsert() also use binds?

    public function testUpsertArrayAndCheckBinds()
    {
        $builder  = $this->db->table('user');
        $userData = [
            'email'   => '[email protected]',
            'name'    => 'Upsert One',
            'country' => 'US',
        ];
        $builder->testMode()->set($userData);

        $expectedSQL = <<<'SQL'
            INSERT INTO `db_user` (`country`, `email`, `name`) VALUES ('US','[email protected]','Upsert One') ON DUPLICATE KEY UPDATE `db_user`.`country` = VALUES(`country`), `db_user`.`email` = VALUES(`email`), `db_user`.`name` = VALUES(`name`)
            SQL;
        $this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledUpsert()));

        $expectedBinds = [
            'email' => [
                '[email protected]',
                true,
            ],
            'name' => [
                'Upsert One',
                true,
            ],
            'country' => [
                'US',
                true,
            ],
        ];
        $this->assertSame($expectedBinds, $builder->getBinds());
    }

1) CodeIgniter\Database\Live\UpsertTest::testUpsertArrayAndCheckBinds
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 (
-    'email' => Array &1 (
-        0 => '[email protected]'
-        1 => true
-    )
-    'name' => Array &2 (
-        0 => 'Upsert One'
-        1 => true
-    )
-    'country' => Array &3 (
-        0 => 'US'
-        1 => true
-    )
-)
+Array &0 ()

[kenjis]

@codeigniter4/database-team Can you comment?

[kenjis]

I don't know if this is relevant to this case, in my opinion, it is always safer to use prepared statements when we can use. This is because prepared statements bind the value by database, so escape failures cannot occur.

On the other hand, if we use QueryBuilder to escape values when generating a query, any bugs in the escaping process will cause escaping to fail and might allow SQL injections.

Therefore, it is safer to change the implementation to execute queries as prepared statements by default. Or I want to have such an option.

[sclubricants]

upsert() clears binds once it extracts the data. getCompiledUpsert() calls upsert() so therefore clears binds.

I thought of taking a look at using binds with upsert. Just had a lot going on and haven't had the chance yet.

One problem for now with using binds is that upsert() is compatible with setData() which does not use binds. I'd have to see how we might could work it all in together. I thought of a method used with setData() such as: $builder->binds(true)->setData().

[michalsn]

+1 for binds.

[kenjis]

$builder->binds(true)->setData() seems not good. Binding is the default behavior in QB. We just dropped it when *batch() because of the performance issue.

[sclubricants]

Allowing you to decide whether or not to use binds allows the developer to choose based on performance. Surely if your only updating a few records the performance wouldn't be such an issue.

[sclubricants]

Taking a look at binds it appears it does just about absolutely nothing but use escape() which is done anyways with *batch().

I thought it was using mysqli prepared statements but it is not.

Everything is done here:

    /**
     * Match bindings
     */
    protected function matchNamedBinds(string $sql, array $binds): string
    {
        $replacers = [];

        foreach ($binds as $placeholder => $value) {
            // $value[1] contains the boolean whether should be escaped or not
            $escapedValue = $value[1] ? $this->db->escape($value[0]) : $value[0];

            // In order to correctly handle backlashes in saved strings
            // we will need to preg_quote, so remove the wrapping escape characters
            // otherwise it will get escaped.
            if (is_array($value[0])) {
                $escapedValue = '(' . implode(',', $escapedValue) . ')';
            }

            $replacers[":{$placeholder}:"] = $escapedValue;
        }

        return strtr($sql, $replacers);
    }

[kenjis]

Yes, we should use the database's prepared statements, but the feature is missing now.