DBD::Mock bugfixes #1

Closed
wants to merge 7 commits into
from
View
@@ -322,7 +322,8 @@ sub prepare {
if ($dbh->FETCH('AutoCommit')) {
$dbh->STORE('AutoCommit', 0);
$begin_work_commit = 1;
- my $sth = $dbh->prepare( 'BEGIN WORK' );
+ my $sth = $dbh->prepare( 'BEGIN WORK' )
+ or return;
my $rc = $sth->execute();
$sth->finish();
return $rc;
@@ -338,7 +339,8 @@ sub prepare {
return $dbh->set_err(1, "commit ineffective with AutoCommit" );
}
- my $sth = $dbh->prepare( 'COMMIT' );
+ my $sth = $dbh->prepare( 'COMMIT' )
+ or return;
my $rc = $sth->execute();
$sth->finish();
@@ -356,7 +358,8 @@ sub prepare {
return $dbh->set_err(1, "rollback ineffective with AutoCommit" );
}
- my $sth = $dbh->prepare( 'ROLLBACK' );
+ my $sth = $dbh->prepare( 'ROLLBACK' )
+ or return;
my $rc = $sth->execute();
$sth->finish();
@@ -619,9 +622,12 @@ sub execute {
if (my $session = $dbh->{mock_session}) {
eval {
+ my $state = $session->current_state;
$session->verify_bound_params($dbh, $tracker->bound_params());
- my $idx = $session->{state_index} - 1;
- my @results = @{$session->{states}->[$idx]->{results}};
+
+ # Load a copy of the results to return (minus the field
+ # names) into the tracker
+ my @results = @{$state->{results}};
shift @results;
$tracker->{return_data} = \@results;
};
@@ -1190,6 +1196,12 @@ sub name { (shift)->{name} }
sub reset { (shift)->{state_index} = 0 }
sub num_states { scalar( @{ (shift)->{states} } ) }
+sub current_state {
+ my $self = shift;
+ my $idx = $self->{state_index};
+ return $self->{states}[$idx];
+}
+
sub has_states_left {
my $self = shift;
return $self->{state_index} < scalar(@{$self->{states}});
@@ -1201,7 +1213,7 @@ sub verify_statement {
($self->has_states_left)
|| die "Session states exhausted, only '" . scalar(@{$self->{states}}) . "' in DBD::Mock::Session (" . $self->{name} . ")";
- my $current_state = $self->{states}->[$self->{state_index}];
+ my $current_state = $self->current_state;
# make sure our state is good
(exists ${$current_state}{statement} && exists ${$current_state}{results})
|| die "Bad state '" . $self->{state_index} . "' in DBD::Mock::Session (" . $self->{name} . ")";
@@ -1233,7 +1245,11 @@ sub verify_statement {
sub verify_bound_params {
my ($self, $dbh, $params) = @_;
- my $current_state = $self->{states}->[$self->{state_index}];
+
+ ($self->has_states_left)
+ || die "Session states exhausted, only '" . scalar(@{$self->{states}}) . "' in DBD::Mock::Session (" . $self->{name} . ")";
+
+ my $current_state = $self->current_state;
if (exists ${$current_state}{bound_params}) {
my $expected = $current_state->{bound_params};
(scalar(@{$expected}) == scalar(@{$params}))
View
@@ -0,0 +1,127 @@
+#!/usr/bin/perl
+use Test::More tests => 15;
+use strict;
+use warnings;
+use Test::Exception;
+use DBI;
+use DBD::Mock;
+
+# This test is designed to expose the bug found in the DBD::Mock
+# methods begin_work, commit and rollback (RT #66815), where a failing
+# ->prepare invocation (returning nothing) is not detected and the
+# undefined value resulting is used anyway. In this test, as in the
+# example found in the wild, the failure is triggered by exhaustion of
+# the session states.
+#
+# This is a list of sessions designed to engineer the right condition
+# to trigger the bug. They all start with a dummy statement (so that
+# there are at least two states) then the final statement is removed
+# before it is passed to DBD::Mock::Session->new (which requires at
+# least one state). The final statements are 'BEGIN WORK', 'COMMIT'
+# and 'ROLLBACK', respectively.
+#
+# Hence, when the test tries to invoke the final state, the session
+# will have run out and DBD::Mock->verify_statement will cause the
+# prepare method to fail.
+
+my @cases = (
+ 'begin_work' => [
+ {
+ statement => 'SELECT something FROM somewhere',
+ results => [],
+ },
+ {
+ statement => 'BEGIN WORK',
+ results => [],
+ },
+ ],
+
+ 'commit' => [
+ {
+ statement => 'SELECT something FROM somewhere',
+ results => [],
+ },
+ {
+ statement => 'BEGIN WORK',
+ results => [],
+ },
+ {
+ statement => 'INSERT INTO foo (bar) VALUES (?);',
+ results => [],
+ bound_params => [1],
+ },
+ {
+ statement => 'COMMIT',
+ results => [],
+ },
+ ],
+
+ 'rollback' => [
+ {
+ statement => 'SELECT something FROM somewhere',
+ results => [],
+ },
+ {
+ statement => 'BEGIN WORK',
+ results => [],
+ },
+ {
+ statement => 'INSERT INTO foo (bar) VALUES (?);',
+ results => [],
+ bound_params => [1],
+ },
+ {
+ statement => 'ROLLBACK',
+ results => [],
+ },
+ ],
+);
+
+while(@cases) {
+ my ($name, $states) = splice @cases, 0, 2;
+ my $case_name = "case $name";
+
+ my $dbh = DBI->connect('dbi:Mock:', '', '',
+ { PrintError => 0,
+ RaiseError => 1 });
+
+ # Add all but the last state of the expected session
+ my $missing_state = pop @$states;
+ my $num_states = @$states;
+ $dbh->{mock_session} = DBD::Mock::Session->new($name => @$states);
+
+ # Execute the initial dummy statement.
+ my $state = $states->[0];
+ my $sth = $dbh->prepare($state->{statement});
+ ok $sth,
+ "$case_name: prepare statement";
+
+ ok $sth->execute(),
+ "$case_name: execute statement";
+
+ # Now try and do the next steps in @session, but using the
+ # appropriate transaction methods directly. This should fail when
+ # the session is exhausted with a useful message. (The original
+ # bug meant that the message got clobbered by "Can't call method
+ # 'execute' on an undefined value".)
+ throws_ok {
+ # This statement is always the same.
+ ok $dbh->begin_work,
+ "$case_name: start transaction";
+
+ my $state = $states->[2];
+ my $sth = $dbh->prepare($state->{statement});
+ ok $sth,
+ "$case_name: prepare statement";
+
+ ok $sth->execute(@{$state->{bound_params}}),
+ "$case_name: execute statement";
+
+ # get the final operation from the session
+ my $operation = lc $missing_state->{statement};
+
+ ok $dbh->$operation,
+ "$case_name: $operation transaction";
+ } qr/\QSession states exhausted, only '$num_states' in DBD::Mock::Session\E/;
+
+}
View
@@ -0,0 +1,45 @@
+#!/usr/bin/perl
+use Test::More tests => 3;
+use strict;
+use warnings;
+use Test::Exception;
+use DBI;
+use DBD::Mock;
+
+# This tests that spurious extra ->execute invocations fail with a
+# useful message. This is because there was a bug in which
+# DBD::Mock->verify_bound_params didn't check that the session had run
+# out, and on return out-of-bounds element of the state array is
+# accessed, causing an unhelpful error "Can't use an undefined value
+# as an ARRAY reference at ../lib/DBD/Mock.pm line 635."
+
+my @session = (
+ {
+ 'statement' => 'INSERT INTO foo (bar) values (?);',
+ 'results' => [],
+ 'bound_params' => [1]
+ },
+);
+
+my $dbh = DBI->connect('dbi:Mock:', '', '', { PrintError => 0, RaiseError => 1});
+
+# Add just part of the expected session, such that the next step would be a 'BEGIN WORK'
+$dbh->{mock_session} = DBD::Mock::Session->new(@session);
+
+# now execute the steps in the session
+my $step = $session[0];
+
+my $sth = $dbh->prepare($step->{statement});
+ok $sth,
+ "prepare statement";
+
+my $params = $step->{bound_params} || [];
+ok $sth->execute(@$params),
+ "execute statement";
+
+# Session expects that to be all. So let's surprise it with another
+# ->execute. It should fail appropriately.
+throws_ok {
+ ok $sth->execute(@$params),
+} qr/\QSession states exhausted, only '1' in DBD::Mock::Session\E/,
+ "fails on executing one too many times";