Trailing WS crusade - got to save them bits
[dbsrgits/DBIx-Class.git] / lib / DBIx / Class / Ordered.pm
index 6c7446f..0b4384b 100644 (file)
@@ -3,6 +3,9 @@ use strict;
 use warnings;
 use base qw( DBIx::Class );
 
+use List::Util 'first';
+use namespace::clean;
+
 =head1 NAME
 
 DBIx::Class::Ordered - Modify the position of objects in an ordered list.
@@ -17,7 +20,7 @@ Create a table for your ordered data.
     position INTEGER NOT NULL
   );
 
-Optionally, add one or more columns to specify groupings, allowing you 
+Optionally, add one or more columns to specify groupings, allowing you
 to maintain independent ordered lists within one table:
 
   CREATE TABLE items (
@@ -37,12 +40,12 @@ Or even
     other_group_id INTEGER NOT NULL
   );
 
-In your Schema or DB class add "Ordered" to the top 
+In your Schema or DB class add "Ordered" to the top
 of the component list.
 
   __PACKAGE__->load_components(qw( Ordered ... ));
 
-Specify the column that stores the position number for 
+Specify the column that stores the position number for
 each row.
 
   package My::Item;
@@ -86,13 +89,13 @@ That's it, now you can change the position of your objects.
 
 =head1 DESCRIPTION
 
-This module provides a simple interface for modifying the ordered 
+This module provides a simple interface for modifying the ordered
 position of DBIx::Class objects.
 
 =head1 AUTO UPDATE
 
-All of the move_* methods automatically update the rows involved in 
-the query.  This is not configurable and is due to the fact that if you 
+All of the move_* methods automatically update the rows involved in
+the query.  This is not configurable and is due to the fact that if you
 move a record it always causes other records in the list to be updated.
 
 =head1 METHODS
@@ -101,7 +104,7 @@ move a record it always causes other records in the list to be updated.
 
   __PACKAGE__->position_column('position');
 
-Sets and retrieves the name of the column that stores the 
+Sets and retrieves the name of the column that stores the
 positional value of each record.  Defaults to "position".
 
 =cut
@@ -112,8 +115,8 @@ __PACKAGE__->mk_classdata( 'position_column' => 'position' );
 
   __PACKAGE__->grouping_column('group_id');
 
-This method specifies a column to limit all queries in 
-this module by.  This effectively allows you to have multiple 
+This method specifies a column to limit all queries in
+this module by.  This effectively allows you to have multiple
 ordered lists within the same table.
 
 =cut
@@ -127,7 +130,7 @@ __PACKAGE__->mk_classdata( 'grouping_column' );
 This method specifies a value of L</position_column> which B<would
 never be assigned to a row> during normal operation. When
 a row is moved, its position is set to this value temporarily, so
-that any unique constrainst can not be violated. This value defaults
+that any unique constraints can not be violated. This value defaults
 to 0, which should work for all cases except when your positions do
 indeed start from 0.
 
@@ -215,7 +218,7 @@ sub previous_sibling {
 
   my $sibling = $item->first_sibling();
 
-Returns the first sibling object, or 0 if the first sibling 
+Returns the first sibling object, or 0 if the first sibling
 is this sibling.
 
 =cut
@@ -256,7 +259,7 @@ sub next_sibling {
 
   my $sibling = $item->last_sibling();
 
-Returns the last sibling, or 0 if the last sibling is this 
+Returns the last sibling, or 0 if the last sibling is this
 sibling.
 
 =cut
@@ -364,33 +367,20 @@ sub move_to {
 
     my $position_column = $self->position_column;
 
-    my $guard;
-
     if ($self->is_column_changed ($position_column) ) {
-      # something changed our position, we have no idea where we
-      # used to be - requery without using discard_changes
-      # (we need only a specific column back)
-
-      $guard = $self->result_source->schema->txn_scope_guard;
-
-      my $cursor = $self->result_source->resultset->search(
-        $self->ident_condition,
-        { select => $position_column },
-      )->cursor;
-
-      my ($pos) = $cursor->next;
-      $self->$position_column ($pos);
+      # 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});
       delete $self->{_dirty_columns}{$position_column};
     }
 
     my $from_position = $self->_position;
 
     if ( $from_position == $to_position ) {   # FIXME this will not work for non-numeric order
-      $guard->commit if $guard;
       return 0;
     }
 
-    $guard ||= $self->result_source->schema->txn_scope_guard;
+    my $guard = $self->result_source->schema->txn_scope_guard;
 
     my ($direction, @between);
     if ( $from_position < $to_position ) {
@@ -425,7 +415,7 @@ group, or to the end of the group if $position is undef.
 1 is returned on success, and 0 is returned if the object is
 already at the specified position of the specified group.
 
-$group may be specified as a single scalar if only one 
+$group may be specified as a single scalar if only one
 grouping column is in use, or as a hashref of column => value pairs
 if multiple grouping columns are in use.
 
@@ -447,28 +437,13 @@ sub move_to_group {
     return 0 if ( defined($to_position) and $to_position < 1 );
 
     # check if someone changed the _grouping_columns - this will
-    # prevent _is_in_group working, so we need to requery the db
-    # for the original values
-    my (@dirty_cols, %values, $guard);
+    # prevent _is_in_group working, so we need to restore the
+    # original stashed values
     for ($self->_grouping_columns) {
-      $values{$_} = $self->get_column ($_);
-      push @dirty_cols, $_ if $self->is_column_changed ($_);
-    }
-
-    # re-query only the dirty columns, and restore them on the
-    # object (subsequent code will update them to the correct
-    # after-move values)
-    if (@dirty_cols) {
-      $guard = $self->result_source->schema->txn_scope_guard;
-
-      my $cursor = $self->result_source->resultset->search(
-        $self->ident_condition,
-        { select => \@dirty_cols },
-      )->cursor;
-
-      my @original_values = $cursor->next;
-      $self->set_inflated_columns ({ %values, map { $_ => shift @original_values } (@dirty_cols) });
-      delete $self->{_dirty_columns}{$_} for (@dirty_cols);
+      if ($self->is_column_changed ($_)) {
+        $self->store_column($_, delete $self->{_column_data_in_storage}{$_});
+        delete $self->{_dirty_columns}{$_};
+      }
     }
 
     if ($self->_is_in_group ($to_group) ) {
@@ -477,11 +452,10 @@ sub move_to_group {
         $ret = $self->move_to ($to_position);
       }
 
-      $guard->commit if $guard;
       return $ret||0;
     }
 
-    $guard ||= $self->result_source->schema->txn_scope_guard;
+    my $guard = $self->result_source->schema->txn_scope_guard;
 
     # Move to end of current group to adjust siblings
     $self->move_last;
@@ -501,7 +475,7 @@ sub move_to_group {
     }
     else {
       my $bumped_pos_val = $self->_position_value ($to_position);
-      my @between = ($to_position, $new_group_last_position);
+      my @between = map { $self->_position_value ($_) } ($to_position, $new_group_last_position);
       $self->_shift_siblings (1, @between);   #shift right
       $self->set_column( $position_column => $bumped_pos_val );
     }
@@ -515,8 +489,8 @@ sub move_to_group {
 
 =head2 insert
 
-Overrides the DBIC insert() method by providing a default 
-position number.  The default will be the number of rows in 
+Overrides the DBIC insert() method by providing a default
+position number.  The default will be the number of rows in
 the table +1, thus positioning the new record at the last position.
 
 =cut
@@ -549,104 +523,52 @@ of a new group if it has been changed to undef.
 =cut
 
 sub update {
-    my $self = shift;
-
-    # this is set by _ordered_internal_update()
-    return $self->next::method(@_) if $self->{_ORDERED_INTERNAL_UPDATE};
-
-    my $position_column = $self->position_column;
-    my @ordering_columns = ($self->_grouping_columns, $position_column);
-
-
-    # these steps are necessary to keep the external appearance of
-    # ->update($upd) so that other things overloading update() will
-    # work properly
-    my %original_values = $self->get_columns;
-    my %existing_changes = $self->get_dirty_columns;
-
-    # See if any of the *supplied* changes would affect the ordering
-    # The reason this is so contrived, is that we want to leverage
-    # the datatype aware value comparing, while at the same time
-    # keep the original value intact (it will be updated later by the
-    # corresponding routine)
-
-    my %upd = %{shift || {}};
-    my %changes = %existing_changes;
-
-    for (@ordering_columns) {
-        next unless exists $upd{$_};
-
-        # we do not want to keep propagating this to next::method
-        # as it will be a done deal by the time get there
-        my $value = delete $upd{$_};
-        $self->set_inflated_columns ({ $_ => $value });
-
-        # see if an update resulted in a dirty column
-        # it is important to preserve the old value, as it
-        # will be needed to carry on a successfull move()
-        # operation without re-querying the database
-        if ($self->is_column_changed ($_) && not exists $existing_changes{$_}) {
-            $changes{$_} = $value;
-            $self->set_inflated_columns ({ $_ => $original_values{$_} });
-            delete $self->{_dirty_columns}{$_};
-        }
-    }
-
-    # if nothing group/position related changed - short circuit
-    if (not grep { exists $changes{$_} } ( @ordering_columns ) ) {
-        return $self->next::method( \%upd, @_ );
-    }
-
-    {
-        my $guard = $self->result_source->schema->txn_scope_guard;
-
-        # if any of our grouping columns have been changed
-        if (grep { exists $changes{$_} } ($self->_grouping_columns) ) {
-
-            # create new_group by taking the current group and inserting changes
-            my $new_group = {$self->_grouping_clause};
-            foreach my $col (keys %$new_group) {
-                $new_group->{$col} = $changes{$col} if exists $changes{$col};
-            }
-
-            $self->move_to_group(
-                $new_group,
-                (exists $changes{$position_column}
-                    # The FIXME bit contradicts the documentation: POD states that
-                    # when changing groups without supplying explicit positions in
-                    # move_to_group(), we push the item to the end of the group.
-                    # However when I was rewriting this, the position from the old
-                    # group was clearly passed to the new one
-                    # Probably needs to go away (by ribasushi)
-                    ? $changes{$position_column}    # means there was a position change supplied with the update too
-                    : $self->_position              # FIXME! (replace with undef)
-                ),
-            );
-        }
-        elsif (exists $changes{$position_column}) {
-            $self->move_to($changes{$position_column});
-        }
-
-        my @res;
-        my $want = wantarray();
-        if (not defined $want) {
-            $self->next::method( \%upd, @_ );
-        }
-        elsif ($want) {
-            @res = $self->next::method( \%upd, @_ );
-        }
-        else {
-            $res[0] = $self->next::method( \%upd, @_ );
-        }
+  my $self = shift;
+
+  # this is set by _ordered_internal_update()
+  return $self->next::method(@_) if $self->result_source->schema->{_ORDERED_INTERNAL_UPDATE};
+
+  my $upd = shift;
+  $self->set_inflated_columns($upd) if $upd;
+
+  my $position_column = $self->position_column;
+  my @group_columns = $self->_grouping_columns;
+
+  # see if the order is already changed
+  my $changed_ordering_cols = { map { $_ => $self->get_column($_) } grep { $self->is_column_changed($_) } ($position_column, @group_columns) };
+
+  # nothing changed - short circuit
+  if (! keys %$changed_ordering_cols) {
+    return $self->next::method( undef, @_ );
+  }
+  elsif (defined first { 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
+      { $self->_grouping_clause },
+
+      # The FIXME bit contradicts the documentation: POD states that
+      # when changing groups without supplying explicit positions in
+      # move_to_group(), we push the item to the end of the group.
+      # However when I was rewriting this, the position from the old
+      # group was clearly passed to the new one
+      # Probably needs to go away (by ribasushi)
+      (exists $changed_ordering_cols->{$position_column}
+        ? $changed_ordering_cols->{$position_column}  # means there was a position change supplied with the update too
+        : $self->_position                            # FIXME! (replace with undef)
+      ),
+    );
+  }
+  else {
+    $self->move_to($changed_ordering_cols->{$position_column});
+  }
 
-        $guard->commit;
-        return $want ? @res : $res[0];
-    }
+  return $self;
 }
 
 =head2 delete
 
-Overrides the DBIC delete() method by first moving the object 
+Overrides the DBIC delete() method by first moving the object
 to the last position, then deleting it, thus ensuring the
 integrity of the positions.
 
@@ -660,11 +582,10 @@ sub delete {
     $self->move_last;
 
     my @res;
-    my $want = wantarray();
-    if (not defined $want) {
+    if (not defined wantarray) {
         $self->next::method( @_ );
     }
-    elsif ($want) {
+    elsif (wantarray) {
         @res = $self->next::method( @_ );
     }
     else {
@@ -672,7 +593,13 @@ sub delete {
     }
 
     $guard->commit;
-    return $want ? @res : $res[0];
+    return wantarray ? @res : $res[0];
+}
+
+# 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);
 }
 
 =head1 METHODS FOR EXTENDING ORDERED
@@ -682,27 +609,9 @@ You would want to override the methods below if you use sparse
 if you are working with preexisting non-normalised position data,
 or if you need to work with materialized path columns.
 
-=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.
-By default simply returns the value of L</position_column>.
-
-=cut
-sub _position {
-    my $self = shift;
-
-#    #the right way to do this
-#    return $self->previous_siblings->count + 1;
-
-    return $self->get_column ($self->position_column);
-}
-
 =head2 _position_from_value
 
-  my $num_pos = $item->_position_of_value ( $pos_value )
+  my $num_pos = $item->_position_from_value ( $pos_value )
 
 Returns the B<absolute numeric position> of an object with a B<position
 value> set to C<$pos_value>. By default simply returns C<$pos_value>.
@@ -779,11 +688,11 @@ sub _next_position_value {
 Shifts all siblings with B<positions values> in the range @between
 (inclusive) by one position as specified by $direction (left if < 0,
  right if > 0). By default simply increments/decrements each
-L<position_column> value by 1, doing so in a way as to not violate
+L</position_column> value by 1, doing so in a way as to not violate
 any existing constraints.
 
 Note that if you override this method and have unique constraints
-including the L<position_column> the shift is not a trivial task.
+including the L</position_column> the shift is not a trivial task.
 Refer to the implementation source of the default method for more
 information.
 
@@ -810,23 +719,36 @@ sub _shift_siblings {
     # 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)
 
     my $rsrc = $self->result_source;
 
-    if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) ) ) {
-
-        my @pcols = $rsrc->primary_columns;
-        my $cursor = $shift_rs->search ({}, { order_by => { "-$ord", $position_column }, columns => \@pcols } )->cursor;
+    # 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 }} ) )
+    ) {
+        my $cursor = $shift_rs->search (
+          {}, { order_by => { "-$ord", $position_column }, select => [$position_column, @pcols] }
+        )->cursor;
         my $rs = $self->result_source->resultset;
 
-        while (my @pks = $cursor->next ) {
-
+        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]} = $pks[$i];
+            $cond->{$pcols[$i]} = $data->[$i];
           }
 
-          $rs->search($cond)->update ({ $position_column => \ "$position_column $op 1" } );
+          $rs->find($cond)->update ({ $position_column => $pos + ( ($op eq '+') ? 1 : -1 ) });
         }
     }
     else {
@@ -836,7 +758,7 @@ sub _shift_siblings {
 
 =head1 PRIVATE METHODS
 
-These methods are used internally.  You should never have the 
+These methods are used internally.  You should never have the
 need to use them.
 
 =head2 _group_rs
@@ -859,15 +781,32 @@ excluding the object you called this method on.
 sub _siblings {
     my $self = shift;
     my $position_column = $self->position_column;
-    return $self->_group_rs->search(
-        { $position_column => { '!=' => $self->get_column($position_column) } },
-    );
+    my $pos;
+    return defined ($pos = $self->get_column($position_column))
+        ? $self->_group_rs->search(
+            { $position_column => { '!=' => $pos } },
+          )
+        : $self->_group_rs
+    ;
+}
+
+=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
+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 
+by the grouping column(s).  If the grouping column is not defined then
 this will return an empty list.
 
 =cut
@@ -926,7 +865,7 @@ 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 (i.e. duplicate position values within the same group,
+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
@@ -938,7 +877,7 @@ this module.
 
 sub _ordered_internal_update {
     my $self = shift;
-    local $self->{_ORDERED_INTERNAL_UPDATE} = 1;
+    local $self->result_source->schema->{_ORDERED_INTERNAL_UPDATE} = 1;
     return $self->update (@_);
 }
 
@@ -948,9 +887,23 @@ __END__
 
 =head1 CAVEATS
 
+=head2 Resultset Methods
+
+Note that all Insert/Create/Delete overrides are happening on
+L<DBIx::Class::Row> methods only. If you use the
+L<DBIx::Class::ResultSet> versions of
+L<update|DBIx::Class::ResultSet/update> or
+L<delete|DBIx::Class::ResultSet/delete>, all logic present in this
+module will be bypassed entirely (possibly resulting in a broken
+order-tree). Instead always use the
+L<update_all|DBIx::Class::ResultSet/update_all> and
+L<delete_all|DBIx::Class::ResultSet/delete_all> methods, which will
+invoke the corresponding L<row|DBIx::Class::Row> method on every
+member of the given resultset.
+
 =head2 Race Condition on Insert
 
-If a position is not specified for an insert than a position 
+If a position is not specified for an insert, a position
 will be chosen based either on L</_initial_position_value> or
 L</_next_position_value>, depending if there are already some
 items in the current group. The space of time between the
@@ -961,17 +914,17 @@ will prevent such race conditions going undetected.
 
 =head2 Multiple Moves
 
-Be careful when issueing 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 
+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>.
 
-There are times when you will want to move objects as groups, such 
-as changeing 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 
+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.
 
 =head2 Default Values