Refactor ::DBIHacks::_extract_fixed_condition_columns (sequel to 8d005ad9)
Peter Rabbitson [Sat, 12 Jul 2014 10:32:20 +0000 (12:32 +0200)]
Instead of just returning an array of column names, switch to a hashref that
can double as an inferred-value bag. As a bonus deduplicates and folds-in
another codepath from ::ResultSet::_merge_with_rscond

In the process fixup _collapse_cond to be even more robust in some arrayref
corner-cases. Now it is guaranteed to return a hashref at all times

lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/ResultSource.pm
lib/DBIx/Class/Storage/DBIHacks.pm
t/sqlmaker/dbihacks_internals.t

index f817d3a..f71bf38 100644 (file)
@@ -7,7 +7,7 @@ use DBIx::Class::Carp;
 use DBIx::Class::ResultSetColumn;
 use Scalar::Util qw/blessed weaken reftype/;
 use DBIx::Class::_Util qw(
-  fail_on_internal_wantarray is_plain_value is_literal_value UNRESOLVABLE_CONDITION
+  fail_on_internal_wantarray UNRESOLVABLE_CONDITION
 );
 use Try::Tiny;
 use Data::Compare (); # no imports!!! guard against insane architecture
@@ -2485,7 +2485,7 @@ sub new_result {
 sub _merge_with_rscond {
   my ($self, $data) = @_;
 
-  my (%new_data, @cols_from_relations);
+  my ($implied_data, @cols_from_relations);
 
   my $alias = $self->{attrs}{alias};
 
@@ -2493,43 +2493,25 @@ sub _merge_with_rscond {
     # just massage $data below
   }
   elsif ($self->{cond} eq UNRESOLVABLE_CONDITION) {
-    %new_data = %{ $self->{attrs}{related_objects} || {} };  # nothing might have been inserted yet
-    @cols_from_relations = keys %new_data;
-  }
-  elsif (ref $self->{cond} ne 'HASH') {
-    $self->throw_exception(
-      "Can't abstract implicit construct, resultset condition not a hash"
-    );
+    $implied_data = $self->{attrs}{related_objects};  # nothing might have been inserted yet
+    @cols_from_relations = keys %{ $implied_data || {} };
   }
   else {
-    if ($self->{cond}) {
-      my $implied = $self->_remove_alias(
-        $self->result_source->schema->storage->_collapse_cond($self->{cond}),
-        $alias,
-      );
-
-      for my $c (keys %$implied) {
-        my $v = $implied->{$c};
-        if ( ! length ref $v or is_plain_value($v) ) {
-          $new_data{$c} = $v;
-        }
-        elsif (
-          ref $v eq 'HASH' and keys %$v == 1 and exists $v->{'='} and is_literal_value($v->{'='})
-        ) {
-          $new_data{$c} = $v->{'='};
-        }
-      }
-    }
+    my $eqs = $self->result_source->schema->storage->_extract_fixed_condition_columns($self->{cond}, 'consider_nulls');
+    $implied_data = { map {
+      ( ($eqs->{$_}||'') eq UNRESOLVABLE_CONDITION ) ? () : ( $_ => $eqs->{$_} )
+    } keys %$eqs };
   }
 
-  # precedence must be given to passed values over values inherited from
-  # the cond, so the order here is important.
-  %new_data = (
-    %new_data,
-    %{ $self->_remove_alias($data, $alias) },
+  return (
+    { map
+      { %{ $self->_remove_alias($_, $alias) } }
+      # precedence must be given to passed values over values inherited from
+      # the cond, so the order here is important.
+      ( $implied_data||(), $data)
+    },
+    \@cols_from_relations
   );
-
-  return (\%new_data, \@cols_from_relations);
 }
 
 # _has_resolved_attr
index 81ce7d8..886f47e 100644 (file)
@@ -9,7 +9,7 @@ use DBIx::Class::ResultSet;
 use DBIx::Class::ResultSourceHandle;
 
 use DBIx::Class::Carp;
-use DBIx::Class::_Util qw(is_literal_value UNRESOLVABLE_CONDITION);
+use DBIx::Class::_Util 'UNRESOLVABLE_CONDITION';
 use Devel::GlobalDestruction;
 use Try::Tiny;
 use List::Util 'first';
@@ -1834,14 +1834,13 @@ sub _resolve_relationship_condition {
       }
 
       # see which parts of the joinfree cond are *NOT* foreign-source-column equalities
-      my $joinfree_cond_equality_columns = { map
-        {( $_ => 1 )}
-        @{ $self->schema->storage->_extract_fixed_condition_columns($joinfree_cond) }
-      };
+      my $joinfree_cond_equality_columns =
+        $self->schema->storage->_extract_fixed_condition_columns($joinfree_cond, 'consider_nulls');
+
       @nonvalue_cols = map
         { $_ =~ /^\Q$joinfree_alias.\E(.+)/ }
         grep
-          { ! $joinfree_cond_equality_columns->{$_} }
+          { ! exists $joinfree_cond_equality_columns->{$_} }
           keys %$joinfree_cond;
 
       return ($joinfree_cond, 0, (@nonvalue_cols ? \@nonvalue_cols : undef));
index d78ab74..ae04942 100644 (file)
@@ -16,7 +16,7 @@ use mro 'c3';
 use List::Util 'first';
 use Scalar::Util 'blessed';
 use Sub::Name 'subname';
-use DBIx::Class::_Util qw(is_plain_value is_literal_value);
+use DBIx::Class::_Util qw(is_plain_value is_literal_value UNRESOLVABLE_CONDITION);
 use namespace::clean;
 
 #
@@ -887,7 +887,7 @@ sub _order_by_is_stable {
 
   my @cols = (
     ( map { $_->[0] } $self->_extract_order_criteria($order_by) ),
-    ( $where ? @{ $self->_extract_fixed_condition_columns($where) || [] } : () ),
+    ( $where ? keys %{ $self->_extract_fixed_condition_columns($where) } : () ),
   ) or return 0;
 
   my $colinfo = $self->_resolve_column_info($ident, \@cols);
@@ -965,7 +965,7 @@ sub _extract_colinfo_of_stable_main_source_order_by_portion {
           ? $colinfos->{$_}{-colname}
           : ()
       }
-      @{$self->_extract_fixed_condition_columns($attrs->{where}) || []}
+      keys %{ $self->_extract_fixed_condition_columns($attrs->{where}) }
     )
     : ()
   ];
@@ -1074,10 +1074,7 @@ sub _collapse_cond {
         : { $w[0] => undef }
       ;
     }
-    elsif ( ref $w[0] ) {
-      return \@w;
-    }
-    elsif ( @w == 2 ) {
+    elsif ( @w == 2 and ! ref $w[0]) {
       if ( ( $w[0]||'' ) =~ /^\-and$/i ) {
         return (ref $w[1] eq 'HASH' or ref $w[1] eq 'ARRAY')
           ? $self->_collapse_cond($w[1], (ref $w[1] eq 'ARRAY') )
@@ -1088,14 +1085,16 @@ sub _collapse_cond {
         return $self->_collapse_cond({ @w });
       }
     }
+    else {
+      return { -or => \@w };
+    }
   }
   else {
     # not a hash not an array
     return { '' => $where };
   }
 
-  # catchall, some of the things above fall through
-  return $where;
+  die 'should not get here';
 }
 
 sub _collapse_cond_unroll_pairs {
@@ -1179,48 +1178,72 @@ sub _collapse_cond_unroll_pairs {
   return @conds;
 }
 
-
-# returns an arrayref of column names which *definitely* have some
-# sort of non-nullable *single* equality requested in the given condition
-# specification. This is used to figure out if a resultset is
-# constrained to a column which is part of a unique constraint,
-# which in turn allows us to better predict how ordering will behave
-# etc.
+# Analyzes a given condition and attempts to extract all columns
+# with a definitive fixed-condition criteria. Returns a hashref
+# of k/v pairs suitable to be passed to set_columns(), with a
+# MAJOR CAVEAT - multi-value (contradictory) equalities are still
+# represented as a reference to the UNRESOVABLE_CONDITION constant
+# The reason we do this is that some codepaths only care about the
+# codition being stable, as opposed to actually making sense
 #
-# this is a rudimentary, incomplete, and error-prone extractor
-# however this is OK - it is conservative, and if we can not find
-# something that is in fact there - the stack will recover gracefully
+# The normal mode is used to figure out if a resultset is constrained
+# to a column which is part of a unique constraint, which in turn
+# allows us to better predict how ordering will behave etc.
+#
+# With the optional "consider_nulls" boolean argument, the function
+# is instead used to infer inambiguous values from conditions
+# (e.g. the inheritance of resultset conditions on new_result)
+#
+my $undef_marker = \ do{ my $x = 'undef' };
 sub _extract_fixed_condition_columns {
-  my $self = shift;
-  my $where_hash = $self->_collapse_cond(shift);
-
-  my $res;
-  for my $c (keys %$where_hash) {
-    if (defined (my $v = $where_hash->{$c}) ) {
-      if (
-        ! length ref $v
-          or
-        is_plain_value ($v)
-          or
-        (
-          ref $v eq 'HASH'
-            and
-          keys %$v == 1
-            and
-          ref $v->{'='}
-            and
-          is_literal_value($v->{'='})
-        )
-      ) {
-        $res->{$c} = 1;
-      }
-      elsif (ref $v eq 'ARRAY' and ($v->[0]||'') eq '-and') {
-        $res->{$_} = 1 for map { @{ $self->_extract_fixed_condition_columns({ $c => $_ }) } } @{$v}[1..$#$v];
+  my ($self, $where, $consider_nulls) = @_;
+  my $where_hash = $self->_collapse_cond($_[1]);
+
+  my $res = {};
+  my ($c, $v);
+  for $c (keys %$where_hash) {
+    my $vals;
+
+    if (!defined ($v = $where_hash->{$c}) ) {
+      $vals->{$undef_marker} = $v if $consider_nulls
+    }
+    elsif (
+      ! length ref $v
+        or
+      is_plain_value ($v)
+    ) {
+      $vals->{$v} = $v;
+    }
+    elsif (
+      ref $v eq 'HASH'
+        and
+      keys %$v == 1
+        and
+      ref $v->{'='}
+        and
+      # do not need to check for plain values - _collapse_cond did it for us
+      is_literal_value($v->{'='})
+    ) {
+      $vals->{$v->{'='}} = $v->{'='};
+    }
+    elsif (ref $v eq 'ARRAY' and ($v->[0]||'') eq '-and') {
+      for ( @{$v}[1..$#$v] ) {
+        my $subval = $self->_extract_fixed_condition_columns({ $c => $_ }, 'consider nulls');  # always fish nulls out on recursion
+        next unless exists $subval->{$c};  # didn't find anything
+        $vals->{defined $subval->{$c} ? $subval->{$c} : $undef_marker} = $subval->{$c};
       }
     }
+
+    if (keys %$vals == 1) {
+      ($res->{$c}) = (values %$vals)
+        unless !$consider_nulls and exists $vals->{$undef_marker};
+    }
+    elsif (keys %$vals > 1) {
+      $res->{$c} = UNRESOLVABLE_CONDITION;
+    }
   }
 
-  return [ sort keys %$res ];
+  $res;
 }
 
 1;
index 81deb89..66f0148 100644 (file)
@@ -5,6 +5,7 @@ use Test::Warn;
 
 use lib qw(t/lib);
 use DBICTest ':DiffSQL';
+use DBIx::Class::_Util 'UNRESOLVABLE_CONDITION';
 
 use Data::Dumper;
 
@@ -30,67 +31,89 @@ for my $t (
     where => { artistid => 1, charfield => undef },
     cc_result => { artistid => 1, charfield => undef },
     sql => 'WHERE artistid = ? AND charfield IS NULL',
-    efcc_result => [qw( artistid )],
+    efcc_result => { artistid => 1 },
+    efcc_n_result => { artistid => 1, charfield => undef },
   },
   {
     where => { -and => [ artistid => 1, charfield => undef, { rank => 13 } ] },
     cc_result => { artistid => 1, charfield => undef, rank => 13 },
     sql => 'WHERE artistid = ?  AND charfield IS NULL AND rank = ?',
-    efcc_result => [qw( artistid rank )],
+    efcc_result => { artistid => 1, rank => 13 },
+    efcc_n_result => { artistid => 1, charfield => undef, rank => 13 },
   },
   {
     where => { -and => [ { artistid => 1, charfield => undef}, { rank => 13 } ] },
     cc_result => { artistid => 1, charfield => undef, rank => 13 },
     sql => 'WHERE artistid = ?  AND charfield IS NULL AND rank = ?',
-    efcc_result => [qw( artistid rank )],
+    efcc_result => { artistid => 1, rank => 13 },
+    efcc_n_result => { artistid => 1, charfield => undef, rank => 13 },
   },
   {
     where => { -and => [ -or => { name => 'Caterwauler McCrae' }, 'rank' ] },
     cc_result => { name => 'Caterwauler McCrae', rank => undef },
     sql => 'WHERE name = ? AND rank IS NULL',
-    efcc_result => [qw( name )],
+    efcc_result => { name => 'Caterwauler McCrae' },
+    efcc_n_result => { name => 'Caterwauler McCrae', rank => undef },
   },
   {
     where => { -and => [ [ [ artist => {'=' => \'foo' } ] ], { name => \[ '= ?', 'bar' ] } ] },
     cc_result => { artist => {'=' => \'foo' }, name => \[ '= ?', 'bar' ] },
     sql => 'WHERE artist = foo AND name = ?',
-    efcc_result => [qw( artist )],
+    efcc_result => { artist => \'foo' },
   },
   {
     where => { -and => [ -or => { name => 'Caterwauler McCrae', artistid => 2 } ] },
     cc_result => { -or => [ artistid => 2, name => 'Caterwauler McCrae' ] },
     sql => 'WHERE artistid = ? OR name = ?',
-    efcc_result => [],
+    efcc_result => {},
+  },
+  {
+    where => { -or => { name => 'Caterwauler McCrae', artistid => 2 } },
+    cc_result => { -or => [ artistid => 2, name => 'Caterwauler McCrae' ] },
+    sql => 'WHERE artistid = ? OR name = ?',
+    efcc_result => {},
   },
   {
     where => { -and => [ \'foo=bar',  [ { artistid => { '=', $num } } ], { name => 'Caterwauler McCrae'} ] },
     cc_result => { '' => \'foo=bar', name => 'Caterwauler McCrae', artistid => $num },
     sql => 'WHERE foo=bar AND artistid = ? AND name = ?',
-    efcc_result => [qw( artistid name )],
+    efcc_result => { name => 'Caterwauler McCrae', artistid => $num },
   },
   {
     where => { artistid => [ $num ], rank => [ 13, 2, 3 ], charfield => [ undef ] },
     cc_result => { artistid => $num, charfield => undef, rank => [13, 2, 3] },
     sql => 'WHERE artistid = ? AND charfield IS NULL AND ( rank = ? OR rank = ? OR rank = ? )',
-    efcc_result => [qw( artistid )],
+    efcc_result => { artistid => $num },
+    efcc_n_result => { artistid => $num, charfield => undef },
   },
   {
     where => { artistid => { '=' => 1 }, rank => { '>' => 12 }, charfield => { '=' => undef } },
     cc_result => { artistid => 1, charfield => undef, rank => { '>' => 12 } },
     sql => 'WHERE artistid = ? AND charfield IS NULL AND rank > ?',
-    efcc_result => [qw( artistid )],
+    efcc_result => { artistid => 1 },
+    efcc_n_result => { artistid => 1, charfield => undef },
   },
   {
     where => { artistid => { '=' => [ 1 ], }, charfield => { '=' => [-and => \'1', \['?',2] ] }, rank => { '=' => [ $num, $num ] } },
     cc_result => { artistid => 1, charfield => [-and => { '=' => \'1' }, { '=' => \['?',2] } ], rank => { '=' => [$num, $num] } },
     sql => 'WHERE artistid = ? AND charfield = 1 AND charfield = ? AND ( rank = ? OR rank = ? )',
-    efcc_result => [qw( artistid charfield )],
+    efcc_result => { artistid => 1, charfield => UNRESOLVABLE_CONDITION },
   },
   {
     where => { -and => [ artistid => 1, artistid => 2 ], name => [ -and => { '!=', 1 }, 2 ], charfield => [ -or => { '=', 2 } ], rank => [-and => undef, { '=', undef }, { '!=', 2 } ] },
     cc_result => { artistid => [ -and => 1, 2 ], name => [ -and => { '!=', 1 }, 2 ], charfield => 2, rank => [ -and => undef, undef, { '!=', 2 } ] },
     sql => 'WHERE artistid = ? AND artistid = ? AND charfield = ? AND name != ? AND name = ? AND rank IS NULL AND rank IS NULL AND rank != ?',
-    efcc_result => [qw( artistid charfield name )],
+    efcc_result => {
+      artistid => UNRESOLVABLE_CONDITION,
+      name => 2,
+      charfield => 2,
+    },
+    efcc_n_result => {
+      artistid => UNRESOLVABLE_CONDITION,
+      name => 2,
+      charfield => 2,
+      rank => undef,
+    },
   },
   {
     where => { -and => [
@@ -106,24 +129,24 @@ for my $t (
       ],
     },
     sql => 'WHERE ( _macro.to LIKE ? OR _wc_macros.to LIKE ? ) AND group.is_active = ? AND me.is_active = ?',
-    efcc_result => [qw( group.is_active me.is_active )],
+    efcc_result => { 'group.is_active' => 1, 'me.is_active' => 1 },
   },
   {
     where => { artistid => [] },
     cc_result => { artistid => [] },
-    efcc_result => [],
+    efcc_result => {},
   },
   (map {
     {
       where => { -and => $_ },
       cc_result => undef,
-      efcc_result => [],
+      efcc_result => {},
       sql => '',
     },
     {
       where => { -or => $_ },
       cc_result => undef,
-      efcc_result => [],
+      efcc_result => {},
       sql => '',
     },
   } (
@@ -138,14 +161,15 @@ for my $t (
   )),
 
   # FIXME legacy compat crap, possibly worth undef/dieing in SQLMaker
-  { where => { artistid => {} }, sql => '', cc_result => undef, efcc_result => [] },
+  { where => { artistid => {} }, sql => '', cc_result => undef, efcc_result => {}, efcc_n_result => {} },
 
   # batshit insanity, just to be thorough
   {
     where => { -and => [ [ 'artistid' ], [ -and => [ artistid => { '!=', 69 }, artistid => undef, artistid => { '=' => 200 } ]], artistid => [], { -or => [] }, { -and => [] }, [ 'charfield' ], { name => [] }, 'rank' ] },
     cc_result => { artistid => [ -and => undef, { '!=', 69 }, undef, 200, [] ], charfield => undef, name => [], rank => undef },
     sql => 'WHERE artistid IS NULL AND artistid != ? AND artistid IS NULL AND artistid = ? AND 0=1 AND charfield IS NULL AND 0=1 AND rank IS NULL',
-    efcc_result => [qw( artistid )],
+    efcc_result => { artistid => UNRESOLVABLE_CONDITION },
+    efcc_n_result => { artistid => UNRESOLVABLE_CONDITION, charfield => undef, rank => undef },
   },
 
   # original test from RT#93244
@@ -166,14 +190,21 @@ for my $t (
       'me.title' => 'Spoonful of bees',
     },
     sql => 'WHERE LOWER(me.title) LIKE ? AND me.title = ?',
-    efcc_result => [qw( me.title )],
+    efcc_result => { 'me.title' => 'Spoonful of bees' },
   }
 ) {
 
   for my $w (
     $t->{where},
     [ -and => $t->{where} ],
-    ( keys %{$t->{where}} <= 1 ) ? [ %{$t->{where}} ] : ()
+    ( keys %{$t->{where}} <= 1 ? [ %{$t->{where}} ] : () ),
+    ( (keys %{$t->{where}} == 1 and $t->{where}{-or})
+      ? ( ref $t->{where}{-or} eq 'HASH'
+        ? [ map { $_ => $t->{where}{-or}{$_} } sort keys %{$t->{where}{-or}} ]
+        : $t->{where}{-or}
+      )
+      : ()
+    ),
   ) {
     my $name = do { local ($Data::Dumper::Indent, $Data::Dumper::Terse, $Data::Dumper::Sortkeys) = (0, 1, 1); Dumper $w };
 
@@ -201,6 +232,12 @@ for my $t (
       $t->{efcc_result},
       "Expected fixed_condition produced on $name",
     );
+
+    is_deeply(
+      $schema->storage->_extract_fixed_condition_columns($w, 'consider_nulls'),
+      $t->{efcc_n_result},
+      "Expected fixed_condition including NULLs produced on $name",
+    ) if $t->{efcc_n_result};
   }
 }