Optimize some Ordered.pm code
[dbsrgits/DBIx-Class.git] / lib / DBIx / Class / Ordered.pm
index 6566f7b..b3fd102 100644 (file)
@@ -272,6 +272,20 @@ sub last_sibling {
     return defined $lsib ? $lsib : 0;
 }
 
+# an optimised method to get the last sibling position without inflating a row object
+sub _last_sibling_pos {
+    my $self = shift;
+    my $position_column = $self->position_column;
+
+    my $cursor = $self->next_siblings->search(
+        {},
+        { rows => 1, order_by => { '-desc' => $position_column }, columns => $position_column },
+    )->cursor;
+
+    my ($pos) = $cursor->next;
+    return $pos;
+}
+
 =head2 move_previous
 
   $item->move_previous();
@@ -349,8 +363,9 @@ sub move_to {
 
     my $position_column = $self->position_column;
 
-    # FIXME this needs to be wrapped in a transaction
     {
+        my $guard = $self->result_source->schema->txn_scope_guard;
+
         my ($direction, @between);
         if ( $from_position < $to_position ) {
             $direction = -1;
@@ -371,6 +386,8 @@ sub move_to {
         $self->_shift_siblings ($direction, @between);
         $self->_ordered_internal_update({ $position_column => $new_pos_val });
 
+        $guard->commit;
+
         return 1;
     }
 }
@@ -412,8 +429,9 @@ sub move_to_group {
         return $self->move_to ($to_position);
     }
 
-    # FIXME this needs to be wrapped in a transaction
     {
+        my $guard = $self->result_source->schema->txn_scope_guard;
+
         # Move to end of current group to adjust siblings
         $self->move_last;
 
@@ -423,7 +441,7 @@ sub move_to_group {
         if ( not defined($to_position) or $to_position > $new_group_count) {
             $self->set_column(
                 $position_column => $new_group_count
-                    ? $self->_next_position_value ( $self->last_sibling->get_column ($position_column) )    # FIXME - no need to inflate last_sibling
+                    ? $self->_next_position_value ( $self->_last_sibling_pos )
                     : $self->_initial_position_value
             );
         }
@@ -436,6 +454,8 @@ sub move_to_group {
 
         $self->_ordered_internal_update;
 
+        $guard->commit;
+
         return 1;
     }
 }
@@ -453,10 +473,10 @@ sub insert {
     my $position_column = $self->position_column;
 
     unless ($self->get_column($position_column)) {
-        my $lsib = $self->last_sibling;     # FIXME - no need to inflate last_sibling
+        my $lsib_pos = $self->_last_sibling_pos;
         $self->set_column(
-            $position_column => ($lsib
-                ? $self->_next_position_value ( $lsib->get_column ($position_column) )
+            $position_column => (defined $lsib_pos
+                ? $self->_next_position_value ( $lsib_pos )
                 : $self->_initial_position_value
             )
         );
@@ -493,8 +513,9 @@ sub update {
         return $self->next::method( \%changes, @_ );
     }
 
-    # FIXME this needs to be wrapped in a transaction
     {
+        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) ) {
 
@@ -522,7 +543,20 @@ sub update {
             $self->move_to(delete $changes{$position_column});
         }
 
-        return $self->next::method( \%changes, @_ );
+        my @res;
+        my $want = wantarray();
+        if (not defined $want) {
+            $self->next::method( \%changes, @_ );
+        }
+        elsif ($want) {
+            @res = $self->next::method( \%changes, @_ );
+        }
+        else {
+            $res[0] = $self->next::method( \%changes, @_ );
+        }
+
+        $guard->commit;
+        return $want ? @res : $res[0];
     }
 }
 
@@ -536,11 +570,25 @@ integrity of the positions.
 
 sub delete {
     my $self = shift;
-    # FIXME this needs to be wrapped in a transaction
-    {
-        $self->move_last;
-        return $self->next::method( @_ );
+
+    my $guard = $self->result_source->schema->txn_scope_guard;
+
+    $self->move_last;
+
+    my @res;
+    my $want = wantarray();
+    if (not defined $want) {
+        $self->next::method( @_ );
+    }
+    elsif ($want) {
+        @res = $self->next::method( @_ );
     }
+    else {
+        $res[0] = $self->next::method( @_ );
+    }
+
+    $guard->commit;
+    return $want ? @res : $res[0];
 }
 
 =head1 METHODS FOR EXTENDING ORDERED
@@ -658,12 +706,22 @@ sub _shift_siblings {
     # position column is part of a unique constraint, and do a
     # one-by-one update if this is the case
 
-    if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $self->result_source->unique_constraints }} ) ) ) {
+    my $rsrc = $self->result_source;
+
+    if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) ) ) {
 
-        my $rs = $shift_rs->search ({}, { order_by => { "-$ord", $position_column } } );
-        # FIXME - no need to inflate each row
-        while (my $r = $rs->next) {
-            $r->_ordered_internal_update ({ $position_column => \ "$position_column $op 1" } );
+        my @pcols = $rsrc->primary_columns;
+        my $cursor = $shift_rs->search ({}, { order_by => { "-$ord", $position_column }, columns => \@pcols } )->cursor;
+        my $rs = $self->result_source->resultset;
+
+        while (my @pks = $cursor->next ) {
+
+          my $cond;
+          for my $i (0.. $#pcols) {
+            $cond->{$pcols[$i]} = $pks[$i];
+          }
+
+          $rs->search($cond)->update ({ $position_column => \ "$position_column $op 1" } );
         }
     }
     else {
@@ -762,13 +820,13 @@ 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 ambiguos
-pogitioning data (i.e. duplicate position value within the same group,
+Some day you might get confronted by datasets that have ambiguous
+positioning data (i.e. 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 data before
+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 bypases any hooks introduced by
+you are doing use this method which bypasses any hooks introduced by
 this module.
 
 =cut
@@ -818,7 +876,11 @@ could result in the position not being assigned correctly.
 
 =head1 AUTHOR
 
-Aran Deltac <bluefeet@cpan.org>
+ Original code framework
+   Aran Deltac <bluefeet@cpan.org>
+
+ Constraints support and code generalisation
+   Peter Rabbitson <ribasushi@cpan.org>
 
 =head1 LICENSE