From: Peter Rabbitson <ribasushi@cpan.org>
Date: Sat, 23 Mar 2013 11:35:12 +0000 (+0100)
Subject: Recognize root source unqualified columns under limited prefetch
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=e6977bbbc3dfb119cc11ee7e235ca58dfef7e7e6;p=dbsrgits%2FDBIx-Class-Historic.git

Recognize root source unqualified columns under limited prefetch
---

diff --git a/Changes b/Changes
index 6b9a3b4..4cb852a 100644
--- a/Changes
+++ b/Changes
@@ -8,6 +8,8 @@ Revision history for DBIx::Class
           RT#81378 and RT#81897
         - Stop Sybase ASE storage from generating invalid SQL in subselects
           when a limit without offset is encountered
+        - Correctly recognize root source unqualified columns when
+          resolving right-side ordered limited prefetch
 
 0.08210 2013-04-04 15:30 (UTC)
     * New Features / Changes
diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm
index 10101f8..225ab01 100644
--- a/lib/DBIx/Class/Storage/DBIHacks.pm
+++ b/lib/DBIx/Class/Storage/DBIHacks.pm
@@ -251,29 +251,50 @@ sub _adjust_select_args_for_complex_prefetch {
         my $sql_maker = $self->sql_maker;
         my ($lquote, $rquote, $sep) = map { quotemeta $_ } ($sql_maker->_quote_chars, $sql_maker->name_sep);
         my $own_re = qr/ $lquote \Q$root_alias\E $rquote $sep | \b \Q$root_alias\E $sep /x;
-        my @order = @{$attrs->{order_by}};
-        my @order_chunks = map { ref $_ eq 'ARRAY' ? $_ : [ $_ ] } $sql_maker->_order_by_chunks (\@order);
-        $self->throw_exception ('Order By parsing failed...') if @order != @order_chunks;
-        for my $i (0 .. $#order) {
-          # skip ourselves, and anything that looks like a literal
-          next if $order_chunks[$i][0] =~ $own_re;
-          next if (ref $order[$i] and ref $order[$i] ne 'HASH');
-
-          my $is_desc = $order_chunks[$i][0] =~ s/\sDESC$//i;
-          $order_chunks[$i][0] =~ s/\sASC$//i;
-
-          $order[$i] = \[
+        my @order_chunks = map { ref $_ eq 'ARRAY' ? $_ : [ $_ ] } $sql_maker->_order_by_chunks($attrs->{order_by});
+        my @new_order = map { \$_ } @order_chunks;
+        my $inner_columns_info = $self->_resolve_column_info($inner_from);
+
+        # loop through and replace stuff that is not "ours" with a min/max func
+        # everything is a literal at this point, since we are likely properly
+        # quoted and stuff
+        for my $i (0 .. $#new_order) {
+          my $chunk = $order_chunks[$i][0];
+
+          # skip ourselves
+          next if $chunk =~ $own_re;
+
+          my $is_desc = $chunk =~ s/\sDESC$//i;
+          $chunk =~ s/\sASC$//i;
+
+          # maybe our own unqualified column
+          my ($ord_bit) = ($lquote and $sep)
+            ? $chunk =~ /^ $lquote ([^$sep]+) $rquote $/x
+            : $chunk
+          ;
+          next if (
+            $ord_bit
+              and
+            $inner_columns_info->{$ord_bit}
+              and
+            $inner_columns_info->{$ord_bit}{-source_alias} eq $root_alias
+          );
+
+          $new_order[$i] = \[
             sprintf(
               '%s(%s)%s',
               ($is_desc ? 'MAX' : 'MIN'),
-              $order_chunks[$i][0],
+              $chunk,
               ($is_desc ? ' DESC' : ''),
             ),
             @ {$order_chunks[$i]} [ 1 .. $#{$order_chunks[$i]} ]
           ];
         }
 
-        $inner_attrs->{order_by} = \@order;
+        $inner_attrs->{order_by} = \@new_order;
+
+        # do not care about leftovers here - it will be all the functions
+        # we just created
         ($inner_attrs->{group_by}) = $self->_group_over_selection (
           $inner_from, $inner_select, $inner_attrs->{order_by}
         );
@@ -752,15 +773,26 @@ sub _extract_order_criteria {
   my ($self, $order_by, $sql_maker) = @_;
 
   my $parser = sub {
-    my ($sql_maker, $order_by) = @_;
+    my ($sql_maker, $order_by, $orig_quote_chars) = @_;
 
     return scalar $sql_maker->_order_by_chunks ($order_by)
       unless wantarray;
 
+    my ($lq, $rq, $sep) = map { quotemeta($_) } (
+      ($orig_quote_chars ? @$orig_quote_chars : $sql_maker->_quote_chars),
+      $sql_maker->name_sep
+    );
+
     my @chunks;
     for ($sql_maker->_order_by_chunks ($order_by) ) {
-      my $chunk = ref $_ ? $_ : [ $_ ];
+      my $chunk = ref $_ ? [ @$_ ] : [ $_ ];
       $chunk->[0] =~ s/\s+ (?: ASC|DESC ) \s* $//ix;
+
+      # order criteria may have come back pre-quoted (literals and whatnot)
+      # this is fragile, but the best we can currently do
+      $chunk->[0] =~ s/^ $lq (.+?) $rq $sep $lq (.+?) $rq $/"$1.$2"/xe
+        or $chunk->[0] =~ s/^ $lq (.+) $rq $/$1/x;
+
       push @chunks, $chunk;
     }
 
@@ -772,8 +804,13 @@ sub _extract_order_criteria {
   }
   else {
     $sql_maker = $self->sql_maker;
+
+    # pass these in to deal with literals coming from
+    # the user or the deep guts of prefetch
+    my $orig_quote_chars = [$sql_maker->_quote_chars];
+
     local $sql_maker->{quote_char};
-    return $parser->($sql_maker, $order_by);
+    return $parser->($sql_maker, $order_by, $orig_quote_chars);
   }
 }
 
diff --git a/t/prefetch/o2m_o2m_order_by_with_limit.t b/t/prefetch/o2m_o2m_order_by_with_limit.t
index c8972c0..f9f78ca 100644
--- a/t/prefetch/o2m_o2m_order_by_with_limit.t
+++ b/t/prefetch/o2m_o2m_order_by_with_limit.t
@@ -13,7 +13,7 @@ my ($ROWS, $OFFSET) = (
    DBIx::Class::SQLMaker::LimitDialects->__offset_bindtype,
 );
 
-my $schema = DBICTest->init_schema();
+my $schema = DBICTest->init_schema(quote_names => 1);
 
 my $artist_rs = $schema->resultset('Artist');
 
@@ -22,7 +22,7 @@ my $filtered_cd_rs = $artist_rs->search_related('cds_unordered',
   {
     prefetch => 'tracks',
     join => 'genre',
-    order_by => [ { -desc => 'genre.name' }, 'tracks.title DESC', { -asc => "me.name" }, { -desc => 'cds_unordered.title' } ], # me. is the artist, *NOT* the cd
+    order_by => [ { -desc => 'genre.name' }, { -desc => \ 'tracks.title' }, { -asc => "me.name" }, { -desc => [qw(year cds_unordered.title)] } ], # me. is the artist, *NOT* the cd
   },
 );
 
@@ -86,33 +86,41 @@ for (
 
   is_same_sql_bind(
     $rs->as_query,
-    "(
-      SELECT  cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track,
-              tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at
-        FROM artist me
+    qq{(
+      SELECT  "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track",
+              "tracks"."trackid", "tracks"."cd", "tracks"."position", "tracks"."title", "tracks"."last_updated_on", "tracks"."last_updated_at"
+        FROM "artist" "me"
         JOIN (
-          SELECT cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track
-            FROM artist me
-            JOIN cd cds_unordered
-              ON cds_unordered.artist = me.artistid
-            LEFT JOIN genre genre
-              ON genre.genreid = cds_unordered.genreid
-            LEFT JOIN track tracks
-              ON tracks.cd = cds_unordered.cdid
-          WHERE ( me.rank = ? )
-          GROUP BY cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track
-          ORDER BY MAX(genre.name) DESC, MAX(tracks.title) DESC, MIN(me.name), cds_unordered.title DESC
+          SELECT "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track"
+            FROM "artist" "me"
+            JOIN cd "cds_unordered"
+              ON "cds_unordered"."artist" = "me"."artistid"
+            LEFT JOIN "genre" "genre"
+              ON "genre"."genreid" = "cds_unordered"."genreid"
+            LEFT JOIN "track" "tracks"
+              ON "tracks"."cd" = "cds_unordered"."cdid"
+          WHERE "me"."rank" = ?
+          GROUP BY "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track"
+          ORDER BY  MAX("genre"."name") DESC,
+                    MAX( tracks.title ) DESC,
+                    MIN("me"."name"),
+                    "year" DESC,
+                    "cds_unordered"."title" DESC
           LIMIT ?
           $offset_str
-        ) cds_unordered
-          ON cds_unordered.artist = me.artistid
-        LEFT JOIN genre genre
-          ON genre.genreid = cds_unordered.genreid
-        LEFT JOIN track tracks
-          ON tracks.cd = cds_unordered.cdid
-      WHERE ( me.rank = ? )
-      ORDER BY genre.name DESC, tracks.title DESC, me.name ASC, cds_unordered.title DESC
-    )",
+        ) "cds_unordered"
+          ON "cds_unordered"."artist" = "me"."artistid"
+        LEFT JOIN "genre" "genre"
+          ON "genre"."genreid" = "cds_unordered"."genreid"
+        LEFT JOIN "track" "tracks"
+          ON "tracks"."cd" = "cds_unordered"."cdid"
+      WHERE "me"."rank" = ?
+      ORDER BY  "genre"."name" DESC,
+                tracks.title DESC,
+                "me"."name" ASC,
+                "year" DESC,
+                "cds_unordered"."title" DESC
+    )},
     [
       [ { sqlt_datatype => 'integer', dbic_colname => 'me.rank' } => 13 ],
       [ $ROWS => $used_limit ],