mirror/bugsink
1
0
Fork 0
mirror of https://github.com/bugsink/bugsink.git synced 2025-12-30 18:00:17 -06:00
Code Issues Packages Projects Releases Wiki Activity

Use atomic transactions in views

Browse Source
This commit is contained in:
Klaas van Schelven
2024-04-18 13:15:46 +02:00
parent 1e1f7c4a6d
commit c50780ab4e
8 changed files with 118 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

46
bugsink/decorators.py
View File

@@ -2,11 +2,14 @@ from functools import wraps
from django.shortcuts import get_object_or_404
from django.core.exceptions import PermissionDenied
from django.db import transaction
from projects.models import Project
from issues.models import Issue
from events.models import Event
from .transaction import immediate_atomic
def login_exempt(view):
view.login_exempt = True
@@ -65,3 +68,46 @@ def event_membership_required(function):
raise PermissionDenied("You don't have permission to access this project")
return wrapper
def atomic_for_request_method(function, *decorator_args, **decorator_kwargs):
"""
Wrap the request in the kind of atomic transaction matching its request method:
## for data-altering (POST etc), use immediate_atomic (the desired transaction type for writes)
This is what immediate_atomic is for.
## for read requests, use the plain old transaction.atomic.
This might be surprising if you think about transactions as mainly a means of guaranteeing atomicity of writes (as
is directly implied by Django's naming). The thing we're going for is snapshot isolation (given by SQLite in WAL
mode (which we have turned on) in combination with use of transactions).
I want to have snapshot isolation because it's a mental model that I can understand. I'd argue it's the natural or
implicit mental model, and I'd rather have my program behave like so than spend _any_ time thinking about subtleties
such as "what if you select an event and an issue that are slightly out of sync" or to hunt down any hard to
reproduce bugs caused by such inconsistencies.
## Usage:
This is provided as a decorator; the expected use case is to wrap an entire view function. The reason is that, in
practice, reads which should be inside the transaction happen very early, namely when doing the various
membership_required checks. (the results of these reads are passed into the view function as event/issue/project)
(Path not taken: one could say that the membership_required tests are separate from the actual handling of the
request, whether that's a pure display request or an update. Instead of wrapping the transaction around everything,
you could re-select inside the view function. Potential advantage: shorter transactions (mostly relevant for writes,
since read-transactions are non-blocking). Disadvantage: one more query, and more complexity)
"""
@wraps(function)
def wrapper(request, *args, **kwargs):
if request.method in ["POST", "PUT", "PATCH", "DELETE"]:
with immediate_atomic(*decorator_args, **decorator_kwargs):
return function(request, *args, **kwargs)
with transaction.atomic(*decorator_args, **decorator_kwargs):
return function(request, *args, **kwargs)
return wrapper

4
bugsink/transaction.py
View File

@@ -71,8 +71,8 @@ class ImmediateAtomic(django_db_transaction.Atomic):
del connection._start_transaction_under_autocommit_original
def immediate_atomic(using=None, savepoint=True, durable=False):
# this is the Django 4.2 db.transaction.atomic, but using ImmediateAtomic (and with an extra assert)
def immediate_atomic(using=None, savepoint=True, durable=True):
# this is the Django 4.2 db.transaction.atomic, but using ImmediateAtomic, and with durable=True by default
# the following assertion is because "BEGIN IMMEDIATE" supposes a "BEGIN" (of a transaction), i.e. has no meaning in
# the context of savepoints.

4
events/views.py
View File

@@ -3,9 +3,10 @@ from django.http import HttpResponse
from django.utils.http import content_disposition_header
from django.shortcuts import render
from bugsink.decorators import event_membership_required
from bugsink.decorators import event_membership_required, atomic_for_request_method
@atomic_for_request_method
@event_membership_required
def event_download(request, event, as_attachment=False):
result = HttpResponse(event.data, content_type="application/json")
@@ -14,6 +15,7 @@ def event_download(request, event, as_attachment=False):
return result
@atomic_for_request_method
@event_membership_required
def event_plaintext(request, event):
parsed_data = json.loads(event.data)

13
ingest/tests.py
View File

@@ -2,7 +2,7 @@ import datetime
from unittest.mock import patch
from django.conf import settings
from django.test import TestCase as DjangoTestCase
from django.test import TestCase as DjangoTestCase, TransactionTestCase
from django.utils import timezone
from django.test.client import RequestFactory
@@ -18,10 +18,19 @@ from .models import DecompressedEvent
from .views import BaseIngestAPIView
class IngestViewTestCase(DjangoTestCase):
class IngestViewTestCase(TransactionTestCase):
# this TestCase started out as focussed on alert-sending, but has grown beyond that. Sometimes simply by extending
# existing tests. This is not a problem in itself, but may be slightly confusing if you don't realize that.
# We use TransactionTestCase because of the following:
#
# > Django’s TestCase class wraps each test in a transaction and rolls back that transaction after each test, in
# > order to provide test isolation. This means that no transaction is ever actually committed, thus your
# > on_commit() callbacks will never be run.
# > [..]
# > Another way to overcome the limitation is to use TransactionTestCase instead of TestCase. This will mean your
# > transactions are committed, and the callbacks will run. However [..] significantly slower [..]
def setUp(self):
# the existence of loud/quiet reflect that parts of this test focusses on alert-sending
self.request_factory = RequestFactory()

42
ingest/views.py
View File

@@ -1,9 +1,11 @@
from datetime import datetime, timezone
import json # TODO consider faster APIs
from functools import partial
from django.shortcuts import get_object_or_404
from django.conf import settings
from django.db.models import Max
from django.db import transaction
from rest_framework import permissions, status
from rest_framework.response import Response
@@ -11,7 +13,7 @@ from rest_framework.views import APIView
from rest_framework import exceptions
from rest_framework.exceptions import ValidationError
# from projects.models import Project
import sentry_sdk_extensions
from compat.auth import parse_auth_header_value
from projects.models import Project
@@ -19,14 +21,15 @@ from issues.models import Issue, IssueStateManager, Grouping, TurningPoint, Turn
from issues.utils import get_type_and_value_for_data, get_issue_grouper_for_data, get_denormalized_fields_for_data
from issues.regressions import issue_is_regression
import sentry_sdk_extensions
from events.models import Event
from releases.models import create_release_if_needed
from bugsink.registry import get_pc_registry
from bugsink.period_counter import PeriodCounter
from alerts.tasks import send_new_issue_alert, send_regression_alert
from bugsink.transaction import immediate_atomic
from bugsink.exceptions import ViolatedExpectation
from events.models import Event
from releases.models import create_release_if_needed
from alerts.tasks import send_new_issue_alert, send_regression_alert
from .negotiation import IgnoreClientContentNegotiation
from .parsers import EnvelopeParser
from .models import DecompressedEvent
@@ -101,11 +104,18 @@ class BaseIngestAPIView(APIView):
@classmethod
def digest_event(cls, ingested_event, event_data):
event, issue = cls._digest_event_to_db(ingested_event, event_data)
cls._digest_event_python_postprocessing(ingested_event, event, issue)
@classmethod
@immediate_atomic()
def _digest_event_to_db(cls, ingested_event, event_data):
# event_data is passed explicitly to avoid re-parsing something that may be availabe anyway; we'll come up with
# a better signature later if this idea sticks
# leave this at the top -- it may involve reading from the DB which should come before any DB writing
pc_registry = get_pc_registry()
# leave this at the top -- the point is to trigger load_from_scratch if needed, which may involve reading from
# the DB which should come before any DB writing
get_pc_registry()
# I resisted the temptation to put `get_denormalized_fields_for_data` in an if-statement: you basically "always"
# need this info... except when duplicate event-ids are sent. But the latter is the exception, and putting this
@@ -186,7 +196,7 @@ class BaseIngestAPIView(APIView):
kind=TurningPointKind.FIRST_SEEN)
if ingested_event.project.alert_on_new_issue:
send_new_issue_alert.delay(issue.id)
transaction.on_commit(partial(send_new_issue_alert.delay, issue.id))
else:
# new issues cannot be regressions by definition, hence this is in the 'else' branch
@@ -196,7 +206,7 @@ class BaseIngestAPIView(APIView):
kind=TurningPointKind.REGRESSED)
if ingested_event.project.alert_on_regression:
send_regression_alert.delay(issue.id)
transaction.on_commit(partial(send_regression_alert.delay, issue.id))
IssueStateManager.reopen(issue)
@@ -220,14 +230,18 @@ class BaseIngestAPIView(APIView):
issue.last_seen = ingested_event.timestamp
issue.event_count += 1
if issue.id not in get_pc_registry().by_issue:
pc_registry.by_issue[issue.id] = PeriodCounter()
issue_pc = get_pc_registry().by_issue[issue.id]
issue_pc.inc(ingested_event.timestamp, counted_entity=event)
if release.version + "\n" not in issue.events_at:
issue.events_at += release.version + "\n"
issue.save()
return event, issue
@classmethod
def _digest_event_python_postprocessing(cls, ingested_event, event, issue):
pc_registry = get_pc_registry()
if issue.id not in pc_registry.by_issue:
pc_registry.by_issue[issue.id] = PeriodCounter()
issue_pc = pc_registry.by_issue[issue.id]
issue_pc.inc(ingested_event.timestamp, counted_entity=event)
class IngestEventAPIView(BaseIngestAPIView):

20
issues/models.py
View File

@@ -2,8 +2,9 @@ from datetime import datetime, timezone
import json
import uuid
from dateutil.relativedelta import relativedelta
from functools import partial
from django.db import models
from django.db import models, transaction
from django.db.models import F, Value
from django.template.defaultfilters import date as default_date_filter
@@ -215,7 +216,8 @@ class IssueStateManager(object):
issue.is_muted = True
issue.unmute_on_volume_based_conditions = unmute_on_volume_based_conditions
IssueStateManager.set_unmute_handlers(get_pc_registry().by_issue, issue, now)
transaction.on_commit(partial(IssueStateManager.set_unmute_handlers,
get_pc_registry().by_issue, issue, now))
if unmute_after is not None:
issue.unmute_after = unmute_after
@@ -241,7 +243,9 @@ class IssueStateManager(object):
# (note: we can expect project to be set, because it will be None only when projects are deleted, in
# which case no more unmuting happens)
if issue.project.alert_on_unmute:
send_unmute_alert.delay(issue.id, format_unmute_reason(unmute_metadata))
transaction.on_commit(partial(
send_unmute_alert.delay,
issue.id, format_unmute_reason(unmute_metadata)))
# this is in a funny place but it's still simpler than introducing an Encoder
if unmute_metadata is not None and "mute_for" in unmute_metadata:
@@ -387,7 +391,9 @@ class IssueQuerysetStateManager(object):
unmute_on_volume_based_conditions=unmute_on_volume_based_conditions,
)
IssueQuerysetStateManager.set_unmute_handlers(get_pc_registry().by_issue, issue_qs, now)
transaction.on_commit(partial(
IssueQuerysetStateManager.set_unmute_handlers,
get_pc_registry().by_issue, [i for i in issue_qs], now))
if unmute_after is not None:
issue_qs.update(unmute_after=unmute_after)
@@ -408,10 +414,12 @@ class IssueQuerysetStateManager(object):
IssueStateManager.unmute(issue, triggering_event)
@staticmethod
def set_unmute_handlers(by_issue, issue_qs, now):
def set_unmute_handlers(by_issue, issue_list, now):
# in this method there's no fancy queryset based stuff (we don't actually do updates on the DB)
# the use of 'issue_list' as opposed to 'issue_qs' is a (non-enforced) indication that for correct usage (in
# on_commit) as QS should be evaluated inside the commit and the resulting list should be dealt with afterwards.
for issue in issue_qs:
for issue in issue_list:
IssueStateManager.set_unmute_handlers(by_issue, issue, now)

4
issues/tests.py
View File

@@ -7,7 +7,7 @@ from unittest import TestCase as RegularTestCase
from unittest.mock import patch
from datetime import datetime, timezone
from django.test import TestCase as DjangoTestCase
from django.test import TestCase as DjangoTestCase, TransactionTestCase
from django.contrib.auth.models import User
from django.test import tag
@@ -296,7 +296,7 @@ seen as an undo rather than anything else.
"""
class MuteUnmuteTestCase(DjangoTestCase):
class MuteUnmuteTestCase(TransactionTestCase):
"""
Somewhat of an integration test. The unit-under-test here is the whole of
* the pc_registry

13
issues/views.py
View File

@@ -11,7 +11,7 @@ from django.urls import reverse
from django.core.exceptions import PermissionDenied
from events.models import Event
from bugsink.decorators import project_membership_required, issue_membership_required
from bugsink.decorators import project_membership_required, issue_membership_required, atomic_for_request_method
from compat.timestamp import format_timestamp
from .models import (
@@ -162,6 +162,7 @@ def _apply_action(manager, issue_or_qs, action, user):
manager.unmute(issue_or_qs)
@atomic_for_request_method
@project_membership_required
def issue_list(request, project, state_filter="open"):
if request.method == "POST":
@@ -215,6 +216,7 @@ def event_by_internal_id(request, event_pk):
return redirect(issue_event_stacktrace, issue_pk=issue.pk, event_pk=event.pk)
@atomic_for_request_method
@issue_membership_required
def issue_last_event(request, issue):
last_event = issue.event_set.order_by("timestamp").last()
@@ -250,6 +252,7 @@ def _get_event(issue, event_pk, ingest_order):
raise ValueError("either event_pk or ingest_order must be provided")
@atomic_for_request_method
@issue_membership_required
def issue_event_stacktrace(request, issue, event_pk=None, ingest_order=None):
if request.method == "POST":
@@ -296,6 +299,7 @@ def issue_event_stacktrace(request, issue, event_pk=None, ingest_order=None):
})
@atomic_for_request_method
@issue_membership_required
def issue_event_breadcrumbs(request, issue, event_pk=None, ingest_order=None):
if request.method == "POST":
@@ -323,6 +327,7 @@ def _date_with_milis_html(timestamp):
'<span class="text-xs">' + date(timestamp, "u")[:3] + '</span>')
@atomic_for_request_method
@issue_membership_required
def issue_event_details(request, issue, event_pk=None, ingest_order=None):
if request.method == "POST":
@@ -361,6 +366,7 @@ def issue_event_details(request, issue, event_pk=None, ingest_order=None):
})
@atomic_for_request_method
@issue_membership_required
def issue_history(request, issue):
if request.method == "POST":
@@ -378,6 +384,7 @@ def issue_history(request, issue):
})
@atomic_for_request_method
@issue_membership_required
def issue_grouping(request, issue):
if request.method == "POST":
@@ -395,6 +402,7 @@ def issue_grouping(request, issue):
})
@atomic_for_request_method
@issue_membership_required
def issue_event_list(request, issue):
if request.method == "POST":
@@ -415,6 +423,7 @@ def issue_event_list(request, issue):
})
@atomic_for_request_method
@issue_membership_required
def history_comment_new(request, issue):
@@ -436,6 +445,7 @@ def history_comment_new(request, issue):
return HttpResponseNotAllowed(["POST"])
@atomic_for_request_method
@issue_membership_required
def history_comment_edit(request, issue, comment_pk):
comment = get_object_or_404(TurningPoint, pk=comment_pk, issue_id=issue.pk)
@@ -450,6 +460,7 @@ def history_comment_edit(request, issue, comment_pk):
return redirect(reverse(issue_history, kwargs={'issue_pk': issue.pk}) + f"#comment-{ comment_pk }")
@atomic_for_request_method
@issue_membership_required
def history_comment_delete(request, issue, comment_pk):
comment = get_object_or_404(TurningPoint, pk=comment_pk, issue_id=issue.pk)