From: Peter Rabbitson <ribasushi@cpan.org>
Date: Mon, 17 Jan 2011 10:50:07 +0000 (+0100)
Subject: Rewrite the check for nontrivial joinfree conditions to throw on both new_related... 
X-Git-Tag: v0.08190~1^2
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=78b948c3f914722aaecae145119500aa0f46ac74;p=dbsrgits%2FDBIx-Class.git

Rewrite the check for nontrivial joinfree conditions to throw on both new_related and set_from_related
---

diff --git a/lib/DBIx/Class/Relationship/Base.pm b/lib/DBIx/Class/Relationship/Base.pm
index 7100ea1..506d51d 100644
--- a/lib/DBIx/Class/Relationship/Base.pm
+++ b/lib/DBIx/Class/Relationship/Base.pm
@@ -556,7 +556,36 @@ on it.
 
 sub new_related {
   my ($self, $rel, $values, $attrs) = @_;
-  return $self->search_related($rel)->new($values, $attrs);
+
+  # FIXME - this is a bad position for this (also an identical copy in
+  # set_from_related), but I have no saner way to hook, and I absolutely
+  # want this to throw at least for coderefs, instead of the "insert a NULL
+  # when it gets hard" insanity --ribasushi
+  #
+  # sanity check - currently throw when a complex coderef rel is encountered
+  # FIXME - should THROW MOAR!
+
+  if (ref $self) {  # cdbi calls this as a class method, /me vomits
+
+    my $rsrc = $self->result_source;
+    my (undef, $crosstable, $relcols) = $rsrc->_resolve_condition (
+      $rsrc->relationship_info($rel)->{cond}, $rel, $self, $rel
+    );
+
+    $self->throw_exception("Custom relationship '$rel' does not resolve to a join-free condition fragment")
+      if $crosstable;
+
+    if (@{$relcols || []} and @$relcols = grep { ! exists $values->{$_} } @$relcols) {
+      $self->throw_exception(sprintf (
+        "Custom relationship '%s' not definitive - returns conditions instead of values for column(s): %s",
+        $rel,
+        map { "'$_'" } @$relcols
+      ));
+    }
+  }
+
+  my $row = $self->search_related($rel)->new($values, $attrs);
+  return $row;
 }
 
 =head2 create_related
@@ -572,41 +601,7 @@ in L<DBIx::Class::ResultSet> for details.
 sub create_related {
   my $self = shift;
   my $rel = shift;
-
-  $self->throw_exception("Can't call *_related as class methods")
-    unless ref $self;
-
-  # we need to stop and check if this is at all possible. If this is
-  # an extended relationship with an incomplete definition, we should
-  # just forbid it right now.
-  my $rel_info = $self->result_source->relationship_info($rel);
-  if (ref $rel_info->{cond} eq 'CODE') {
-    my ($cond, $ext) = $rel_info->{cond}->({
-      self_alias => 'me',
-      foreign_alias => $rel,
-      self_rowobj => $self,
-      self_resultsource => $self->result_source,
-      foreign_relname => $rel,
-    });
-    $self->throw_exception("unable to set_from_related - no simplified condition available for '${rel}'")
-      unless $ext;
-
-    # now we need to make sure all non-identity relationship
-    # definitions are overriden.
-    my ($argref) = @_;
-    while ( my($col, $value) = each %$ext ) {
-      $col =~ s/^$rel\.//;
-      my $vref = ref $value;
-      if ($vref eq 'HASH') {
-        if (keys(%$value) && (keys %$value)[0] ne '=' &&
-            !exists $argref->{$col}) {
-          $self->throw_exception("unable to set_from_related via complex '${rel}' condition on column(s): '${col}'")
-        }
-      }
-    }
-  }
-
-  my $obj = $self->search_related($rel)->create(@_);
+  my $obj = $self->new_related($rel, @_)->insert;
   delete $self->{related_resultsets}->{$rel};
   return $obj;
 }
@@ -693,7 +688,8 @@ set them in the storage.
 sub set_from_related {
   my ($self, $rel, $f_obj) = @_;
 
-  my $rel_info = $self->relationship_info($rel)
+  my $rsrc = $self->result_source;
+  my $rel_info = $rsrc->relationship_info($rel)
     or $self->throw_exception( "No such relationship ${rel}" );
 
   if (defined $f_obj) {
@@ -702,12 +698,24 @@ sub set_from_related {
       unless blessed $f_obj and $f_obj->isa($f_class);
   }
 
-  my ($cond, $crosstable) = $self->result_source->_resolve_condition
-    ($rel_info->{cond}, $f_obj, $rel, $rel);
 
-  $self->throw_exception(
-    "Custom relationship '$rel' does not resolve to a join-free condition fragment"
-  ) if $crosstable;
+  # FIXME - this is a bad position for this (also an identical copy in
+  # new_related), but I have no saner way to hook, and I absolutely
+  # want this to throw at least for coderefs, instead of the "insert a NULL
+  # when it gets hard" insanity --ribasushi
+  #
+  # sanity check - currently throw when a complex coderef rel is encountered
+  # FIXME - should THROW MOAR!
+  my ($cond, $crosstable, $relcols) = $rsrc->_resolve_condition (
+    $rel_info->{cond}, $f_obj, $rel, $rel
+  );
+  $self->throw_exception("Custom relationship '$rel' does not resolve to a join-free condition fragment")
+    if $crosstable;
+  $self->throw_exception(sprintf (
+    "Custom relationship '%s' not definitive - returns conditions instead of values for column(s): %s",
+    $rel,
+    map { "'$_'" } @$relcols
+  )) if @{$relcols || []};
 
   $self->set_columns($cond);
 
diff --git a/lib/DBIx/Class/ResultSource.pm b/lib/DBIx/Class/ResultSource.pm
index 63a8ddf..53e866d 100644
--- a/lib/DBIx/Class/ResultSource.pm
+++ b/lib/DBIx/Class/ResultSource.pm
@@ -1549,7 +1549,9 @@ sub resolve_condition {
 our $UNRESOLVABLE_CONDITION = \ '1 = 0';
 
 # Resolves the passed condition to a concrete query fragment and a flag
-# indicating whether this is a cross-table condition.
+# indicating whether this is a cross-table condition. Also an optional
+# list of non-triviail values (notmally conditions) returned as a part
+# of a joinfree condition hash
 sub _resolve_condition {
   my ($self, $cond, $as, $for, $relname) = @_;
 
@@ -1566,6 +1568,7 @@ sub _resolve_condition {
       self_rowobj => $obj_rel ? $for : undef
     });
 
+    my $cond_cols;
     if ($joinfree_cond) {
 
       # FIXME sanity check until things stabilize, remove at some point
@@ -1596,7 +1599,30 @@ sub _resolve_condition {
         );
       }
 
-      return wantarray ? ($joinfree_cond, 0) : $joinfree_cond;
+      # see which parts of the joinfree cond are conditionals
+      my $relcol_list = { map { $_ => 1 } $self->related_source($relname)->columns };
+
+      for my $c (keys %$joinfree_cond) {
+        my ($colname) = $c =~ /^ (?: \Q$relalias.\E )? (.+)/x;
+
+        unless ($relcol_list->{$colname}) {
+          push @$cond_cols, $colname;
+          next;
+        }
+
+        if (
+          ref $joinfree_cond->{$c}
+            and
+          ref $joinfree_cond->{$c} ne 'SCALAR'
+            and
+          ref $joinfree_cond->{$c} ne 'REF'
+        ) {
+          push @$cond_cols, $colname;
+          next;
+        }
+      }
+
+      return wantarray ? ($joinfree_cond, 0, $cond_cols) : $joinfree_cond;
     }
     else {
       return wantarray ? ($crosstable_cond, 1) : $crosstable_cond;
@@ -1662,7 +1688,6 @@ sub _resolve_condition {
   }
 }
 
-
 # Accepts one or more relationships for the current source and returns an
 # array of column names for each of those relationships. Column names are
 # prefixed relative to the current source, in accordance with where they appear
diff --git a/t/relationship/custom.t b/t/relationship/custom.t
index 5d44d18..e60bad6 100644
--- a/t/relationship/custom.t
+++ b/t/relationship/custom.t
@@ -110,7 +110,8 @@ is_deeply(
 # try to create_related a 80s cd
 throws_ok {
   $artist->create_related('cds_80s', { title => 'related creation 1' });
-} qr/\Qunable to set_from_related via complex 'cds_80s' condition on column(s): 'year'/, 'Create failed - complex cond';
+} qr/\QCustom relationship 'cds_80s' not definitive - returns conditions instead of values for column(s): 'year'/,
+'Create failed - complex cond';
 
 # now supply an explicit arg overwriting the ambiguous cond
 my $id_2020 = $artist->create_related('cds_80s', { title => 'related creation 2', year => '2020' })->id;
@@ -131,7 +132,8 @@ is(
 # try a specific everything via a non-simplified rel
 throws_ok {
   $artist->create_related('cds_90s', { title => 'related_creation 4', year => '2038' });
-} qr/\Qunable to set_from_related - no simplified condition available for 'cds_90s'/, 'Create failed - non-simplified rel';
+} qr/\QCustom relationship 'cds_90s' does not resolve to a join-free condition fragment/,
+'Create failed - non-simplified rel';
 
 # Do a self-join last-entry search
 my @last_tracks;