Bug with $db->setQueryOption('IGNORE')->insertMulti

[LunarDevelopment]

Hello, I'm trying to use IGNORE with insertMulti but getting duplicate key errors, code and error below, with the only notable oddity being the table being inserted into has a compound primary key - that shouldn't affect IGNORE though :

        $records = $db->setQueryOption('IGNORE')->insertMulti('table', $array_of_arrays);

        if(!$records) {
            echo 'insert failed: ' . $db->getLastError();
        } else {
            echo 'new records inserted with following id\'s: ' . implode(', ', $records);
        }

insert failed: Duplicate entry 'recordname-2018-07-05' for key 'PRIMARY'

Results of $db->getLastQuery()

INSERT  INTO table... 

composer.json :

    "joshcam/mysqli-database-class": "dev-master",

Also tried on v2.9.2 with same error messages.

[michabbb]

because this looks like a regular mysql error, i would suggest, you show us your $array_of_arrays 😏

[LunarDevelopment]

$array_of_arrays is arbitrary - the issue is because the getLastQuery doesn't have "Ignore" in it - if it did then the script wouldn't bomb.

For illustration I've just tried with the following and had the same error:

$array_of_arrays = array(
array('name' =>'recordname', 'date' => '2018-07-05'),
array('name' =>'recordname', 'date' => '2018-07-05')
); 

[VeNoMouS]

I dunno... the code logic seems sound..

It goes insertMulti -> insert -> _buildInsert which then puts the option into the query

$this->_query = $operation . " " . implode(' ', $this->_queryOptions) . " INTO " . self::$prefix . $tableName;

And _queryOptions is populated in the function setQueryOption

Do you get the same issue if you

$db->setQueryOption('IGNORE')->insert('table', ['name' =>'recordname', 'date' => '2018-07-05']);

[michabbb]

maybe a trace shows more infos what´s going on here 🤔

[LunarDevelopment]

@michabbb @VeNoMouS

Array
(
    [0] => Array
        (
            [0] => INSERT IGNORE INTO db.table (`name`, `date`)  VALUES ('test',  '2018-07-18')
            [1] => 0.61394000053406
            [2] => MysqliDb->insertMulti() >>  file "/dir/bulkInsertTest.php" line #102 
        )

    [1] => Array
        (
            [0] => INSERT  INTO db.table (`name`, `date`)  VALUES ('test',  '2018-07-18')
            [1] => 0.54291319847107
            [2] => MysqliDb->insertMulti() >>  file "/dir/bulkInsertTest.php" line #102 
        )

)

[michabbb]

@LunarDevelopment i am new to this project, so sorry if i can´t offer much help. i study some issues here to evaluate if this class works for me. after seeing your case i got interested in this so i ask myself, what would i do ? 😏

i saw this class offers a trace, which seems to be nice feature.
so the IGNORE is only used in the first statment , do i see this correctly ?

[LunarDevelopment]

@michabbb Ah sorry, because you were being so helpful I assumed you were a collaborator or something.

Yeah it looks like the library is forgetting that IGNORE is set as a query option after first query..

I'll play around and see if I can fix, but any external help is much appreciated - it seems like a fairly large oversight in the library considering there's a closed thread about this issue in 2017.

[VeNoMouS]

This is due to _buildInsert calling the function reset which redefines _queryOptions $this->_queryOptions = array();

What needs to happen, is in multiInsert in the foreach, before the insert function is called, you need to reissue the options..

public function insertMulti($tableName, array $multiInsertData, array $dataKeys = null)
    {
        // only auto-commit our inserts, if no transaction is currently running
        $autoCommit = (isset($this->_transaction_in_progress) ? !$this->_transaction_in_progress : true);
        $ids = array();

        $options = [
            '_queryOptions' => $this->_queryOptions,
            '_nestJoin' => $this->_nestJoin,
            '_forUpdate' => $this->_forUpdate,
            '_lockInShareMode' => $this->_lockInShareMode 
        ];
        
        if($autoCommit) {
            $this->startTransaction();
        }

        foreach ($multiInsertData as $insertData) {
            if($dataKeys !== null) {
                // apply column-names if given, else assume they're already given in the data
                $insertData = array_combine($dataKeys, $insertData);
            }
       
            foreach ($options as $k => $v) {
                $this->${k} = $v;
            }

            $id = $this->insert($tableName, $insertData);
            if(!$id) {
                if($autoCommit) {
                    $this->rollback();
                }
                return false;
            }
            $ids[] = $id;
        }
        if($autoCommit) {
            $this->commit();
        }
        return $ids;
    }

[avbdr]

good catch guys. should be pretty trivial to fix

[VeNoMouS]

actually my above wont fully work, as @avbdr has some custom class variables in setQueryOption on certain ifs that arent populated over to _queryOptions

 if ($option == 'MYSQLI_NESTJOIN') {
                $this->_nestJoin = true;
            } elseif ($option == 'FOR UPDATE') {
                $this->_forUpdate = true;
            } elseif ($option == 'LOCK IN SHARE MODE') {
                $this->_lockInShareMode = true;
            } else {
                $this->_queryOptions[] = $option;
            }

[avbdr]

yep. we would need to store those variables before insert cycle, and then redefine them before each insert()

[VeNoMouS]

@avbdr i've updated my above fix... that should be enough?

[avbdr]

            $this->_nestJoin
            $this->_forUpdate
            $this->_lockInShareMode
            $this->_queryOptions[]

you would need to store all of those

[VeNoMouS]

@avbdr I have, refer the above fix :) in my post earlier

[avbdr]

seems i have checked cached page. Yes, your fix looks like a good one.
@LunarDevelopment can you test that?

[LunarDevelopment]

@avbdr

The below from @VeNoMouS kinda works.. but produces the following :

Stack trace shows that it's adding IGNORE into all insert queries! woo!

If there are no duplicates then the script completes and commits rows to DB.

If there are duplicates then the script doesn't commit any of the rows to the DB and returns insert failed: .

Minor issue is:

Notice: Use of undefined constant k - assumed 'k' in /dir/script.php
Coming from:

            foreach ($options as $k => $v) {
                $this->${k} = $v;
            }

Function I've used from @VeNoMouS is ...

public function insertMulti($tableName, array $multiInsertData, array $dataKeys = null)
    {
        // only auto-commit our inserts, if no transaction is currently running
        $autoCommit = (isset($this->_transaction_in_progress) ? !$this->_transaction_in_progress : true);
        $ids = array();

        $options = [
            '_queryOptions' => $this->_queryOptions,
            '_nestJoin' => $this->_nestJoin,
            '_forUpdate' => $this->_forUpdate,
            '_lockInShareMode' => $this->_lockInShareMode 
        ];
        
        if($autoCommit) {
            $this->startTransaction();
        }

        foreach ($multiInsertData as $insertData) {
            if($dataKeys !== null) {
                // apply column-names if given, else assume they're already given in the data
                $insertData = array_combine($dataKeys, $insertData);
            }
       
            foreach ($options as $k => $v) {
                $this->${k} = $v;
            }

            $id = $this->insert($tableName, $insertData);
            if(!$id) {
                if($autoCommit) {
                    $this->rollback();
                }
                return false;
            }
            $ids[] = $id;
        }
        if($autoCommit) {
            $this->commit();
        }
        return $ids;
    }

[avbdr]

thanks. This is an expected behavior since multiInsert() does all data entry within a single transaction as you can see from the function.

@VeNoMouS can you please fix a warning and submit a pull request with this change?

[avbdr]

function returns false in case of failure and you can use getLastError() to get an error.

[LunarDevelopment]

I've submitted a pull request with a fix for the k warning.

When using multi insert with ignore though, is it desired to roll back?

If so, then the "ignore" option is redundant with insertMulti.

If roll back is desired then I'll just create an extension of the class.

[avbdr]

public function insertMulti($tableName, array $multiInsertData, array $dataKeys = null)
    {
        // only auto-commit our inserts, if no transaction is currently running
        $autoCommit = (isset($this->_transaction_in_progress) ? !$this->_transaction_in_progress : true);
        $ids = array();

        $options = [
            '_queryOptions' => $this->_queryOptions,
            '_nestJoin' => $this->_nestJoin,
            '_forUpdate' => $this->_forUpdate,
            '_lockInShareMode' => $this->_lockInShareMode 
        ];
        
        if($autoCommit) {
            $this->startTransaction();
        }

        foreach ($multiInsertData as $insertData) {
            if($dataKeys !== null) {
                // apply column-names if given, else assume they're already given in the data
                $insertData = array_combine($dataKeys, $insertData);
            }
       
            foreach ($options as $k => $v) {
                $this->${k} = $v;
            }

            $id = $this->insert($tableName, $insertData);
            if(!$id && !in_array('IGNORE', $this->_queryOptions)) {
                if($autoCommit) {
                    $this->rollback();
                }
                return false;
            }
            $ids[] = $id;
        }
        if($autoCommit) {
            $this->commit();
        }
        return $ids;
    }

[avbdr]

this should properly handle IGNORE case

[LunarDevelopment]

@avbdr just tested and that doesn't work, can't see why not though.

I tried moving a bit of logic around too:

            $id = $this->insert($tableName, $insertData);
            if(!$id ) {
                if($autoCommit &&  !in_array('IGNORE', $this->_queryOptions)) {
                    $this->rollback();
                }
                return false;
            }
            $ids[] = $id;

[LunarDevelopment]

Seems solid, returns array of ids/ true and false if IGNORE is set :

    public function insertMulti($tableName, array $multiInsertData, array $dataKeys = null)
    {
        // only auto-commit our inserts, if no transaction is currently running
        $autoCommit = (isset($this->_transaction_in_progress) ? !$this->_transaction_in_progress : true);
        $ids = array();

        $options = [
            '_queryOptions' => $this->_queryOptions,
            '_nestJoin' => $this->_nestJoin,
            '_forUpdate' => $this->_forUpdate,
            '_lockInShareMode' => $this->_lockInShareMode
        ];

        if($autoCommit) {
            $this->startTransaction();
        }

        foreach ($multiInsertData as $insertData) {
            if($dataKeys !== null) {
                // apply column-names if given, else assume they're already given in the data
                $insertData = array_combine($dataKeys, $insertData);
            }

            foreach ($options as $k => $v) {
                $this->${'k'} = $v;
            }

            $id = $this->insert($tableName, $insertData);
            if(!$id ) {
                if($autoCommit &&  !in_array('IGNORE', $options['_queryOptions'])) {
                    $this->rollback();
                }
            }
            $ids[] = $id;
        }
        if($autoCommit) {
            $this->commit();
        }
        return $ids;
    }

[LunarDevelopment]

I know we're solving a bug in the above, but in thinking a bit more about it - there's no speed benefit to using insertMulti over using insert in a foreach in the calling script.

Surely insertMulti should build a statement like:

insert into db.table (`name`, `date`)
VALUES 
('lewis', '01-02,2018'),
('dave', '01-02,2018'),
('charles', '01-02,2018') ; 

Or maybe there should be an insertBatch function which does the above.. ?

[avbdr]

you last changes are breaking return type on rollback. Please if you want to submit a patch -- make it less intrusive

[VeNoMouS]

Sorry was thinking of http://php.net/manual/en/migration70.incompatible.php

foreach ($options as $k => $v) {
    $this->${k} = $v;
}

Should be

foreach ($options as $k => $v) {
    $this->{$k} = $v;
}

I wrote that on the fly without thinking :P my bad

[auth1299]

I'm having the same issue as described above. Is this bug fixed?

[VeNoMouS]

Doesn't look like the PR was merged.. #775

@ThingEngineer