From 3c1f3a26cf9c196821931ecc4ebe0b4001cbf3b1 Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Wed, 25 Jan 2023 11:44:11 +0100 Subject: [PATCH] Always make explicit which tables to lock (#3058) Co-authored-by: Richard Schreiber --- src/pretix/api/middleware.py | 3 ++- src/pretix/api/views/organizer.py | 5 +++-- src/pretix/api/webhooks.py | 7 +++++-- src/pretix/base/models/orders.py | 5 +++-- src/pretix/base/payment.py | 3 ++- src/pretix/base/services/cancelevent.py | 3 ++- src/pretix/base/services/checkin.py | 8 ++++++-- src/pretix/base/services/export.py | 5 +++-- src/pretix/base/services/invoices.py | 4 ++-- src/pretix/base/services/memberships.py | 3 ++- src/pretix/base/services/orders.py | 19 ++++++++++--------- src/pretix/control/views/organizer.py | 4 ++-- src/pretix/helpers/database.py | 8 +++++++- src/pretix/plugins/sendmail/signals.py | 2 ++ src/pretix/plugins/stripe/payment.py | 3 ++- src/pretix/plugins/stripe/views.py | 11 ++++++----- 16 files changed, 59 insertions(+), 34 deletions(-) diff --git a/src/pretix/api/middleware.py b/src/pretix/api/middleware.py index 81c2354720..9e18c121cf 100644 --- a/src/pretix/api/middleware.py +++ b/src/pretix/api/middleware.py @@ -32,6 +32,7 @@ from rest_framework import status from pretix.api.models import ApiCall from pretix.base.models import Organizer +from pretix.helpers import OF_SELF class IdempotencyMiddleware: @@ -56,7 +57,7 @@ class IdempotencyMiddleware: idempotency_key = request.headers.get('X-Idempotency-Key', '') with transaction.atomic(): - call, created = ApiCall.objects.select_for_update().get_or_create( + call, created = ApiCall.objects.select_for_update(of=OF_SELF).get_or_create( auth_hash=auth_hash, idempotency_key=idempotency_key, defaults={ diff --git a/src/pretix/api/views/organizer.py b/src/pretix/api/views/organizer.py index 05b689826f..949723c35b 100644 --- a/src/pretix/api/views/organizer.py +++ b/src/pretix/api/views/organizer.py @@ -51,6 +51,7 @@ from pretix.base.models import ( User, ) from pretix.base.settings import SETTINGS_AFFECTING_CSS +from pretix.helpers import OF_SELF from pretix.helpers.dicts import merge_dicts from pretix.presale.style import regenerate_organizer_css @@ -178,7 +179,7 @@ class GiftCardViewSet(viewsets.ModelViewSet): def perform_update(self, serializer): if 'include_accepted' in self.request.GET: raise PermissionDenied("Accepted gift cards cannot be updated, use transact instead.") - GiftCard.objects.select_for_update().get(pk=self.get_object().pk) + GiftCard.objects.select_for_update(of=OF_SELF).get(pk=self.get_object().pk) old_value = serializer.instance.value value = serializer.validated_data.pop('value') inst = serializer.save(secret=serializer.instance.secret, currency=serializer.instance.currency, @@ -196,7 +197,7 @@ class GiftCardViewSet(viewsets.ModelViewSet): @action(detail=True, methods=["POST"]) @transaction.atomic() def transact(self, request, **kwargs): - gc = GiftCard.objects.select_for_update().get(pk=self.get_object().pk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=self.get_object().pk) value = serializers.DecimalField(max_digits=10, decimal_places=2).to_internal_value( request.data.get('value') ) diff --git a/src/pretix/api/webhooks.py b/src/pretix/api/webhooks.py index ce88cbdff7..db33d3601f 100644 --- a/src/pretix/api/webhooks.py +++ b/src/pretix/api/webhooks.py @@ -42,6 +42,7 @@ from pretix.base.models import LogEntry from pretix.base.services.tasks import ProfiledTask, TransactionAwareTask from pretix.base.signals import periodic_task from pretix.celery_app import app +from pretix.helpers import OF_SELF logger = logging.getLogger(__name__) _ALL_EVENTS = None @@ -502,7 +503,8 @@ def manually_retry_all_calls(webhook_id: int): webhook = WebHook.objects.get(id=webhook_id) with scope(organizer=webhook.organizer), transaction.atomic(): for whcr in webhook.retries.select_for_update( - skip_locked=connection.features.has_select_for_update_skip_locked + skip_locked=connection.features.has_select_for_update_skip_locked, + of=OF_SELF ): send_webhook.apply_async( args=(whcr.logentry_id, whcr.action_type, whcr.webhook_id, whcr.retry_count), @@ -515,7 +517,8 @@ def manually_retry_all_calls(webhook_id: int): def schedule_webhook_retries_on_celery(sender, **kwargs): with transaction.atomic(): for whcr in WebHookCallRetry.objects.select_for_update( - skip_locked=connection.features.has_select_for_update_skip_locked + skip_locked=connection.features.has_select_for_update_skip_locked, + of=OF_SELF ).filter(retry_not_before__lt=now()): send_webhook.apply_async( args=(whcr.logentry_id, whcr.action_type, whcr.webhook_id, whcr.retry_count), diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index dd19a99932..a7d08cf5b9 100644 --- a/src/pretix/base/models/orders.py +++ b/src/pretix/base/models/orders.py @@ -79,6 +79,7 @@ from pretix.base.services.locking import LOCK_TIMEOUT, NoLockManager from pretix.base.settings import PERSON_NAME_SCHEMES from pretix.base.signals import order_gracefully_delete +from ...helpers import OF_SELF from ...helpers.countries import CachedCountries, FastCountryField from ...helpers.format import format_map from ._transactions import ( @@ -1628,7 +1629,7 @@ class OrderPayment(models.Model): been marked as paid. """ with transaction.atomic(): - locked_instance = OrderPayment.objects.select_for_update().get(pk=self.pk) + locked_instance = OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=self.pk) if locked_instance.state not in (OrderPayment.PAYMENT_STATE_CREATED, OrderPayment.PAYMENT_STATE_PENDING): # Race condition detected, this payment is already confirmed logger.info('Failed payment {} but ignored due to likely race condition.'.format( @@ -1673,7 +1674,7 @@ class OrderPayment(models.Model): :raises Quota.QuotaExceededException: if the quota is exceeded and ``force`` is ``False`` """ with transaction.atomic(): - locked_instance = OrderPayment.objects.select_for_update().get(pk=self.pk) + locked_instance = OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=self.pk) if locked_instance.state == self.PAYMENT_STATE_CONFIRMED: # Race condition detected, this payment is already confirmed logger.info('Confirmed payment {} but ignored due to likely race condition.'.format( diff --git a/src/pretix/base/payment.py b/src/pretix/base/payment.py index a9571923f0..4901a7b07d 100644 --- a/src/pretix/base/payment.py +++ b/src/pretix/base/payment.py @@ -67,6 +67,7 @@ from pretix.base.settings import SettingsSandbox from pretix.base.signals import register_payment_providers from pretix.base.templatetags.money import money_filter from pretix.base.templatetags.rich_text import rich_text +from pretix.helpers import OF_SELF from pretix.helpers.countries import CachedCountries from pretix.helpers.format import format_map from pretix.helpers.money import DecimalTextInput @@ -1399,7 +1400,7 @@ class GiftCardPayment(BasePaymentProvider): try: with transaction.atomic(): try: - gc = GiftCard.objects.select_for_update().get(pk=gcpk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gcpk) except GiftCard.DoesNotExist: raise PaymentException(_("This gift card does not support this currency.")) if gc.currency != self.event.currency: # noqa - just a safeguard diff --git a/src/pretix/base/services/cancelevent.py b/src/pretix/base/services/cancelevent.py index 2bad4e5b9b..ca03f11bb9 100644 --- a/src/pretix/base/services/cancelevent.py +++ b/src/pretix/base/services/cancelevent.py @@ -41,6 +41,7 @@ from pretix.base.services.orders import ( ) from pretix.base.services.tasks import ProfiledEventTask from pretix.celery_app import app +from pretix.helpers import OF_SELF from pretix.helpers.format import format_map logger = logging.getLogger(__name__) @@ -239,7 +240,7 @@ def cancel_event(self, event: Event, subevent: int, auto_refund: bool, for o in orders_to_change.values_list('id', flat=True).iterator(): with transaction.atomic(): - o = event.orders.select_for_update().get(pk=o) + o = event.orders.select_for_update(of=OF_SELF).get(pk=o) total = Decimal('0.00') fee = Decimal('0.00') positions = [] diff --git a/src/pretix/base/services/checkin.py b/src/pretix/base/services/checkin.py index d4d3fe629d..899baa101d 100644 --- a/src/pretix/base/services/checkin.py +++ b/src/pretix/base/services/checkin.py @@ -56,6 +56,7 @@ from pretix.base.models import ( Checkin, CheckinList, Device, Order, OrderPosition, QuestionOption, ) from pretix.base.signals import checkin_created, order_placed, periodic_task +from pretix.helpers import OF_SELF from pretix.helpers.jsonlogic import Logic from pretix.helpers.jsonlogic_boolalg import convert_to_dnf from pretix.helpers.jsonlogic_query import ( @@ -729,8 +730,11 @@ def perform_checkin(op: OrderPosition, clist: CheckinList, given_answers: dict, _save_answers(op, answers, given_answers) with transaction.atomic(): - # Lock order positions - op = OrderPosition.all.select_for_update().get(pk=op.pk) + # Lock order positions, if it is an entry. We don't need it for exits, as a race condition wouldn't be problematic + opqs = OrderPosition.all + if type != Checkin.TYPE_EXIT: + opqs = opqs.select_for_update(of=OF_SELF) + op = opqs.get(pk=op.pk) if not clist.all_products and op.item_id not in [i.pk for i in clist.limit_products.all()]: raise CheckInError( diff --git a/src/pretix/base/services/export.py b/src/pretix/base/services/export.py index 49c5669376..d902347aaa 100644 --- a/src/pretix/base/services/export.py +++ b/src/pretix/base/services/export.py @@ -49,6 +49,7 @@ from pretix.base.signals import ( periodic_task, register_data_exporters, register_multievent_data_exporters, ) from pretix.celery_app import app +from pretix.helpers import OF_SELF from pretix.helpers.urls import build_absolute_uri logger = logging.getLogger(__name__) @@ -344,7 +345,7 @@ def run_scheduled_exports(sender, **kwargs): qs = ScheduledEventExport.objects.filter( schedule_next_run__lt=now(), error_counter__lt=5, - ).select_for_update(skip_locked=connection.features.has_select_for_update_skip_locked).select_related('event') + ).select_for_update(skip_locked=connection.features.has_select_for_update_skip_locked, of=OF_SELF).select_related('event') for s in qs: scheduled_event_export.apply_async(kwargs={ 'event': s.event_id, @@ -355,7 +356,7 @@ def run_scheduled_exports(sender, **kwargs): qs = ScheduledOrganizerExport.objects.filter( schedule_next_run__lt=now(), error_counter__lt=5, - ).select_for_update(skip_locked=connection.features.has_select_for_update_skip_locked).select_related('organizer') + ).select_for_update(skip_locked=connection.features.has_select_for_update_skip_locked, of=OF_SELF).select_related('organizer') for s in qs: scheduled_organizer_export.apply_async(kwargs={ 'organizer': s.organizer_id, diff --git a/src/pretix/base/services/invoices.py b/src/pretix/base/services/invoices.py index edc0393bb1..d5f68e4572 100644 --- a/src/pretix/base/services/invoices.py +++ b/src/pretix/base/services/invoices.py @@ -63,7 +63,7 @@ from pretix.base.services.tasks import TransactionAwareTask from pretix.base.settings import GlobalSettingsObject from pretix.base.signals import invoice_line_text, periodic_task from pretix.celery_app import app -from pretix.helpers.database import rolledback_transaction +from pretix.helpers.database import OF_SELF, rolledback_transaction from pretix.helpers.models import modelcopy logger = logging.getLogger(__name__) @@ -500,7 +500,7 @@ def send_invoices_to_organizer(sender, **kwargs): with transaction.atomic(): qs = Invoice.objects.filter( sent_to_organizer__isnull=True - ).prefetch_related('event').select_for_update(skip_locked=connection.features.has_select_for_update_skip_locked) + ).prefetch_related('event').select_for_update(of=OF_SELF, skip_locked=connection.features.has_select_for_update_skip_locked) for i in qs[:batch_size]: if i.event.settings.invoice_email_organizer: with language(i.event.settings.locale): diff --git a/src/pretix/base/services/memberships.py b/src/pretix/base/services/memberships.py index 7ac480eeda..6a7ca3cf56 100644 --- a/src/pretix/base/services/memberships.py +++ b/src/pretix/base/services/memberships.py @@ -32,6 +32,7 @@ from pretix.base.models import ( AbstractPosition, Customer, Event, Item, Membership, Order, OrderPosition, SubEvent, ) +from pretix.helpers import OF_SELF def membership_validity(item: Item, subevent: Optional[SubEvent], event: Event): @@ -118,7 +119,7 @@ def validate_memberships_in_order(customer: Customer, positions: List[AbstractPo base_qs = Membership.objects.with_usages(ignored_order=ignored_order) if lock: - base_qs = base_qs.select_for_update() + base_qs = base_qs.select_for_update(of=OF_SELF) membership_cache = base_qs\ .select_related('membership_type')\ diff --git a/src/pretix/base/services/orders.py b/src/pretix/base/services/orders.py index cd06735f07..2ba9494595 100644 --- a/src/pretix/base/services/orders.py +++ b/src/pretix/base/services/orders.py @@ -97,6 +97,7 @@ from pretix.base.signals import ( order_placed, order_split, periodic_task, validate_order, ) from pretix.celery_app import app +from pretix.helpers import OF_SELF from pretix.helpers.models import modelcopy from pretix.helpers.periodic import minimum_interval @@ -184,7 +185,7 @@ def reactivate_order(order: Order, force: bool=False, user: User=None, auth=None Voucher.objects.filter(pk=position.voucher.pk).update(redeemed=Greatest(0, F('redeemed') + 1)) for gc in position.issued_gift_cards.all(): - gc = GiftCard.objects.select_for_update().get(pk=gc.pk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gc.pk) gc.transactions.create(value=position.price, order=order) break @@ -397,7 +398,7 @@ def _cancel_order(order, user=None, send_mail: bool=True, api_token=None, device # If new actions are added to this function, make sure to add the reverse operation to reactivate_order() with transaction.atomic(): if isinstance(order, int): - order = Order.objects.select_for_update().get(pk=order) + order = Order.objects.select_for_update(of=OF_SELF).get(pk=order) if isinstance(user, int): user = User.objects.get(pk=user) if isinstance(api_token, int): @@ -419,7 +420,7 @@ def _cancel_order(order, user=None, send_mail: bool=True, api_token=None, device for position in order.positions.all(): for gc in position.issued_gift_cards.all(): - gc = GiftCard.objects.select_for_update().get(pk=gc.pk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gc.pk) if gc.value < position.price: raise OrderError( _('This order can not be canceled since the gift card {card} purchased in ' @@ -1205,7 +1206,7 @@ def send_expiry_warnings(sender, **kwargs): if days and (o.expires - today).days <= days: with transaction.atomic(): - o = Order.objects.select_related('event').select_for_update().get(pk=o.pk) + o = Order.objects.select_related('event').select_for_update(of=OF_SELF).get(pk=o.pk) if o.status != Order.STATUS_PENDING or o.expiry_reminder_sent: # Race condition continue @@ -1264,7 +1265,7 @@ def send_download_reminders(sender, **kwargs): continue with transaction.atomic(): - o = Order.objects.select_for_update().get(pk=o.pk) + o = Order.objects.select_for_update(of=OF_SELF).get(pk=o.pk) if o.download_reminder_sent: # Race condition continue @@ -2059,7 +2060,7 @@ class OrderChangeManager: op.fee.save(update_fields=['canceled']) elif isinstance(op, self.CancelOperation): for gc in op.position.issued_gift_cards.all(): - gc = GiftCard.objects.select_for_update().get(pk=gc.pk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gc.pk) if gc.value < op.position.price: raise OrderError(_( 'A position can not be canceled since the gift card {card} purchased in this order has ' @@ -2075,7 +2076,7 @@ class OrderChangeManager: for opa in op.position.addons.all(): for gc in opa.issued_gift_cards.all(): - gc = GiftCard.objects.select_for_update().get(pk=gc.pk) + gc = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=gc.pk) if gc.value < opa.position.price: raise OrderError(_( 'A position can not be canceled since the gift card {card} purchased in this order has ' @@ -2648,9 +2649,9 @@ def change_payment_provider(order: Order, payment_provider, amount=None, new_pay open_payment = None if new_payment: - lp = order.payments.select_for_update().exclude(pk=new_payment.pk).last() + lp = order.payments.select_for_update(of=OF_SELF).exclude(pk=new_payment.pk).last() else: - lp = order.payments.select_for_update().last() + lp = order.payments.select_for_update(of=OF_SELF).last() if lp and lp.state in (OrderPayment.PAYMENT_STATE_PENDING, OrderPayment.PAYMENT_STATE_CREATED): open_payment = lp diff --git a/src/pretix/control/views/organizer.py b/src/pretix/control/views/organizer.py index b10cd442bc..908e4dfd39 100644 --- a/src/pretix/control/views/organizer.py +++ b/src/pretix/control/views/organizer.py @@ -110,7 +110,7 @@ from pretix.control.permissions import ( from pretix.control.signals import nav_organizer from pretix.control.views import PaginationMixin from pretix.control.views.mailsetup import MailSettingsSetupView -from pretix.helpers import GroupConcat +from pretix.helpers import OF_SELF, GroupConcat from pretix.helpers.compat import CompatDeleteView from pretix.helpers.dicts import merge_dicts from pretix.helpers.format import format_map @@ -1381,7 +1381,7 @@ class GiftCardDetailView(OrganizerDetailViewMixin, OrganizerPermissionRequiredMi @transaction.atomic() def post(self, request, *args, **kwargs): - self.object = GiftCard.objects.select_for_update().get(pk=self.get_object().pk) + self.object = GiftCard.objects.select_for_update(of=OF_SELF).get(pk=self.get_object().pk) if 'revert' in request.POST: t = get_object_or_404(self.object.transactions.all(), pk=request.POST.get('revert'), order__isnull=False) if self.object.value - t.value < Decimal('0.00'): diff --git a/src/pretix/helpers/database.py b/src/pretix/helpers/database.py index dc0786f339..b9568a204d 100644 --- a/src/pretix/helpers/database.py +++ b/src/pretix/helpers/database.py @@ -21,8 +21,9 @@ # import contextlib -from django.db import transaction +from django.db import connection, transaction from django.db.models import Aggregate, Expression, Field, Lookup, Value +from django.utils.functional import lazy class DummyRollbackException(Exception): @@ -142,3 +143,8 @@ class PostgresWindowFrame(Expression): "start": self.start.value, "end": self.end.value, } + + +# This is a short-hand for .select_for_update(of=("self,")), that falls back gracefully on databases that don't support +# the SELECT FOR UPDATE OF ... query. +OF_SELF = lazy(lambda: ("self",) if connection.features.has_select_for_update_of else (), tuple)() diff --git a/src/pretix/plugins/sendmail/signals.py b/src/pretix/plugins/sendmail/signals.py index 6e3c7e2cfd..cc05479a29 100644 --- a/src/pretix/plugins/sendmail/signals.py +++ b/src/pretix/plugins/sendmail/signals.py @@ -51,6 +51,7 @@ from pretix.base.signals import ( EventPluginSignal, event_copy_data, logentry_display, periodic_task, ) from pretix.control.signals import nav_event +from pretix.helpers import OF_SELF from pretix.plugins.sendmail.models import ScheduledMail from pretix.plugins.sendmail.views import OrderSendView, WaitinglistSendView @@ -187,6 +188,7 @@ def sendmail_run_rules(sender, **kwargs): with transaction.atomic(durable=True): m = ScheduledMail.objects.select_for_update( + of=OF_SELF, skip_locked=connection.features.has_select_for_update_skip_locked ).filter(pk=m_id).first() if not m or m.state not in (ScheduledMail.STATE_SCHEDULED, ScheduledMail.STATE_FAILED): diff --git a/src/pretix/plugins/stripe/payment.py b/src/pretix/plugins/stripe/payment.py index 8b422fe4a1..49f3ddd2b5 100644 --- a/src/pretix/plugins/stripe/payment.py +++ b/src/pretix/plugins/stripe/payment.py @@ -63,6 +63,7 @@ from pretix.base.payment import BasePaymentProvider, PaymentException from pretix.base.plugins import get_all_plugins from pretix.base.services.mail import SendMailException from pretix.base.settings import SettingsSandbox +from pretix.helpers import OF_SELF from pretix.helpers.urls import build_absolute_uri as build_global_uri from pretix.multidomain.urlreverse import build_absolute_uri, eventreverse from pretix.plugins.stripe.forms import StripeKeyValidator @@ -586,7 +587,7 @@ class StripeMethod(BasePaymentProvider): self._init_api() payment_info = refund.payment.info_data - OrderPayment.objects.select_for_update().get(pk=refund.payment.pk) + OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=refund.payment.pk) if not payment_info: raise PaymentException(_('No payment information found.')) diff --git a/src/pretix/plugins/stripe/views.py b/src/pretix/plugins/stripe/views.py index fce2c4bbc7..76864dc32f 100644 --- a/src/pretix/plugins/stripe/views.py +++ b/src/pretix/plugins/stripe/views.py @@ -64,6 +64,7 @@ from pretix.control.permissions import ( ) from pretix.control.views.event import DecoupleMixin from pretix.control.views.organizer import OrganizerDetailViewMixin +from pretix.helpers import OF_SELF from pretix.multidomain.urlreverse import build_absolute_uri, eventreverse from pretix.plugins.stripe.forms import OrganizerStripeSettingsForm from pretix.plugins.stripe.models import ReferencedStripeObject @@ -297,13 +298,13 @@ def charge_webhook(event, event_json, charge_id, rso): with transaction.atomic(): if payment: - payment = OrderPayment.objects.select_for_update().get(pk=payment.pk) + payment = OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=payment.pk) else: payment = order.payments.filter( info__icontains=charge['id'], provider__startswith='stripe', amount=prov._amount_to_decimal(charge['amount']), - ).select_for_update().last() + ).select_for_update(of=OF_SELF).last() if not payment: payment = order.payments.create( state=OrderPayment.PAYMENT_STATE_CREATED, @@ -393,13 +394,13 @@ def source_webhook(event, event_json, source_id, rso): payment = None if payment: - payment = OrderPayment.objects.select_for_update().get(pk=payment.pk) + payment = OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=payment.pk) else: payment = order.payments.filter( info__icontains=src['id'], provider__startswith='stripe', amount=prov._amount_to_decimal(src['amount']) if src['amount'] is not None else order.total, - ).select_for_update().last() + ).select_for_update(of=OF_SELF).last() if not payment: payment = order.payments.create( state=OrderPayment.PAYMENT_STATE_CREATED, @@ -534,7 +535,7 @@ class ReturnView(StripeOrderView, View): with transaction.atomic(): self.order.refresh_from_db() - self.payment = OrderPayment.objects.select_for_update().get(pk=self.payment.pk) + self.payment = OrderPayment.objects.select_for_update(of=OF_SELF).get(pk=self.payment.pk) if self.payment.state == OrderPayment.PAYMENT_STATE_CONFIRMED: if 'payment_stripe_token' in request.session: del request.session['payment_stripe_token']