Audit and annotate all context-sensitive spots in ::Ordered
Peter Rabbitson [Fri, 30 Sep 2016 13:15:36 +0000 (15:15 +0200)]
Ensure an upcoming commit will not disturb the established (silly but still)
API of the resultset-returning methods. Review, annotate and tighten up spots
that have to do with wantarray-like behavior

Not using the ASSERT_NO_INTERNAL_WANTARRAY macro as it is about to be retired
in a subsequent commit. Instead adjust the INDIRECT guard to correctly interpret
eval frames

Zero functional changes

lib/DBIx/Class/Ordered.pm
lib/DBIx/Class/_Util.pm

index bf7f954..2ac0a07 100644 (file)
@@ -3,6 +3,9 @@ use strict;
 use warnings;
 use base qw( DBIx::Class );
 
+use DBIx::Class::_Util qw( bag_eq fail_on_internal_call );
+use namespace::clean;
+
 =head1 NAME
 
 DBIx::Class::Ordered - Modify the position of objects in an ordered list.
@@ -143,13 +146,28 @@ __PACKAGE__->mk_classaccessor( 'null_position_value' => 0 );
 Returns an B<ordered> resultset of all other objects in the same
 group excluding the one you called it on.
 
+Underneath calls L<DBIx::Class::ResultSet/search>, and therefore returns
+objects by implicitly invoking L<C<< ->all() >>|DBIx::Class::ResultSet/all>
+in list context.
+
 The ordering is a backwards-compatibility artifact - if you need
 a resultset with no ordering applied use C<_siblings>
 
 =cut
+
 sub siblings {
-    my $self = shift;
-    return $self->_siblings->search ({}, { order_by => $self->position_column } );
+    #my $self = shift;
+
+    DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS
+      and
+    wantarray
+      and
+    ! eval { fail_on_internal_call; 1 }
+      and
+    die "ILLEGAL LIST CONTEXT INVOCATION: $@";
+
+    # *MUST* be context sensitive due to legacy (DO NOT call search_rs)
+    $_[0]->_siblings->search ({}, { order_by => $_[0]->position_column } );
 }
 
 =head2 previous_siblings
@@ -160,15 +178,29 @@ sub siblings {
 Returns a resultset of all objects in the same group
 positioned before the object on which this method was called.
 
+Underneath calls L<DBIx::Class::ResultSet/search>, and therefore returns
+objects by implicitly invoking L<C<< ->all() >>|DBIx::Class::ResultSet/all>
+in list context.
+
 =cut
 sub previous_siblings {
     my $self = shift;
     my $position_column = $self->position_column;
     my $position = $self->get_column ($position_column);
-    return ( defined $position
+
+    DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS
+      and
+    wantarray
+      and
+    ! eval { fail_on_internal_call; 1 }
+      and
+    die "ILLEGAL LIST CONTEXT INVOCATION: $@";
+
+    # *MUST* be context sensitive due to legacy (DO NOT call search_rs)
+    defined( $position )
         ? $self->_siblings->search ({ $position_column => { '<', $position } })
         : $self->_siblings
-    );
+    ;
 }
 
 =head2 next_siblings
@@ -179,15 +211,29 @@ sub previous_siblings {
 Returns a resultset of all objects in the same group
 positioned after the object on which this method was called.
 
+Underneath calls L<DBIx::Class::ResultSet/search>, and therefore returns
+objects by implicitly invoking L<C<< ->all() >>|DBIx::Class::ResultSet/all>
+in list context.
+
 =cut
 sub next_siblings {
     my $self = shift;
     my $position_column = $self->position_column;
     my $position = $self->get_column ($position_column);
-    return ( defined $position
+
+    DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS
+      and
+    wantarray
+      and
+    ! eval { fail_on_internal_call; 1 }
+      and
+    die "ILLEGAL LIST CONTEXT INVOCATION: $@";
+
+    # *MUST* be context sensitive due to legacy (DO NOT call search_rs)
+    defined( $position )
         ? $self->_siblings->search ({ $position_column => { '>', $position } })
         : $self->_siblings
-    );
+    ;
 }
 
 =head2 previous_sibling
@@ -208,7 +254,7 @@ sub previous_sibling {
         { rows => 1, order_by => { '-desc' => $position_column } },
     )->single;
 
-    return defined $psib ? $psib : 0;
+    return defined( $psib ) ? $psib : 0;
 }
 
 =head2 first_sibling
@@ -229,7 +275,7 @@ sub first_sibling {
         { rows => 1, order_by => { '-asc' => $position_column } },
     )->single;
 
-    return defined $fsib ? $fsib : 0;
+    return defined( $fsib ) ? $fsib : 0;
 }
 
 =head2 next_sibling
@@ -249,7 +295,7 @@ sub next_sibling {
         { rows => 1, order_by => { '-asc' => $position_column } },
     )->single;
 
-    return defined $nsib ? $nsib : 0;
+    return defined( $nsib ) ? $nsib : 0;
 }
 
 =head2 last_sibling
@@ -269,7 +315,7 @@ sub last_sibling {
         { rows => 1, order_by => { '-desc' => $position_column } },
     )->single;
 
-    return defined $lsib ? $lsib : 0;
+    return defined( $lsib ) ? $lsib : 0;
 }
 
 # an optimized method to get the last sibling position value without inflating a result object
@@ -282,8 +328,7 @@ sub _last_sibling_posval {
         { rows => 1, order_by => { '-desc' => $position_column }, select => $position_column },
     )->cursor;
 
-    my ($pos) = $cursor->next;
-    return $pos;
+    ($cursor->next)[0];
 }
 
 =head2 move_previous
@@ -762,8 +807,18 @@ sub _shift_siblings {
 # This method returns a resultset containing all members of the row
 # group (including the row itself).
 sub _group_rs {
-    my $self = shift;
-    return $self->result_source->resultset->search({$self->_grouping_clause()});
+    #my $self = shift;
+
+    DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS
+      and
+    wantarray
+      and
+    ! eval { fail_on_internal_call; 1 }
+      and
+    die "ILLEGAL LIST CONTEXT INVOCATION: $@";
+
+    # *MUST* be context sensitive due to legacy (DO NOT call search_rs)
+    $_[0]->result_source->resultset->search({ $_[0]->_grouping_clause() });
 }
 
 # Returns an unordered resultset of all objects in the same group
@@ -772,7 +827,17 @@ sub _siblings {
     my $self = shift;
     my $position_column = $self->position_column;
     my $pos;
-    return defined ($pos = $self->get_column($position_column))
+
+    DBIx::Class::_ENV_::ASSERT_NO_INTERNAL_INDIRECT_CALLS
+      and
+    wantarray
+      and
+    ! eval { fail_on_internal_call; 1 }
+      and
+    die "ILLEGAL LIST CONTEXT INVOCATION: $@";
+
+    # *MUST* be context sensitive due to legacy (DO NOT call search_rs)
+    defined( $pos = $self->get_column($position_column) )
         ? $self->_group_rs->search(
             { $position_column => { '!=' => $pos } },
           )
@@ -815,17 +880,26 @@ sub _is_in_group {
     my ($self, $other) = @_;
     my $current = {$self->_grouping_clause};
 
-    no warnings qw/uninitialized/;
-
-    return 0 if (
-        join ("\x00", sort keys %$current)
-            ne
-        join ("\x00", sort keys %$other)
-    );
-    for my $key (keys %$current) {
-        return 0 if $current->{$key} ne $other->{$key};
-    }
-    return 1;
+    (
+      bag_eq(
+        [ keys %$current ],
+        [ keys %$other ],
+      )
+        and
+      ! grep {
+        (
+          defined( $current->{$_} )
+            xor
+          defined( $other->{$_} )
+        )
+          or
+        (
+          defined $current->{$_}
+            and
+          $current->{$_} ne $other->{$_}
+        )
+      } keys %$other
+    ) ? 1 : 0;
 }
 
 # This is a short-circuited method, that is used internally by this
@@ -841,9 +915,8 @@ sub _is_in_group {
 # you are doing use this method which bypasses any hooks introduced by
 # this module.
 sub _ordered_internal_update {
-    my $self = shift;
-    local $self->result_source->schema->{_ORDERED_INTERNAL_UPDATE} = 1;
-    return $self->update (@_);
+    local $_[0]->result_source->schema->{_ORDERED_INTERNAL_UPDATE} = 1;
+    shift->update (@_);
 }
 
 1;
index 2d2caaa..147614f 100644 (file)
@@ -1126,38 +1126,7 @@ sub mkdir_p ($) {
 }
 
 sub fail_on_internal_call {
-  my ($fr, $argdesc);
-  {
-    package DB;
-    $fr = [ CORE::caller(1) ];
-
-    # screwing with $DB::args is rather volatile - be extra careful
-    no warnings 'uninitialized';
-
-    $argdesc =
-      ( not defined $DB::args[0] )  ? 'UNAVAILABLE'
-    : ( length ref $DB::args[0] )   ? DBIx::Class::_Util::refdesc($DB::args[0])
-    : $DB::args[0] . ''
-    ;
-  };
-
-  my @fr2;
-  # need to make allowance for a proxy-yet-direct call
-  my $check_fr = (
-    $fr->[0] eq 'DBIx::Class::ResultSourceProxy'
-      and
-    @fr2 = (CORE::caller(2))
-      and
-    (
-      ( $fr->[3] =~ /([^:])+$/ )[0]
-        eq
-      ( $fr2[3] =~ /([^:])+$/ )[0]
-    )
-  )
-    ? \@fr2
-    : $fr
-  ;
-
+  my $fr = [ CORE::caller(1) ];
 
   die "\nMethod $fr->[3] is not marked with the 'DBIC_method_is_indirect_sugar' attribute\n\n" unless (
 
@@ -1194,12 +1163,36 @@ sub fail_on_internal_call {
   );
 
 
+  my @fr2;
+  # need to make allowance for a proxy-yet-direct call
+  # or for an exception wrapper
+  $fr = \@fr2 if (
+    (
+      $fr->[3] eq '(eval)'
+        and
+      @fr2 = (CORE::caller(2))
+    )
+      or
+    (
+      $fr->[0] eq 'DBIx::Class::ResultSourceProxy'
+        and
+      @fr2 = (CORE::caller(2))
+        and
+      (
+        ( $fr->[3] =~ /([^:])+$/ )[0]
+          eq
+        ( $fr2[3] =~ /([^:])+$/ )[0]
+      )
+    )
+  );
+
+
   if (
     defined $fr->[0]
       and
-    $check_fr->[0] =~ /^(?:DBIx::Class|DBICx::)/
+    $fr->[0] =~ /^(?:DBIx::Class|DBICx::)/
       and
-    $check_fr->[1] !~ /\b(?:CDBICompat|ResultSetProxy)\b/  # no point touching there
+    $fr->[1] !~ /\b(?:CDBICompat|ResultSetProxy)\b/  # no point touching there
       and
     # one step higher
     @fr2 = CORE::caller(@fr2 ? 3 : 2)
@@ -1212,6 +1205,24 @@ sub fail_on_internal_call {
         attributes::get( \&{ $fr2[3] })
       }
   ) {
+
+    my $argdesc;
+
+    {
+      package DB;
+
+      my @throwaway = caller( @fr2 ? 2 : 1 );
+
+      # screwing with $DB::args is rather volatile - be extra careful
+      no warnings 'uninitialized';
+
+      $argdesc =
+        ( not defined $DB::args[0] )  ? 'UNAVAILABLE'
+      : ( length ref $DB::args[0] )   ? DBIx::Class::_Util::refdesc($DB::args[0])
+      : $DB::args[0] . ''
+      ;
+    };
+
     DBIx::Class::Exception->throw( sprintf (
       "Illegal internal call of indirect proxy-method %s() with argument '%s': examine the last lines of the proxy method deparse below to determine what to call directly instead at %s on line %d\n\n%s\n\n    Stacktrace starts",
       $fr->[3], $argdesc, @{$fr}[1,2], ( $fr->[6] || do {