Fix default selection resolution - make frew happy :)
Peter Rabbitson [Wed, 5 Jan 2011 23:27:05 +0000 (00:27 +0100)]
Changes
lib/DBIx/Class/ResultSet.pm
t/search/select_chains.t

diff --git a/Changes b/Changes
index 96478b4..99ba5c7 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,8 @@
 Revision history for DBIx::Class
 
     * Fixes
+        - Revert default selection to being lazy again (eagerness introduced
+          in 0.08126) - fixes DBIx::Class::Helper::ResultSet::RemoveColumns
         - Unaliased "dark" selectors no longer throw off prefetch
         - Fix proper composition of bind values across all possible
           SQL areas ( group_by => \[ ... ] now works properly )
index 554c93d..a8e468c 100644 (file)
@@ -207,10 +207,6 @@ sub new {
 
   $attrs->{alias} ||= 'me';
 
-  # default selection list
-  $attrs->{columns} = [ $source->resolve->columns ]
-    unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as _trailing_select/;
-
   # Creation of {} and bless separated to mitigate RH perl bug
   # see https://bugzilla.redhat.com/show_bug.cgi?id=196836
   my $self = {
@@ -320,9 +316,15 @@ sub search_rs {
   }
 
   my $call_attrs = {};
-  $call_attrs = pop(@_) if (
-   @_ > 1 and ( ! defined $_[-1] or ref $_[-1] eq 'HASH' )
-  );
+  if (@_ > 1) {
+    if (ref $_[-1] eq 'HASH') {
+      # copy for _normalize_selection
+      $call_attrs = { %{ pop @_ } };
+    }
+    elsif (! defined $_[-1] ) {
+      pop @_;   # search({}, undef)
+    }
+  }
 
   # see if we can keep the cache (no $rs changes)
   my $cache;
@@ -343,33 +345,28 @@ sub search_rs {
   my $old_having = delete $old_attrs->{having};
   my $old_where = delete $old_attrs->{where};
 
+  my $new_attrs = { %$old_attrs };
 
-  # start with blind overwriting merge
-  my $new_attrs = { %{$old_attrs}, %{$call_attrs} };
-
-  # join/prefetch use their own crazy merging heuristics
-  foreach my $key (qw/join prefetch/) {
-    $new_attrs->{$key} = $self->_merge_joinpref_attr($old_attrs->{$key}, $call_attrs->{$key})
-      if exists $call_attrs->{$key};
-  }
-
-  # stack binds together
-  $new_attrs->{bind} = [ @{ $old_attrs->{bind} || [] }, @{ $call_attrs->{bind} || [] } ];
-
-  # take care of selects (only if anything changed)
+  # take care of call attrs (only if anything is changing)
   if (keys %$call_attrs) {
 
     $self->throw_exception ('_trailing_select is not a public attribute - do not use it in search()')
-      if exists $call_attrs->{_trailing_select};
+      if ( exists $call_attrs->{_trailing_select} or exists $call_attrs->{'+_trailing_select'} );
+
+    my @selector_attrs = qw/select as columns cols +select +as +columns include_columns _trailing_select +_trailing_select/;
+
+    # Normalize the selector list (operates on the passed-in attr structure)
+    # Need to do it on every chain instead of only once on _resolved_attrs, in
+    # order to separate 'as'-ed from blind 'select's
+    $self->_normalize_selection ($call_attrs);
 
-    my @selector_attrs = qw/select as columns cols +select +as +columns include_columns/;
+    # start with blind overwriting merge, exclude selector attrs
+    $new_attrs = { %{$old_attrs}, %{$call_attrs} };
+    delete @{$new_attrs}{@selector_attrs};
 
     # reset the current selector list if new selectors are supplied
     if (List::Util::first { exists $call_attrs->{$_} } qw/columns cols select as/) {
-      # the new/old acrobatics is because of the merger in the next loop
-      for ($new_attrs, $old_attrs) {
-        delete @{$_}{@selector_attrs, '_trailing_select'};
-      }
+      delete @{$old_attrs}{@selector_attrs};
     }
 
     for (@selector_attrs) {
@@ -387,10 +384,15 @@ sub search_rs {
       }
     }
 
-    # Normalize the selector list (operates on the passed-in attr structure)
-    # Need to do it on every chain instead of only once on _resolved_attrs, in
-    # order to separate 'as'-ed from blind 'select's
-    $self->_normalize_selection ($new_attrs);
+
+    # join/prefetch use their own crazy merging heuristics
+    foreach my $key (qw/join prefetch/) {
+      $new_attrs->{$key} = $self->_merge_joinpref_attr($old_attrs->{$key}, $call_attrs->{$key})
+        if exists $call_attrs->{$key};
+    }
+
+    # stack binds together
+    $new_attrs->{bind} = [ @{ $old_attrs->{bind} || [] }, @{ $call_attrs->{bind} || [] } ];
   }
 
 
@@ -448,12 +450,14 @@ sub search_rs {
 sub _normalize_selection {
   my ($self, $attrs) = @_;
 
-  # merge all balanced selectors into the 'columns' stack, deleting the rest
-  foreach my $key (qw/+columns include_columns/) {
-    $attrs->{columns} = $self->_merge_attr($attrs->{columns}, delete $attrs->{$key})
-      if exists $attrs->{$key};
-  }
+  # legacy syntax
+  $attrs->{'+columns'} = $self->_merge_attr($attrs->{'+columns'}, delete $attrs->{include_columns})
+    if exists $attrs->{include_columns};
 
+  # Keep the X vs +X separation until _resolved_attrs time - this allows to
+  # delay the decision on whether to use a default select list ($rsrc->columns)
+  # allowing stuff like the remove_columns helper to work
+  #
   # select/as +select/+as pairs need special handling - the amount of select/as
   # elements in each pair does *not* have to be equal (think multicolumn
   # selectors like distinct(foo, bar) ). If the selector is bare (no 'as'
@@ -507,7 +511,7 @@ sub _normalize_selection {
       }
 
       @$sel = @new_sel;
-      $attrs->{'_trailing_select'} = $self->_merge_attr($attrs->{'_trailing_select'}, \@new_trailing)
+      $attrs->{"${pref}_trailing_select"} = $self->_merge_attr($attrs->{"${pref}_trailing_select"}, \@new_trailing)
         if @new_trailing;
     }
     elsif (@$as < @$sel) {
@@ -517,23 +521,21 @@ sub _normalize_selection {
     }
 
     # now see what the result for this pair looks like:
-
     if (@$as == @$sel) {
+
       # if balanced - treat as a columns entry
-      $attrs->{columns} = $self->_merge_attr(
-        $attrs->{columns},
+      $attrs->{"${pref}columns"} = $self->_merge_attr(
+        $attrs->{"${pref}columns"},
         { map { $as->[$_] => $sel->[$_] } ( 0 .. $#$as ) }
       );
     }
     else {
       # unbalanced - shove in select/as, not subject to deduplication in _resolved_attrs
-      $attrs->{select} = $self->_merge_attr($attrs->{select}, $sel);
-      $attrs->{as} = $self->_merge_attr($attrs->{as}, $as);
+      $attrs->{"${pref}select"} = $self->_merge_attr($attrs->{"${pref}select"}, $sel);
+      $attrs->{"${pref}as"} = $self->_merge_attr($attrs->{"${pref}as"}, $as);
     }
   }
 
-  # simplify
-  delete $attrs->{$_} for grep { $attrs->{$_} and ! @{$attrs->{$_}} } qw/select as columns/;
 }
 
 sub _stack_cond {
@@ -3214,8 +3216,18 @@ sub _resolved_attrs {
   my $source = $self->result_source;
   my $alias  = $attrs->{alias};
 
-  # take care of any selector merging
-  $self->_normalize_selection ($attrs);
+  # one last pass of normalization
+  $self->_normalize_selection($attrs);
+
+  # default selection list
+  $attrs->{columns} = [ $source->columns ]
+    unless List::Util::first { exists $attrs->{$_} } qw/columns cols select as _trailing_select/;
+
+  # merge selectors together
+  for (qw/columns select as _trailing_select/) {
+    $attrs->{$_} = $self->_merge_attr($attrs->{$_}, $attrs->{"+$_"})
+      if $attrs->{$_} or $attrs->{"+$_"};
+  }
 
   # disassemble columns
   my (@sel, @as);
@@ -3383,7 +3395,7 @@ sub _resolved_attrs {
   }
 
 
-  push @sel, @{$attrs->{_trailing_select}}
+  push @{ $attrs->{select} }, @{$attrs->{_trailing_select}}
     if $attrs->{_trailing_select};
 
   # if both page and offset are specified, produce a combined offset
@@ -3543,6 +3555,7 @@ sub _merge_joinpref_attr {
             return [$_[0], @{$_[1]}]
           },
           HASH  => sub {
+            return [] if !defined $_[0] and !keys %{$_[1]};
             return [ $_[1] ] if !defined $_[0];
             return [ $_[0] ] if !keys %{$_[1]};
             return [$_[0], $_[1]]
@@ -3570,17 +3583,20 @@ sub _merge_joinpref_attr {
         },
         HASH => {
           SCALAR => sub {
+            return [] if !keys %{$_[0]} and !defined $_[1];
             return [ $_[0] ] if !defined $_[1];
             return [ $_[1] ] if !keys %{$_[0]};
             return [$_[0], $_[1]]
           },
           ARRAY => sub {
+            return [] if !keys %{$_[0]} and !@{$_[1]};
             return [ $_[0] ] if !@{$_[1]};
             return $_[1] if !keys %{$_[0]};
             return $_[1] if __HM_DEDUP and List::Util::first { $_ eq $_[0] } @{$_[1]};
             return [ $_[0], @{$_[1]} ];
           },
           HASH => sub {
+            return [] if !keys %{$_[0]} and !keys %{$_[1]};
             return [ $_[0] ] if !keys %{$_[1]};
             return [ $_[1] ] if !keys %{$_[0]};
             return [ $_[0] ] if $_[0] eq $_[1];
index 6adbca5..234439d 100644 (file)
@@ -84,22 +84,36 @@ while (@chain) {
 }
 
 # Make sure we don't lose bits even with weird selector specs
-$rs = $schema->resultset('CD')->search ({}, {
-  'columns'   => [ 'me.title' ],
-})->search ({}, {
-  '+select'   => \'me.year AS foo',
-})->search ({}, {
-  '+select'   => [ \'me.artistid AS bar' ],
-})->search ({}, {
-  '+select'   => { count => 'artistid', -as => 'baz' },
-});
+# also check that the default selector list is lazy
+$rs = $schema->resultset('CD');
+for my $attr (
+  { '+columns'  => [ 'me.title' ] },    # this one should be de-duplicated but not the select's
+
+  { '+select'   => \'me.year AS foo' },   # duplication of identical select expected (FIXME ?)
+  { '+select'   => \['me.year AS foo'] },
+
+  { '+select'   => [ \'me.artistid AS bar' ] },
+  { '+select'   => { count => 'artistid', -as => 'baz' } },
+) {
+  for (qw/columns select as/) {
+    ok (! exists $rs->{attrs}{$_}, "No eager '$_' attr on fresh resultset" );
+  }
+
+  $rs = $rs->search({}, $attr);
+}
 
 is_same_sql_bind (
   $rs->as_query,
   '( SELECT
+      me.cdid,
+      me.artist,
       me.title,
+      me.year,
+      me.genreid,
+      me.single_track,
       COUNT( artistid ) AS baz,
       me.year AS foo,
+      me.year AS foo,
       me.artistid AS bar
         FROM cd me
   )',