Perl won't execute SQL statement in a while loop

Question

I wrote this piece of code: it is supposed to keep checking names until it finds one that does not exist, and then it should go on.

 while ($repeating == 1) {
            $new_name = $i . "_" . $file;
            my $sql= "SELECT file_name FROM PDFdocument WHERE user_id = '$id' AND file_name = '$new_name' ";
            my $sth = $dbh->prepare($sql);
            $sth->execute();
            while (my @row = $sth->fetchrow_array) {
                  //never enters here
                if ($new_name ne $row[0]) {

                    $repeating = 0;

                }
            }
            $i++;
        }

It never enters the second while loop, so it gets stuck in this repeating loop. I don't know why it does not work; I do some other sql statements before and they all work. This is the only one that does not work.

Any help?

And you're sure that the query will actually return data, yes? (just making sure ...) — AntonH
– AntonH, Commented Dec 14, 2017 at 22:32
@AntonH Well if it does not return data then $new_name will not be equal to $row[0], $repeating would be set to 0 and the loop would end. That's what I think... — prgrm
– prgrm, Commented Dec 14, 2017 at 22:36
I meant outside of the program. You're sure that the database has the right table and the table contains the right data for this query to return a result? — AntonH
– AntonH, Commented Dec 14, 2017 at 22:37
if the sql doesn't return data, then the second while loop will never even start — beasy
– beasy, Commented Dec 14, 2017 at 22:37
yeah so your SQL satemtent doesn't return any data. so like I said, the second while loop never starts. — beasy
– beasy, Commented Dec 14, 2017 at 22:52

janh · Accepted Answer · 2017-12-14 23:20:24Z

2

The problem is that you won't get any rows back if the name does not exist. The solution is to just check if you do get any rows - otherwise the file name mus be unused. BTW, let DBI escape stuff you sent to the database. This should work:

while ($repeating == 1) {
        $new_name = $i . "_" . $file;
        # the question marks are placeholders
        my $sql= "SELECT file_name FROM PDFdocument WHERE user_id = ? AND file_name = ? ";
        my $sth = $dbh->prepare($sql);
        # filling the placeholders while executing
        $sth->execute($id, $new_name); 
        if(!$sth->fetch) {
        # no rows found? this name must be fresh
              $repeating = 0;
        }
        $i++;
    }

Edit: As @ikegami mentioned in a comment, the behavior of $sth->rows depends on the driver, so it might return different values for different database engines when dealing with SELECT statements (see also the DBI docs. Asking the driver to fetch a row should work the same on all drivers.

Keep in mind that this is susceptible to race conditions, i.e. if two scripts run at the same time, they might both chose the same "unused" filename. Make sure you're using some kind of locking mechanism to avoid that.

edited Dec 14, 2017 at 23:20

answered Dec 14, 2017 at 22:48

janh

3,0402 gold badges25 silver badges24 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

ikegami Over a year ago

$sth->rows is not guaranteed to be meaningful for SELECT statements. Replace if($sth->rows == 0) { $repeating = 0; } with last if !$sth->fetch; $sth->finish;

ikegami · Accepted Answer · 2017-12-14 23:52:29Z

2

When you finally find the $i you should use, $sth->fetchrow_array returns an empty list, so = returns 0, so the loop isn't entered.

Solution 1:

my $new_name;
for (my $i=1; ; ++$i) {
   $new_name = $i . "_" . $file;
   $dbh->selectrow_arrayref(
      "SELECT 1 FROM `PDFdocument` WHERE `user_id` = ? AND `file_name` = ?",
      undef,
      $id, $new_name,
   )
      and last;
}

Solution 2:

my $i = $dbh->selectrow_array(
   "
      SELECT CAST(LEFT(`file_name`, LOCATE("_", `file_name`)-1) AS INT) AS `i`
        FROM `PDFdocument`
       WHERE `user_id` = ?
         AND `file_name` LIKE ?
       ORDER BY DESC `i`
       LIMIT 1
   ",
   undef,
   $id, "%\\_\Q$file\E"
);
++$i;
my $new_name = $i . "_" . $file;

Note the use of placeholders. Your buggy way of building the SQL statement leaves you vulnerable to malfunctions if not attacks.

edited Dec 14, 2017 at 23:52

answered Dec 14, 2017 at 22:55

ikegami

391k17 gold badges291 silver badges555 bronze badges

2 Comments

prgrm Over a year ago

I did this for all my queries although there was not risk for injection attack ; there is no user input. Thanks.

ikegami Over a year ago

The fact that there's no user input doesn't mean there's no bug. You actually have to make sure the string will never have any special characters. Why go through the extra work of making sure of that (and risking that it changes) instead of always using placeholders (which actually make things more readable)?

Collectives™ on Stack Overflow

Perl won't execute SQL statement in a while loop

2 Answers 2

1 Comment

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

2 Comments

Your Answer

Sign up or log in

Post as a guest