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

Dev: upsert()/setData() and binds #6688

Open
kenjis opened this issue Oct 14, 2022 · 8 comments
Open

Dev: upsert()/setData() and binds #6688

kenjis opened this issue Oct 14, 2022 · 8 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@kenjis
Copy link
Member

kenjis commented Oct 14, 2022

See #6600 (comment)

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 kenjis added the 4.3 label Oct 14, 2022
@kenjis
Copy link
Member Author

kenjis commented Oct 14, 2022

@codeigniter4/database-team Can you comment?

@kenjis
Copy link
Member Author

kenjis commented Oct 15, 2022

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
Copy link
Member

sclubricants commented Oct 15, 2022

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().

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Oct 17, 2022
@michalsn
Copy link
Member

+1 for binds.

@kenjis
Copy link
Member Author

kenjis commented Oct 27, 2022

$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
Copy link
Member

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
Copy link
Member

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

kenjis commented Aug 31, 2023

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

@kenjis kenjis changed the title Dev: upsert() and binds Dev: upsert()/setData() and binds Aug 31, 2023
@paulbalandan paulbalandan removed the 4.3 label Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

4 participants