Audit and annotate all context-sensitive spots in ::Ordered
[dbsrgits/DBIx-Class.git] / lib / DBIx / Class / Ordered.pm
index 0b4384b..2ac0a07 100644 (file)
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use base qw( DBIx::Class );
 
-use List::Util 'first';
+use DBIx::Class::_Util qw( bag_eq fail_on_internal_call );
 use namespace::clean;
 
 =head1 NAME
@@ -109,7 +109,7 @@ positional value of each record.  Defaults to "position".
 
 =cut
 
-__PACKAGE__->mk_classdata( 'position_column' => 'position' );
+__PACKAGE__->mk_classaccessor( 'position_column' => 'position' );
 
 =head2 grouping_column
 
@@ -121,7 +121,7 @@ ordered lists within the same table.
 
 =cut
 
-__PACKAGE__->mk_classdata( 'grouping_column' );
+__PACKAGE__->mk_group_accessors( inherited => 'grouping_column' );
 
 =head2 null_position_value
 
@@ -136,7 +136,7 @@ indeed start from 0.
 
 =cut
 
-__PACKAGE__->mk_classdata( 'null_position_value' => 0 );
+__PACKAGE__->mk_classaccessor( 'null_position_value' => 0 );
 
 =head2 siblings
 
@@ -146,13 +146,28 @@ __PACKAGE__->mk_classdata( '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 L</_siblings>
+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
@@ -163,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
@@ -182,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
@@ -211,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
@@ -232,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
@@ -252,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
@@ -272,10 +315,10 @@ 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 row object
+# an optimized method to get the last sibling position value without inflating a result object
 sub _last_sibling_posval {
     my $self = shift;
     my $position_column = $self->position_column;
@@ -285,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
@@ -367,7 +409,31 @@ sub move_to {
 
     my $position_column = $self->position_column;
 
-    if ($self->is_column_changed ($position_column) ) {
+    my $rsrc = $self->result_source;
+
+    my $is_txn;
+    if ($is_txn = $rsrc->schema->storage->transaction_depth) {
+      # Reload position state from storage
+      # The thinking here is that if we are in a transaction, it is
+      # *more likely* the object went out of sync due to resultset
+      # level shenanigans. Instead of always reloading (slow) - go
+      # ahead and hand-hold only in the case of higher layers
+      # requesting the safety of a txn
+
+      $self->store_column(
+        $position_column,
+        (  $rsrc->resultset
+                 ->search($self->_storage_ident_condition, { rows => 1, columns => $position_column })
+                  ->cursor
+                   ->next
+        )[0] || $self->throw_exception(
+          sprintf "Unable to locate object '%s' in storage - object went ouf of sync...?",
+          $self->ID
+        ),
+      );
+      delete $self->{_dirty_columns}{$position_column};
+    }
+    elsif ($self->is_column_changed ($position_column) ) {
       # something changed our position, we need to know where we
       # used to be - use the stashed value
       $self->store_column($position_column, delete $self->{_column_data_in_storage}{$position_column});
@@ -380,7 +446,7 @@ sub move_to {
       return 0;
     }
 
-    my $guard = $self->result_source->schema->txn_scope_guard;
+    my $guard = $is_txn ? undef : $rsrc->schema->txn_scope_guard;
 
     my ($direction, @between);
     if ( $from_position < $to_position ) {
@@ -395,14 +461,14 @@ sub move_to {
     my $new_pos_val = $self->_position_value ($to_position);  # record this before the shift
 
     # we need to null-position the moved row if the position column is part of a constraint
-    if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $self->result_source->unique_constraints }} ) ) ) {
+    if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) ) ) {
       $self->_ordered_internal_update({ $position_column => $self->null_position_value });
     }
 
     $self->_shift_siblings ($direction, @between);
     $self->_ordered_internal_update({ $position_column => $new_pos_val });
 
-    $guard->commit;
+    $guard->commit if $guard;
     return 1;
 }
 
@@ -541,7 +607,7 @@ sub update {
   if (! keys %$changed_ordering_cols) {
     return $self->next::method( undef, @_ );
   }
-  elsif (defined first { exists $changed_ordering_cols->{$_} } @group_columns ) {
+  elsif (grep { exists $changed_ordering_cols->{$_} } @group_columns ) {
     $self->move_to_group(
       # since the columns are already re-set the _grouping_clause is correct
       # move_to_group() knows how to get the original storage values
@@ -581,25 +647,21 @@ sub delete {
 
     $self->move_last;
 
-    my @res;
-    if (not defined wantarray) {
-        $self->next::method( @_ );
-    }
-    elsif (wantarray) {
-        @res = $self->next::method( @_ );
-    }
-    else {
-        $res[0] = $self->next::method( @_ );
-    }
+    $self->next::method( @_ );
 
     $guard->commit;
-    return wantarray ? @res : $res[0];
+
+    return $self;
 }
 
 # add the current position/group to the things we track old values for
 sub _track_storage_value {
   my ($self, $col) = @_;
-  return $self->next::method($col) || defined first { $_ eq $col } ($self->position_column, $self->_grouping_columns);
+  return (
+    $self->next::method($col)
+      ||
+    grep { $_ eq $col } ($self->position_column, $self->_grouping_columns)
+  );
 }
 
 =head1 METHODS FOR EXTENDING ORDERED
@@ -663,7 +725,7 @@ L</_next_position_value> below. Defaults to 1.
 
 =cut
 
-__PACKAGE__->mk_classdata( '_initial_position_value' => 1 );
+__PACKAGE__->mk_classaccessor( '_initial_position_value' => 1 );
 
 =head2 _next_position_value
 
@@ -715,40 +777,25 @@ sub _shift_siblings {
 
     my $shift_rs = $self->_group_rs-> search ({ $position_column => { -between => \@between } });
 
-    # some databases (sqlite) are dumb and can not do a blanket
-    # increment/decrement. So what we do here is check if the
-    # position column is part of a unique constraint, and do a
-    # one-by-one update if this is the case
-    # Also we do a one-by-one if the position is part of the PK
-    # since once we update a column via scalarref we lose the
-    # ability to retrieve this column back (we do not know the
-    # id anymore)
-
+    # some databases (sqlite, pg, perhaps others) are dumb and can not do a
+    # blanket increment/decrement without violating a unique constraint.
+    # So what we do here is check if the position column is part of a unique
+    # constraint, and do a one-by-one update if this is the case.
     my $rsrc = $self->result_source;
 
     # set in case there are more cascades combined with $rs->update => $rs_update_all overrides
     local $rsrc->schema->{_ORDERED_INTERNAL_UPDATE} = 1;
     my @pcols = $rsrc->primary_columns;
-    my $pos_is_pk = first { $_ eq $position_column } @pcols;
     if (
-      $pos_is_pk
-        or
-      first { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) )
+      grep { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) )
     ) {
-        my $cursor = $shift_rs->search (
+        my $clean_rs = $rsrc->resultset;
+
+        for ( $shift_rs->search (
           {}, { order_by => { "-$ord", $position_column }, select => [$position_column, @pcols] }
-        )->cursor;
-        my $rs = $self->result_source->resultset;
-
-        my @all_data = $cursor->all;
-        while (my $data = shift @all_data) {
-          my $pos = shift @$data;
-          my $cond;
-          for my $i (0.. $#pcols) {
-            $cond->{$pcols[$i]} = $data->[$i];
-          }
-
-          $rs->find($cond)->update ({ $position_column => $pos + ( ($op eq '+') ? 1 : -1 ) });
+        )->cursor->all ) {
+          my $pos = shift @$_;
+          $clean_rs->find(@$_)->update ({ $position_column => $pos + ( ($op eq '+') ? 1 : -1 ) });
         }
     }
     else {
@@ -756,33 +803,41 @@ sub _shift_siblings {
     }
 }
 
-=head1 PRIVATE METHODS
 
-These methods are used internally.  You should never have the
-need to use them.
-
-=head2 _group_rs
-
-This method returns a resultset containing all members of the row
-group (including the row itself).
-
-=cut
+# 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() });
 }
 
-=head2 _siblings
-
-Returns an unordered resultset of all objects in the same group
-excluding the object you called this method on.
-
-=cut
+# Returns an unordered resultset of all objects in the same group
+# excluding the object you called this method on.
 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 } },
           )
@@ -790,38 +845,24 @@ sub _siblings {
     ;
 }
 
-=head2 _position
-
-  my $num_pos = $item->_position;
-
-Returns the B<absolute numeric position> of the current object, with the
-first object being at position 1, its sibling at position 2 and so on.
-
-=cut
+# Returns the B<absolute numeric position> of the current object, with the
+# first object being at position 1, its sibling at position 2 and so on.
 sub _position {
     my $self = shift;
     return $self->_position_from_value ($self->get_column ($self->position_column) );
 }
 
-=head2 _grouping_clause
-
-This method returns one or more name=>value pairs for limiting a search
-by the grouping column(s).  If the grouping column is not defined then
-this will return an empty list.
-
-=cut
+# This method returns one or more name=>value pairs for limiting a search
+# by the grouping column(s).  If the grouping column is not defined then
+# this will return an empty list.
 sub _grouping_clause {
     my( $self ) = @_;
     return map {  $_ => $self->get_column($_)  } $self->_grouping_columns();
 }
 
-=head2 _get_grouping_columns
-
-Returns a list of the column names used for grouping, regardless of whether
-they were specified as an arrayref or a single string, and returns ()
-if there is no grouping.
-
-=cut
+# Returns a list of the column names used for grouping, regardless of whether
+# they were specified as an arrayref or a single string, and returns ()
+# if there is no grouping.
 sub _grouping_columns {
     my( $self ) = @_;
     my $col = $self->grouping_column();
@@ -834,51 +875,48 @@ sub _grouping_columns {
     }
 }
 
-=head2 _is_in_group
-
-    $item->_is_in_group( {user => 'fred', list => 'work'} )
-
-Returns true if the object is in the group represented by hashref $other
-
-=cut
+# Returns true if the object is in the group represented by hashref $other
 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;
 }
 
-=head2 _ordered_internal_update
-
-This is a short-circuited method, that is used internally by this
-module to update positioning values in isolation (i.e. without
-triggering any of the positioning integrity code).
-
-Some day you might get confronted by datasets that have ambiguous
-positioning data (e.g. duplicate position values within the same group,
-in a table without unique constraints). When manually fixing such data
-keep in mind that you can not invoke L<DBIx::Class::Row/update> like
-you normally would, as it will get confused by the wrong data before
-having a chance to update the ill-defined row. If you really know what
-you are doing use this method which bypasses any hooks introduced by
-this module.
-
-=cut
-
+# This is a short-circuited method, that is used internally by this
+# module to update positioning values in isolation (i.e. without
+# triggering any of the positioning integrity code).
+#
+# Some day you might get confronted by datasets that have ambiguous
+# positioning data (e.g. duplicate position values within the same group,
+# in a table without unique constraints). When manually fixing such data
+# keep in mind that you can not invoke L<DBIx::Class::Row/update> like
+# you normally would, as it will get confused by the wrong data before
+# having a chance to update the ill-defined row. If you really know what
+# 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;
@@ -914,33 +952,31 @@ will prevent such race conditions going undetected.
 
 =head2 Multiple Moves
 
-Be careful when issuing move_* methods to multiple objects.  If
-you've pre-loaded the objects then when you move one of the objects
-the position of the other object will not reflect their new value
-until you reload them from the database - see
-L<DBIx::Class::Row/discard_changes>.
+If you have multiple same-group result objects already loaded from storage,
+you need to be careful when executing C<move_*> operations on them:
+without a L</position_column> reload the L</_position_value> of the
+"siblings" will be out of sync with the underlying storage.
+
+Starting from version C<0.082800> DBIC will implicitly perform such
+reloads when the C<move_*> happens as a part of a transaction
+(a good example of such situation is C<< $ordered_resultset->delete_all >>).
 
-There are times when you will want to move objects as groups, such
-as changing the parent of several objects at once - this directly
-conflicts with this problem.  One solution is for us to write a
-ResultSet class that supports a parent() method, for example.  Another
-solution is to somehow automagically modify the objects that exist
-in the current object's result set to have the new position value.
+If it is not possible for you to wrap the entire call-chain in a transaction,
+you will need to call L<DBIx::Class::Row/discard_changes> to get an object
+up-to-date before proceeding, otherwise undefined behavior will result.
 
 =head2 Default Values
 
 Using a database defined default_value on one of your group columns
 could result in the position not being assigned correctly.
 
-=head1 AUTHOR
-
- Original code framework
-   Aran Deltac <bluefeet@cpan.org>
-
- Constraints support and code generalisation
-   Peter Rabbitson <ribasushi@cpan.org>
+=head1 FURTHER QUESTIONS?
 
-=head1 LICENSE
+Check the list of L<additional DBIC resources|DBIx::Class/GETTING HELP/SUPPORT>.
 
-You may distribute this code under the same terms as Perl itself.
+=head1 COPYRIGHT AND LICENSE
 
+This module is free software L<copyright|DBIx::Class/COPYRIGHT AND LICENSE>
+by the L<DBIx::Class (DBIC) authors|DBIx::Class/AUTHORS>. You can
+redistribute it and/or modify it under the same terms as the
+L<DBIx::Class library|DBIx::Class/COPYRIGHT AND LICENSE>.