From: Devin Austin Date: Tue, 5 Apr 2011 21:58:04 +0000 (-0600) Subject: added resultset override for ordered as well as tests X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=335ed892d6401773cb157137400d9e26a8178687;p=dbsrgits%2FDBIx-Class.git added resultset override for ordered as well as tests --- diff --git a/lib/DBIx/Class/Ordered.pm b/lib/DBIx/Class/Ordered.pm index b6c4177..2ee65c5 100644 --- a/lib/DBIx/Class/Ordered.pm +++ b/lib/DBIx/Class/Ordered.pm @@ -118,7 +118,7 @@ ordered lists within the same table. =cut -__PACKAGE__->mk_classdata( 'grouping_column' ); +__PACKAGE__->mk_classdata('grouping_column'); =head2 null_position_value @@ -147,9 +147,10 @@ The ordering is a backwards-compatibility artifact - if you need a resultset with no ordering applied use L =cut + sub siblings { - my $self = shift; - return $self->_siblings->search ({}, { order_by => $self->position_column } ); + my $self = shift; + return $self->_siblings->search( {}, { order_by => $self->position_column } ); } =head2 previous_siblings @@ -161,14 +162,16 @@ Returns a resultset of all objects in the same group positioned before the object on which this method was called. =cut + sub previous_siblings { - my $self = shift; - my $position_column = $self->position_column; - my $position = $self->get_column ($position_column); - return ( defined $position - ? $self->_siblings->search ({ $position_column => { '<', $position } }) - : $self->_siblings - ); + my $self = shift; + my $position_column = $self->position_column; + my $position = $self->get_column($position_column); + return ( + defined $position + ? $self->_siblings->search( { $position_column => { '<', $position } } ) + : $self->_siblings + ); } =head2 next_siblings @@ -180,14 +183,16 @@ Returns a resultset of all objects in the same group positioned after the object on which this method was called. =cut + sub next_siblings { - my $self = shift; - my $position_column = $self->position_column; - my $position = $self->get_column ($position_column); - return ( defined $position - ? $self->_siblings->search ({ $position_column => { '>', $position } }) - : $self->_siblings - ); + my $self = shift; + my $position_column = $self->position_column; + my $position = $self->get_column($position_column); + return ( + defined $position + ? $self->_siblings->search( { $position_column => { '>', $position } } ) + : $self->_siblings + ); } =head2 previous_sibling @@ -200,15 +205,15 @@ if the current object is the first one. =cut sub previous_sibling { - my $self = shift; - my $position_column = $self->position_column; + my $self = shift; + my $position_column = $self->position_column; - my $psib = $self->previous_siblings->search( - {}, - { rows => 1, order_by => { '-desc' => $position_column } }, + my $psib = + $self->previous_siblings->search( {}, + { rows => 1, order_by => { '-desc' => $position_column } }, )->single; - return defined $psib ? $psib : 0; + return defined $psib ? $psib : 0; } =head2 first_sibling @@ -221,15 +226,15 @@ is this sibling. =cut sub first_sibling { - my $self = shift; - my $position_column = $self->position_column; + my $self = shift; + my $position_column = $self->position_column; - my $fsib = $self->previous_siblings->search( - {}, - { rows => 1, order_by => { '-asc' => $position_column } }, + my $fsib = + $self->previous_siblings->search( {}, + { rows => 1, order_by => { '-asc' => $position_column } }, )->single; - return defined $fsib ? $fsib : 0; + return defined $fsib ? $fsib : 0; } =head2 next_sibling @@ -242,14 +247,14 @@ if the current object is the last one. =cut sub next_sibling { - my $self = shift; - my $position_column = $self->position_column; - my $nsib = $self->next_siblings->search( - {}, - { rows => 1, order_by => { '-asc' => $position_column } }, + my $self = shift; + my $position_column = $self->position_column; + my $nsib = + $self->next_siblings->search( {}, + { rows => 1, order_by => { '-asc' => $position_column } }, )->single; - return defined $nsib ? $nsib : 0; + return defined $nsib ? $nsib : 0; } =head2 last_sibling @@ -262,28 +267,32 @@ sibling. =cut sub last_sibling { - my $self = shift; - my $position_column = $self->position_column; - my $lsib = $self->next_siblings->search( - {}, - { rows => 1, order_by => { '-desc' => $position_column } }, + my $self = shift; + my $position_column = $self->position_column; + my $lsib = + $self->next_siblings->search( {}, + { 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 sub _last_sibling_posval { - my $self = shift; - my $position_column = $self->position_column; + my $self = shift; + my $position_column = $self->position_column; - my $cursor = $self->next_siblings->search( - {}, - { rows => 1, order_by => { '-desc' => $position_column }, select => $position_column }, - )->cursor; - - my ($pos) = $cursor->next; - return $pos; + my $cursor = $self->next_siblings->search( + {}, + { + rows => 1, + order_by => { '-desc' => $position_column }, + select => $position_column + }, + )->cursor; + + my ($pos) = $cursor->next; + return $pos; } =head2 move_previous @@ -297,8 +306,8 @@ already the first one. =cut sub move_previous { - my $self = shift; - return $self->move_to ($self->_position - 1); + my $self = shift; + return $self->move_to( $self->_position - 1 ); } =head2 move_next @@ -312,9 +321,11 @@ the last in the list. =cut sub move_next { - my $self = shift; - return 0 unless defined $self->_last_sibling_posval; # quick way to check for no more siblings - return $self->move_to ($self->_position + 1); + my $self = shift; + return 0 + unless defined + $self->_last_sibling_posval; # quick way to check for no more siblings + return $self->move_to( $self->_position + 1 ); } =head2 move_first @@ -327,7 +338,7 @@ on success, and 0 if the object is already the first. =cut sub move_first { - return shift->move_to( 1 ); + return shift->move_to(1); } =head2 move_last @@ -340,12 +351,12 @@ on success, and 0 if the object is already the last one. =cut sub move_last { - my $self = shift; - my $last_posval = $self->_last_sibling_posval; + my $self = shift; + my $last_posval = $self->_last_sibling_posval; - return 0 unless defined $last_posval; + return 0 unless defined $last_posval; - return $self->move_to( $self->_position_from_value ($last_posval) ); + return $self->move_to( $self->_position_from_value($last_posval) ); } =head2 move_to @@ -359,61 +370,71 @@ position. =cut sub move_to { - my( $self, $to_position ) = @_; - return 0 if ( $to_position < 1 ); - - my $position_column = $self->position_column; + my ( $self, $to_position ) = @_; + return 0 if ( $to_position < 1 ); - my $guard; + my $position_column = $self->position_column; - 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) + my $guard; - $guard = $self->result_source->schema->txn_scope_guard; + if ( $self->is_column_changed($position_column) ) { - my $cursor = $self->result_source->resultset->search( - $self->ident_condition, - { select => $position_column }, - )->cursor; - - my ($pos) = $cursor->next; - $self->$position_column ($pos); - delete $self->{_dirty_columns}{$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) - my $from_position = $self->_position; + $guard = $self->result_source->schema->txn_scope_guard; - 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 ($direction, @between); - if ( $from_position < $to_position ) { - $direction = -1; - @between = map { $self->_position_value ($_) } ( $from_position + 1, $to_position ); - } - else { - $direction = 1; - @between = map { $self->_position_value ($_) } ( $to_position, $from_position - 1 ); - } - - 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 }} ) ) ) { - $self->_ordered_internal_update({ $position_column => $self->null_position_value }); - } - - $self->_shift_siblings ($direction, @between); - $self->_ordered_internal_update({ $position_column => $new_pos_val }); + my $cursor = + $self->result_source->resultset->search( $self->ident_condition, + { select => $position_column }, + )->cursor; - $guard->commit; - return 1; + my ($pos) = $cursor->next; + $self->$position_column($pos); + 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 ( $direction, @between ); + if ( $from_position < $to_position ) { + $direction = -1; + @between = + map { $self->_position_value($_) } ( $from_position + 1, $to_position ); + } else { + $direction = 1; + @between = + map { $self->_position_value($_) } ( $to_position, $from_position - 1 ); + } + + 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 } } ) + ) + ) + { + $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; } =head2 move_to_group @@ -432,85 +453,88 @@ if multiple grouping columns are in use. =cut sub move_to_group { - my( $self, $to_group, $to_position ) = @_; - - # if we're given a single value, turn it into a hashref - unless (ref $to_group eq 'HASH') { - my @gcols = $self->_grouping_columns; - - $self->throw_exception ('Single group supplied for a multi-column group identifier') if @gcols > 1; - $to_group = {$gcols[0] => $to_group}; - } - - my $position_column = $self->position_column; - - 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); - 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 }, + my ( $self, $to_group, $to_position ) = @_; + + # if we're given a single value, turn it into a hashref + unless ( ref $to_group eq 'HASH' ) { + my @gcols = $self->_grouping_columns; + + $self->throw_exception( + 'Single group supplied for a multi-column group identifier') + if @gcols > 1; + $to_group = { $gcols[0] => $to_group }; + } + + my $position_column = $self->position_column; + + 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 ); + 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_in_group ($to_group) ) { - my $ret; - if (defined $to_position) { - $ret = $self->move_to ($to_position); - } + my @original_values = $cursor->next; + $self->set_inflated_columns( + { %values, map { $_ => shift @original_values } (@dirty_cols) } ); + delete $self->{_dirty_columns}{$_} for (@dirty_cols); + } - $guard->commit if $guard; - return $ret||0; + if ( $self->_is_in_group($to_group) ) { + my $ret; + if ( defined $to_position ) { + $ret = $self->move_to($to_position); } - $guard ||= $self->result_source->schema->txn_scope_guard; + $guard->commit if $guard; + return $ret || 0; + } - # Move to end of current group to adjust siblings - $self->move_last; + $guard ||= $self->result_source->schema->txn_scope_guard; - $self->set_inflated_columns({ %$to_group, $position_column => undef }); - my $new_group_last_posval = $self->_last_sibling_posval; - my $new_group_last_position = $self->_position_from_value ( - $new_group_last_posval - ); + # Move to end of current group to adjust siblings + $self->move_last; + + $self->set_inflated_columns( { %$to_group, $position_column => undef } ); + my $new_group_last_posval = $self->_last_sibling_posval; + my $new_group_last_position = + $self->_position_from_value($new_group_last_posval); - if ( not defined($to_position) or $to_position > $new_group_last_position) { - $self->set_column( + if ( not defined($to_position) or $to_position > $new_group_last_position ) { + $self->set_column( $position_column => $new_group_last_position - ? $self->_next_position_value ( $new_group_last_posval ) - : $self->_initial_position_value - ); - } - else { - my $bumped_pos_val = $self->_position_value ($to_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 ); - } + ? $self->_next_position_value($new_group_last_posval) + : $self->_initial_position_value + ); + } else { + my $bumped_pos_val = $self->_position_value($to_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 ); + } - $self->_ordered_internal_update; + $self->_ordered_internal_update; - $guard->commit; + $guard->commit; - return 1; + return 1; } =head2 insert @@ -522,20 +546,21 @@ the table +1, thus positioning the new record at the last position. =cut sub insert { - my $self = shift; - my $position_column = $self->position_column; - - unless ($self->get_column($position_column)) { - my $lsib_posval = $self->_last_sibling_posval; - $self->set_column( - $position_column => (defined $lsib_posval - ? $self->_next_position_value ( $lsib_posval ) - : $self->_initial_position_value - ) - ); - } + my $self = shift; + my $position_column = $self->position_column; + + unless ( $self->get_column($position_column) ) { + my $lsib_posval = $self->_last_sibling_posval; + $self->set_column( + $position_column => ( + defined $lsib_posval + ? $self->_next_position_value($lsib_posval) + : $self->_initial_position_value + ) + ); + } - return $self->next::method( @_ ); + return $self->next::method(@_); } =head2 update @@ -549,98 +574,97 @@ 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}{$_}; - } + 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}; + } - # if nothing group/position related changed - short circuit - if (not grep { exists $changes{$_} } ( @ordering_columns ) ) { - return $self->next::method( \%upd, @_ ); + $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 $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, @_ ); - } - - $guard->commit; - return wantarray ? @res : $res[0]; + my @res; + if ( not defined wantarray ) { + $self->next::method( \%upd, @_ ); + } elsif (wantarray) { + @res = $self->next::method( \%upd, @_ ); + } else { + $res[0] = $self->next::method( \%upd, @_ ); } + + $guard->commit; + return wantarray ? @res : $res[0]; + } } =head2 delete @@ -652,25 +676,23 @@ integrity of the positions. =cut sub delete { - my $self = shift; + my $self = shift; - my $guard = $self->result_source->schema->txn_scope_guard; + my $guard = $self->result_source->schema->txn_scope_guard; - $self->move_last; + $self->move_last; - my @res; - if (not defined wantarray) { - $self->next::method( @_ ); - } - elsif (wantarray) { - @res = $self->next::method( @_ ); - } - else { - $res[0] = $self->next::method( @_ ); - } + my @res; + if ( not defined wantarray ) { + $self->next::method(@_); + } elsif (wantarray) { + @res = $self->next::method(@_); + } else { + $res[0] = $self->next::method(@_); + } - $guard->commit; - return wantarray ? @res : $res[0]; + $guard->commit; + return wantarray ? @res : $res[0]; } =head1 METHODS FOR EXTENDING ORDERED @@ -688,17 +710,18 @@ Returns the B of an object with a B set to C<$pos_value>. By default simply returns C<$pos_value>. =cut + sub _position_from_value { - my ($self, $val) = @_; + my ( $self, $val ) = @_; - return 0 unless defined $val; + return 0 unless defined $val; -# #the right way to do this -# return $self -> _group_rs -# -> search({ $self->position_column => { '<=', $val } }) -# -> count + # #the right way to do this + # return $self -> _group_rs + # -> search({ $self->position_column => { '<=', $val } }) + # -> count - return $val; + return $val; } =head2 _position_value @@ -709,18 +732,19 @@ Returns the B of L of the object at numeric position C<$pos>. By default simply returns C<$pos>. =cut + sub _position_value { - my ($self, $pos) = @_; + my ( $self, $pos ) = @_; -# #the right way to do this (not optimized) -# my $position_column = $self->position_column; -# return $self -> _group_rs -# -> search({}, { order_by => $position_column }) -# -> slice ( $pos - 1) -# -> single -# -> get_column ($position_column); + # #the right way to do this (not optimized) + # my $position_column = $self->position_column; + # return $self -> _group_rs + # -> search({}, { order_by => $position_column }) + # -> slice ( $pos - 1) + # -> single + # -> get_column ($position_column); - return $pos; + return $pos; } =head2 _initial_position_value @@ -748,8 +772,9 @@ default method expects C<$position_value> to be numeric, and returns C<$position_value + 1> =cut + sub _next_position_value { - return $_[1] + 1; + return $_[1] + 1; } =head2 _shift_siblings @@ -768,50 +793,56 @@ Refer to the implementation source of the default method for more information. =cut -sub _shift_siblings { - my ($self, $direction, @between) = @_; - return 0 unless $direction; - - my $position_column = $self->position_column; - - my ($op, $ord); - if ($direction < 0) { - $op = '-'; - $ord = 'asc'; - } - else { - $op = '+'; - $ord = 'desc'; - } - - 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 - - my $rsrc = $self->result_source; - - if (grep { $_ eq $position_column } ( map { @$_ } (values %{{ $rsrc->unique_constraints }} ) ) ) { - my @pcols = $rsrc->_pri_cols; - my $cursor = $shift_rs->search ({}, { order_by => { "-$ord", $position_column }, columns => \@pcols } )->cursor; - my $rs = $self->result_source->resultset; - - my @all_pks = $cursor->all; - while (my $pks = shift @all_pks) { - my $cond; - for my $i (0.. $#pcols) { - $cond->{$pcols[$i]} = $pks->[$i]; - } +sub _shift_siblings { + my ( $self, $direction, @between ) = @_; + return 0 unless $direction; + + my $position_column = $self->position_column; + + my ( $op, $ord ); + if ( $direction < 0 ) { + $op = '-'; + $ord = 'asc'; + } else { + $op = '+'; + $ord = 'desc'; + } + + 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 + + my $rsrc = $self->result_source; + + if ( grep { $_ eq $position_column } + ( map { @$_ } ( values %{ { $rsrc->unique_constraints } } ) ) ) + { + + my @pcols = $rsrc->_pri_cols; + my $cursor = + $shift_rs->search( {}, + { order_by => { "-$ord", $position_column }, columns => \@pcols } ) + ->cursor; + my $rs = $self->result_source->resultset; + + my @all_pks = $cursor->all; + while ( my $pks = shift @all_pks ) { + my $cond; + for my $i ( 0 .. $#pcols ) { + $cond->{ $pcols[$i] } = $pks->[$i]; + } - $rs->search($cond)->update ({ $position_column => \ "$position_column $op 1" } ); - } - } - else { - $shift_rs->update ({ $position_column => \ "$position_column $op 1" } ); + $rs->search($cond) + ->update( { $position_column => \"$position_column $op 1" } ); } + } else { + $shift_rs->update( { $position_column => \"$position_column $op 1" } ); + } } =head1 PRIVATE METHODS @@ -825,9 +856,11 @@ This method returns a resultset containing all members of the row group (including the row itself). =cut + sub _group_rs { - my $self = shift; - return $self->result_source->resultset->search({$self->_grouping_clause()}); + my $self = shift; + return $self->result_source->resultset->search( + { $self->_grouping_clause() } ); } =head2 _siblings @@ -836,12 +869,13 @@ Returns an unordered resultset of all objects in the same group excluding the object you called this method on. =cut + sub _siblings { - my $self = shift; - my $position_column = $self->position_column; - return $self->_group_rs->search( - { $position_column => { '!=' => $self->get_column($position_column) } }, - ); + my $self = shift; + my $position_column = $self->position_column; + return $self->_group_rs->search( + { $position_column => { '!=' => $self->get_column($position_column) } }, + ); } =head2 _position @@ -852,9 +886,11 @@ Returns the B 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) ); + my $self = shift; + return $self->_position_from_value( + $self->get_column( $self->position_column ) ); } =head2 _grouping_clause @@ -864,9 +900,10 @@ by the grouping column(s). If the grouping column is not defined then this will return an empty list. =cut + sub _grouping_clause { - my( $self ) = @_; - return map { $_ => $self->get_column($_) } $self->_grouping_columns(); + my ($self) = @_; + return map { $_ => $self->get_column($_) } $self->_grouping_columns(); } =head2 _get_grouping_columns @@ -876,16 +913,17 @@ they were specified as an arrayref or a single string, and returns () if there is no grouping. =cut + sub _grouping_columns { - my( $self ) = @_; - my $col = $self->grouping_column(); - if (ref $col eq 'ARRAY') { - return @$col; - } elsif ($col) { - return ( $col ); - } else { - return (); - } + my ($self) = @_; + my $col = $self->grouping_column(); + if ( ref $col eq 'ARRAY' ) { + return @$col; + } elsif ($col) { + return ($col); + } else { + return (); + } } =head2 _is_in_group @@ -895,21 +933,20 @@ sub _grouping_columns { Returns true if the object is in the group represented by hashref $other =cut -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; +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; } =head2 _ordered_internal_update @@ -930,9 +967,26 @@ this module. =cut sub _ordered_internal_update { - my $self = shift; - local $self->{_ORDERED_INTERNAL_UPDATE} = 1; - return $self->update (@_); + my $self = shift; + local $self->{_ORDERED_INTERNAL_UPDATE} = 1; + return $self->update(@_); +} + +=head2 table + +Overridden to provide a resultset class to override delete and update methods. + +Shamelessly stolen from InflateColumn::FS + +=cut + +sub table { + my $self = shift; + warn "**INSIDE Ordered->table**"; + my $ret = $self->next::method(@_); + $self->result_source_instance->resultset_class( + 'DBIx::Class::Ordered::ResultSet'); + return $ret; } 1; @@ -994,6 +1048,9 @@ could result in the position not being assigned correctly. Constraints support and code generalisation Peter Rabbitson + C and C fix + Devin Austin + =head1 LICENSE You may distribute this code under the same terms as Perl itself. diff --git a/lib/DBIx/Class/Ordered/ResultSet.pm b/lib/DBIx/Class/Ordered/ResultSet.pm new file mode 100644 index 0000000..baca121 --- /dev/null +++ b/lib/DBIx/Class/Ordered/ResultSet.pm @@ -0,0 +1,13 @@ +package DBIx::Class::Ordered::ResultSet; +use strict; +use base qw/DBIx::Class::ResultSet/; + +sub update { + shift->update_all(@_); +} + +sub delete { + shift->delete_all(@_); +} + +1; diff --git a/t/39load_namespaces_5.t b/t/39load_namespaces_5.t deleted file mode 100644 index a69fcd7..0000000 --- a/t/39load_namespaces_5.t +++ /dev/null @@ -1,23 +0,0 @@ -use strict; -use warnings; -use Test::More; - -use lib qw(t/lib); -use DBICTest; # do not remove even though it is not used - -my $warnings; -eval { - local $SIG{__WARN__} = sub { $warnings .= shift }; - package DBICNSTest; - use base qw/DBIx::Class::Schema/; - __PACKAGE__->load_namespaces; -}; -my $source_mro_order = DBICNSTest->source('MROOrder'); -isa_ok($source_mro_order , 'DBIx::Class::ResultSource::Table'); - -my $schema = DBICNSTest->connect("dbi:SQLite::memory:", "", ""); -$schema->deploy; -use Data::Dumper; - -warn "linear: " . Dumper mro::get_linear_isa(ref $schema->resultset('MROOrder')); -done_testing(); diff --git a/t/lib/DBICNSTest/Result/MROOrder.pm b/t/lib/DBICNSTest/Result/MROOrder.pm index 5ac1596..75ab10e 100644 --- a/t/lib/DBICNSTest/Result/MROOrder.pm +++ b/t/lib/DBICNSTest/Result/MROOrder.pm @@ -1,7 +1,7 @@ package DBICNSTest::Result::MROOrder; use base qw/DBIx::Class::Core/; use DBICNSTest::ResultSet::MROOrder; -__PACKAGE__->load_components(qw/ InflateColumn::FS /); +__PACKAGE__->load_components(qw/ InflateColumn::Fargh /); __PACKAGE__->table('mroorder'); __PACKAGE__->add_columns('mroorder'); __PACKAGE__->resultset_class ('DBICNSTest::ResultSet::MROOrder'); diff --git a/t/ordered/inject_component_update.t b/t/ordered/inject_component_update.t new file mode 100644 index 0000000..1bc11f3 --- /dev/null +++ b/t/ordered/inject_component_update.t @@ -0,0 +1,52 @@ +use strict; +use warnings; +use Test::More; +use Test::Exception; +use lib qw(t/lib); +use DBICTest; # do not remove even though it is not used + +#15:01 <@ribasushi> dhoss: you are complicating your life +#15:01 <@ribasushi> dhoss: start from the other side: +#15:02 <@ribasushi> if currently you add a test to t/??ordered.t that does +# $ordered_rs->search({ condition to not delete +# everything})->delete; +#15:03 <@ribasushi> dhoss: and then examine the database - you will see +# numbering is broken +#15:03 <@ribasushi> dhoss: use your newly found component injection powers to +# change the delete into a delete_all behind the scenes - the +# remaining rows will then be reordered correctly +#15:03 <@ribasushi> dhoss: this way you both test that injection works AND you +# fix Ordered + +my $schema = DBICTest->init_schema(); +my $artist = + $schema->resultset('Artist')->search( {}, { rows => 1 } ) + ->single; # braindead sqlite +my $cd = $schema->resultset('CD')->create( + { + artist => $artist, + title => 'Get in order', + year => 2009, + tracks => [ { title => 'T1' }, { title => 'T2' }, { title => 'T3' }, ], + } +); + + +lives_ok( sub { $cd->delete }, + "Cascade delete on ordered has_many doesn't bomb" ); + +is_deeply( + mro::get_linear_isa( ref $schema->resultset("Track") ), + [ + qw( + DBIx::Class::Ordered::ResultSet + DBIx::Class::ResultSet + DBIx::Class + DBIx::Class::Componentised + Class::C3::Componentised + Class::Accessor::Grouped + ) + ], + "MRO for class is correct" +); +done_testing();