diff --git a/backend/apps/bookings/serializers.py b/backend/apps/bookings/serializers.py index 4ad920f..129e919 100644 --- a/backend/apps/bookings/serializers.py +++ b/backend/apps/bookings/serializers.py @@ -1,3 +1,4 @@ +from django.db import transaction from django.utils.translation import gettext_lazy as _ from rest_framework import serializers from apps.bookings.models import Booking, BookingStatus @@ -74,14 +75,40 @@ class BookingCreateSerializer(serializers.ModelSerializer): def create(self, validated_data): request = self.context["request"] service = validated_data["service"] - return Booking.objects.create( - salon=service.salon, - customer=request.user, - service=service, - staff=validated_data.get("staff"), - start_time=validated_data["start_time"], - end_time=validated_data["end_time"], - notes=validated_data.get("notes", ""), - price_amount=service.price_amount, - currency=service.currency, - ) + staff = validated_data.get("staff") + start_time = validated_data["start_time"] + end_time = validated_data["end_time"] + + with transaction.atomic(): + # Lock the staff row so concurrent booking requests for the same staff + # member are serialized. Without this, two requests that both pass the + # overlap check in validate() can race and both commit overlapping + # bookings. On SQLite (dev/tests) the FOR UPDATE clause is silently + # ignored but the transaction still serializes writes; PostgreSQL + # (production) gets true row-level locking. + StaffProfile.objects.select_for_update().get(pk=staff.pk) + + # Re-run the overlap check inside the lock so the check and the insert + # are atomic with respect to other writers. + overlap = Booking.objects.filter( + staff=staff, + status__in=[BookingStatus.PENDING, BookingStatus.CONFIRMED], + start_time__lt=end_time, + end_time__gt=start_time, + ).exists() + if overlap: + raise serializers.ValidationError( + {"start_time": _("Booking overlaps an existing appointment")} + ) + + return Booking.objects.create( + salon=service.salon, + customer=request.user, + service=service, + staff=staff, + start_time=start_time, + end_time=end_time, + notes=validated_data.get("notes", ""), + price_amount=service.price_amount, + currency=service.currency, + ) diff --git a/docs/risks.md b/docs/risks.md index c56cd40..3661198 100644 --- a/docs/risks.md +++ b/docs/risks.md @@ -11,7 +11,7 @@ This file tracks known gaps and risks to address in future iterations. ## Booking Integrity - Availability checks and overlap prevention are now enforced for staff bookings. -- **Race condition — booking overlap check is not atomic:** `validate_booking_request` runs the overlap query and returns; the view then calls `serializer.save()` in a separate step with no lock held. Two concurrent POST `/api/bookings/` requests for the same staff slot will both pass validation and both commit. Fix: wrap the overlap check and booking insert in a `select_for_update()` query (or use serializable transaction isolation) so only one request can hold the lock at a time. +- **Race condition — fixed:** `BookingCreateSerializer.create()` now locks the staff row with `select_for_update()` inside `transaction.atomic()` and re-runs the overlap check before inserting. Concurrent requests for the same staff slot are serialized at the DB level. Requires PostgreSQL in production (SQLite ignores `FOR UPDATE` but still serializes writes). - No timezone handling or business hours enforcement. - No cancellation rules or refund logic.