0

I've now been trying for hour and can't figure the problem out. I've made a php file that fetch all items in a table and retrieves that as JSON. But for some reason after I inserted the second mysql-query, it stopped fetching the first item. My code is following:

...
    case "LoadEntryList":
        $result2 = performquery("SELECT * FROM Entries WHERE Category = '" . $_POST["Category"] .
        "' LIMIT " . $_POST["Offset"] . ", " . $_POST["Quantity"] . "");
        $row2 = $result2->fetch_assoc();
    while($row = $result2->fetch_assoc()) {
        $result3 = performquery("SELECT Username FROM Users WHERE ID = '" . $row2["UserID"] . "'");
        $row3 = $result3->fetch_assoc();
   echo substr(json_encode($row),0,
        strlen(json_encode($row))-1) . ",\"Username\":\"" . $row3["Username"]  . "\"}";
}
...

Any help is greatly appreciated.

EDIT: Thanks for all those super fast responses.

Fusseldieb
  • 1,324
  • 2
  • 19
  • 44
  • Note that LIMIT without ORDER BY is fairly meaningless. – Strawberry Nov 13 '15 at 14:06
  • @Strawberry I used ORDER BY 'ID' ASC and it gave me an error... – Fusseldieb Nov 13 '15 at 14:15
  • @Fusseldieb ORDER BY ID ASC, without the quote. Also I don't mean to rant or anything, but beside the wide sql inject, it's also messy and poorly written. Query inside a loop, is also a bad thing, avoid where possible. Use a left join on `Entries` and `Users`. Your manually editing the json string. Don't! Before you encode it add to your `$row` another index `$row['Username'] = $row3['Username']` and then encode it. Limit $result3 query to 1. – Mujnoi Gyula Tamas Nov 13 '15 at 14:25
  • @Strawberry How do I join the two queries to retrieve a entry and then replace the UserID with the username in one query? The only solution I found by myself is that... And the injection, don't worry, I sanitize the inputs normally later, its just for debugging now... – Fusseldieb Nov 13 '15 at 14:33
  • @Fusseldieb 'ID' is a string. ID or \`ID\` might be a column name. And you only need one query here. The accepted answer is poor. – Strawberry Nov 13 '15 at 14:36
  • not only that, but there is a problem with the encoded json as well... – Mujnoi Gyula Tamas Nov 13 '15 at 14:49

4 Answers4

3

First you're fetching a row:

$row2 = $result2->fetch_assoc();

Then you start looping at the next row:

while($row = $result2->fetch_assoc()) {

If you want to loop over all of the rows, don't skip the first one. Just loop over all of the rows:

$result2 = // your very SQL-injectable query
while($row2 = $result2->fetch_assoc()) {
  $result3 = // your other very SQL-injectable query
  $row3 = $result3->fetch_assoc();
  // etc.
}

Note that errors like this would be a lot more obvious if you used meaningful variable names. "row2", "result3", etc. are pretty confusing when you have overlapping levels of abstraction.


Important: Your code is wide open to SQL injection attacks. You're basically allowing users to execute any code they want on your database. Please look into using prepared statements and treating user input as values rather than as executable code. This is a good place to start reading, as is this.

Community
  • 1
  • 1
David
  • 208,112
  • 36
  • 198
  • 279
  • Thanks, I know about this injection attack. Its just for the debug for now... Normally I sanitize the inputs. – Fusseldieb Nov 13 '15 at 14:07
1

No Need of $row2 = $result2->fetch_assoc();

    <?
    case "LoadEntryList":
    $result2 = performquery("SELECT * FROM Entries WHERE Category = '" . $_POST["Category"] .
        "' LIMIT " . $_POST["Offset"] . ", " . $_POST["Quantity"] . "");
    while($row = $result2->fetch_assoc()) 
    {
        $result3 = performquery("SELECT Username FROM Users WHERE ID = '" . $row["UserID"] . "'");
        $row3 = $result3->fetch_assoc();
        echo substr(json_encode($row),0,strlen(json_encode($row))-1) . ",\"Username\":\"" . $row3["Username"]  . "\"}";
    }
    ?>

Or,

<?
...
    case "LoadEntryList":
    $Category=$_POST["Category"];
    $Offset=$_POST["Offset"];
    $Quantity=$_POST["Quantity"];

    $result3 = performquery("SELECT Entries.*, Users.Username FROM Entries, Users WHERE Entries.Category=$Category AND Entries.UserID=Users.ID LIMIT $Offset, $Quantity");

    $row3 = $result3->fetch_assoc();
    echo substr(json_encode($row),0,strlen(json_encode($row))-1) . ",\"Username\":\"" . $row3["Username"]  . "\"}";
}
...
?>
Nana Partykar
  • 10,556
  • 10
  • 48
  • 77
  • It actually solved my problem almost instantly. Thank you. – Fusseldieb Nov 13 '15 at 14:17
  • Glad It Helped Mr @Fusseldieb. Thanks. – Nana Partykar Nov 13 '15 at 14:19
  • Please explain about the downvote ?? Don't simply downvote and escape by hiding faces. – Nana Partykar Nov 13 '15 at 14:42
  • I don't know who down voted, but you fixed a small portion of a bad written code. You should have updated the code to fix all of his issues. Don't support poorly written code, fix it or suggest something else so they can learn. – Mujnoi Gyula Tamas Nov 13 '15 at 14:55
  • Mr @MujnoiGyulaTamas : As OP wrote `Thanks, I know about this injection attack. Its just for the debug for now... Normally I sanitize the inputs. – Fusseldieb` To Mr David . He was only interested in knowing the problem why he is getting only one row. I did that. Simple. – Nana Partykar Nov 13 '15 at 14:57
  • @NanaPartykar I'm not talking about the sanitizing part because I saw the comment as well. It's about the query. Join the 2 queries, eliminating 1 extra query, also the need to substr which is eliminated if prior the encode you add it to the array $row, see my answer for more details – Mujnoi Gyula Tamas Nov 13 '15 at 15:01
  • @Fusseldieb : For JOIN : See My Updated Answer. You can reduce 2 query in one single query. – Nana Partykar Nov 13 '15 at 19:53
1

I have a addition to David answer(can't comment on it yet)

This line of code:

   $result3 = performquery("SELECT Username FROM Users WHERE ID = '" . $row2["UserID"] . "'");

will always return with the same result. If you were to change $row2[... into $row[... the code would take the rows that get updated by the while loop.

Jelmergu
  • 973
  • 1
  • 7
  • 19
0

I am not content with the accepted result. The snippet can be fixed / replaced, and also a bad code must be replaced. Also not to mention is that I don't know if anyone spotted a really big mistake in the output. Here is the fix and I'll explain why.

$JSON = array();
$result2 = performquery( '
    SELECT
        e.*, u.Username
    FROM Entries AS e
    LEFT JOIN Users AS u ON u.ID = e.UserID
    WHERE
        e.Category = ' . $_POST['Category'] . '
    LIMIT ' . $_POST['Offset'] . ', ' . $_POST['Quantity'] . '
' );
while( $row2 = $result2->fetch_assoc() ){
    $JSON[] = $row2;
}
echo json_encode( $JSON );

Obviously the main issue is the query, so I fixed it with a LEFT JOIN, now the second part is the output. First it's the way you include the username, and the second what if you had multiple results? Than your output will be:

{"ID":1,"Username":"John"}{"ID":2,"Username":"Doe"}

How do you parse it? So the $JSON part comes in place. You add it to an array and will encode that array. Now the result is:

{["ID":1,"Username":"John"],["ID":2,"Username":"Doe"]}

LE: I left out the sql inject part which as stated by the OP, will be done afterwards? I'm not sure why not do it at the point of writing it, because you may forget later on that you need to sanitize it.

Mujnoi Gyula Tamas
  • 1,759
  • 1
  • 12
  • 13
  • SQL injection I'm preventing now by converting some string into integers.... limiting string length.... etc. – Fusseldieb Nov 13 '15 at 19:46
  • As mentioned previously you should use prepared statments. also make sure you don't run into the second problem I mentioned. – Mujnoi Gyula Tamas Nov 13 '15 at 19:49
  • For the output I use replace "}{" to "},{"... And your code gives me an 500 error, I don't know why... – Fusseldieb Nov 13 '15 at 20:03
  • I missed a column identifier, my mistake. `Category` should be `e.Category`. I updated my answer. Also why search and replace, because you can simply add it to an array prior encodeing it with json_encode. – Mujnoi Gyula Tamas Nov 13 '15 at 20:30
  • @Fusseldieb replacing what you described may still cause error your final result will be `{"ID":1,"Username":"John"},{"ID":2,"Username":"Doe"}`, which isn't wrapped in anything, thus you still have multiple objects. You should than wrap all the replaces in a wrapper like `echo '{', substr(json_encode($row),0,strlen(json_encode($row))-1) . ",\"Username\":\"" . $row3["Username"] . "\"}}";`, but its just a waste of resource and bad practise. I'm not trying to be a bad person, I'm just trying to help you realise and code in a more efficient and cleaner way. Hope you don't take it the wrong way. – Mujnoi Gyula Tamas Nov 13 '15 at 20:39
  • I have taken your updated code, it gave me 500 error too, then I've put a quotation mark between "Category" and now it seems to work. Thanks very much. – Fusseldieb Nov 13 '15 at 20:42
  • EDIT: Yes, now it works. And I agree, It's much cleaner now. Thanks!! – Fusseldieb Nov 13 '15 at 20:46