Petr Písař
11 years ago
parent
ada648d3d9
commit
f52cb4c6be
@ -0,0 +1,144 @@
|
|||||||
|
From 72e545f8b77baec541e229d740308886f0598a82 Mon Sep 17 00:00:00 2001
|
||||||
|
From: Peter Rabbitson <ribasushi@cpan.org>
|
||||||
|
Date: Mon, 16 Jun 2014 16:15:44 +0200
|
||||||
|
Subject: [PATCH] Fix ::Ordered in combination with delete_all
|
||||||
|
MIME-Version: 1.0
|
||||||
|
Content-Type: text/plain; charset=UTF-8
|
||||||
|
Content-Transfer-Encoding: 8bit
|
||||||
|
|
||||||
|
See pod-changes for description of the solution. I cringe but... oh well
|
||||||
|
|
||||||
|
Petr Pisar: Ported to 0.08270.
|
||||||
|
|
||||||
|
Signed-off-by: Petr Písař <ppisar@redhat.com>
|
||||||
|
---
|
||||||
|
lib/DBIx/Class/Ordered.pm | 53 ++++++++++++++++++++++++++++++------------
|
||||||
|
t/ordered/unordered_movement.t | 25 +++++++++++---------
|
||||||
|
2 files changed, 52 insertions(+), 26 deletions(-)
|
||||||
|
|
||||||
|
diff --git a/lib/DBIx/Class/Ordered.pm b/lib/DBIx/Class/Ordered.pm
|
||||||
|
index 5e40dc0..a5db68b 100644
|
||||||
|
--- a/lib/DBIx/Class/Ordered.pm
|
||||||
|
+++ b/lib/DBIx/Class/Ordered.pm
|
||||||
|
@@ -367,7 +367,30 @@ sub move_to {
|
||||||
|
|
||||||
|
my $position_column = $self->position_column;
|
||||||
|
|
||||||
|
- if ($self->is_column_changed ($position_column) ) {
|
||||||
|
+ my $is_txn;
|
||||||
|
+ if ($is_txn = $self->result_source->schema->storage->transaction_depth) {
|
||||||
|
+ # Reload position state from storage
|
||||||
|
+ # The thinking here is that if we are in a transaction, it is
|
||||||
|
+ # *more likely* the object went out of sync due to resultset
|
||||||
|
+ # level shenanigans. Instead of always reloading (slow) - go
|
||||||
|
+ # ahead and hand-hold only in the case of higher layers
|
||||||
|
+ # requesting the safety of a txn
|
||||||
|
+
|
||||||
|
+ $self->store_column(
|
||||||
|
+ $position_column,
|
||||||
|
+ ( $self->result_source
|
||||||
|
+ ->resultset
|
||||||
|
+ ->search($self->_storage_ident_condition, { rows => 1, columns => $position_column })
|
||||||
|
+ ->cursor
|
||||||
|
+ ->next
|
||||||
|
+ )[0] || $self->throw_exception(
|
||||||
|
+ sprintf "Unable to locate object '%s' in storage - object went ouf of sync...?",
|
||||||
|
+ $self->ID
|
||||||
|
+ ),
|
||||||
|
+ );
|
||||||
|
+ delete $self->{_dirty_columns}{$position_column};
|
||||||
|
+ }
|
||||||
|
+ elsif ($self->is_column_changed ($position_column) ) {
|
||||||
|
# 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});
|
||||||
|
@@ -380,7 +403,7 @@ sub move_to {
|
||||||
|
return 0;
|
||||||
|
}
|
||||||
|
|
||||||
|
- my $guard = $self->result_source->schema->txn_scope_guard;
|
||||||
|
+ my $guard = $is_txn ? undef : $self->result_source->schema->txn_scope_guard;
|
||||||
|
|
||||||
|
my ($direction, @between);
|
||||||
|
if ( $from_position < $to_position ) {
|
||||||
|
@@ -402,7 +425,7 @@ sub move_to {
|
||||||
|
$self->_shift_siblings ($direction, @between);
|
||||||
|
$self->_ordered_internal_update({ $position_column => $new_pos_val });
|
||||||
|
|
||||||
|
- $guard->commit;
|
||||||
|
+ $guard->commit if $guard;
|
||||||
|
return 1;
|
||||||
|
}
|
||||||
|
|
||||||
|
@@ -861,18 +884,18 @@ will prevent such race conditions going undetected.
|
||||||
|
|
||||||
|
=head2 Multiple Moves
|
||||||
|
|
||||||
|
-Be careful when issuing 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 - see
|
||||||
|
-L<DBIx::Class::Row/discard_changes>.
|
||||||
|
-
|
||||||
|
-There are times when you will want to move objects as groups, such
|
||||||
|
-as changing 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.
|
||||||
|
+If you have multiple same-group result objects already loaded from storage,
|
||||||
|
+you need to be careful when executing C<move_*> operations on them:
|
||||||
|
+without a L</position_column> reload the L</_position_value> of the
|
||||||
|
+"siblings" will be out of sync with the underlying storage.
|
||||||
|
+
|
||||||
|
+Starting from version C<0.082800> DBIC will implicitly perform such
|
||||||
|
+reloads when the C<move_*> happens as a part of a transaction
|
||||||
|
+(a good example of such situation is C<< $ordered_resultset->delete_all >>).
|
||||||
|
+
|
||||||
|
+If it is not possible for you to wrap the entire call-chain in a transaction,
|
||||||
|
+you will need to call L<DBIx::Class::Row/discard_changes> to get an object
|
||||||
|
+up-to-date before proceeding, otherwise undefined behavior will result.
|
||||||
|
|
||||||
|
=head2 Default Values
|
||||||
|
|
||||||
|
diff --git a/t/ordered/unordered_movement.t b/t/ordered/unordered_movement.t
|
||||||
|
index 9cbc3da..dc08306 100644
|
||||||
|
--- a/t/ordered/unordered_movement.t
|
||||||
|
+++ b/t/ordered/unordered_movement.t
|
||||||
|
@@ -9,19 +9,22 @@ use DBICTest;
|
||||||
|
my $schema = DBICTest->init_schema();
|
||||||
|
|
||||||
|
my $cd = $schema->resultset('CD')->next;
|
||||||
|
+$cd->tracks->delete;
|
||||||
|
|
||||||
|
-lives_ok {
|
||||||
|
- $cd->tracks->delete;
|
||||||
|
+$schema->resultset('CD')->related_resultset('tracks')->delete;
|
||||||
|
|
||||||
|
- my @tracks = map
|
||||||
|
- { $cd->create_related('tracks', { title => "t_$_", position => $_ }) }
|
||||||
|
- (4,2,5,1,3)
|
||||||
|
- ;
|
||||||
|
+is $cd->tracks->count, 0, 'No tracks';
|
||||||
|
|
||||||
|
- for (@tracks) {
|
||||||
|
- $_->discard_changes;
|
||||||
|
- $_->delete;
|
||||||
|
- }
|
||||||
|
-} 'Creation/deletion of out-of order tracks successful';
|
||||||
|
+$cd->create_related('tracks', { title => "t_$_", position => $_ })
|
||||||
|
+ for (4,2,3,1,5);
|
||||||
|
+
|
||||||
|
+is $cd->tracks->count, 5, 'Created 5 tracks';
|
||||||
|
+
|
||||||
|
+# a txn should force the implicit pos reload, regardless of order
|
||||||
|
+$schema->txn_do(sub {
|
||||||
|
+ $cd->tracks->delete_all
|
||||||
|
+});
|
||||||
|
+
|
||||||
|
+is $cd->tracks->count, 0, 'Successfully deleted everything';
|
||||||
|
|
||||||
|
done_testing;
|
||||||
|
--
|
||||||
|
1.9.3
|
||||||
|
|
||||||
Loading…
Reference in new issue