Problems with Prepare and Execute of a Query

[gmccammon]

I'm having problems with my Prepare and Execute statements.

I get a syntax error onmy prepare and of course the execute won't run on an undefined value.

I don't know what I am overlooking.

#!/Perl/bin/perl
# Program to read information sent to the server
# from the form in the contact_list.xhtml document.
 
use CGI qw( :standard );
use DBI;
 
$dtd =
"-//W3C//DTD XHTML 1.0 Transitional//EN\"
   \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";
 
print( header() );
print( start_html( { dtd    => $dtd,
                     title  => "Order Response" } ) );
 
# generic variable to hold output message for user, with default success value
my $message = "<b>Order Successful!  <br /> Thanks!</b>";
 
# try updating database
# open connection to Access database
# no passwords on our database
$dbh = DBI->connect('dbi:ODBC:KTC')or die "Database connection failed.";
 
# server-side check of quantity
$requestedQuantity = param( "quantity" );
my $check=quantityValid();
if ( ! $check ) {
   # @TODO add not available message here
  $message = "<b>Order NOT Successful!  <br /> Please Try Again!</b>";
} else {
   $customerID=param("customer_id");
   $productID=param("product_id");
   $quantity=param("quantity");
   $deliveryDate=param("delivery_date");
 
   $query1 = "INSERT INTO Orders (CustomerID, ProductID, OrderQuantity, RequestedShipDate)
                    VALUES ($customerID,$productID,$quantity,$deliveryDate);
   ##                  VALUES (?,?,?,?)";
 
   my $insertHandle = $databaseHandle->prepare_cached($query1) 
   or die "Couldn't prepare insert query; aborting "
      unless defined $insertHandle;
 
   my $success = 1;
   $success &&= $insertHandle->execute($customerID,$productID,$quantity,$deliveryDate)
   or die "Execute failed.";
 
   # autocommit is on, so no need to commit
   # my $result = ($success ? $databaseHandle->commit : $databaseHandle->rollback);
   unless ($success) { 
      die "Couldn't finish transaction: " . $databaseHandle->errstr 
   }
 
   $insertHandle->finish();
}
 
$databaseHandle->disconnect();
 
print( $message );
print( end_html() );
 
 
sub quantityValid {
 
   # use ProductID to find a product name
 
   $productID = param("product_id");
   # @TODO get product ID from web page
 
   $query1 = "SELECT UnitsInStock FROM Products WHERE ProductID = $productID";
 
   #@TODO add query to product table
 
   # @TODO prepare query with existing database handle
   $query_handle = $dbh->prepare($query1);
 
   # @TODO execute query
   $query_handle->execute() or die $dbh->errstr;
 
   # @TODO loop through results put available amount into $availAmount
   while ( @availAmount = $query_handle->fetchrow_array()){
        $availAmount = $availAmount[0];
   }
 
      # @TODO test quantity and return 1 (for true) is ok
   #                         return 0 (for false) if not ok
   if ($requestedQuantity <= $availAmount){
        return 1;
   } else {
        return 0;
}
}
 

Any help wopuld be greatly appreciated.

[miller]

Your error occurs because you created the database handle using the variable name "$dbh", but you atttempt to use the variable name "$databaseHandle" which does not exist.

Ultimately your biggest problem is the fact that you do not have the line "use strict;" included at the top of your file, therefore enforcing that you scope all of your variables. If you had done this it would have been able to throw an error when you attempted to access the $databaseHandle variable which would be undefined.

Before noticing that error through, I went through your script and added comments to all of the lines that I think you should change or at least think about. I hope that this helps, and good luck.

#!/Perl/bin/perl
# Program to read information sent to the server
# from the form in the contact_list.xhtml document.
 
use CGI qw( :standard );
use DBI;
 
#WARNING: "use strict;"  It will help you avoid all sorts of simple typo and spelling errors.  Yes, it will require that you always scope your variables with either "my" or "our", but it is good to be thinking about such things anyway.
 
$dtd =
"-//W3C//DTD XHTML 1.0 Transitional//EN\"
   \"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd";
 
print( header() );
print( start_html( { dtd    => $dtd,
                     title  => "Order Response" } ) );
 
# generic variable to hold output message for user, with default success value
my $message = "<b>Order Successful!  <br /> Thanks!</b>";
 
# try updating database
# open connection to Access database
# no passwords on our database
$dbh = DBI->connect('dbi:ODBC:KTC')or die "Database connection failed.";
 
# server-side check of quantity
$requestedQuantity = param( "quantity" );
my $check=quantityValid();
if ( ! $check ) {
   # @TODO add not available message here
  $message = "<b>Order NOT Successful!  <br /> Please Try Again!</b>";
} else {
   $customerID=param("customer_id");
   $productID=param("product_id");
   $quantity=param("quantity");
   $deliveryDate=param("delivery_date");
 
#WARNING: When INSERTing a single record, it is better to use the SET syntax for the query instead of values as that way you can confirm that the key value pairs are matched properly.
#WARNING: Stick with placeholders.  It appears that you commented out their use, but they take care of a lot of important security and functional concerns dealing with escaping characters, dealing with empty values, and avoiding security vulnerabilities.
   $query1 = "INSERT INTO Orders (CustomerID, ProductID, OrderQuantity, RequestedShipDate)
                    VALUES ($customerID,$productID,$quantity,$deliveryDate);
   ##                  VALUES (?,?,?,?)";
 
#WARNING: your "unless defined" statement is never reached.  In this instance it doesn't do anything wrong since there is no way that $insertHandle is defined in this script.  But if there were an $insertHandle higher in scope, this statement could potentially lead to bad bad things.  Never a statement like this: "my $var = 'value' if $some_condition;"  This leads to really really wierd things in perl.  Instead if you truly mean optional assignment, do "my $var = $some_condition ? 'value' : undef;".  In this instance though, that test always passes so just remove it.
#WARNING: Once your $dbh is created, always include the $dbh->errstr in your "or die" statement.  This will ensure the best changes of debugging hte problem
   my $insertHandle = $databaseHandle->prepare_cached($query1) 
   or die "Couldn't prepare insert query; aborting "
      unless defined $insertHandle;
 
#WARNING 1: Your assignment of $success here is redundant.  Simply assign it the return value of the execute statement directly instead of the unnecessary boolean operator trick.
#WARNING 2: Your assignment of $success is redundant for a second reason.  If $success is assigned a null value, than also means that the die statement would have been called, and so the assignment is meaningless.  Instead simply include the $dbh->errstr directly in your "or die" statement.
   my $success = 1;
   $success &&= $insertHandle->execute($customerID,$productID,$quantity,$delivery  Date)
   or die "Execute failed.";
 
   # autocommit is on, so no need to commit
   # my $result = ($success ? $databaseHandle->commit : $databaseHandle->rollback);
   unless ($success) { 
      die "Couldn't finish transaction: " . $databaseHandle->errstr 
   }
 
   $insertHandle->finish();
}
 
$databaseHandle->disconnect();
 
print( $message );
print( end_html() );
 
 
sub quantityValid {
 
   # use ProductID to find a product name
 
   $productID = param("product_id");
   # @TODO get product ID from web page
 
#ERROR Possible: What if $productID has not value?  The below sql query will likely fail.  Instead use placeholders in this instance.
   $query1 = "SELECT UnitsInStock FROM Products WHERE ProductID = $productID";
 
   #@TODO add query to product table
 
   # @TODO prepare query with existing database handle
   $query_handle = $dbh->prepare($query1);
 
#NOTE: Very good use of "or die $dbh->errstr" here.  You could also add it to the prepare statement, but in truth most errors are through in the execute.
   # @TODO execute query
   $query_handle->execute() or die $dbh->errstr;
 
#WARNING: Is ProductID really a 1 to many relationship for Products?  This use of a while is very suspect.  If there really are more than one record, than there is no predicted which amount will be used for the test.  This is just messy. 
   # @TODO loop through results put available amount into $availAmount
   while ( @availAmount = $query_handle->fetchrow_array()){
        $availAmount = $availAmount[0];
   }
 
#WARNING: you use a global variable $requestedQuantity here.  Any subroutine should be passed all the variables that it requires.  This is just smarter programming style.
      # @TODO test quantity and return 1 (for true) is ok
   #                         return 0 (for false) if not ok
   if ($requestedQuantity <= $availAmount){
        return 1;
   } else {
        return 0;
}
}