From: Peter Rabbitson <ribasushi@cpan.org>
Date: Sat, 9 Oct 2010 09:01:35 +0000 (+0200)
Subject: Major overhaul of select/as resolution handling (fixes RT#61235)
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=37aafa2e;p=dbsrgits%2FDBIx-Class-Historic.git

Major overhaul of select/as resolution handling (fixes RT#61235)
---

diff --git a/Changes b/Changes
index 3a9f460..0392453 100644
--- a/Changes
+++ b/Changes
@@ -14,6 +14,8 @@ Revision history for DBIx::Class
           (RT#62642)
         - Fix incomplete logic while detecting correct Oracle sequence
           on insert
+        - Fixed incorrect composition of select/as/columns attributes during
+          chaining (RT#61235)
         - Refactor handling of RDBMS-side values during insert() - fix
           regression of inserts into a Postgres / ::Replicated combination
         - Missing dependency check in t/60core.t (RT#62635)
diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 36647ed..5609572 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -10,11 +10,17 @@ use Storable;
 use DBIx::Class::ResultSetColumn;
 use DBIx::Class::ResultSourceHandle;
 use List::Util ();
+use Hash::Merge ();
 use Scalar::Util qw/blessed weaken/;
 use Try::Tiny;
 use Storable qw/nfreeze thaw/;
 use namespace::clean;
 
+BEGIN {
+  # De-duplication in _merge_attr() is disabled, but left in for reference
+  *__HM_DEDUP = sub () { 0 };
+}
+
 use overload
         '0+'     => "count",
         'bool'   => "_bool",
@@ -326,7 +332,11 @@ sub search_rs {
   my $new_attrs = { %{$old_attrs}, %{$call_attrs} };
 
   # merge new attrs into inherited
-  foreach my $key (qw/join prefetch +select +as +columns include_columns bind/) {
+  foreach my $key (qw/join prefetch/) {
+    next unless exists $call_attrs->{$key};
+    $new_attrs->{$key} = $self->_merge_joinpref_attr($old_attrs->{$key}, $call_attrs->{$key});
+  }
+  foreach my $key (qw/+select +as +columns include_columns bind/) {
     next unless exists $call_attrs->{$key};
     $new_attrs->{$key} = $self->_merge_attr($old_attrs->{$key}, $call_attrs->{$key});
   }
@@ -2913,7 +2923,7 @@ sub _chain_relationship {
 
   # we need to take the prefetch the attrs into account before we
   # ->_resolve_join as otherwise they get lost - captainL
-  my $join = $self->_merge_attr( $attrs->{join}, $attrs->{prefetch} );
+  my $join = $self->_merge_joinpref_attr( $attrs->{join}, $attrs->{prefetch} );
 
   delete @{$attrs}{qw/join prefetch collapse group_by distinct select as columns +select +as +columns/};
 
@@ -2931,7 +2941,7 @@ sub _chain_relationship {
     # are resolved (prefetch is useless - we are wrapping
     # a subquery anyway).
     my $rs_copy = $self->search;
-    $rs_copy->{attrs}{join} = $self->_merge_attr (
+    $rs_copy->{attrs}{join} = $self->_merge_joinpref_attr (
       $rs_copy->{attrs}{join},
       delete $rs_copy->{attrs}{prefetch},
     );
@@ -3014,97 +3024,171 @@ sub _resolved_attrs {
   my $source = $self->result_source;
   my $alias  = $attrs->{alias};
 
-  $attrs->{columns} ||= delete $attrs->{cols} if exists $attrs->{cols};
-  my @colbits;
+########
+# resolve selectors, this one is quite hairy
 
-  # build columns (as long as select isn't set) into a set of as/select hashes
-  unless ( $attrs->{select} ) {
+  my $selection_pieces;
 
-    my @cols;
-    if ( ref $attrs->{columns} eq 'ARRAY' ) {
-      @cols = @{ delete $attrs->{columns}}
-    } elsif ( defined $attrs->{columns} ) {
-      @cols = delete $attrs->{columns}
-    } else {
-      @cols = $source->columns
-    }
+  $attrs->{columns} ||= delete $attrs->{cols}
+    if exists $attrs->{cols};
 
-    for (@cols) {
-      if ( ref $_ eq 'HASH' ) {
-        push @colbits, $_
-      } else {
-        my $key = /^\Q${alias}.\E(.+)$/
-          ? "$1"
-          : "$_";
-        my $value = /\./
-          ? "$_"
-          : "${alias}.$_";
-        push @colbits, { $key => $value };
-      }
-    }
-  }
+  # disassemble columns / +columns
+  (
+    $selection_pieces->{columns}{select},
+    $selection_pieces->{columns}{as},
+    $selection_pieces->{'+columns'}{select},
+    $selection_pieces->{'+columns'}{as},
+  ) = map
+    {
+      my (@sel, @as);
+
+      for my $colbit (@$_) {
 
-  # add the additional columns on
-  foreach (qw{include_columns +columns}) {
-    if ( $attrs->{$_} ) {
-      my @list = ( ref($attrs->{$_}) eq 'ARRAY' )
-        ? @{ delete $attrs->{$_} }
-        : delete $attrs->{$_};
-      for (@list) {
-        if ( ref($_) eq 'HASH' ) {
-          push @colbits, $_
-        } else {
-          my $key = ( split /\./, $_ )[-1];
-          my $value = ( /\./ ? $_ : "$alias.$_" );
-          push @colbits, { $key => $value };
+        if (ref $colbit eq 'HASH') {
+          for my $as (keys %$colbit) {
+            push @sel, $colbit->{$as};
+            push @as, $as;
+          }
+        }
+        elsif ($colbit) {
+          push @sel, $colbit;
+          push @as, $colbit;
         }
       }
+
+      (\@sel, \@as)
+    }
+    (
+      (ref $attrs->{columns} eq 'ARRAY' ? delete $attrs->{columns} : [ delete $attrs->{columns} ]),
+      # include_columns is a legacy add-on to +columns
+      [ map { ref $_ eq 'ARRAY' ? @$_ : ($_ || () ) } delete @{$attrs}{qw/+columns include_columns/} ] )
+  ;
+
+  # make copies of select/as and +select/+as
+  (
+    $selection_pieces->{'select/as'}{select},
+    $selection_pieces->{'select/as'}{as},
+    $selection_pieces->{'+select/+as'}{select},
+    $selection_pieces->{'+select/+as'}{as},
+  ) = map
+    { $_ ? [ ref $_ eq 'ARRAY' ? @$_ : $_ ] : [] }
+    ( delete @{$attrs}{qw/select as +select +as/} )
+  ;
+
+  # default to * only when neither no non-plus selectors are available
+  if (
+    ! @{$selection_pieces->{'select/as'}{select}}
+      and
+    ! @{$selection_pieces->{'columns'}{select}}
+  ) {
+    for ($source->columns) {
+      push @{$selection_pieces->{'select/as'}{select}}, $_;
+      push @{$selection_pieces->{'select/as'}{as}}, $_;
     }
   }
 
-  # start with initial select items
-  if ( $attrs->{select} ) {
-    $attrs->{select} =
-        ( ref $attrs->{select} eq 'ARRAY' )
-      ? [ @{ $attrs->{select} } ]
-      : [ $attrs->{select} ];
+  # final composition order (important)
+  my @sel_pairs = grep {
+    $selection_pieces->{$_}
+      &&
+    (
+      ( $selection_pieces->{$_}{select} && @{$selection_pieces->{$_}{select}} )
+        ||
+      ( $selection_pieces->{$_}{as} && @{$selection_pieces->{$_}{as}} )
+    )
+  } qw|columns select/as +columns +select/+as|;
 
-    if ( $attrs->{as} ) {
-      $attrs->{as} =
-        (
-          ref $attrs->{as} eq 'ARRAY'
-            ? [ @{ $attrs->{as} } ]
-            : [ $attrs->{as} ]
-        )
-    } else {
-      $attrs->{as} = [ map {
-         m/^\Q${alias}.\E(.+)$/
-           ? $1
-           : $_
-         } @{ $attrs->{select} }
-      ]
+  # fill in missing as bits for each pair
+  # if it's the last pair we can let things slide ( bare +select is sadly popular)
+  my $out_of_sync;
+
+  for my $i (0 .. $#sel_pairs) {
+
+    my $pairname = $sel_pairs[$i];
+
+    my ($sel, $as) = @{$selection_pieces->{$pairname}}{qw/select as/};
+
+    $self->throw_exception(
+      "Unable to assemble final selection list: $pairname specified in addition to unbalanced $sel_pairs[$i-1]"
+    ) if ($out_of_sync);
+
+    if (@$sel == @$as) {
+      next;
+    }
+    elsif (@$sel < @$as) {
+      $self->throw_exception(
+        "More 'as' elements than 'select' elements for $pairname, unable to continue"
+      );
+    }
+    else {
+      # try to deduce the 'as' part, will work only if all the selectors are "plain", or contain an explicit -as
+      # if we can not deduce something - stop right there and leave the rest of the selector un-as'ed
+      # if there is an extra selection pair coming after that - it will die due to out_of_sync being set
+      for my $j ($#$as+1 .. $#$sel) {
+        if (my $ref = ref $sel->[$j]) {
+          if ($ref eq 'HASH' and exists $sel->[$j]{-as}) {
+            push @$as, $sel->[$j]{-as};
+          }
+          else {
+            $out_of_sync++;
+            last;
+          }
+        }
+        else {
+          push @$as, $sel->[$j];
+        }
+      }
     }
   }
-  else {
 
-    # otherwise we intialise select & as to empty
-    $attrs->{select} = [];
-    $attrs->{as}     = [];
+  # assume all unqualified selectors to apply to the current alias (legacy stuff)
+  # disqualify all $alias.col as-bits (collapser mandated)
+  for (values %$selection_pieces) {
+    $_->{select} = [ map { (ref $_ or $_ =~ /\./) ? $_ : "$alias.$_" } @{$_->{select}} ];
+    $_->{as} = [  map { $_ =~ /^\Q$alias.\E(.+)$/ ? $1 : $_ } @{$_->{as}} ];
+  }
+
+  # FIXME !!!
+  # Blatant bugwardness encoded into multiple tests.
+  # While columns behaves sensibly, +columns is expected
+  # to dump *any* foreign columns into the main object
+  # /me vomits
+  $selection_pieces->{'+columns'}{as} = [ map
+    { (split /\./, $_)[-1] }
+    @{$selection_pieces->{'+columns'}{as}}
+  ];
+
+  # merge everything
+  for (@sel_pairs) {
+    $attrs->{select} = $self->_merge_attr ($attrs->{select}, $selection_pieces->{$_}{select});
+    $attrs->{as} = $self->_merge_attr ($attrs->{as}, $selection_pieces->{$_}{as});
+  }
+
+  # de-duplicate the result (remove *identical* select/as pairs)
+  # and also die on duplicate {as} pointing to different {select}s
+  # not using a c-style for as the condition is prone to shrinkage
+  my $seen;
+  my $i = 0;
+  while ($i <= $#{$attrs->{as}} ) {
+    my ($sel, $as) = map { $attrs->{$_}[$i] } (qw/select as/);
+
+    if ($seen->{"$sel \x00\x00 $as"}++) {
+      splice @$_, $i, 1
+        for @{$attrs}{qw/select as/};
+    }
+    elsif ($seen->{$as}++) {
+      $self->throw_exception(
+        "inflate_result() alias '$as' specified twice with different SQL-side {select}-ors"
+      );
+    }
+    else {
+      $i++;
+    }
   }
 
-  # now add colbits to select/as
-  push @{ $attrs->{select} }, map values %{$_}, @colbits;
-  push @{ $attrs->{as}     }, map keys   %{$_}, @colbits;
+## selector resolution done
+########
 
-  if ( my $adds = delete $attrs->{'+select'} ) {
-    $adds = [$adds] unless ref $adds eq 'ARRAY';
-    push @{ $attrs->{select} },
-      map { /\./ || ref $_ ? $_ : "$alias.$_" } @$adds;
-  }
-  if ( my $adds = delete $attrs->{'+as'} ) {
-    $adds = [$adds] unless ref $adds eq 'ARRAY';
-    push @{ $attrs->{as} }, @$adds;
-  }
 
   $attrs->{from} ||= [{
     -source_handle => $source->handle,
@@ -3120,7 +3204,7 @@ sub _resolved_attrs {
     my $join = delete $attrs->{join} || {};
 
     if ( defined $attrs->{prefetch} ) {
-      $join = $self->_merge_attr( $join, $attrs->{prefetch} );
+      $join = $self->_merge_joinpref_attr( $join, $attrs->{prefetch} );
     }
 
     $attrs->{from} =    # have to copy here to avoid corrupting the original
@@ -3165,7 +3249,7 @@ sub _resolved_attrs {
 
   $attrs->{collapse} ||= {};
   if ( my $prefetch = delete $attrs->{prefetch} ) {
-    $prefetch = $self->_merge_attr( {}, $prefetch );
+    $prefetch = $self->_merge_joinpref_attr( {}, $prefetch );
 
     my $prefetch_ordering = [];
 
@@ -3288,7 +3372,7 @@ sub _calculate_score {
   }
 }
 
-sub _merge_attr {
+sub _merge_joinpref_attr {
   my ($self, $orig, $import) = @_;
 
   return $import unless defined($orig);
@@ -3320,7 +3404,7 @@ sub _merge_attr {
         $orig->[$best_candidate->{position}] = $import_element;
       } elsif (ref $import_element eq 'HASH') {
         my ($key) = keys %{$orig_best};
-        $orig->[$best_candidate->{position}] = { $key => $self->_merge_attr($orig_best->{$key}, $import_element->{$key}) };
+        $orig->[$best_candidate->{position}] = { $key => $self->_merge_joinpref_attr($orig_best->{$key}, $import_element->{$key}) };
       }
     }
     $seen_keys->{$import_key} = 1; # don't merge the same key twice
@@ -3329,6 +3413,88 @@ sub _merge_attr {
   return $orig;
 }
 
+{
+  my $hm;
+
+  sub _merge_attr {
+    $hm ||= do {
+      my $hm = Hash::Merge->new;
+
+      $hm->specify_behavior({
+        SCALAR => {
+          SCALAR => sub {
+            my ($defl, $defr) = map { defined $_ } (@_[0,1]);
+
+            if ($defl xor $defr) {
+              return $defl ? $_[0] : $_[1];
+            }
+            elsif (! $defl) {
+              return ();
+            }
+            elsif (__HM_DEDUP and $_[0] eq $_[1]) {
+              return $_[0];
+            }
+            else {
+              return [$_[0], $_[1]];
+            }
+          },
+          ARRAY => sub {
+            return $_[1] if !defined $_[0];
+            return $_[1] if __HM_DEDUP and List::Util::first { $_ eq $_[0] } @{$_[1]};
+            return [$_[0], @{$_[1]}]
+          },
+          HASH  => sub {
+            return $_[1] if !defined $_[0];
+            return $_[0] if !keys %{$_[1]};
+            return [$_[0], $_[1]]
+          },
+        },
+        ARRAY => {
+          SCALAR => sub {
+            return $_[0] if !defined $_[1];
+            return $_[0] if __HM_DEDUP and List::Util::first { $_ eq $_[1] } @{$_[0]};
+            return [@{$_[0]}, $_[1]]
+          },
+          ARRAY => sub {
+            my @ret = @{$_[0]} or return $_[1];
+            return [ @ret, @{$_[1]} ] unless __HM_DEDUP;
+            my %idx = map { $_ => 1 } @ret;
+            push @ret, grep { ! defined $idx{$_} } (@{$_[1]});
+            \@ret;
+          },
+          HASH => sub {
+            return [ $_[1] ] if ! @{$_[0]};
+            return $_[0] if !keys %{$_[1]};
+            return $_[0] if __HM_DEDUP and List::Util::first { $_ eq $_[1] } @{$_[0]};
+            return [ @{$_[0]}, $_[1] ];
+          },
+        },
+        HASH => {
+          SCALAR => sub {
+            return $_[0] if !defined $_[1];
+            return $_[1] if !keys %{$_[0]};
+            return [$_[0], $_[1]]
+          },
+          ARRAY => sub {
+            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 $_[0] if !keys %{$_[1]};
+            return $_[1] if !keys %{$_[0]};
+            return $_[0] if $_[0] eq $_[1];
+            return [ $_[0], $_[1] ];
+          },
+        }
+      } => 'DBIC_RS_ATTR_MERGER');
+      $hm;
+    };
+
+    return $hm->merge ($_[1], $_[2]);
+  }
+}
 
 sub result_source {
     my $self = shift;
diff --git a/lib/DBIx/Class/ResultSetColumn.pm b/lib/DBIx/Class/ResultSetColumn.pm
index b4707e6..12623a3 100644
--- a/lib/DBIx/Class/ResultSetColumn.pm
+++ b/lib/DBIx/Class/ResultSetColumn.pm
@@ -71,7 +71,7 @@ sub new {
   ) {
     # nuke the prefetch before collapsing to sql
     my $subq_rs = $rs->search;
-    $subq_rs->{attrs}{join} = $subq_rs->_merge_attr( $subq_rs->{attrs}{join}, delete $subq_rs->{attrs}{prefetch} );
+    $subq_rs->{attrs}{join} = $subq_rs->_merge_joinpref_attr( $subq_rs->{attrs}{join}, delete $subq_rs->{attrs}{prefetch} );
     $new_parent_rs = $subq_rs->as_subselect_rs;
   }
 
@@ -82,7 +82,7 @@ sub new {
   # rs via the _resolved_attrs trick - we need to retain the separation between
   # +select/+as and select/as. At the same time we want to preserve any joins that the
   # prefetch would otherwise generate.
-  $new_attrs->{join} = $rs->_merge_attr( $new_attrs->{join}, delete $new_attrs->{prefetch} );
+  $new_attrs->{join} = $rs->_merge_joinpref_attr( $new_attrs->{join}, delete $new_attrs->{prefetch} );
 
   # {collapse} would mean a has_many join was injected, which in turn means
   # we need to group *IF WE CAN* (only if the column in question is unique)
diff --git a/t/52leaks.t b/t/52leaks.t
index 792c3d3..79f6a1c 100644
--- a/t/52leaks.t
+++ b/t/52leaks.t
@@ -128,6 +128,10 @@ unless (DBICTest::RunMode->is_plain) {
 
     resultset => $rs,
 
+    # twice so that we make sure only one H::M object spawned
+    chained_resultset => $rs->search_rs ({}, { '+columns' => [ 'foo' ] } ),
+    chained_resultset2 => $rs->search_rs ({}, { '+columns' => [ 'bar' ] } ),
+
     row_object => $row_obj,
 
     result_source => $rs->result_source,
@@ -165,7 +169,7 @@ for my $slot (keys %$weak_registry) {
     delete $weak_registry->{$slot};
   }
   elsif ($slot =~ /^\QHash::Merge/) {
-    # only clear one object - more would indicate trouble
+    # only clear one object of a specific behavior - more would indicate trouble
     delete $weak_registry->{$slot}
       unless $cleared->{hash_merge_singleton}{$weak_registry->{$slot}{weakref}{behavior}}++;
   }
diff --git a/t/73oracle.t b/t/73oracle.t
index 734e411..cde22cd 100644
--- a/t/73oracle.t
+++ b/t/73oracle.t
@@ -618,27 +618,27 @@ if ( $schema->storage->isa('DBIx::Class::Storage::DBI::Oracle::Generic') ) {
     # combine a connect_by with group_by and having
     {
       my $rs = $schema->resultset('Artist')->search({}, {
-        select => ['count(rank)'],
+        select => { count => 'rank', -as => 'cnt' },
         start_with => { name => 'root' },
         connect_by => { parentid => { -prior => { -ident => 'artistid' } } },
         group_by => ['rank'],
-        having => { 'count(rank)' => { '<', 2 } },
+        having => \[ 'count(rank) < ?', [ cnt => 2 ] ],
       });
 
       is_same_sql_bind (
         $rs->as_query,
         '(
-            SELECT count(rank)
+            SELECT COUNT(rank) AS cnt
             FROM artist me
             START WITH name = ?
             CONNECT BY parentid = PRIOR artistid
             GROUP BY rank HAVING count(rank) < ?
         )',
-        [ [ name => 'root' ], [ 'count(rank)' => 2 ] ],
+        [ [ name => 'root' ], [ cnt => 2 ] ],
       );
 
       is_deeply (
-        [ $rs->get_column ('count(rank)')->all ],
+        [ $rs->get_column ('cnt')->all ],
         [1, 1],
         'Group By a Connect By query - correct values'
       );
@@ -667,7 +667,8 @@ if ( $schema->storage->isa('DBIx::Class::Storage::DBI::Oracle::Generic') ) {
 
       my $rs = $schema->resultset('Artist')->search({}, {
         start_with => { name => 'cycle-root' },
-        '+select'  => [ \ 'CONNECT_BY_ISCYCLE' ],
+        '+select'  => \ 'CONNECT_BY_ISCYCLE',
+        '+as'      => [ 'connector' ],
         connect_by_nocycle => { parentid => { -prior => { -ident => 'artistid' } } },
       });
 
@@ -687,7 +688,7 @@ if ( $schema->storage->isa('DBIx::Class::Storage::DBI::Oracle::Generic') ) {
         'got artist tree with nocycle (name)',
       );
       is_deeply (
-        [ $rs->get_column ('CONNECT_BY_ISCYCLE')->all ],
+        [ $rs->get_column ('connector')->all ],
         [ qw/1 0 0 0/ ],
         'got artist tree with nocycle (CONNECT_BY_ISCYCLE)',
       );
diff --git a/t/76select.t b/t/76select.t
index 8f45fd0..d2cc72e 100644
--- a/t/76select.t
+++ b/t/76select.t
@@ -103,7 +103,7 @@ $rs = $schema->resultset('CD')->search({},
 
 is_same_sql_bind (
   $rs->as_query,
-  '(SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track, me.cdid, me.title, artist.name FROM cd me  JOIN artist artist ON artist.artistid = me.artist)',
+  '(SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track, artist.name FROM cd me  JOIN artist artist ON artist.artistid = me.artist)',
   [],
   'Use of columns attribute results in proper sql'
 );
diff --git a/t/91merge_attr.t b/t/91merge_joinpref_attr.t
similarity index 80%
rename from t/91merge_attr.t
rename to t/91merge_joinpref_attr.t
index 6699150..17b40ca 100644
--- a/t/91merge_attr.t
+++ b/t/91merge_joinpref_attr.t
@@ -15,7 +15,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = 'artist';
   my $b = 'cd';
   my $expected = [ 'artist', 'cd' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -23,7 +23,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist' ];
   my $b = [ 'cd' ];
   my $expected = [ 'artist', 'cd' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -31,7 +31,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd' ];
   my $b = [ 'cd' ];
   my $expected = [ 'artist', 'cd' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -39,7 +39,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'artist' ];
   my $b = [ 'artist', 'cd' ];
   my $expected = [ 'artist', 'artist', 'cd' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -47,7 +47,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd' ];
   my $b = [ 'artist', 'artist' ];
   my $expected = [ 'artist', 'cd', 'artist' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -55,7 +55,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'twokeys' ];
   my $b = [ 'cds', 'cds' ];
   my $expected = [ 'twokeys', 'cds', 'cds' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -63,7 +63,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = 'artist';
   my $expected = [ 'artist', 'cd', { 'artist' => 'manager' } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -71,7 +71,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = [ 'artist', 'cd' ];
   my $expected = [ 'artist', 'cd', { 'artist' => 'manager' } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -79,7 +79,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = { 'artist' => 'manager' };
   my $expected = [ 'artist', 'cd', { 'artist' => [ 'manager' ] } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -87,7 +87,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = { 'artist' => 'agent' };
   my $expected = [ { 'artist' => 'agent' }, 'cd', { 'artist' => 'manager' } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -95,7 +95,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = { 'artist' => { 'manager' => 'artist' } };
   my $expected = [ 'artist', 'cd', { 'artist' => [ { 'manager' => 'artist' } ] } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -103,7 +103,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = { 'artist' => { 'manager' => [ 'artist', 'label' ] } };
   my $expected = [ 'artist', 'cd', { 'artist' => [ { 'manager' => [ 'artist', 'label' ] } ] } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -111,7 +111,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd', { 'artist' => 'manager' } ];
   my $b = { 'artist' => { 'tour_manager' => [ 'venue', 'roadie' ] } };
   my $expected = [ { 'artist' => { 'tour_manager' => [ 'venue', 'roadie' ] } }, 'cd', { 'artist' =>  'manager' } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -119,7 +119,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ 'artist', 'cd' ];
   my $b = { 'artist' => { 'tour_manager' => [ 'venue', 'roadie' ] } };
   my $expected = [ { 'artist' => { 'tour_manager' => [ 'venue', 'roadie' ] } }, 'cd' ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
@@ -127,7 +127,7 @@ my $rs = $schema->resultset( 'CD' );
   my $a = [ { 'artist' => 'manager' }, 'cd' ];
   my $b = [ 'artist', { 'artist' => 'manager' } ];
   my $expected = [ { 'artist' => 'manager' }, 'cd', { 'artist' => 'manager' } ];
-  my $result = $rs->_merge_attr($a, $b);
+  my $result = $rs->_merge_joinpref_attr($a, $b);
   is_deeply( $result, $expected );
 }
 
diff --git a/t/from_subquery.t b/t/from_subquery.t
index d206d0e..34b88dc 100644
--- a/t/from_subquery.t
+++ b/t/from_subquery.t
@@ -20,7 +20,7 @@ my $cdrs = $schema->resultset('CD');
 
   is_same_sql_bind(
     $cdrs2->as_query,
-    "(SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track FROM cd me WHERE artist_id IN ( SELECT id FROM artist me LIMIT 1 ))",
+    "(SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track FROM cd me WHERE artist_id IN ( SELECT me.id FROM artist me LIMIT 1 ))",
     [],
   );
 }
@@ -37,7 +37,7 @@ my $cdrs = $schema->resultset('CD');
 
   is_same_sql_bind(
     $rs->as_query,
-    "(SELECT (SELECT id FROM cd me LIMIT 1) FROM artist me)",
+    "(SELECT (SELECT me.id FROM cd me LIMIT 1) FROM artist me)",
     [],
   );
 }
@@ -54,7 +54,7 @@ my $cdrs = $schema->resultset('CD');
 
   is_same_sql_bind(
     $rs->as_query,
-    "(SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT id FROM cd me LIMIT 1) FROM artist me)",
+    "(SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT me.id FROM cd me LIMIT 1) FROM artist me)",
     [],
   );
 }
diff --git a/t/search/select_chains.t b/t/search/select_chains.t
index 58b6ff0..8873608 100644
--- a/t/search/select_chains.t
+++ b/t/search/select_chains.t
@@ -14,16 +14,16 @@ my $schema = DBICTest->init_schema();
 my @chain = (
   {
     columns     => [ 'cdid' ],
-    '+columns'  => [ { title_lc => { lower => 'title' } } ],
+    '+columns'  => [ { title_lc => { lower => 'title', -as => 'lctitle' } } ],
     '+select'   => [ 'genreid' ],
     '+as'       => [ 'genreid' ],
-  } => 'SELECT me.cdid, LOWER( title ), me.genreid FROM cd me',
+  } => 'SELECT me.cdid, LOWER( title ) AS lctitle, me.genreid FROM cd me',
 
   {
-    '+columns'  => [ { max_year => { max => 'me.year' }}, ],
+    '+columns'  => [ { max_year => { max => 'me.year', -as => 'last_y' }}, ],
     '+select'   => [ { count => 'me.cdid' }, ],
     '+as'       => [ 'cnt' ],
-  } => 'SELECT me.cdid, LOWER( title ), MAX( me.year ), me.genreid, COUNT( me.cdid ) FROM cd me',
+  } => 'SELECT me.cdid, LOWER( title ) AS lctitle, MAX( me.year ) AS last_y, me.genreid, COUNT( me.cdid ) FROM cd me',
 
   {
     select      => [ { min => 'me.cdid' }, ],
@@ -31,12 +31,38 @@ my @chain = (
   } => 'SELECT MIN( me.cdid ) FROM cd me',
 
   {
-    '+columns' => [ { cnt => { count => 'cdid' } } ],
-  } => 'SELECT MIN( me.cdid ), COUNT ( cdid ) FROM cd me',
+    '+columns' => [ { cnt => { count => 'cdid', -as => 'cnt' } } ],
+  } => 'SELECT MIN( me.cdid ), COUNT ( cdid ) AS cnt FROM cd me',
 
   {
-    columns => [ 'year' ],
-  } => 'SELECT me.year FROM cd me',
+    columns => [ { foo => { coalesce => [qw/a b c/], -as => 'firstfound' } }  ],
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound FROM cd me',
+
+  {
+    '+columns' => [ 'me.year' ],
+    '+select' => [ { max => 'me.year', -as => 'last_y' } ],
+    '+as' => [ 'ly' ],
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y FROM cd me',
+
+  {
+    '+select'   => [ { count => 'me.cdid', -as => 'cnt' } ],
+    '+as'       => [ 'cnt' ],
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me',
+
+  # adding existing stuff should not alter selector
+  {
+    '+select'   => [ 'me.year' ],
+    '+as'       => [ 'year' ],
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me',
+
+  {
+    '+columns'   => [ 'me.year' ],
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me',
+
+  {
+    '+columns'   => 'me.year',
+  } => 'SELECT COALESCE( a, b, c ) AS firstfound, me.year, MAX( me.year ) AS last_y, COUNT( me.cdid ) AS cnt FROM cd me',
+
 );
 
 my $rs = $schema->resultset('CD');
@@ -58,4 +84,28 @@ while (@chain) {
   $testno++;
 }
 
+# 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' },
+});
+
+is_same_sql_bind (
+  $rs->as_query,
+  '( SELECT
+      me.title,
+      me.year AS foo,
+      me.artistid AS bar,
+      COUNT( artistid ) AS baz
+        FROM cd me
+  )',
+  [],
+  'Correct chaining before attr resolution'
+);
+
 done_testing;
diff --git a/t/search/subquery.t b/t/search/subquery.t
index 49efcb2..be0febf 100644
--- a/t/search/subquery.t
+++ b/t/search/subquery.t
@@ -29,7 +29,7 @@ my @tests = (
       artist_id => { 'in' => $art_rs->search({}, { rows => 1 })->get_column( 'id' )->as_query },
     },
     sqlbind => \[
-      "( SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track FROM cd me WHERE artist_id IN ( SELECT id FROM artist me LIMIT 1 ) )",
+      "( SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track FROM cd me WHERE artist_id IN ( SELECT me.id FROM artist me LIMIT 1 ) )",
     ],
   },
 
@@ -41,7 +41,7 @@ my @tests = (
       ],
     },
     sqlbind => \[
-      "( SELECT (SELECT id FROM cd me LIMIT 1) FROM artist me )",
+      "( SELECT (SELECT me.id FROM cd me LIMIT 1) FROM artist me )",
     ],
   },
 
@@ -53,7 +53,7 @@ my @tests = (
       ],
     },
     sqlbind => \[
-      "( SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT id FROM cd me LIMIT 1) FROM artist me )",
+      "( SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT me.id FROM cd me LIMIT 1) FROM artist me )",
     ],
   },