Rewrite handling of "storage-side old values" both for PK/ident and for
Peter Rabbitson [Sun, 5 Jun 2011 05:29:42 +0000 (07:29 +0200)]
Ordered position/grouping

Massive code simplification, also opens the door to vastly simplifying
(and optimizing) DBIx::Class::Helper::Row::StorageValues, as only
differing values are kept by the core

Changes
lib/DBIx/Class/Ordered.pm
lib/DBIx/Class/PK.pm
lib/DBIx/Class/Row.pm
t/87ordered.t

diff --git a/Changes b/Changes
index 4f1167c..9b4179e 100644 (file)
--- a/Changes
+++ b/Changes
@@ -4,6 +4,8 @@ Revision history for DBIx::Class
         - Allow schema cloning to mutate attributes
         - DBIC now attempts more aggressive de-duplication of where
           conditions on resultset chaining
+        - The Ordered component is now smarter wrt reordering of dirty
+          objects, and does its job with less storage queries
 
     * Fixes
         - Fix issue where the query was becoming overly mangled when trying
index 06e842c..54c6d46 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.
@@ -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 ) {
@@ -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;
@@ -549,103 +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;
-        if (not defined wantarray) {
-            $self->next::method( \%upd, @_ );
-        }
-        elsif (wantarray) {
-            @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->{_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 wantarray ? @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.
 
@@ -673,6 +596,12 @@ sub delete {
     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
 
 You would want to override the methods below if you use sparse
@@ -839,9 +768,12 @@ 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) } },
-    );
+    return defined (my $pos = $self->get_column($position_column))
+        ? $self->_group_rs->search(
+            { $position_column => { '!=' => $self->get_column($position_column) } },
+          )
+        : $self->_group_rs
+    ;
 }
 
 =head2 _position
index c4d3b93..722dcf6 100644 (file)
@@ -41,7 +41,10 @@ sub _ident_values {
   my (@ids, @missing);
 
   for ($self->_pri_cols) {
-    push @ids, $self->get_column($_);
+    push @ids, exists $self->{_column_data_in_storage}{$_}
+      ? $self->{_column_data_in_storage}{$_}
+      : $self->get_column($_)
+    ;
     push @missing, $_ if (! defined $ids[-1] and ! $self->has_column_loaded ($_) );
   }
 
index 2280c50..1984d6c 100644 (file)
@@ -7,6 +7,7 @@ use base qw/DBIx::Class/;
 
 use DBIx::Class::Exception;
 use Scalar::Util 'blessed';
+use List::Util 'first';
 use Try::Tiny;
 
 ###
@@ -362,6 +363,9 @@ sub insert {
       );
   }
 
+  delete $self->{_column_data_in_storage};
+  $self->in_storage(1);
+
   $self->{_dirty_columns} = {};
   $self->{related_resultsets} = {};
 
@@ -394,10 +398,8 @@ sub insert {
     }
   }
 
-  $self->in_storage(1);
-  delete $self->{_orig_ident};
-  delete $self->{_orig_ident_failreason};
   delete $self->{_ignore_at_insert};
+
   $rollback_guard->commit if $rollback_guard;
 
   return $self;
@@ -494,14 +496,10 @@ sub update {
   my %to_update = $self->get_dirty_columns
     or return $self;
 
-  my $ident_cond = $self->{_orig_ident} || $self->ident_condition;
   $self->throw_exception( "Not in database" ) unless $self->in_storage;
 
-  $self->throw_exception($self->{_orig_ident_failreason})
-    if ! keys %$ident_cond;
-
   my $rows = $self->result_source->storage->update(
-    $self->result_source, \%to_update, $ident_cond
+    $self->result_source, \%to_update, $self->ident_condition
   );
   if ($rows == 0) {
     $self->throw_exception( "Can't update ${self}: row not found" );
@@ -510,7 +508,7 @@ sub update {
   }
   $self->{_dirty_columns} = {};
   $self->{related_resultsets} = {};
-  delete $self->{_orig_ident};
+  delete $self->{_column_data_in_storage};
   return $self;
 }
 
@@ -562,15 +560,11 @@ sub delete {
   if (ref $self) {
     $self->throw_exception( "Not in database" ) unless $self->in_storage;
 
-    my $ident_cond = $self->{_orig_ident} || $self->ident_condition;
-    $self->throw_exception($self->{_orig_ident_failreason})
-      if ! keys %$ident_cond;
-
     $self->result_source->storage->delete(
-      $self->result_source, $ident_cond
+      $self->result_source, $self->ident_condition
     );
 
-    delete $self->{_orig_ident};  # no longer identifiable
+    delete $self->{_column_data_in_storage};
     $self->in_storage(undef);
   }
   else {
@@ -835,25 +829,16 @@ instead, see L</set_inflated_columns>.
 sub set_column {
   my ($self, $column, $new_value) = @_;
 
-  # if we can't get an ident condition on first try - mark the object as unidentifiable
-  # (by using an empty hashref) and store the error for further diag
-  unless ($self->{_orig_ident}) {
-    try {
-      $self->{_orig_ident} = $self->ident_condition
-    }
-    catch {
-      $self->{_orig_ident_failreason} = $_;
-      $self->{_orig_ident} = {};
-    };
-  }
+  my $had_value = $self->has_column_loaded($column);
+  my ($old_value, $in_storage) = ($self->get_column($column), $self->in_storage)
+    if $had_value;
 
-  my $old_value = $self->get_column($column);
   $new_value = $self->store_column($column, $new_value);
 
   my $dirty =
     $self->{_dirty_columns}{$column}
       ||
-    $self->in_storage # no point tracking dirtyness on uninserted data
+    $in_storage # no point tracking dirtyness on uninserted data
       ? ! $self->_eq_column_values ($column, $old_value, $new_value)
       : 1
   ;
@@ -882,6 +867,21 @@ sub set_column {
         delete $self->{_inflated_column}{$rel};
       }
     }
+
+    if (
+      # value change from something (even if NULL)
+      $had_value
+        and
+      # no storage - no storage-value
+      $in_storage
+        and
+      # no value already stored (multiple changes before commit to storage)
+      ! exists $self->{_column_data_in_storage}{$column}
+        and
+      $self->_track_storage_value($column)
+    ) {
+      $self->{_column_data_in_storage}{$column} = $old_value;
+    }
   }
 
   return $new_value;
@@ -907,6 +907,13 @@ sub _eq_column_values {
   }
 }
 
+# returns a boolean indicating if the passed column should have its original
+# value tracked between column changes and commitment to storage
+sub _track_storage_value {
+  my ($self, $col) = @_;
+  return defined first { $col eq $_ } ($self->primary_columns);
+}
+
 =head2 set_columns
 
   $row->set_columns({ $col => $val, ... });
@@ -1363,12 +1370,7 @@ sub get_from_storage {
       $resultset = $resultset->search(undef, $attrs);
     }
 
-    my $ident_cond = $self->{_orig_ident} || $self->ident_condition;
-
-    $self->throw_exception($self->{_orig_ident_failreason})
-      if ! keys %$ident_cond;
-
-    return $resultset->find($ident_cond);
+    return $resultset->find($self->ident_condition);
 }
 
 =head2 discard_changes ($attrs?)
index 63651f6..0980509 100644 (file)
@@ -96,7 +96,32 @@ ok(
   "overloaded update 7"
 );
 
+$employee->group_id(2);
+$employee->name('E of the month');
+$employee->update({ employee_id => 666, position => 2 });
+is_deeply(
+  { $employee->get_columns },
+  {
+    employee_id => 666,
+    encoded => undef,
+    group_id => 2,
+    group_id_2 => undef,
+    group_id_3 => undef,
+    name => "E of the month",
+    position => 2
+  },
+  'combined update() worked correctly'
+);
+is_deeply(
+  { $employee->get_columns },
+  { $employee->get_from_storage->get_columns },
+  'object matches database state',
+);
+
+#####
 # multicol tests begin here
+#####
+
 DBICTest::Employee->grouping_column(['group_id_2', 'group_id_3']);
 $employees->delete();
 foreach my $group_id_2 (1..4) {