diff --git a/bugsink/decorators.py b/bugsink/decorators.py index f018db3..8a6fa05 100644 --- a/bugsink/decorators.py +++ b/bugsink/decorators.py @@ -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 diff --git a/bugsink/transaction.py b/bugsink/transaction.py index 3b840b4..2ab58f6 100644 --- a/bugsink/transaction.py +++ b/bugsink/transaction.py @@ -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. diff --git a/events/views.py b/events/views.py index a9fc466..53f3b7d 100644 --- a/events/views.py +++ b/events/views.py @@ -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) diff --git a/ingest/tests.py b/ingest/tests.py index 434640b..bbfa80b 100644 --- a/ingest/tests.py +++ b/ingest/tests.py @@ -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() diff --git a/ingest/views.py b/ingest/views.py index 0dfacc5..321d00b 100644 --- a/ingest/views.py +++ b/ingest/views.py @@ -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): diff --git a/issues/models.py b/issues/models.py index 361923f..17209e0 100644 --- a/issues/models.py +++ b/issues/models.py @@ -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) diff --git a/issues/tests.py b/issues/tests.py index 3afab87..bf81b86 100644 --- a/issues/tests.py +++ b/issues/tests.py @@ -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 diff --git a/issues/views.py b/issues/views.py index 1d25aea..863a701 100644 --- a/issues/views.py +++ b/issues/views.py @@ -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): '' + date(timestamp, "u")[:3] + '') +@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)