From: Peter Rabbitson <ribasushi@cpan.org>
Date: Fri, 27 Aug 2010 14:29:30 +0000 (+0200)
Subject: Fix RT#58554 (make sure FOR X is always the last part of a select)
X-Git-Tag: v0.08124~99
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=e5372da42c1bb393cd2aadde5765a09d64428581;p=dbsrgits%2FDBIx-Class.git

Fix RT#58554 (make sure FOR X is always the last part of a select)
---

diff --git a/Changes b/Changes
index 7683396..e0ba08f 100644
--- a/Changes
+++ b/Changes
@@ -21,6 +21,7 @@ Revision history for DBIx::Class
         - Fixed ::Schema::Versioned to work properly with quoting on
           (RT#59619)
         - Fixed t/54taint fails under local-lib
+        - Fixed SELECT ... FOR UPDATE with LIMIT regression (RT#58554)
 
     * Misc
         - Refactored capability handling in Storage::DBI, allows for
diff --git a/lib/DBIx/Class/SQLMaker.pm b/lib/DBIx/Class/SQLMaker.pm
index d0fbc77..f041ce6 100644
--- a/lib/DBIx/Class/SQLMaker.pm
+++ b/lib/DBIx/Class/SQLMaker.pm
@@ -121,6 +121,9 @@ sub select {
 # this *must* be called, otherwise extra binds will remain in the sql-maker
   my @all_bind = $self->_assemble_binds;
 
+  $sql .= $self->_lock_select ($rs_attrs->{for})
+    if $rs_attrs->{for};
+
   return wantarray ? ($sql, @all_bind) : $sql;
 }
 
@@ -129,6 +132,16 @@ sub _assemble_binds {
   return map { @{ (delete $self->{"${_}_bind"}) || [] } } (qw/from where having order/);
 }
 
+my $for_syntax = {
+  update => 'FOR UPDATE',
+  shared => 'FOR SHARE',
+};
+sub _lock_select {
+  my ($self, $type) = @_;
+  my $sql = $for_syntax->{$type} || croak "Unknown SELECT .. FOR type '$type' requested";
+  return " $sql";
+}
+
 # Handle default inserts
 sub insert {
 # optimized due to hotttnesss
@@ -198,10 +211,6 @@ sub _recurse_fields {
   }
 }
 
-my $for_syntax = {
-  update => 'FOR UPDATE',
-  shared => 'FOR SHARE',
-};
 
 # this used to be a part of _order_by but is broken out for clarity.
 # What we have been doing forever is hijacking the $order arg of
@@ -230,10 +239,6 @@ sub _parse_rs_attrs {
     $sql .= $self->_order_by ($arg->{order_by});
   }
 
-  if (my $for = $arg->{for}) {
-    $sql .= " $for_syntax->{$for}" if $for_syntax->{$for};
-  }
-
   return $sql;
 }
 
diff --git a/lib/DBIx/Class/SQLMaker/LimitDialects.pm b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
index d4689a7..e189e2c 100644
--- a/lib/DBIx/Class/SQLMaker/LimitDialects.pm
+++ b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
@@ -590,7 +590,6 @@ EOS
 # also return a hashref (order doesn't matter) of QUOTED EXTRA-SEL =>
 # QUOTED ALIAS pairs, which is a list of extra selectors that do *not*
 # exist in the original select list
-
 sub _subqueried_limit_attrs {
   my ($self, $rs_attrs) = @_;
 
@@ -610,7 +609,6 @@ sub _subqueried_limit_attrs {
     my $sql_sel = $self->_recurse_fields ($s);
     my $sql_alias = (ref $s) eq 'HASH' ? $s->{-as} : undef;
 
-
     push @sel, {
       sql => $sql_sel,
       unquoted_sql => do { local $self->{quote_char}; $self->_recurse_fields ($s) },
diff --git a/lib/DBIx/Class/SQLMaker/SQLite.pm b/lib/DBIx/Class/SQLMaker/SQLite.pm
index 24b26ee..b3d6d2c 100644
--- a/lib/DBIx/Class/SQLMaker/SQLite.pm
+++ b/lib/DBIx/Class/SQLMaker/SQLite.pm
@@ -7,15 +7,6 @@ use Carp::Clan qw/^DBIx::Class|^SQL::Abstract/;
 #
 # SQLite does not understand SELECT ... FOR UPDATE
 # Disable it here
-#
-sub _parse_rs_attrs {
-  my ($self, $attrs) = @_;
-
-  return $self->SUPER::_parse_rs_attrs ($attrs)
-    if ref $attrs ne 'HASH';
-
-  local $attrs->{for};
-  return $self->SUPER::_parse_rs_attrs ($attrs);
-}
+sub _lock_select { '' };
 
 1;
diff --git a/t/71mysql.t b/t/71mysql.t
index 0526dd8..f935afd 100644
--- a/t/71mysql.t
+++ b/t/71mysql.t
@@ -75,6 +75,16 @@ $it->next;
 $it->next;
 is( $it->next, undef, "next past end of resultset ok" );
 
+# Limit with select-lock
+lives_ok {
+  $schema->txn_do (sub {
+    isa_ok (
+      $schema->resultset('Artist')->find({artistid => 1}, {for => 'update', rows => 1}),
+      'DBICTest::Schema::Artist',
+    );
+  });
+} 'Limited FOR UPDATE select works';
+
 my $test_type_info = {
     'artistid' => {
         'data_type' => 'INT',
diff --git a/t/72pg.t b/t/72pg.t
index a981fc8..10ef8b2 100644
--- a/t/72pg.t
+++ b/t/72pg.t
@@ -72,6 +72,16 @@ DBICTest::Schema->load_classes( map {s/.+:://;$_} @test_classes ) if @test_class
   $it->next;
   $it->next;
   is( $it->next, undef, "next past end of resultset ok" );
+
+  # Limit with select-lock
+  lives_ok {
+    $schema->txn_do (sub {
+      isa_ok (
+        $schema->resultset('Artist')->find({artistid => 1}, {for => 'update', rows => 1}),
+        'DBICTest::Schema::Artist',
+      );
+    });
+  } 'Limited FOR UPDATE select works';
 }
 
 # check if we indeed do support stuff
diff --git a/t/745db2.t b/t/745db2.t
index bd931d2..c323529 100644
--- a/t/745db2.t
+++ b/t/745db2.t
@@ -66,6 +66,19 @@ my $lim = $ars->search( {},
 is( $lim->count, 2, 'ROWS+OFFSET count ok' );
 is( $lim->all, 2, 'Number of ->all objects matches count' );
 
+# Limit with select-lock
+TODO: {
+  local $TODO = "Seems we can't SELECT ... FOR ... on subqueries";
+  lives_ok {
+    $schema->txn_do (sub {
+      isa_ok (
+        $schema->resultset('Artist')->find({artistid => 1}, {for => 'update', rows => 1}),
+        'DBICTest::Schema::Artist',
+      );
+    });
+  } 'Limited FOR UPDATE select works';
+}
+
 # test iterator
 $lim->reset;
 is( $lim->next->artistid, 101, "iterator->next ok" );