Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

DBD::Mock bugfixes #1

Closed
wants to merge 7 commits into from

2 participants

wu-lee Gines R
wu-lee
wu-lee commented June 30, 2011

Here's a request to pull my fixes - I hope the commit messages are mostly self explanatory. If not, please feel welcome to ask.

The original issue was RT #66815 (https://rt.cpan.org/Public/Bug/Display.html?id=66815), but I found some others and fixed those too.

N

added some commits March 25, 2011
Fix bug RT#66815 - check statement handles are valid before using them
$dbh->prepare does not guarantee to return a valid statement handle.
It may return a false value.  So check the returned value, and abort
if it is false, otherwise the error caused ->prepare to fail will get
overwritten by a less helpful one which says "Can't call method
'execute' on an undefined value" error.
6d8932e
A test showing DBD::Mock->begin_work, commit and rollback don't detec…
…t ->prepare failure

This is reported in RT ticket #66815

Basically any call to ->begin_work, ->commit, >rollback made when
their internal call to ->prepare returns an invalid statement handle
will not notice and proceed to use it anyway.

In particular, this happens when a DBD::Mock::Session is in effect,
and the session state list has been exhausted.  When this happens, the
method in question fails with an unhelpful error "Can't use an
undefined value as an ARRAY reference at ../lib/DBD/Mock.pm line
635", masking the real reason.
2c12ab9
Merge branch 'rt66815'
* rt66815:
  Fix bug RT#66815 - check statement handles are valid before using them
  A test showing DBD::Mock->begin_work, commit and rollback don't detect ->prepare failure
4dcccc7
Add DBD::Mock::Session->current_state method
This just makes the code simpler to follow. Particularly the portion
in validate_bound_params where results are loaded into the $tracker
object.
23af0c7
A test showing DBD::Mock::Session->verify_bound_params doesn't check …
…for session exhaustion

Basically it succeeds despite there being no states left to verify,
and on return the ->execute method fails with an unhelpful error
"Can't use an undefined value # as an ARRAY reference at
../lib/DBD/Mock.pm line 635."
3a1d41b
Fix DBD::Mock::Session->verify_bound_params to check for state exhaus…
…tion
179e83a
Merge branch 'fix-verify-bound-params'
* fix-verify-bound-params:
  Fix DBD::Mock::Session->verify_bound_params to check for state exhaustion
  A test showing DBD::Mock::Session->verify_bound_params doesn't check for session exhaustion
  Add DBD::Mock::Session->current_state method
09b2b02
Gines R
Collaborator

Fixing in a separate branch to merge with master

Gines R ginesr closed this October 09, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 7 unique commits by 1 author.

Mar 30, 2011
Fix bug RT#66815 - check statement handles are valid before using them
$dbh->prepare does not guarantee to return a valid statement handle.
It may return a false value.  So check the returned value, and abort
if it is false, otherwise the error caused ->prepare to fail will get
overwritten by a less helpful one which says "Can't call method
'execute' on an undefined value" error.
6d8932e
A test showing DBD::Mock->begin_work, commit and rollback don't detec…
…t ->prepare failure

This is reported in RT ticket #66815

Basically any call to ->begin_work, ->commit, >rollback made when
their internal call to ->prepare returns an invalid statement handle
will not notice and proceed to use it anyway.

In particular, this happens when a DBD::Mock::Session is in effect,
and the session state list has been exhausted.  When this happens, the
method in question fails with an unhelpful error "Can't use an
undefined value as an ARRAY reference at ../lib/DBD/Mock.pm line
635", masking the real reason.
2c12ab9
Merge branch 'rt66815'
* rt66815:
  Fix bug RT#66815 - check statement handles are valid before using them
  A test showing DBD::Mock->begin_work, commit and rollback don't detect ->prepare failure
4dcccc7
Add DBD::Mock::Session->current_state method
This just makes the code simpler to follow. Particularly the portion
in validate_bound_params where results are loaded into the $tracker
object.
23af0c7
A test showing DBD::Mock::Session->verify_bound_params doesn't check …
…for session exhaustion

Basically it succeeds despite there being no states left to verify,
and on return the ->execute method fails with an unhelpful error
"Can't use an undefined value # as an ARRAY reference at
../lib/DBD/Mock.pm line 635."
3a1d41b
Fix DBD::Mock::Session->verify_bound_params to check for state exhaus…
…tion
179e83a
Merge branch 'fix-verify-bound-params'
* fix-verify-bound-params:
  Fix DBD::Mock::Session->verify_bound_params to check for state exhaustion
  A test showing DBD::Mock::Session->verify_bound_params doesn't check for session exhaustion
  Add DBD::Mock::Session->current_state method
09b2b02
This page is out of date. Refresh to see the latest.
30  lib/DBD/Mock.pm
@@ -322,7 +322,8 @@ sub prepare {
322 322
         if ($dbh->FETCH('AutoCommit')) {
323 323
             $dbh->STORE('AutoCommit', 0);
324 324
             $begin_work_commit = 1;
325  
-            my $sth = $dbh->prepare( 'BEGIN WORK' );
  325
+            my $sth = $dbh->prepare( 'BEGIN WORK' )
  326
+                or return;
326 327
             my $rc = $sth->execute();
327 328
             $sth->finish();
328 329
             return $rc;
@@ -338,7 +339,8 @@ sub prepare {
338 339
             return $dbh->set_err(1, "commit ineffective with AutoCommit" );
339 340
         }
340 341
 
341  
-        my $sth = $dbh->prepare( 'COMMIT' );
  342
+        my $sth = $dbh->prepare( 'COMMIT' )
  343
+            or return;
342 344
         my $rc = $sth->execute();
343 345
         $sth->finish();
344 346
 
@@ -356,7 +358,8 @@ sub prepare {
356 358
             return $dbh->set_err(1, "rollback ineffective with AutoCommit" );
357 359
         }
358 360
 
359  
-        my $sth = $dbh->prepare( 'ROLLBACK' );
  361
+        my $sth = $dbh->prepare( 'ROLLBACK' )
  362
+            or return;
360 363
         my $rc = $sth->execute();
361 364
         $sth->finish();
362 365
 
@@ -619,9 +622,12 @@ sub execute {
619 622
 
620 623
     if (my $session = $dbh->{mock_session}) {
621 624
         eval {
  625
+            my $state = $session->current_state;
622 626
             $session->verify_bound_params($dbh, $tracker->bound_params());
623  
-            my $idx = $session->{state_index} - 1;
624  
-            my @results = @{$session->{states}->[$idx]->{results}};
  627
+            
  628
+            # Load a copy of the results to return (minus the field
  629
+            # names) into the tracker
  630
+            my @results = @{$state->{results}};
625 631
             shift @results;
626 632
             $tracker->{return_data} = \@results;
627 633
         };
@@ -1190,6 +1196,12 @@ sub name  { (shift)->{name} }
1190 1196
 sub reset { (shift)->{state_index} = 0 }
1191 1197
 sub num_states { scalar( @{ (shift)->{states} } ) }
1192 1198
 
  1199
+sub current_state {
  1200
+    my $self = shift;
  1201
+    my $idx = $self->{state_index};
  1202
+    return $self->{states}[$idx];
  1203
+}
  1204
+
1193 1205
 sub has_states_left {
1194 1206
     my $self = shift;
1195 1207
     return $self->{state_index} < scalar(@{$self->{states}});
@@ -1201,7 +1213,7 @@ sub verify_statement {
1201 1213
     ($self->has_states_left)
1202 1214
         || die "Session states exhausted, only '" . scalar(@{$self->{states}}) . "' in DBD::Mock::Session (" . $self->{name} . ")";
1203 1215
 
1204  
-    my $current_state = $self->{states}->[$self->{state_index}];
  1216
+    my $current_state = $self->current_state;
1205 1217
     # make sure our state is good
1206 1218
     (exists ${$current_state}{statement} && exists ${$current_state}{results})
1207 1219
         || die "Bad state '" . $self->{state_index} .  "' in DBD::Mock::Session (" . $self->{name} . ")";
@@ -1233,7 +1245,11 @@ sub verify_statement {
1233 1245
 
1234 1246
 sub verify_bound_params {
1235 1247
     my ($self, $dbh, $params) = @_;
1236  
-    my $current_state = $self->{states}->[$self->{state_index}];
  1248
+
  1249
+    ($self->has_states_left)
  1250
+        || die "Session states exhausted, only '" . scalar(@{$self->{states}}) . "' in DBD::Mock::Session (" . $self->{name} . ")";
  1251
+
  1252
+    my $current_state = $self->current_state;
1237 1253
     if (exists ${$current_state}{bound_params}) {
1238 1254
         my $expected = $current_state->{bound_params};
1239 1255
         (scalar(@{$expected}) == scalar(@{$params}))
127  t/bug_0002.t
... ...
@@ -0,0 +1,127 @@
  1
+#!/usr/bin/perl
  2
+use Test::More tests => 15; 
  3
+use strict;
  4
+use warnings;
  5
+use Test::Exception;
  6
+use DBI;
  7
+use DBD::Mock;
  8
+
  9
+# This test is designed to expose the bug found in the DBD::Mock
  10
+# methods begin_work, commit and rollback (RT #66815), where a failing
  11
+# ->prepare invocation (returning nothing) is not detected and the
  12
+# undefined value resulting is used anyway. In this test, as in the
  13
+# example found in the wild, the failure is triggered by exhaustion of
  14
+# the session states.
  15
+#
  16
+# This is a list of sessions designed to engineer the right condition
  17
+# to trigger the bug.  They all start with a dummy statement (so that
  18
+# there are at least two states) then the final statement is removed
  19
+# before it is passed to DBD::Mock::Session->new (which requires at
  20
+# least one state).  The final statements are 'BEGIN WORK', 'COMMIT'
  21
+# and 'ROLLBACK', respectively.
  22
+#
  23
+# Hence, when the test tries to invoke the final state, the session
  24
+# will have run out and DBD::Mock->verify_statement will cause the
  25
+# prepare method to fail.
  26
+
  27
+my @cases = (
  28
+    'begin_work' => [
  29
+        {
  30
+            statement => 'SELECT something FROM somewhere',
  31
+            results => [],
  32
+        },
  33
+        {
  34
+            statement => 'BEGIN WORK',
  35
+            results => [],
  36
+        },
  37
+    ],
  38
+
  39
+    'commit' => [
  40
+        {
  41
+            statement => 'SELECT something FROM somewhere',
  42
+            results => [],
  43
+        },
  44
+        {
  45
+            statement => 'BEGIN WORK',
  46
+            results => [],
  47
+        },
  48
+        {
  49
+            statement => 'INSERT INTO foo (bar) VALUES (?);',
  50
+            results => [],
  51
+            bound_params => [1],
  52
+        },
  53
+        {
  54
+            statement => 'COMMIT',
  55
+            results => [],
  56
+        },
  57
+    ],
  58
+
  59
+    'rollback' => [
  60
+        {
  61
+            statement => 'SELECT something FROM somewhere',
  62
+            results => [],
  63
+        },
  64
+        {
  65
+            statement => 'BEGIN WORK',
  66
+            results => [],
  67
+        },
  68
+        {
  69
+            statement => 'INSERT INTO foo (bar) VALUES (?);',
  70
+            results => [],
  71
+            bound_params => [1],
  72
+        },
  73
+        {
  74
+            statement => 'ROLLBACK',
  75
+            results => [],
  76
+        },
  77
+    ],
  78
+);
  79
+
  80
+while(@cases) {
  81
+    my ($name, $states) = splice @cases, 0, 2;
  82
+    my $case_name = "case $name";
  83
+
  84
+    my $dbh = DBI->connect('dbi:Mock:', '',  '', 
  85
+                           { PrintError => 0, 
  86
+                             RaiseError => 1 });
  87
+
  88
+    # Add all but the last state of the expected session
  89
+    my $missing_state = pop @$states;
  90
+    my $num_states = @$states;
  91
+    $dbh->{mock_session} = DBD::Mock::Session->new($name => @$states);
  92
+
  93
+    # Execute the initial dummy statement.
  94
+    my $state = $states->[0];
  95
+    my $sth = $dbh->prepare($state->{statement});
  96
+    ok $sth, 
  97
+        "$case_name: prepare statement";
  98
+        
  99
+    ok $sth->execute(),
  100
+        "$case_name: execute statement";
  101
+    
  102
+    # Now try and do the next steps in @session, but using the
  103
+    # appropriate transaction methods directly.  This should fail when
  104
+    # the session is exhausted with a useful message. (The original
  105
+    # bug meant that the message got clobbered by "Can't call method
  106
+    # 'execute' on an undefined value".)
  107
+    throws_ok {
  108
+        # This statement is always the same.
  109
+        ok $dbh->begin_work,
  110
+            "$case_name: start transaction";
  111
+
  112
+        my $state = $states->[2];
  113
+        my $sth = $dbh->prepare($state->{statement});
  114
+        ok $sth, 
  115
+            "$case_name: prepare statement";
  116
+        
  117
+        ok $sth->execute(@{$state->{bound_params}}),
  118
+            "$case_name: execute statement";
  119
+
  120
+        # get the final operation from the session
  121
+        my $operation = lc $missing_state->{statement};
  122
+
  123
+        ok $dbh->$operation,
  124
+            "$case_name: $operation transaction";        
  125
+    } qr/\QSession states exhausted, only '$num_states' in DBD::Mock::Session\E/;
  126
+
  127
+}
45  t/bug_0003.t
... ...
@@ -0,0 +1,45 @@
  1
+#!/usr/bin/perl
  2
+use Test::More tests => 3; 
  3
+use strict;
  4
+use warnings;
  5
+use Test::Exception;
  6
+use DBI;
  7
+use DBD::Mock;
  8
+
  9
+# This tests that spurious extra ->execute invocations fail with a
  10
+# useful message. This is because there was a bug in which
  11
+# DBD::Mock->verify_bound_params didn't check that the session had run
  12
+# out, and on return out-of-bounds element of the state array is
  13
+# accessed, causing an unhelpful error "Can't use an undefined value
  14
+# as an ARRAY reference at ../lib/DBD/Mock.pm line 635."
  15
+
  16
+my @session = (
  17
+    {
  18
+        'statement' => 'INSERT INTO foo (bar) values (?);',
  19
+        'results' => [],
  20
+        'bound_params' => [1]
  21
+    },
  22
+);
  23
+
  24
+my $dbh = DBI->connect('dbi:Mock:', '',  '', { PrintError => 0, RaiseError => 1});
  25
+
  26
+# Add just part of the expected session, such that the next step would be a 'BEGIN WORK'
  27
+$dbh->{mock_session} = DBD::Mock::Session->new(@session);
  28
+
  29
+# now execute the steps in the session
  30
+my $step = $session[0];
  31
+
  32
+my $sth = $dbh->prepare($step->{statement});
  33
+ok $sth, 
  34
+    "prepare statement";
  35
+
  36
+my $params = $step->{bound_params} || [];
  37
+ok $sth->execute(@$params),
  38
+    "execute statement";
  39
+
  40
+# Session expects that to be all.  So let's surprise it with another
  41
+# ->execute.  It should fail appropriately.
  42
+throws_ok {
  43
+    ok $sth->execute(@$params),
  44
+} qr/\QSession states exhausted, only '1' in DBD::Mock::Session\E/,
  45
+    "fails on executing one too many times";
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.