correct syntax required with 'ON DUPLICATE KEY UPDATE', SET option

Question

I have form.php in which a record is either created or edited. This page is called either by a a 'New Record' link, in which case there's no ID set, or by an 'EDIT' link in which case $_GET['ID'] is set (and used to retrieve the record).

Plan A was: Submitting form.php to process.php; in process.php, if there's an ID, the query is an UPDATE, otherwise it's an INSERT. At one point this if/else was working as intended but refreshing created dupes so I began playing with 'ON DUPLICATE KEY UPDATE', however w/out success. Plan B eventually occurred to my tiny little brain: shouldn't process.php have only an INSERT query, with ON DUPLICATE KEY UPDATE added? Haven't got this working yet either.

process.php:

    <?php
    // get $_POST from form.php *** note: no ID if it's a New Record ***
    $id     = $_POST['ID'];
    $invNumber  = $_POST['invoice-number'];
    $invDate    = $_POST['invoice-date'];
    $projNumber = $_POST['project-number'];
    $client = $_POST['client'];
    $issueDate  = $_POST['issue-date'];
    $task       = $_POST['task'];
    $subTotal   = $_POST['sub-total'];
    $tax        = $_POST['tax'];
    $invTotal   = $_POST['invoice-total'];
    $datePaid1  = $_POST['payment-date-1'];
    $datePaid2  = $_POST['payment-date-2'];
    $comments   = $_POST['comments'];

    if (isset($_POST['submit'])) {
        $query = "INSERT INTO $table SET
            invNumber   = '$invNumber',
            invDate     = '$invDate',
            projNumber  = '$projNumber',
            client      = '$client',
            task            = '$task',
            issueDate   = '$issueDate',
            subTotal        = '$subTotal',
            tax         = '$tax',
            invTotal        = '$invTotal',
            datePaid1   = '$datePaid1',
            datePaid2   = '$datePaid2',
            comments        = '$comments'

            ON DUPLICATE KEY UPDATE
            invNumber   = $invNumber,
            invDate     = $invDate,
            projNumber  = $projNumber,
            client      = $client,
            task            = $task,
            issueDate   = $issueDate,
            subTotal        = $subTotal,
            tax         = $tax,
            invTotal        = $invTotal,
            datePaid1   = $datePaid1,
            datePaid2   = $datePaid2
            ID              = LAST_INSERT_ID(ID)
        ";

        $lastID = mysql_insert_id();
        $result = mysql_query($query) or die(mysql_error());
        $affRows = mysql_affected_rows();
        if (($result) && ($affRows))    {
            echo "<p class=\"status\">
            <strong>RECORD #".$id." UPDATED.</strong><br />
            <strong>Records updated: " . $affRows . "</strong>
            </p>";
        } // END if ($result ...
    } // END CASE 1
?>

Refreshing process.php INSERTs dupes whether there's an ID or not. My 'ID' column, btw, is primary key, unique index, auto-increment. So how does the $query check the ID before either INSERTing or UPDATEing?[enter pulling-hair-out cliché following days and nights of research and experimentation]

Thanks in advance, s

p.s. re: injection:

I've been including this chunk in my head.php - pls let me know if this covers injection:

<?php
    // prevent SQL Injection in $_POST variables:
    foreach ($_POST as $key => $value)  {
        $_POST[$key] = mysql_real_escape_string($value);
    }

    // prevent SQL Injection in $_GET variables:
    foreach ($_GET as $key => $value)   {
        $_GET[$key] = mysql_real_escape_string($value);
    }
?>

You can simplify your `ON DUPLICATE KEY UPDATE` clause by referencing `VALUES`. See http://stackoverflow.com/questions/302544/is-there-a-way-to-do-an-insert-on-duplicate-key-udpate-in-zend-framework/1207076#1207076 — , Oct 05 '11 at 23:58
Thanks Phoenix. I've been holding fast to the SET option because I find it easier to read. Based on what I've read to date, I'm unaware of any other advantage to using VALUES, unless it's the sole solution to my present problem. Cheers! — shecky, Oct 06 '11 at 13:56

score 6 · Accepted Answer · edited Oct 06 '11 at 06:30

6

Fix that SQL-injection hole
You must not insert $_POST vars (or any super global $_*) directly into a query.
That's an SQL-injection hole.

Do this instead:

$id = mysql_real_escape_string($_POST['ID']);
$invnumber = mysql_real_escape_string($_POST['invoice_number']);
....
etc

The correct syntax for INSERT .. ON DUPLICATE KEY UPDATE is:

INSERT INTO TABLE (ID,invNumber,invDate,projNumber,client,task,issueDate
                  ,subTotal,tax,invTotal,datePaid1,datePaid2,comments)
VALUES ('$id','$invNumber','$invDate','$projNumber','$client','$task'
       ,'$issueDate','$subTotal','$tax','$invTotal','$datePaid1','$datePaid2'
       ,'$comments')
ON DUPLICATE KEY UPDATE invNumber = '$invNumber', invDate = '$invDate', .....

The last line can also be changed to (so, your code does not pass the parameters data twice):

ON DUPLICATE KEY UPDATE invNumber = VALUES(invNumber)
                      , invDate = VALUES(invDate)
                      , .....
                      , comments = VALUES(comments)

Do not use primary and unique keys in the update part
Note that it makes no sense to have exactly the same field in your insert part as in your update part.
If you use this statement, the update part must exclude all primary and unique keys from the SET clause!

edited Oct 06 '11 at 06:30

ypercubeᵀᴹ

113,259
19
174
235

answered Oct 05 '11 at 20:15

Johan

74,508
24
191
319

1

Correct syntax could also be: `ON DUPLICATE KEY UPDATE invNumber = VALUES(invNumber), invDate = VALUES(invDate), .....` – ypercubeᵀᴹ Oct 05 '11 at 22:02
And I thought it was `ON DUPLICATE KEY UPDATE`. Is `ON DUPLICATE KEY **SET**` valid syntax? – ypercubeᵀᴹ Oct 05 '11 at 22:10
Thank you Johan & ypercube. Please note my edited question; I'd very much appreciate confirmation that the code I include in my head.php is sufficient to prevent injection. – shecky Oct 06 '11 at 14:10
Johan, ypercube, a couple of follow up questions if you don't mind. First, is it not possible to use ON DUPLICATE KEY UPDATE when using the SET option rather than VALUES? I realize that the latter is far more common but I find the syntax of SET far easier to read and I'd like to understand why it seems to be less popular. – shecky Oct 06 '11 at 14:19
@shecky, the manual does not list the `set` variant you love as allowed, so I doubt it, not 100% sure though. That's one question. What's your second question? – Johan Oct 06 '11 at 14:30
Johan, ypercube; ID is the only column in my table with primary key/unique index, and I've removed it from the SET clause. I'm still getting dupes inserted even when there's an ID. – shecky Oct 06 '11 at 14:37
@shecky, it's impossible to insert duplicate primary key values, so I think you are formulating your previous sentence incorrectly. Could you please rephrase that? – Johan Oct 06 '11 at 15:24
@Johan, per Ole's info below, I've moved the ID='$id'; to the INSERT clause. My results presently are that when editing a record (an ID is present) I'm successfully updating and on refresh nothing happens - this is what I want. OTOH, after inserting a new record, if I refresh I get another record inserted, with a new ID but otherwise a dupe. Thx again for your help; what would you suggest next? I'm going batty. – shecky Oct 06 '11 at 17:04
@Johan, Btw, after the new record is inserted, the the url is process.php?ID=, (no ID) ... so is there a way to get the ID of the just-inserted record and get that into the URL? I don't understand how exactly to use mysql_insert_id(); or last_insert_id to solve this; is it a legit way and if so, how exactly to I implement it? Thanks a ton. – shecky Oct 06 '11 at 17:13
`$last_insert_id = mysql_insert_id()` then use $last_insert_id in your url. – Johan Oct 06 '11 at 21:07
@shecky, I think it's best to ask a new question about that subject, along with the relevant details. The comments are not really the right place to handle lots of details and new lines of question and answers. – Johan Oct 06 '11 at 21:10
let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/4095/discussion-between-shecky-and-johan) – shecky Oct 07 '11 at 15:33

score 1 · Answer 2 · answered Oct 05 '11 at 20:14

1

First I must say that ON DUPLICATE KEY UPDATE is not ment to replace the UPDATE, in this case I would use INSERT for new data, and UPDATE when existing data is modified.

To make the ON DUPLICATE KEY UPDATE trigger in your script you would have to also add the ID to the INSERT, since that is the unique column you're reffering to.

If it then finds the ID it will trigger the ON DUPLICATE part of your query.

answered Oct 05 '11 at 20:14

Ole

776
7
10

I'd upvote you, but you forgot the SQL-injection so I cannot :-( – Johan Oct 05 '11 at 20:18
2

Hehe oh. I'm still learning how to "teach". That's probably why I didn't mention the injection issue here :) From each post I learn something new, like now. Address all issues you spot even though they're not a part of the original question :P – Ole Oct 05 '11 at 22:07
Thx Helbom. As I noted, my original setup was as you describe: insert new data, update edited data ... but then I had to resolve the dupes issue and couldn't get ON DUPLICATE KEY UPDATE working in the insert query, so I tried to simplify into an insert query with ON DUPLICATE KEY UPDATE but I'm having trouble with that too – shecky Oct 06 '11 at 19:55

correct syntax required with 'ON DUPLICATE KEY UPDATE', SET option

2 Answers2