diff --git a/pgcommitfest/commitfest/tests/test_newpatch_transaction.py b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py new file mode 100644 index 00000000..ca78d14c --- /dev/null +++ b/pgcommitfest/commitfest/tests/test_newpatch_transaction.py @@ -0,0 +1,68 @@ +from django.db import connection + +from datetime import datetime, timezone +from unittest.mock import patch + +import pytest + +from pgcommitfest.commitfest.models import MailThread, Patch + +pytestmark = pytest.mark.django_db + +MSGID = "test-thread@example.com" + + +def _fail_with_database_error(*args, **kwargs): + # A real failing query, not just a raised exception, so that PostgreSQL + # aborts the transaction and any further query in it would fail too. + with connection.cursor() as cursor: + cursor.execute("SELECT 1/0") + + +@pytest.mark.parametrize("fail", [False, True], ids=["success", "database-error"]) +def test_newpatch_thread_attach( + client, alice, open_cf, in_progress_cf, recent_closed_cf, fail +): + client.force_login(alice) + message = { + "msgid": MSGID, + "date": datetime(2025, 1, 10, 12, tzinfo=timezone.utc), + "from": "author@example.com", + "subj": "[PATCH] Test", + "atts": [], + } + + with ( + patch("pgcommitfest.commitfest.forms._archivesAPI", return_value=[message]), + patch("pgcommitfest.commitfest.ajax._archivesAPI", return_value=[message]), + patch( + "pgcommitfest.commitfest.ajax.parse_and_add_attachments", + side_effect=_fail_with_database_error if fail else None, + ) as attachments_mock, + ): + response = client.post( + f"/{open_cf.pk}/new/", + { + "name": "Transaction test", + "threadmsgid": MSGID, + "no_tags_apply": "on", + "authors": [str(alice.pk)], + }, + ) + + attachments_mock.assert_called_once() + if fail: + # The form is re-rendered with an error and nothing is left behind + assert response.status_code == 200 + assert b"An error occurred looking up the thread" in response.content + assert not Patch.objects.filter(name="Transaction test").exists() + assert not MailThread.objects.filter(messageid=MSGID).exists() + else: + created_patch = Patch.objects.get(name="Transaction test") + assert response.status_code == 302 + assert response["Location"] == f"/patch/{created_patch.pk}/" + assert ( + MailThread.objects.get(messageid=MSGID) + .patches.filter(pk=created_patch.pk) + .exists() + ) diff --git a/pgcommitfest/commitfest/views.py b/pgcommitfest/commitfest/views.py index 34553217..7e03030d 100644 --- a/pgcommitfest/commitfest/views.py +++ b/pgcommitfest/commitfest/views.py @@ -988,8 +988,11 @@ def patchform(request, patchid): ) +class ThreadAttachFailed(Exception): + pass + + @login_required -@transaction.atomic def newpatch(request, cfid): cf = get_object_or_404(CommitFest, pk=cfid) if not cf.status == CommitFest.STATUS_OPEN and not request.user.is_staff: @@ -998,33 +1001,39 @@ def newpatch(request, cfid): if request.method == "POST": form = NewPatchForm(data=request.POST) if form.is_valid(): - patch = form.save() - poc = PatchOnCommitFest( - patch=patch, commitfest=cf, enterdate=datetime.now() - ) - poc.save() - PatchHistory( - patch=patch, by=request.user, what="Created patch record" - ).save() - # Now add the thread + # A failure to attach the thread is re-raised as + # ThreadAttachFailed so that it still propagates out of the + # atomic block, which rolls back the patch and everything else + # created here. The form error is only set after leaving the + # block, so the queries needed to re-render the form still work + # even if the failure was a database error that aborted the + # transaction. Failures in the other statements are not caught + # here, so they still result in a logged 500. try: - doAttachThread( - cf, patch, form.cleaned_data["threadmsgid"], request.user - ) + with transaction.atomic(): + patch = form.save() + PatchOnCommitFest( + patch=patch, commitfest=cf, enterdate=datetime.now() + ).save() + PatchHistory( + patch=patch, by=request.user, what="Created patch record" + ).save() + try: + doAttachThread( + cf, patch, form.cleaned_data["threadmsgid"], request.user + ) + except Http404 as e: + # This is a horrible breakage of API layers + raise ThreadAttachFailed( + "Selected thread did not exist in the archives" + ) from e + except Exception as e: + raise ThreadAttachFailed( + "An error occurred looking up the thread in the archives." + ) from e return HttpResponseRedirect("/patch/%s/" % (patch.id,)) - except Http404: - # Thread not found! - # This is a horrible breakage of API layers - form._errors["threadmsgid"] = form.error_class( - ("Selected thread did not exist in the archives",) - ) - except Exception: - form._errors["threadmsgid"] = form.error_class( - ("An error occurred looking up the thread in the archives.",) - ) - # In this case, we have created a patch - delete it. This causes a agp in id's, but it should - # not happen very often. If we successfully attached to it, we will have already returned. - patch.delete() + except ThreadAttachFailed as e: + form._errors["threadmsgid"] = form.error_class((str(e),)) else: form = NewPatchForm(request=request)