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

The branch, mbs-5655 has been updated
       via  http://git.musicbrainz.org/gitweb/?p=musicbrainz-server/core.git;a=commit;h=7d3e8d525bbe69aee2e6c2b042bd9b55415e74da (commit)
      from  http://git.musicbrainz.org/gitweb/?p=musicbrainz-server/core.git;a=commit;h=f8bc1ff1db8544adabeed5ea94b05ca8e52d5ba7 (commit)

Summary of changes:
 lib/MusicBrainz/Server/Controller/Edit.pm |   47 +++++++++++++++-------------
 lib/MusicBrainz/Server/Data/Edit.pm       |   23 +++++--------
 2 files changed, 34 insertions(+), 36 deletions(-)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit 7d3e8d525bbe69aee2e6c2b042bd9b55415e74da
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