oscarotero / simple-crud

PHP library to provide magic CRUD in MySQL/Sqlite databases with zero configuration
MIT License
242 stars 58 forks source link

Insert duplications overwrites primary key with id=0 #21

Closed andrenam closed 6 years ago

andrenam commented 6 years ago

I'm having problems with the duplications() modifier for Insert statements. I have a table with auto-incrementing ID and a unique column.

database used

CREATE TABLE `test` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `unique_column` int(11) NOT NULL,
  `optional_column` varchar(100) DEFAULT NULL,
  PRIMARY KEY (`id`),
  UNIQUE KEY (`unique_column`)
);

When I try to insert a (duplicate) row without specififying the (autoincrementing) ID and giving a duplicate value for the unique column, the INSERT .. ON DUPLICATE KEY UPDATE statement overwrites the primary key with id=0.

script used

<?php
require './vendor/autoload.php';
require './config.php';
use SimpleCrud\SimpleCrud;

$pdo = new PDO('mysql:host='.$dbhost.';dbname='.$dbname, $dbuser, $dbpass);
$db = new SimpleCrud($pdo);

// print out all queries
$db->onExecute(function ($pdo, $statement, $marks) {
    print($statement->queryString."\n");
    if (is_array($marks) && count($marks)) {
        echo print_r($marks, true)."\n";
    }
});
// activate duplications for test-table
$db->test->addQueryModifier('insert', function ($query) {
    $query->duplications(true);
});

// create new row with auto-incrementing id, unique_column=5 and optional_column=NULL
$new_test = $db->test->create();
$new_test->unique_column = 5;
// leave out optional column
$new_test->save();

// later add a value to optional column
$new_test->optional_column = 'comment';
$new_test->save();

// ... time goes on

// try to re-insert a new row with auto-incrementing id
// and the same default value 5
// buth without the optional column
$new_test2 = $db->test->create();
$new_test2->unique_column = 5;
// leave out optional column
$new_test2->save();

run the script

$ php duplicates_bug.php 
SHOW TABLES
DESCRIBE `test`
INSERT INTO `test` (`id`, `unique_column`, `optional_column`) VALUES (:id, :unique_column, :optional_column) ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id), `id` = :id, `unique_column` = :unique_column, `optional_column` = :optional_column
Array
(
    [:id] => 
    [:unique_column] => 5
    [:optional_column] => 
)

UPDATE `test` SET `id` = :__id, `unique_column` = :__unique_column, `optional_column` = :__optional_column WHERE (`test`.`id` = :id) LIMIT 1
Array
(
    [:id] => 1
    [:__id] => 1
    [:__unique_column] => 5
    [:__optional_column] => comment
)

Expected and actual state of database after this:

MariaDB [test]> SELECT * FROM test;
+----+---------------+-----------------+
| id | unique_column | optional_column |
+----+---------------+-----------------+
|  1 |             5 | comment         |
+----+---------------+-----------------+

The script continues to try to insert the second row:

INSERT INTO `test` (`id`, `unique_column`, `optional_column`) VALUES (:id, :unique_column, :optional_column) ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id), `id` = :id, `unique_column` = :unique_column, `optional_column` = :optional_column
Array
(
    [:id] => 
    [:unique_column] => 5
    [:optional_column] => 
)

Expected state of database after this:

MariaDB [test]> SELECT * FROM test;
+----+---------------+-----------------+
| id | unique_column | optional_column |
+----+---------------+-----------------+
|  1 |             5 |            NULL |
+----+---------------+-----------------+

Actual state of database after this:

MariaDB [test]> SELECT * FROM test;
+----+---------------+-----------------+
| id | unique_column | optional_column |
+----+---------------+-----------------+
|  0 |             5 | NULL            |
+----+---------------+-----------------+

Notice that the auto-incrementing id gets overwritten with 0 which leads to errors like this:

$test = $db->test->select()->run();

PHP Fatal error:  Uncaught SimpleCrud\SimpleCrudException: Only rows with the defined id must be added to collections in /home/nam/project/vendor/simple-crud/simple-crud/src/RowCollection.php:173
Stack trace:
#0 /home/nam/project/vendor/simple-crud/simple-crud/src/Queries/Mysql/Select.php(90): SimpleCrud\RowCollection->offsetSet(NULL, Object(SimpleCrud\Row))
#1 /home/nam/project/duplicates_bug.php(21): SimpleCrud\Queries\Mysql\Select->run()
#2 {main}
  thrown in /home/nam/project/vendor/simple-crud/simple-crud/src/RowCollection.php on line 173

Software used:

Linux altair 4.13.12-1-ARCH #1 SMP PREEMPT Wed Nov 8 11:54:06 CET 2017 x86_64 GNU/Linux
mariadb 10.1.28-1
php 7.1.11-1
simple-crud latest commit 792d528d818dbd56639175ad22d1f3095accdf0e

reason:

The reason is that ID is specified twice in the ON DUPLICATE KEY UPDATE .. clause ... ON DUPLICATE KEY UPDATE id = LAST_INSERT_ID(id),id= :id, ...

quick n dirty hotfix?:

but what if the primary key is not called "id"? etc. etc. so this is not meant as a general bugfix but instead to help point you in the right direction.

diff --git a/src/Queries/Mysql/Insert.php b/src/Queries/Mysql/Insert.php
index 8a2a2ec..bbf6a5c 100644
--- a/src/Queries/Mysql/Insert.php
+++ b/src/Queries/Mysql/Insert.php
@@ -92,6 +92,7 @@ class Insert extends Query
         $query .= ' VALUES ('.self::buildFields($fields).')';

         if ($this->duplications) {
+            unset($fields['id']);
             $query .= ' ON DUPLICATE KEY UPDATE';
             $query .= ' id = LAST_INSERT_ID(id), '.Update::buildFields($fields);
         }

Also what I would like to know, is it possible to not overwrite the non-specified field "optional_column" on the ON DUPLICATE KEY UPDATE?

So that the value in optional columns dont get overwritten when trying to insert a duplicate line. Maybe I should use an INSERT IGNORE ... statement instead then? Do you have any suggestions on how to do that?

oscarotero commented 6 years ago

Hi, thanks for the detailed reporting. I've made a commit to fix the id=0. If the id is null, it can be omitted in order to use the other unique field. And about not ovewriding the optional_column, currently is not possible. We could customize this, for example, accepting a callback to filter the columns you want to replace. Something like:

$query->duplications(function ($data) {
    return array_filter($data); //Update only non-empty fields
});

My only concern is how to do this in sqlite. I'd like to keep the same behaviour in mysql/mariadb and sqlite.

andrenam commented 6 years ago

Thanks, problem solved.

I saw that my problem with overwriting the optional_column doesnt occur, when I use the syntax

$db->test->insert()->data(array('unique_column' => 5))->run();

instead of

$new_test = $db->test->create();
$new_test->unique_column = 5;
// leave out optional column
$new_test->save();