[mb-commits] branch, mbs-5650, created. MBS-5650: Extend edit approval transaction boundaries over race condition

The branch, mbs-5650 has been created
        at  e37ff0059f11cc04c10d82e15dfcfdc3325a97eb (commit)

- Log -----------------------------------------------------------------
commit e37ff0059f11cc04c10d82e15dfcfdc3325a97eb
Author: Oliver Charles <[email protected]>
Date:   Fri Jan 25 17:22:05 2013 +0000

    MBS-5650: Extend edit approval transaction boundaries over race condition
    
    We had a race condition when approving edits, in that between fetching the edit
    to determine whether or not an editor can approve it and actually doing the
    approval, another request could close the edit. This would result in an
    exception when actually doing the approval, as the edit would no longer be open.
    
    This commit changes the transaction boundaries so that the edit is locked for
    the check, and the lock extends to actually approving the edit.

diff --git a/lib/MusicBrainz/Server/Controller/Edit.pm b/lib/MusicBrainz/Server/Controller/Edit.pm
index 1386e3e..5d2c81b 100644
--- a/lib/MusicBrainz/Server/Controller/Edit.pm
+++ b/lib/MusicBrainz/Server/Controller/Edit.pm
@@ -88,29 +88,32 @@ sub approve : Chained('load') RequireAuth(auto_editor)
 {
     my ($self, $c) = @_;
 
-    my $edit = $c->stash->{edit};
-    if (!$edit->can_approve($c->user)) {
-        $c->stash( template => 'edit/cannot_approve.tt' );
-        $c->detach;
-    }
-
-    if($edit->no_votes > 0) {
-        $c->model('EditNote')->load_for_edits($edit);
-        my $left_note;
-        for my $note (@{ $edit->edit_notes }) {
-            next if $note->editor_id != $c->user->id;
-            $left_note = 1;
-            last;
+    $c->model('MB')->with_transaction(sub {
+        my $edit = $c->model('Edit')->get_by_id_and_lock($c->stash->{edit}->id);
+        if (!$edit->can_approve($c->user)) {
+            $c->stash( template => 'edit/cannot_approve.tt' );
+            return;
         }
+        elsif($edit->no_votes > 0) {
+            $c->model('EditNote')->load_for_edits($edit);
+            my $left_note;
+            for my $note (@{ $edit->edit_notes }) {
+                next if $note->editor_id != $c->user->id;
+                $left_note = 1;
+                last;
+            }
 
-        unless($left_note) {
-            $c->stash( template => 'edit/require_note.tt' );
-            $c->detach;
-        };
-    }
-
-    $c->model('Edit')->approve($edit, $c->user->id);
-    $c->response->redirect($c->req->query_params->{url} || $c->uri_for_action('/edit/show', [ $edit->id ]));
+            unless($left_note) {
+                $c->stash( template => 'edit/require_note.tt' );
+                return;
+            };
+        }
+        else {
+            $c->model('Edit')->approve($edit, $c->user->id);
+            $c->response->redirect(
+                $c->req->query_params->{url} || $c->uri_for_action('/edit/show', [ $edit->id ]));
+        }
+    });
 }
 
 sub cancel : Chained('load') RequireAuth
diff --git a/lib/MusicBrainz/Server/Data/Edit.pm b/lib/MusicBrainz/Server/Data/Edit.pm
index 008a868..ac868e3 100644
--- a/lib/MusicBrainz/Server/Data/Edit.pm
+++ b/lib/MusicBrainz/Server/Data/Edit.pm
@@ -494,21 +494,16 @@ sub approve
 {
     my ($self, $edit, $editor_id) = @_;
 
-    Sql::run_in_transaction(sub {
-        # Load the edit again, but this time lock it for updates
-        $edit = $self->get_by_id_and_lock($edit->id);
-
-        $self->c->model('Vote')->enter_votes(
-            $editor_id,
-            {
-                vote    => $VOTE_APPROVE,
-                edit_id => $edit->id
-            }
-        );
+    $self->c->model('Vote')->enter_votes(
+        $editor_id,
+        {
+            vote    => $VOTE_APPROVE,
+            edit_id => $edit->id
+        }
+    );
 
-        # Apply the changes and close the edit
-        $self->accept($edit);
-    }, $self->c->sql);
+    # Apply the changes and close the edit
+    $self->accept($edit);
 }
 
 sub _do_accept

-----------------------------------------------------------------------


hooks/post-receive
-- 
mb_server