From: Peter Rabbitson 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-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=e5372da42c1bb393cd2aadde5765a09d64428581;p=dbsrgits%2FDBIx-Class-Historic.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" );