X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=blobdiff_plain;f=lib%2FDBIx%2FClass%2FOrdered.pm;h=b3fd102c470f15853219512d7ddfdc5e9ec97af5;hb=7d7d697500011de9ac151b6303f27e56f696cec6;hp=e95b693a1d5bb5396029a761b3b5ac53d8ae95a1;hpb=bd7ca9e8410a23a8becfc3c99468fc0b5c25affc;p=dbsrgits%2FDBIx-Class-Historic.git diff --git a/lib/DBIx/Class/Ordered.pm b/lib/DBIx/Class/Ordered.pm index e95b693..b3fd102 100644 --- a/lib/DBIx/Class/Ordered.pm +++ b/lib/DBIx/Class/Ordered.pm @@ -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; @@ -362,10 +377,17 @@ sub move_to { } my $new_pos_val = $self->_position_value ($to_position); # record this before the shift - $self->_ordered_internal_update({ $position_column => $self->null_position_value }); # take the row out of the picture for a bit + + # 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 }} ) ) ) { + $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; + return 1; } } @@ -407,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; @@ -418,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 ); } @@ -431,6 +454,8 @@ sub move_to_group { $self->_ordered_internal_update; + $guard->commit; + return 1; } } @@ -448,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 ) ); @@ -488,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) ) { @@ -517,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]; } } @@ -531,14 +570,28 @@ 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 +=head1 METHODS FOR EXTENDING ORDERED You would want to override the methods below if you use sparse (non-linear) or non-numeric position values. This can be useful @@ -549,7 +602,7 @@ or if you need to work with materialized path columns. my $num_pos = $item->_position; -Returns the absolute numeric position of the current object, with the +Returns the B 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. @@ -567,7 +620,7 @@ sub _position { my $pos_value = $item->_position_value ( $pos ) -Returns the value of L of the object at numeric +Returns the B of L of the object at numeric position C<$pos>. By default simply returns C<$pos>. =cut @@ -589,7 +642,7 @@ sub _position_value { __PACKAGE__->_initial_position_value(0); -This method specifies a value of L which is assigned +This method specifies a B of L which is assigned to the first inserted element of a group, if no value was supplied at insertion time. All subsequent values are derived from this one by L below. Defaults to 1. @@ -602,7 +655,7 @@ __PACKAGE__->mk_classdata( '_initial_position_value' => 1 ); my $new_value = $item->_next_position_value ( $position_value ) -Returns a position value that would be considered C with +Returns a position B that would be considered C with regards to C<$position_value>. Can be pretty much anything, given that C<< $position_value < $new_value >> where C<< < >> is the SQL comparison operator (usually works fine on strings). The @@ -618,10 +671,16 @@ sub _next_position_value { $item->_shift_siblings ($direction, @between) -Shifts all siblings with position 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 value -by 1. +Shifts all siblings with B 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 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 the shift is not a trivial task. +Refer to the implementation source of the default method for more +information. =cut sub _shift_siblings { @@ -647,13 +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 - my %uc = $self->result_source->unique_constraints; - if (grep { $_ eq $position_column } ( map { @$_ } (values %uc) ) ) { + 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 { @@ -668,7 +736,7 @@ need to use them. =head2 _group_rs -This method returns a resultset containing all memebers of the row +This method returns a resultset containing all members of the row group (including the row itself). =cut @@ -746,6 +814,23 @@ sub _is_in_group { return 1; } +=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 (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 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 + sub _ordered_internal_update { my $self = shift; local $self->{_ORDERED_INTERNAL_UPDATE} = 1; @@ -784,9 +869,18 @@ 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 + +Using a database defined default_value on one of your group columns +could result in the position not being assigned correctly. + =head1 AUTHOR -Aran Deltac + Original code framework + Aran Deltac + + Constraints support and code generalisation + Peter Rabbitson =head1 LICENSE