Merge 'trunk' into 'count_distinct'
Peter Rabbitson [Thu, 7 May 2009 17:28:29 +0000 (17:28 +0000)]
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

1  2 
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/Storage/DBI.pm
t/resultset/as_query.t

@@@ -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<COUNT> 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<count> with C<group_by>, L<DBIx::Class> emulates C<GROUP BY>
 -using C<COUNT( DISTINCT( columns ) )>. Some databases (notably SQLite) do
 -not support C<DISTINCT> with multiple columns. If you are using such a
 -database, you should only use columns from the main table in your C<group_by>
 -clause.
 -
  =cut
  
  sub count {
  
  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 L<DBIx::Class::Row/del
  delete may not generate correct SQL for a query with joins or a resultset
  chained from a related resultset.  In this case it will generate a warning:-
  
-   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
  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);
@@@ -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})
    $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;
    }
    @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 .= 
      ) :
      ''
    ;
 -  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
@@@ -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__