From: Peter Rabbitson <ribasushi@cpan.org>
Date: Wed, 24 Apr 2013 02:47:32 +0000 (+0200)
Subject: Fix very subtle but deadly bug in single()
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=1887cd9f0db6e10e4659fa770b064e2bf25bc0b3;p=dbsrgits%2FDBIx-Class-Historic.git

Fix very subtle but deadly bug in single()

The _select_args result storage introduced in 975b573a did not take
into account that single() operates on the $rs itself, not a fresh
one. As such the first single() arg would be carried around and then
applied to subsequent next() etc iterations. Stop that, as we only
need the attrs in case of collapse => 1 and single() explicitly does
not support this.

While at it do not reuse the cached args at all - while it *appears*
safe, it may very well not be. Leave this for another day
---

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 9acc4da..3aa5d23 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -1082,7 +1082,7 @@ sub single {
     $attrs->{from}, $attrs->{select},
     $attrs->{where}, $attrs
   )];
-  $self->{_attrs}{_sqlmaker_select_args} = $attrs->{_sqlmaker_select_args};
+
   return undef unless @$data;
   $self->{_stashed_rows} = [ $data ];
   $self->_construct_results->[0];
diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm
index 5a39029..8ff9868 100644
--- a/lib/DBIx/Class/Storage/DBI.pm
+++ b/lib/DBIx/Class/Storage/DBI.pm
@@ -2330,9 +2330,16 @@ sub _select_args_to_query {
 sub _select_args {
   my ($self, $ident, $select, $where, $orig_attrs) = @_;
 
-  return (
-    'select', @{$orig_attrs->{_sqlmaker_select_args}}
-  ) if $orig_attrs->{_sqlmaker_select_args};
+  # FIXME - that kind of caching would be nice to have
+  # however currently we *may* pass the same $orig_attrs
+  # with different ident/select/where
+  # the whole interface needs to be rethought, since it
+  # was centered around the flawed SQLA API. We can do
+  # soooooo much better now. But that is also another
+  # battle...
+  #return (
+  #  'select', @{$orig_attrs->{_sqlmaker_select_args}}
+  #) if $orig_attrs->{_sqlmaker_select_args};
 
   my $sql_maker = $self->sql_maker;
   my $alias2source = $self->_resolve_ident_sources ($ident);
diff --git a/t/search/preserve_original_rs.t b/t/search/preserve_original_rs.t
index 8896b48..15fdb8d 100644
--- a/t/search/preserve_original_rs.t
+++ b/t/search/preserve_original_rs.t
@@ -86,4 +86,17 @@ for my $s (qw/a2a artw cd artw_back/) {
   is_same_sql_bind ($rs->as_query, $q{$s}{query}, "$s resultset unmodified (as_query matches)" );
 }
 
+# ensure nothing pollutes the attrs of an existing rs
+{
+  my $fresh = $schema->resultset('CD');
+
+  isa_ok ($fresh->find(1), 'DBICTest::CD' );
+  isa_ok ($fresh->single({ cdid => 1}), 'DBICTest::CD' );
+  isa_ok ($fresh->search({ cdid => 1})->next, 'DBICTest::CD' );
+  is ($fresh->count({ cdid => 1}), 1 );
+  is ($fresh->count_rs({ cdid => 1})->next, 1 );
+
+  ok (! exists $fresh->{_attrs}{_sqlmaker_select_args}, 'select args did not leak through' );
+}
+
 done_testing;
diff --git a/t/search/reentrancy.t b/t/search/reentrancy.t
new file mode 100644
index 0000000..8790603
--- /dev/null
+++ b/t/search/reentrancy.t
@@ -0,0 +1,50 @@
+use strict;
+use warnings;
+
+use Test::More;
+
+use lib qw(t/lib);
+use DBICTest;
+
+my $schema = DBICTest->init_schema();
+
+my $track_titles = { map { @$_ }
+  $schema->resultset('Track')
+          ->search({}, { columns => [qw(trackid title)] })
+           ->cursor
+            ->all
+};
+
+my $rs = $schema->resultset('Track');
+
+for my $pass (1,2,3) {
+  for my $meth (qw(search single find)) {
+
+    my $id = (keys %$track_titles)[0];
+    my $tit = delete $track_titles->{$id};
+
+    my ($o) = $rs->$meth({ trackid => $id });
+
+    is(
+      $rs->count({ trackid => $id }),
+      1,
+      "Count works (pass $pass)",
+    );
+
+    is(
+      $o->title,
+      $tit,
+      "Correct object retrieved via $meth() (pass $pass)"
+    );
+
+    $o->delete;
+
+    is(
+      $rs->count_rs({ trackid => $id })->next,
+      0,
+      "Count_rs works (pass $pass)",
+    );
+  }
+}
+
+done_testing;