From: Peter Rabbitson Date: Thu, 7 May 2009 17:28:29 +0000 (+0000) Subject: Merge 'trunk' into 'count_distinct' X-Git-Tag: v0.08103~101^2~8 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=038af71cf6f6c77cb1380378d4887a7da03fe2b2;hp=-c;p=dbsrgits%2FDBIx-Class.git Merge 'trunk' into 'count_distinct' r6164@Thesaurus (orig r6163): ribasushi | 2009-05-07 19:09:01 +0200 r6115@Thesaurus (orig r6114): plu | 2009-05-03 10:39:16 +0200 new branch to fix $rs->update and $rs->delete using the new as_query method r6116@Thesaurus (orig r6115): plu | 2009-05-03 10:52:07 +0200 Methods update/delete on resultset use now new as_query method to updated/delete properly on joined/prefetched resultset using a subquery. Therefore some tests have been added and some have been changed as well as the warnings around $rs->update/delete have been removed. Cheers! r6117@Thesaurus (orig r6116): plu | 2009-05-03 11:13:48 +0200 Using "is" instead of "cmp_ok" r6160@Thesaurus (orig r6159): ribasushi | 2009-05-07 11:58:14 +0200 Back out skip_parens support in as_query r6161@Thesaurus (orig r6160): ribasushi | 2009-05-07 19:00:48 +0200 This test is completely borked, needs a rewrite r6162@Thesaurus (orig r6161): ribasushi | 2009-05-07 19:07:19 +0200 Temporary fix or the IN ( ( ... ) ) problem until we get proper SQLA AST (needs SQLA released with commit 6158 to work) r6165@Thesaurus (orig r6164): ribasushi | 2009-05-07 19:11:46 +0200 Changes, remove merged branch r6169@Thesaurus (orig r6168): ribasushi | 2009-05-07 19:24:54 +0200 Bump SQLA dependency so -in/-between workarounds overload properly --- 038af71cf6f6c77cb1380378d4887a7da03fe2b2 diff --combined lib/DBIx/Class/ResultSet.pm index aa6fa59,e231107..7611945 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@@ -307,7 -307,7 +307,7 @@@ sub search_rs my $new_attrs = { %{$our_attrs}, %{$attrs} }; # merge new attrs into inherited - foreach my $key (qw/join prefetch +select +as/) { + foreach my $key (qw/join prefetch +select +as bind/) { next unless exists $attrs->{$key}; $new_attrs->{$key} = $self->_merge_attr($our_attrs->{$key}, $attrs->{$key}); } @@@ -1151,6 -1151,12 +1151,6 @@@ Performs an SQL C with the same with to find the number of elements. If passed arguments, does a search on the resultset and counts the results of that. -Note: When using C with C, L emulates C -using C. Some databases (notably SQLite) do -not support C with multiple columns. If you are using such a -database, you should only use columns from the main table in your C -clause. - =cut sub count { @@@ -1171,21 -1177,32 +1171,21 @@@ sub _count { # Separated out so pager can get the full count my $self = shift; - my $select = { count => '*' }; - my $attrs = { %{$self->_resolved_attrs} }; - if (my $group_by = delete $attrs->{group_by}) { - delete $attrs->{having}; - my @distinct = (ref $group_by ? @$group_by : ($group_by)); - # todo: try CONCAT for multi-column pk - my @pk = $self->result_source->primary_columns; - if (@pk == 1) { - my $alias = $attrs->{alias}; - foreach my $column (@distinct) { - if ($column =~ qr/^(?:\Q${alias}.\E)?$pk[0]$/) { - @distinct = ($column); - last; - } - } - } - $select = { count => { distinct => \@distinct } }; + if (my $group_by = $attrs->{group_by}) { + delete $attrs->{order_by}; + + $attrs->{select} = $group_by; + $attrs->{from} = [ { 'mesub' => (ref $self)->new($self->result_source, $attrs)->cursor->as_query } ]; + delete $attrs->{where}; } - $attrs->{select} = $select; + $attrs->{select} = { count => '*' }; $attrs->{as} = [qw/count/]; - # offset, order by and page are not needed to count. record_filter is cdbi - delete $attrs->{$_} for qw/rows offset order_by page pager record_filter/; + # offset, order by, group by, where and page are not needed to count. record_filter is cdbi + delete $attrs->{$_} for qw/rows offset order_by group_by page pager record_filter/; my $tmp_rs = (ref $self)->new($self->result_source, $attrs); my ($count) = $tmp_rs->cursor->next; @@@ -1315,49 -1332,8 +1315,8 @@@ sub _cond_for_update_delete # No-op. No condition, we're updating/deleting everything return $cond unless ref $full_cond; - if (ref $full_cond eq 'ARRAY') { - $cond = [ - map { - my %hash; - foreach my $key (keys %{$_}) { - $key =~ /([^.]+)$/; - $hash{$1} = $_->{$key}; - } - \%hash; - } @{$full_cond} - ]; - } - elsif (ref $full_cond eq 'HASH') { - if ((keys %{$full_cond})[0] eq '-and') { - $cond->{-and} = []; - - my @cond = @{$full_cond->{-and}}; - for (my $i = 0; $i < @cond; $i++) { - my $entry = $cond[$i]; - - my $hash; - if (ref $entry eq 'HASH') { - $hash = $self->_cond_for_update_delete($entry); - } - else { - $entry =~ /([^.]+)$/; - $hash->{$1} = $cond[++$i]; - } - - push @{$cond->{-and}}, $hash; - } - } - else { - foreach my $key (keys %{$full_cond}) { - $key =~ /([^.]+)$/; - $cond->{$1} = $full_cond->{$key}; - } - } - } - else { - $self->throw_exception( - "Can't update/delete on resultset with condition unless hash or array" - ); + foreach my $pk ($self->result_source->primary_columns) { + $cond->{$pk} = { -in => $self->get_column($pk)->as_query }; } return $cond; @@@ -1385,13 -1361,8 +1344,8 @@@ sub update $self->throw_exception("Values for update must be a hash") unless ref $values eq 'HASH'; - carp( 'WARNING! Currently $rs->update() does not generate proper SQL' - . ' on joined resultsets, and may affect rows well outside of the' - . ' contents of $rs. Use at your own risk' ) - if ( $self->{attrs}{seen_join} ); - my $cond = $self->_cond_for_update_delete; - + return $self->result_source->storage->update( $self->result_source, $values, $cond ); @@@ -1439,10 -1410,6 +1393,6 @@@ to run. See also Ldelete() does not generate proper SQL on - joined resultsets, and may delete rows well outside of the contents - of $rs. Use at your own risk - In these cases you may find that delete_all is more appropriate, or you need to respecify your query in a way that can be expressed without a join. @@@ -1452,10 -1419,7 +1402,7 @@@ sub delete my ($self) = @_; $self->throw_exception("Delete should not be passed any arguments") if $_[1]; - carp( 'WARNING! Currently $rs->delete() does not generate proper SQL' - . ' on joined resultsets, and may delete rows well outside of the' - . ' contents of $rs. Use at your own risk' ) - if ( $self->{attrs}{seen_join} ); + my $cond = $self->_cond_for_update_delete; $self->result_source->storage->delete($self->result_source, $cond); diff --combined lib/DBIx/Class/Storage/DBI.pm index f346524,7987c40..a1bb464 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@@ -38,10 -38,10 +38,10 @@@ package # Hide from PAUS use base qw/SQL::Abstract::Limit/; sub new { my $self = shift->SUPER::new(@_); + # This prevents the caching of $dbh in S::A::L, I believe # If limit_dialect is a ref (like a $dbh), go ahead and replace # it with what it resolves to: $self->{limit_dialect} = $self->_find_syntax($self->{limit_dialect}) @@@ -50,6 -50,60 +50,60 @@@ $self; } + + + # Some databases (sqlite) do not handle multiple parenthesis + # around in/between arguments. A tentative x IN ( ( 1, 2 ,3) ) + # is interpreted as x IN 1 or something similar. + # + # Since we currently do not have access to the SQLA AST, resort + # to barbaric mutilation of any SQL supplied in literal form + + sub _strip_outer_paren { + my ($self, $arg) = @_; + + use Data::Dumper; + + return $self->_SWITCH_refkind ($arg, { + ARRAYREFREF => sub { + $$arg->[0] = __strip_outer_paren ($$arg->[0]); + return $arg; + }, + SCALARREF => sub { + return \__strip_outer_paren( $$arg ); + }, + FALLBACK => sub { + return $arg + }, + }); + } + + sub __strip_outer_paren { + my $sql = shift; + + if ($sql and not ref $sql) { + while ($sql =~ /^ \s* \( (.*) \) \s* $/x ) { + $sql = $1; + } + } + + return $sql; + } + + sub _where_field_IN { + my ($self, $lhs, $op, $rhs) = @_; + $rhs = $self->_strip_outer_paren ($rhs); + return $self->SUPER::_where_field_IN ($lhs, $op, $rhs); + } + + sub _where_field_BETWEEN { + my ($self, $lhs, $op, $rhs) = @_; + $rhs = $self->_strip_outer_paren ($rhs); + return $self->SUPER::_where_field_BETWEEN ($lhs, $op, $rhs); + } + + + # DB2 is the only remaining DB using this. Even though we are not sure if # RowNumberOver is still needed here (should be part of SQLA) leave the # code in place @@@ -95,9 -149,6 +149,9 @@@ sub _find_syntax sub select { my ($self, $table, $fields, $where, $order, @rest) = @_; + local $self->{having_bind} = []; + local $self->{from_bind} = []; + if (ref $table eq 'SCALAR') { $table = $$table; } @@@ -109,7 -160,8 +163,7 @@@ @rest = (-1) unless defined $rest[0]; die "LIMIT 0 Does Not Compute" if $rest[0] == 0; # and anyway, SQL::Abstract::Limit will cause a barf if we don't first - local $self->{having_bind} = []; - my ($sql, @ret) = $self->SUPER::select( + my ($sql, @where_bind) = $self->SUPER::select( $table, $self->_recurse_fields($fields), $where, $order, @rest ); $sql .= @@@ -121,7 -173,7 +175,7 @@@ ) : '' ; - return wantarray ? ($sql, @ret, @{$self->{having_bind}}) : $sql; + return wantarray ? ($sql, @{$self->{from_bind}}, @where_bind, @{$self->{having_bind}}) : $sql; } sub insert { @@@ -175,7 -227,7 +229,7 @@@ sub _recurse_fields } # Is the second check absolutely necessary? elsif ( $ref eq 'REF' and ref($$fields) eq 'ARRAY' ) { - return $self->_bind_to_sql( $fields ); + return $self->_fold_sqlbind( $fields ); } else { Carp::croak($ref . qq{ unexpected in _recurse_fields()}) @@@ -268,18 -320,19 +322,18 @@@ sub _recurse_from return join('', @sqlf); } -sub _bind_to_sql { - my $self = shift; - my $arr = shift; - my $sql = shift @$$arr; - $sql =~ s/\?/$self->_quote((shift @$$arr)->[1])/eg; - return $sql +sub _fold_sqlbind { + my ($self, $sqlbind) = @_; + my $sql = shift @$$sqlbind; + push @{$self->{from_bind}}, @$$sqlbind; + return $sql; } sub _make_as { my ($self, $from) = @_; - return join(' ', map { (ref $_ eq 'SCALAR' ? $$_ - : ref $_ eq 'REF' ? $self->_bind_to_sql($_) - : $self->_quote($_)) + return join(' ', map { (ref $_ eq 'SCALAR' ? $$_ + : ref $_ eq 'REF' ? $self->_fold_sqlbind($_) + : $self->_quote($_)) } reverse each %{$self->_skip_options($from)}); } diff --combined t/resultset/as_query.t index 6865655,c496085..f849f7a --- a/t/resultset/as_query.t +++ b/t/resultset/as_query.t @@@ -7,7 -7,7 +7,7 @@@ use Data::Dumper use Test::More; -plan ( tests => 4 ); +plan ( tests => 5 ); use lib qw(t/lib); use DBICTest; @@@ -65,15 -65,3 +65,13 @@@ my $rscol = $art_rs->get_column( 'charf [ [ rank => 2 ], [ name => 'Billy Joel' ] ], ); } + +TODO: { + local $TODO = 'Needs -paren fixes in SQLA before it can work'; + my $rs = $schema->resultset("CD")->search( + { 'artist.name' => 'Caterwauler McCrae' }, + { join => [qw/artist/]} + ); + my $subsel_rs = $schema->resultset("CD")->search( { cdid => { IN => $rs->get_column('cdid')->as_query } } ); + cmp_ok($subsel_rs->count, '==', $rs->count, 'Subselect on PK got the same row count'); +} - - __END__