From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 11 Aug 2016 09:06:59 +0000 (+0200)
Subject: Some cleanup of the resolve_relationship_condition callsites
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=09d2e66a5d5558ef9a19dc2ec510d5dafd2fb7d8;p=dbsrgits%2FDBIx-Class.git

Some cleanup of the resolve_relationship_condition callsites

Zero functional changes
Read under -w
---

diff --git a/lib/DBIx/Class/Relationship/Accessor.pm b/lib/DBIx/Class/Relationship/Accessor.pm
index 8fdeab2..9d4d378 100644
--- a/lib/DBIx/Class/Relationship/Accessor.pm
+++ b/lib/DBIx/Class/Relationship/Accessor.pm
@@ -46,21 +46,28 @@ sub add_relationship_accessor {
       else {
         my $rsrc = $self->result_source;
 
-        my $relcond = $rsrc->_resolve_relationship_condition(
-          rel_name => %1$s,
-          foreign_alias => %1$s,
-          self_alias => 'me',
-          self_result_object => $self,
-        );
+        my $jfc;
 
         return undef if (
-          $relcond->{join_free_condition}
+
+          $rsrc->relationship_info(%1$s)->{attrs}{undef_on_null_fk}
+
             and
-          $relcond->{join_free_condition} ne DBIx::Class::_Util::UNRESOLVABLE_CONDITION
+
+          $jfc = ( $rsrc->_resolve_relationship_condition(
+            rel_name => %1$s,
+            foreign_alias => %1$s,
+            self_alias => 'me',
+            self_result_object => $self,
+          )->{join_free_condition} || {} )
+
             and
-          scalar grep { not defined $_ } values %%{ $relcond->{join_free_condition} || {} }
+
+          $jfc ne DBIx::Class::_Util::UNRESOLVABLE_CONDITION
+
             and
-          $rsrc->relationship_info(%1$s)->{attrs}{undef_on_null_fk}
+
+          grep { not defined $_ } values %%$jfc
         );
 
         my $val = $self->related_resultset( %1$s )->single;
diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm
index f82d2ec..6453679 100644
--- a/lib/DBIx/Class/Relationship/Base.pm
+++ b/lib/DBIx/Class/Relationship/Base.pm
@@ -6,7 +6,10 @@ use warnings;
 use base qw/DBIx::Class/;
 
 use Scalar::Util qw/weaken blessed/;
-use DBIx::Class::_Util qw( UNRESOLVABLE_CONDITION fail_on_internal_call );
+use DBIx::Class::_Util qw(
+  UNRESOLVABLE_CONDITION DUMMY_ALIASPAIR
+  fail_on_internal_call
+);
 use DBIx::Class::Carp;
 use namespace::clean;
 
@@ -514,83 +517,89 @@ sub related_resultset {
 
   my ($self, $rel) = @_;
 
-  return $self->{related_resultsets}{$rel} = do {
+  my $rsrc = $self->result_source;
 
-    my $rsrc = $self->result_source;
+  my $rel_info = $rsrc->relationship_info($rel)
+    or $self->throw_exception( "No such relationship '$rel'" );
 
-    my $rel_info = $rsrc->relationship_info($rel)
-      or $self->throw_exception( "No such relationship '$rel'" );
+  my $relcond_is_freeform = ref $rel_info->{cond} eq 'CODE';
 
-    my $cond_res = $rsrc->_resolve_relationship_condition(
-      rel_name => $rel,
-      self_result_object => $self,
+  my $jfc = $rsrc->_resolve_relationship_condition(
 
-      # this may look weird, but remember that we are making a resultset
-      # out of an existing object, with the new source being at the head
-      # of the FROM chain. Having a 'me' alias is nothing but expected there
-      foreign_alias => 'me',
-
-      self_alias => "!!!\xFF()!!!_SHOULD_NEVER_BE_SEEN_IN_USE_!!!()\xFF!!!",
-
-      # not strictly necessary, but shouldn't hurt either
-      require_join_free_condition => !!(ref $rel_info->{cond} ne 'CODE'),
-    );
-
-    # keep in mind that the following if() block is part of a do{} - no return()s!!!
-    if (
-      ! $cond_res->{join_free_condition}
-        and
-      ref $rel_info->{cond} eq 'CODE'
-    ) {
-
-      # A WHOREIFFIC hack to reinvoke the entire condition resolution
-      # with the correct alias. Another way of doing this involves a
-      # lot of state passing around, and the @_ positions are already
-      # mapped out, making this crap a less icky option.
-      #
-      # The point of this exercise is to retain the spirit of the original
-      # $obj->search_related($rel) where the resulting rset will have the
-      # root alias as 'me', instead of $rel (as opposed to invoking
-      # $rs->search_related)
-
-      # make the fake 'me' rel
-      local $rsrc->{_relationships}{me} = {
-        %{ $rsrc->{_relationships}{$rel} },
-        _original_name => $rel,
-      };
-
-      my $obj_table_alias = lc($rsrc->source_name) . '__row';
-      $obj_table_alias =~ s/\W+/_/g;
+    rel_name => $rel,
+    self_result_object => $self,
 
-      $rsrc->resultset->search(
-        $self->ident_condition($obj_table_alias),
-        { alias => $obj_table_alias },
-      )->related_resultset('me')->search(undef, $rel_info->{attrs})
-    }
-    else {
-
-      # FIXME - this conditional doesn't seem correct - got to figure out
-      # at some point what it does. Also the entire UNRESOLVABLE_CONDITION
-      # business seems shady - we could simply not query *at all*
-      my $attrs;
-      if ( $cond_res->{join_free_condition} eq UNRESOLVABLE_CONDITION ) {
-        $attrs = { %{$rel_info->{attrs}} };
-        my $reverse = $rsrc->reverse_relationship_info($rel);
-        foreach my $rev_rel (keys %$reverse) {
-          if ($reverse->{$rev_rel}{attrs}{accessor} && $reverse->{$rev_rel}{attrs}{accessor} eq 'multi') {
-            weaken($attrs->{related_objects}{$rev_rel}[0] = $self);
-          } else {
-            weaken($attrs->{related_objects}{$rev_rel} = $self);
-          }
+    # an extra sanity check guard
+    require_join_free_condition => ! $relcond_is_freeform,
+
+    # an API where these are optional would be too cumbersome,
+    # instead always pass in some dummy values
+    DUMMY_ALIASPAIR,
+
+    # this may look weird, but remember that we are making a resultset
+    # out of an existing object, with the new source being at the head
+    # of the FROM chain. Having a 'me' alias is nothing but expected there
+    foreign_alias => 'me',
+
+  )->{join_free_condition};
+
+  my $rel_rset;
+
+  if (
+    ! $jfc
+      and
+    $relcond_is_freeform
+  ) {
+
+    # A WHOREIFFIC hack to reinvoke the entire condition resolution
+    # with the correct alias. Another way of doing this involves a
+    # lot of state passing around, and the @_ positions are already
+    # mapped out, making this crap a less icky option.
+    #
+    # The point of this exercise is to retain the spirit of the original
+    # $obj->search_related($rel) where the resulting rset will have the
+    # root alias as 'me', instead of $rel (as opposed to invoking
+    # $rs->search_related)
+
+    # make the fake 'me' rel
+    local $rsrc->{_relationships}{me} = {
+      %{ $rsrc->{_relationships}{$rel} },
+      _original_name => $rel,
+    };
+
+    my $obj_table_alias = lc($rsrc->source_name) . '__row';
+    $obj_table_alias =~ s/\W+/_/g;
+
+    $rel_rset = $rsrc->resultset->search(
+      $self->ident_condition($obj_table_alias),
+      { alias => $obj_table_alias },
+    )->related_resultset('me')->search(undef, $rel_info->{attrs})
+  }
+  else {
+
+    # FIXME - this conditional doesn't seem correct - got to figure out
+    # at some point what it does. Also the entire UNRESOLVABLE_CONDITION
+    # business seems shady - we could simply not query *at all*
+    my $attrs;
+    if ( $jfc eq UNRESOLVABLE_CONDITION ) {
+      $attrs = { %{$rel_info->{attrs}} };
+      my $reverse = $rsrc->reverse_relationship_info($rel);
+      foreach my $rev_rel (keys %$reverse) {
+        if ($reverse->{$rev_rel}{attrs}{accessor} && $reverse->{$rev_rel}{attrs}{accessor} eq 'multi') {
+          weaken($attrs->{related_objects}{$rev_rel}[0] = $self);
+        } else {
+          weaken($attrs->{related_objects}{$rev_rel} = $self);
         }
       }
-
-      $rsrc->related_source($rel)->resultset->search(
-        $cond_res->{join_free_condition},
-        $attrs || $rel_info->{attrs},
-      );
     }
-  };
+
+    $rel_rset = $rsrc->related_source($rel)->resultset->search(
+      $jfc,
+      $attrs || $rel_info->{attrs},
+    );
+  }
+
+  $self->{related_resultsets}{$rel} = $rel_rset;
 }
 
 =head2 search_related
@@ -672,8 +681,11 @@ sub new_related {
     infer_values_based_on => $data,
     rel_name => $rel,
     self_result_object => $self,
-    foreign_alias => $rel,
-    self_alias => 'me',
+
+    # an API where these are optional would be too cumbersome,
+    # instead always pass in some dummy values
+    DUMMY_ALIASPAIR,
+
   )->{inferred_values} );
 }
 
@@ -851,8 +863,11 @@ sub set_from_related {
         +{ $f_obj->get_columns };
       }
     ),
-    foreign_alias => $rel,
-    self_alias => 'me',
+
+    # an API where these are optional would be too cumbersome,
+    # instead always pass in some dummy values
+    DUMMY_ALIASPAIR,
+
   )->{inferred_values} );
 
   return 1;
diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm
index 7915e07..128b554 100644
--- a/lib/DBIx/Class/ResultSet.pm
+++ b/lib/DBIx/Class/ResultSet.pm
@@ -12,7 +12,8 @@ use Scalar::Util qw( blessed reftype );
 use SQL::Abstract 'is_literal_value';
 use DBIx::Class::_Util qw(
   dbic_internal_try dbic_internal_catch dump_value emit_loud_diag
-  fail_on_internal_wantarray fail_on_internal_call UNRESOLVABLE_CONDITION
+  fail_on_internal_wantarray fail_on_internal_call
+  UNRESOLVABLE_CONDITION DUMMY_ALIASPAIR
 );
 use DBIx::Class::SQLMaker::Util qw( normalize_sqla_condition extract_equality_conditions );
 use DBIx::Class::ResultSource::FromSpec::Util 'find_join_path_to_alias';
@@ -862,8 +863,9 @@ sub find {
           ),
           infer_values_based_on => {},
 
-          self_alias => "\xFE", # irrelevant
-          foreign_alias => "\xFF", # irrelevant
+          # an API where these are optional would be too cumbersome,
+          # instead always pass in some dummy values
+          DUMMY_ALIASPAIR,
         )->{inferred_values} },
       };
     }
@@ -2531,8 +2533,10 @@ sub populate {
 
           $colinfo->{$rel}{fk_map} = { reverse %{ $rsrc->_resolve_relationship_condition(
             rel_name => $rel,
-            self_alias => "\xFE", # irrelevant
-            foreign_alias => "\xFF", # irrelevant
+
+            # an API where these are optional would be too cumbersome,
+            # instead always pass in some dummy values
+            DUMMY_ALIASPAIR,
           )->{identity_map} || {} } };
 
         }
diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm
index bb5d926..c8c8f2e 100644
--- a/lib/DBIx/Class/ResultSource.pm
+++ b/lib/DBIx/Class/ResultSource.pm
@@ -2392,11 +2392,11 @@ sub _resolve_relationship_condition {
     $self->throw_exception("A custom condition coderef can return at most 2 conditions, but $exception_rel_id returned extra values: @extra")
       if @extra;
 
-    if (my $jfc = $ret->{join_free_condition}) {
+    if( $ret->{join_free_condition} ) {
 
       $self->throw_exception (
         "The join-free condition returned for $exception_rel_id must be a hash reference"
-      ) unless ref $jfc eq 'HASH';
+      ) unless ref $ret->{join_free_condition} eq 'HASH';
 
       my ($joinfree_alias, $joinfree_source);
       if (defined $args->{self_result_object}) {
@@ -2422,7 +2422,7 @@ sub _resolve_relationship_condition {
         "The join-free condition returned for $exception_rel_id may only "
       . 'contain keys that are fully qualified column names of the corresponding source '
       . "'$joinfree_alias' (instead it returned '$_')"
-      ) for keys %$jfc;
+      ) for keys %{$ret->{join_free_condition}};
 
       (
         defined blessed($_)
@@ -2434,7 +2434,7 @@ sub _resolve_relationship_condition {
         . 'contain result objects as values - perhaps instead of invoking '
         . '->$something you meant to return ->get_column($something)'
         )
-      ) for values %$jfc;
+      ) for values %{$ret->{join_free_condition}};
 
     }
   }
diff --git a/lib/DBIx/Class/ResultSource/RowParser.pm b/lib/DBIx/Class/ResultSource/RowParser.pm
index 6fe946f..32fcf31 100644
--- a/lib/DBIx/Class/ResultSource/RowParser.pm
+++ b/lib/DBIx/Class/ResultSource/RowParser.pm
@@ -10,6 +10,7 @@ use DBIx::Class::ResultSource::RowParser::Util qw(
   assemble_simple_parser
   assemble_collapsing_parser
 );
+use DBIx::Class::_Util 'DUMMY_ALIASPAIR';
 
 use DBIx::Class::Carp;
 
@@ -188,8 +189,10 @@ sub _resolve_collapse {
       rsrc => $self->related_source($rel),
       fk_map => $self->_resolve_relationship_condition(
         rel_name => $rel,
-        self_alias => "\xFE", # irrelevant
-        foreign_alias => "\xFF", # irrelevant
+
+        # an API where these are optional would be too cumbersome,
+        # instead always pass in some dummy values
+        DUMMY_ALIASPAIR,
       )->{identity_map},
     };
   }
diff --git a/lib/DBIx/Class/Row.pm b/lib/DBIx/Class/Row.pm
index 4188d10..5adf4ea 100644
--- a/lib/DBIx/Class/Row.pm
+++ b/lib/DBIx/Class/Row.pm
@@ -6,7 +6,10 @@ use warnings;
 use base qw/DBIx::Class/;
 
 use Scalar::Util 'blessed';
-use DBIx::Class::_Util qw( dbic_internal_try fail_on_internal_call );
+use DBIx::Class::_Util qw(
+  dbic_internal_try fail_on_internal_call
+  DUMMY_ALIASPAIR
+);
 use DBIx::Class::Carp;
 use SQL::Abstract qw( is_literal_value is_plain_value );
 
@@ -1195,8 +1198,9 @@ sub copy {
         rel_name => $rel_name,
         self_result_object => $new,
 
-        self_alias => "\xFE", # irrelevant
-        foreign_alias => "\xFF", # irrelevant,
+        # an API where these are optional would be too cumbersome,
+        # instead always pass in some dummy values
+        DUMMY_ALIASPAIR,
       )->{inferred_values}
 
     ) for $self->related_resultset($rel_name)->all;
diff --git a/lib/DBIx/Class/_Util.pm b/lib/DBIx/Class/_Util.pm
index b8f0b06..6b71ceb 100644
--- a/lib/DBIx/Class/_Util.pm
+++ b/lib/DBIx/Class/_Util.pm
@@ -205,11 +205,16 @@ our @EXPORT_OK = qw(
   is_exception dbic_internal_try dbic_internal_catch visit_namespaces
   quote_sub qsub perlstring serialize deep_clone dump_value uniq
   parent_dir mkdir_p
-  UNRESOLVABLE_CONDITION
+  UNRESOLVABLE_CONDITION DUMMY_ALIASPAIR
 );
 
 use constant UNRESOLVABLE_CONDITION => \ '1 = 0';
 
+use constant DUMMY_ALIASPAIR => (
+  foreign_alias => "!!!\xFF()!!!_DUMMY_FOREIGN_ALIAS_SHOULD_NEVER_BE_SEEN_IN_USE_!!!()\xFF!!!",
+  self_alias => "!!!\xFE()!!!_DUMMY_SELF_ALIAS_SHOULD_NEVER_BE_SEEN_IN_USE_!!!()\xFE!!!",
+);
+
 # Override forcing no_defer, and adding naming consistency checks
 our %refs_closed_over_by_quote_sub_installed_crefs;
 sub quote_sub {
diff --git a/lib/SQL/Translator/Parser/DBIx/Class.pm b/lib/SQL/Translator/Parser/DBIx/Class.pm
index 623171e..2535783 100644
--- a/lib/SQL/Translator/Parser/DBIx/Class.pm
+++ b/lib/SQL/Translator/Parser/DBIx/Class.pm
@@ -177,6 +177,11 @@ sub parse {
             my $rel_info = $source->relationship_info($rel);
 
             # Ignore any rel cond that isn't a straight hash
+            #
+            # FIXME - this can be done *WAY* better via the recolcond resolver
+            # but no time to think through the implications for deploy() at
+            # the moment. Grep for {identity_map_matches_condition} for ideas
+            # how to improve this, and the /^\w+\.(\w+)$/ crap below
             next unless ref $rel_info->{cond} eq 'HASH';
 
             my $relsource = dbic_internal_try { $source->related_source($rel) };