From: Aran Deltac Date: Thu, 23 Mar 2006 17:21:32 +0000 (+0000) Subject: Various fixes to Positioned. Tests no longer test AdjacencyList. X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=133dd22a652e4745443b7c45f7d40af5877aaab5;p=dbsrgits%2FDBIx-Class-Historic.git Various fixes to Positioned. Tests no longer test AdjacencyList. --- diff --git a/TODO b/TODO index d0726b3..3e58f61 100644 --- a/TODO +++ b/TODO @@ -17,3 +17,8 @@ 2006-03-18 by bluefeet - Support table locking. +2006-03-21 by bluefeet + - When subclassing a dbic class make it so you don't have to do + __PACKAGE__->table(__PACKAGE__->table()); for the result set to + return the correct object type. + diff --git a/lib/DBIx/Class/Positioned.pm b/lib/DBIx/Class/Positioned.pm index a8daf69..8fe2d36 100644 --- a/lib/DBIx/Class/Positioned.pm +++ b/lib/DBIx/Class/Positioned.pm @@ -56,6 +56,12 @@ Thats it, now you can change the position of your objects. This module provides a simple interface for modifying the 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 +move a record it always causes other records in the list to be updated. + =head1 METHODS =head2 position_column @@ -69,6 +75,18 @@ positional value of each record. Default to "position". __PACKAGE__->mk_classdata( 'position_column' => 'position' ); +=head2 collection_column + + __PACKAGE__->collection_column('thing_id'); + +This method specified a column to limit all queries in +this module by. This effectively allows you to have multiple +positioned lists within the same table. + +=cut + +__PACKAGE__->mk_classdata( 'collection_column' ); + =head2 siblings my $rs = $employee->siblings(); @@ -85,7 +103,7 @@ sub siblings { my $rs = $self->search( { $position_column => { '!=' => $self->get_column($position_column) }, - $self->_parent_clause(), + $self->_collection_clause(), }, { order_by => $self->position_column }, ); @@ -104,7 +122,7 @@ Returns the first sibling object. sub first_sibling { my( $self ) = @_; return ($self->search( - { $self->_parent_clause() }, + { $self->_collection_clause() }, { rows=>1, order_by => $self->position_column }, )->all())[0]; } @@ -120,7 +138,7 @@ Return the last sibling. sub last_sibling { my( $self ) = @_; return ($self->search( - { $self->_parent_clause() }, + { $self->_collection_clause() }, { rows=>1, order_by => $self->position_column.' DESC' }, )->all())[0]; } @@ -140,7 +158,7 @@ sub previous_sibling { return ($self->search( { $position_column => { '<' => $self->get_column($position_column) }, - $self->_parent_clause(), + $self->_collection_clause(), }, { rows=>1, order_by => $position_column.' DESC' }, )->all())[0]; @@ -158,10 +176,10 @@ is returned if the current object is the last one. sub next_sibling { my( $self ) = @_; my $position_column = $self->position_column; - return ($self->search( + return ($self->result_source->resultset->search( { $position_column => { '>' => $self->get_column($position_column) }, - $self->_parent_clause(), + $self->_collection_clause(), }, { rows=>1, order_by => $position_column }, )->all())[0]; @@ -179,15 +197,8 @@ the first one. sub move_previous { my( $self ) = @_; - my $previous = $self->previous_sibling(); - return undef if (!$previous); - my $position_column = $self->position_column; - my $self_position = $self->get_column( $position_column ); - $self->set_column( $position_column, $previous->get_column($position_column) ); - $previous->set_column( $position_column, $self_position ); - $self->update(); - $previous->update(); - return 1; + my $position = $self->get_column( $self->position_column() ); + return $self->move_to( $position - 1 ); } =head2 move_next @@ -201,15 +212,10 @@ success, and 0 is returned if the object is already the last in the list. sub move_next { my( $self ) = @_; - my $next = $self->next_sibling(); - return undef if (!$next); - my $position_column = $self->position_column; - my $self_position = $self->get_column( $position_column ); - $self->set_column( $position_column, $next->get_column($position_column) ); - $next->set_column( $position_column, $self_position ); - $self->update(); - $next->update(); - return 1; + my $position = $self->get_column( $self->position_column() ); + my $count = $self->result_source->resultset->search({$self->_collection_clause()})->count(); + return 0 if ($position==$count); + return $self->move_to( $position + 1 ); } =head2 move_first @@ -237,7 +243,7 @@ success, and 0 is returned if the object is already the last one. sub move_last { my( $self ) = @_; - my $count = $self->search({$self->_parent_clause()})->count(); + my $count = $self->result_source->resultset->search({$self->_collection_clause()})->count(); return $self->move_to( $count ); } @@ -255,13 +261,14 @@ sub move_to { my( $self, $to_position ) = @_; my $position_column = $self->position_column; my $from_position = $self->get_column( $position_column ); - return undef if ( $from_position==$to_position ); - my $rs = $self->search({ + return 0 if ( $to_position < 1 ); + return 0 if ( $from_position==$to_position ); + my $rs = $self->result_source->resultset->search({ -and => [ $position_column => { ($from_position>$to_position?'<':'>') => $from_position }, $position_column => { ($from_position>$to_position?'>=':'<=') => $to_position }, ], - $self->_parent_clause(), + $self->_collection_clause(), }); my $op = ($from_position>$to_position) ? '+' : '-'; $rs->update({ @@ -283,7 +290,7 @@ the table +1, thus positioning the new record at the last position. sub insert { my $self = shift; my $position_column = $self->position_column; - $self->set_column( $position_column => $self->search( {$self->_parent_clause()} )->count()+1 ) + $self->set_column( $position_column => $self->result_source->resultset->search( {$self->_collection_clause()} )->count()+1 ) if (!$self->get_column($position_column)); $self->next::method( @_ ); } @@ -307,22 +314,19 @@ sub delete { These methods are used internally. You should never have the need to use them. -=head2 _parent_clause - - sub _parent_clause { - my( $self ) = @_; - return ( parent_id => $self->parent_id ); - } +=head2 _collection_clause -This method is a placeholder for you, or another component, to -provide additional limits for all the various queries in this -module. This allows for more than one positionable list within -the same table since any move_* method will adhere to the clause -that you specify. +This method returns a name=>value pare for limiting a search +by the collection column. If the collection column is not +defined then this will return an empty list. =cut -sub _parent_clause { +sub _collection_clause { + my $self = shift; + if ($self->collection_column()) { + return ( $self->collection_column() => $self->get_column($self->collection_column()) ); + } return (); } @@ -331,6 +335,8 @@ __END__ =head1 BUGS +=head2 Race Condition on Insert + If a position is not specified for an insert than a position will be chosen based on COUNT(*)+1. But, it first selects the count then inserts the record. The space of time between select @@ -338,6 +344,20 @@ and insert introduces a race condition. To fix this we need the ability to lock tables in DBIC. I've added an entry in the TODO about this. +=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 +until you reload them from the database. + +The 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 +in the current object's result set to have the new position value. + =head1 AUTHOR Aran Deltac diff --git a/t/run/26positioned.tl b/t/run/26positioned.tl index 31503b0..ad6a919 100644 --- a/t/run/26positioned.tl +++ b/t/run/26positioned.tl @@ -5,89 +5,59 @@ sub run_tests { plan tests => 96; my $schema = shift; - foreach my $class ( 'Positioned', 'PositionedAdjacencyList', 'AdjacencyList' ) { + my $employees = $schema->resultset('Employee::Positioned'); - my $employees = $schema->resultset('Employee::'.$class); + if ($employees->result_class->can('position_column')) { - if ($employees->result_class->can('position_column')) { + $employees->delete(); + foreach (1..5) { + $employees->create({ name=>'temp' }); + } - $employees->delete(); - foreach (1..5) { - $employees->create({ name=>'temp' }); - } + $employees = $employees->search(undef,{order_by=>'position'}); + ok( check_positions($employees), "$class: intial positions" ); - $employees = $employees->search(undef,{order_by=>'position'}); - ok( check_positions($employees), "$class: intial positions" ); + my $employee; - my $employee; + foreach my $position (1..$employees->count()) { - foreach my $position (1..$employees->count()) { + $employee = $employees->find({ position=>$position }); + $employee->move_previous(); + ok( check_positions($employees), "$class: move_previous( $position )" ); - $employee = $employees->find({ position=>$position }); - $employee->move_previous(); - ok( check_positions($employees), "$class: move_previous( $position )" ); + $employee = $employees->find({ position=>$position }); + $employee->move_next(); + ok( check_positions($employees), "$class: move_next( $position )" ); - $employee = $employees->find({ position=>$position }); - $employee->move_next(); - ok( check_positions($employees), "$class: move_next( $position )" ); + $employee = $employees->find({ position=>$position }); + $employee->move_first(); + ok( check_positions($employees), "$class: move_first( $position )" ); - $employee = $employees->find({ position=>$position }); - $employee->move_first(); - ok( check_positions($employees), "$class: move_first( $position )" ); + $employee = $employees->find({ position=>$position }); + $employee->move_last(); + ok( check_positions($employees), "$class: move_last( $position )" ); + foreach my $to_position (1..$employees->count()) { $employee = $employees->find({ position=>$position }); - $employee->move_last(); - ok( check_positions($employees), "$class: move_last( $position )" ); - - foreach my $to_position (1..$employees->count()) { - $employee = $employees->find({ position=>$position }); - $employee->move_to($to_position); - ok( check_positions($employees), "$class: move_to( $position => $to_position )" ); - } - - } - } - if ($employees->result_class->can('parent_column')) { - - $employees->delete(); - my $mom = $employees->create({ name=>'temp' }); - foreach (1..14) { - $employees->create({ name=>'temp', parent_id=>$mom->id() }); - } - - my $children = $mom->children(); - ok( ($children->count()==14), 'correct number of children' ); - - my $grandma = $mom; - my @children = $children->all(); - $mom = pop(@children); - foreach my $child (@children) { - $child->parent( $mom ); + $employee->move_to($to_position); + ok( check_positions($employees), "$class: move_to( $position => $to_position )" ); } - ok( ($mom->children->count() == 13), 'correct number of grandchildren' ); - - if ($employees->result_class->can('position_column')) { - # TODO: Test positioning within a tree. - } } } - } sub check_positions { my( $employees ) = @_; $employees->reset(); my $expected_position = 0; - my $is_ok = 1; while (my $employee = $employees->next()) { $expected_position ++; if ($employee->position()!=$expected_position) { - $is_ok = 0; - last; + return 0; } } - return $is_ok; + return 1; } 1;