Insert query won't add data to my database. Do I have my syntax wrong?

Question

I'm trying to assign an incremented id (posid) for each link a user adds to their profile, specific to their id in the table. This is what I have and it does not seem to be working. What is wrong? Do I have my syntax wrong?

mysql_query("INSERT INTO userlinks (id,hash,url,title,posid)
             VALUES (
                 '".$_SESSION['id']."',
                 '".md5($_GET['url'])."',
                 '".$_GET['url']."',
                 '".$_GET['title']."',
                 SELECT(IFNULL(SELECT MAX(posid)+1 FROM userlinks
             WHERE id='".$_SESSION['id']."'), 1)
           )");

add error checking, then you would know if the syntax is worng — , Nov 01 '11 at 04:27
very nice sql injection hole there. Enjoy your server getting destroyed... — Marc B, Nov 01 '11 at 05:03
@Marc I'm using a `$_GET['url'] = sanitize($_GET['url']);` function further up the page. — banjo, Nov 01 '11 at 05:13
@banjo it won't heelp you. also, let me guess that this function is bloated and splils your data instead of sanitizing it. — Your Common Sense, Nov 01 '11 at 05:18

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

2

If you're not going to use prepared statements or PDO, use:

$query = sprintf("INSERT INTO userlinks
                    (id, hash, url, title, posid)
                  SELECT %s,
                         %s,
                         %s,
                         %s,
                         IFNULL(MAX(t.posid), 1)
                    FROM userlinks t
                   WHERE id = %s",
                  mysql_real_escape_string($_SESSION['id']),
                  MD5(mysql_real_escape_string($_SESSION['url'])),
                  mysql_real_escape_string($_GET['url']),
                  mysql_real_escape_string($_GET['title']),
                  mysql_real_escape_string($_SESSION['id']));

$result = mysql_query($query);

With an INSERT statement, you can't mix a SELECT with the VALUES keywords -- one or the other, and SELECT supports defining static values.

Secondly, the MAX(...) + 1 is not reliable in a multi-user situation -- you should use the MySQL autoincrement functionality.

Reference:

edited Jun 20 '20 at 09:12

Community

1
1

answered Nov 01 '11 at 04:38

OMG Ponies

325,700
82
523
502

Basically what I am trying to achieve is the ability to reorder the list with a drag and drop .sortable function. So the table that holds all the users URLs has an auto incremented urlid for each URL. But I need a seperate column that holds an incremented position id (posid) pertaining only to that specific user ($_SESSION['id']). What would be the best way to achieve this? – banjo Nov 01 '11 at 05:06
@banjo why not to just swap 2 numbers? – Your Common Sense Nov 01 '11 at 05:11
@Col.Shrapnel What do you mean? With a drag and drop .sortable jquery dont i need 2 numbers? or is it possible to swap 2 rows based on their auto_incremented id alone? – banjo Nov 01 '11 at 05:18
@banjo definitely not the auto-incremented unique identifier I mean. see update to my answer – Your Common Sense Nov 01 '11 at 05:22

score 2 · Answer 2 · edited May 23 '17 at 12:03

Do I have my syntax wrong?

Definitely YES. you don't do any error handling.
So, if an error occurs, you have no clue what is it.
at least run your queries this way and see what's wrong with them yourself

mysql_query($query) or trigger_error(mysql_error()." ".$query);

Also, your SELECT(IFNULL(SELECT MAX(posid)+1 FROM userlinks subselect looks suspicious as it seems you are not using advantage of auto-increment and in danger of race condition.

also, as it already mentioned, you do not escape your strings.

As for your sorting, what I'd do:

after inserting a row, update it's sort field with id value.
when we have to swap 2 lines - just swap it's sort values.

here is a code, quite ugly but to give you idea:

$id    = intval($_POST['move']);
$place = db("SELECT place FROM $table WHERE id=$id");
if (!$id OR !$place) return(error("id or place is not set"));

if (isset($_POST['up'])) {
  $sort  = db("SELECT sort FROM $table WHERE id=$id");
  $sort2 = db("SELECT max(sort) as msort FROM $table WHEREsort < $sort");
  if ($sort2) $id2=db("SELECT id FROM $table WHEREsort = $sort2");
}
if (isset($_POST['down'])) {
  $sort  = db("SELECT sort FROM $table WHERE id=$id");
  $sort2 = db("SELECT min(sort) as msort FROM $table WHEREsort > $sort");
  if ($sort2) $id2 = db("SELECT id FROM $table WHERE sort = $sort2");
}
if ($sort2) {
  $q1 = "UPDATE $table SET sort=$sort2 WHERE id=$id";
  $q2 = "UPDATE $table SET sort=$sort WHERE id=$id2";
  db($q1);
  db($q2);
}

As for the sanitizing your data, refer to these questions
- PHP secure user variable
- In PHP when submitting strings to the database should I take care of illegal characters using htmlspecialchars() or use a regular expression?
- How to include a PHP variable inside a MySQL insert statement

I am running everything through a sanitize function further up in my code. eg `$_GET['url'] = sanitize($_GET['url']);` and `function sanitize($str) { if(ini_get('magic_quotes_gpc')) $str = stripslashes($str); $str = strip_tags($str); $str = trim($str); $str = htmlspecialchars($str); $str = mysql_real_escape_string($str); return $str; }` Is this sufficient? I just didn't include it to simplify the amount of code i posted. — banjo, Nov 01 '11 at 05:23

score 1 · Answer 3 · answered Nov 01 '11 at 04:35

You can't combine insert into and select the way you are doing it. You need to do something like:

INSERT INTO your_table (col, col, col)
values
Select  '".$_SESSION['id']."',  '".md5($_GET['url'])."',MAX(posid)+1
from userlinks
WHERE id='".$_SESSION['id']."')

Obviously, above query is not correct either, you need to make sure you concatenate your string properly. The intention is to give you the proper structure of the query

Insert query won't add data to my database. Do I have my syntax wrong?

3 Answers3

Reference: