From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 2 Jul 2009 06:08:33 +0000 (+0000)
Subject: Another candidate for somethingawful.com (fix left join-ed count)
X-Git-Tag: v0.08108~32
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=c98169a74e44ef761b38d738d1d16f8a693d0a46;hp=1ec6e7c25e6956b72d05265b43c5be735614bd28;p=dbsrgits%2FDBIx-Class.git

Another candidate for somethingawful.com (fix left join-ed count)
---

diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index e30e2f7..230498a 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -1227,6 +1227,11 @@ sub _count_rs {
   $tmp_attrs->{select} = $rsrc->storage->_count_select ($rsrc, $tmp_attrs);
   $tmp_attrs->{as} = 'count';
 
+  # read the function comment
+  $tmp_attrs->{from} = $self->_switch_to_inner_join_if_needed (
+    $tmp_attrs->{from}, $tmp_attrs->{alias}
+  );
+
   my $tmp_rs = $rsrc->resultset_class->new($rsrc, $tmp_attrs)->get_column ('count');
 
   return $tmp_rs;
@@ -1254,6 +1259,11 @@ sub _count_subq_rs {
 
   $sub_attrs->{select} = $rsrc->storage->_subq_count_select ($rsrc, $sub_attrs);
 
+  # read the function comment
+  $sub_attrs->{from} = $self->_switch_to_inner_join_if_needed (
+    $sub_attrs->{from}, $sub_attrs->{alias}
+  );
+
   $attrs->{from} = [{
     count_subq => $rsrc->resultset_class->new ($rsrc, $sub_attrs )->as_query
   }];
@@ -1265,6 +1275,93 @@ sub _count_subq_rs {
 }
 
 
+# The DBIC relationship chaining implementation is pretty simple - every
+# new related_relationship is pushed onto the {from} stack, and the {select}
+# window simply slides further in. This means that when we count somewhere
+# in the middle, we got to make sure that everything in the join chain is an
+# actual inner join, otherwise the count will come back with unpredictable
+# results (a resultset may be generated with _some_ rows regardless of if
+# the relation which the $rs currently selects has rows or not). E.g.
+# $artist_rs->cds->count - normally generates:
+# SELECT COUNT( * ) FROM artist me LEFT JOIN cd cds ON cds.artist = me.artistid
+# which actually returns the number of artists * (number of cds || 1)
+#
+# So what we do here is crawl {from}, determine if the current alias is at
+# the top of the stack, and if not - make sure the chain is inner-joined down
+# to the root.
+#
+sub _switch_to_inner_join_if_needed {
+  my ($self, $from, $alias) = @_;
+
+  return $from if (
+    ref $from ne 'ARRAY'
+      ||
+    ref $from->[0] ne 'HASH'
+      ||
+    ! $from->[0]{-alias}
+      ||
+    $from->[0]{-alias} eq $alias
+  );
+
+  # this would be the case with a subquery - we'll never find
+  # the target as it is not in the parseable part of {from}
+  return $from if @$from == 1;
+
+  my (@switch_idx, $found_target);
+
+  JOINSCAN:
+  for my $i (1 .. $#$from) {
+
+    push @switch_idx, $i;
+    my $j = $from->[$i];
+    my $jalias = $j->[0]{-alias};
+
+    # we found our current target - delete any siblings (same level joins)
+    # and bail out
+    if ($jalias eq $alias) {
+      $found_target++;
+
+      my $cur_depth = $j->[0]{-relation_chain_depth};
+      # we are -1, so look at -2
+      while (@switch_idx > 1
+              && $from->[$switch_idx[-2]][0]{-relation_chain_depth} == $cur_depth
+      ) {
+        splice @switch_idx, -2, 1;
+      }
+
+      last JOINSCAN;
+    }
+  }
+
+  # something else went wrong
+  return $from unless $found_target;
+
+  # So it looks like we will have to switch some stuff around.
+  # local() is useless here as we will be leaving the scope
+  # anyway, and deep cloning is just too fucking expensive
+  # So replace the inner hashref manually
+  my @new_from;
+  my $sw_idx = { map { $_ => 1 } @switch_idx };
+
+  for my $i (0 .. $#$from) {
+    if ($sw_idx->{$i}) {
+      my %attrs = %{$from->[$i][0]};
+      delete $attrs{-join_type};
+
+      push @new_from, [
+        \%attrs,
+        @{$from->[$i]}[ 1 .. $#{$from->[$i]} ],
+      ];
+    }
+    else {
+      push @new_from, $from->[$i];
+    }
+  }
+
+  return \@new_from;
+}
+
+
 sub _bool {
   return 1;
 }
@@ -1926,16 +2023,25 @@ sub _is_deterministic_value {
 # of the attributes supplied
 #
 # used to determine if a subquery is neccessary
+#
+# supports some  virtual attributes:
+#   -join
+#     This will scan for any joins being present on the resultset.
+#     It is not a mere key-search but a deep inspection of {from}
+#
 
 sub _has_resolved_attr {
   my ($self, @attr_names) = @_;
 
   my $attrs = $self->_resolved_attrs;
 
-  my $join_check_req;
+  my %extra_checks;
 
   for my $n (@attr_names) {
-    ++$join_check_req if $n eq '-join';
+    if (grep { $n eq $_ } (qw/-join/) ) {
+      $extra_checks{$n}++;
+      next;
+    }
 
     my $attr =  $attrs->{$n};
 
@@ -1954,7 +2060,7 @@ sub _has_resolved_attr {
 
   # a resolved join is expressed as a multi-level from
   return 1 if (
-    $join_check_req
+    $extra_checks{-join}
       and
     ref $attrs->{from} eq 'ARRAY'
       and
diff --git a/t/count/count_rs.t b/t/count/count_rs.t
index 7153d3e..acf696c 100644
--- a/t/count/count_rs.t
+++ b/t/count/count_rs.t
@@ -33,7 +33,7 @@ my $schema = DBICTest->init_schema();
     \@bind,
     'SELECT COUNT( * )
       FROM cd me
-      LEFT JOIN track tracks ON tracks.cd = me.cdid
+      JOIN track tracks ON tracks.cd = me.cdid
       JOIN cd disc ON disc.cdid = tracks.cd
       LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid 
      WHERE ( ( position = ? OR position = ? ) )
@@ -51,7 +51,7 @@ my $schema = DBICTest->init_schema();
        FROM (
         SELECT tracks.trackid
           FROM cd me
-          LEFT JOIN track tracks ON tracks.cd = me.cdid
+          JOIN track tracks ON tracks.cd = me.cdid
           JOIN cd disc ON disc.cdid = tracks.cd
           LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid 
         WHERE ( ( position = ? OR position = ? ) )
@@ -85,7 +85,7 @@ my $schema = DBICTest->init_schema();
       FROM (
         SELECT cds.cdid
           FROM artist me
-          LEFT JOIN cd cds ON cds.artist = me.artistid
+          JOIN cd cds ON cds.artist = me.artistid
           LEFT JOIN track tracks ON tracks.cd = cds.cdid
           JOIN artist artist ON artist.artistid = cds.artist
         WHERE tracks.position = ? OR tracks.position = ?
@@ -105,7 +105,7 @@ my $schema = DBICTest->init_schema();
       FROM (
         SELECT cds.cdid
           FROM artist me
-          LEFT JOIN cd cds ON cds.artist = me.artistid
+          JOIN cd cds ON cds.artist = me.artistid
           LEFT JOIN track tracks ON tracks.cd = cds.cdid
           JOIN artist artist ON artist.artistid = cds.artist
         WHERE tracks.position = ? OR tracks.position = ?
diff --git a/t/count/prefetch.t b/t/count/prefetch.t
index 54f6c05..2032a4b 100644
--- a/t/count/prefetch.t
+++ b/t/count/prefetch.t
@@ -32,7 +32,7 @@ my $schema = DBICTest->init_schema();
   is_same_sql_bind (
     $sql,
     \@bind,
-    'SELECT COUNT( * ) FROM (SELECT cds.cdid FROM artist me LEFT JOIN cd cds ON cds.artist = me.artistid LEFT JOIN track tracks ON tracks.cd = cds.cdid JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid) count_subq',
+    'SELECT COUNT( * ) FROM (SELECT cds.cdid FROM artist me JOIN cd cds ON cds.artist = me.artistid LEFT JOIN track tracks ON tracks.cd = cds.cdid JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid) count_subq',
     [ qw/'1' '2'/ ],
   );
 }
@@ -57,7 +57,7 @@ my $schema = DBICTest->init_schema();
   is_same_sql_bind (
     $sql,
     \@bind,
-    'SELECT COUNT( * ) FROM cd me LEFT JOIN track tracks ON tracks.cd = me.cdid JOIN cd disc ON disc.cdid = tracks.cd LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid WHERE ( ( position = ? OR position = ? ) )',
+    'SELECT COUNT( * ) FROM cd me JOIN track tracks ON tracks.cd = me.cdid JOIN cd disc ON disc.cdid = tracks.cd LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid WHERE ( ( position = ? OR position = ? ) )',
     [ qw/'1' '2'/ ],
   );
 }
diff --git a/t/relationship/core.t b/t/relationship/core.t
index 26ecf9b..9087333 100644
--- a/t/relationship/core.t
+++ b/t/relationship/core.t
@@ -279,8 +279,7 @@ cmp_ok($searched->count, '==', 2, "Both artist returned from map after adding an
 # check join through cascaded has_many relationships
 $artist = $schema->resultset("Artist")->find(1);
 my $trackset = $artist->cds->search_related('tracks');
-# LEFT join means we also see the trackless additional album...
-cmp_ok($trackset->count, '==', 11, "Correct number of tracks for artist");
+cmp_ok($trackset->count, '==', 10, "Correct number of tracks for artist");
 
 # now see about updating eveything that belongs to artist 2 to artist 3
 $artist = $schema->resultset("Artist")->find(2);