From 5bb775bccbe0d3be86912d17b8298e9069d4029f Mon Sep 17 00:00:00 2001 From: Cliff Hill Date: Tue, 26 Aug 2025 23:40:47 -0400 Subject: [PATCH] Cleaning up, adding constraints. Signed-off-by: Cliff Hill --- .flake8 | 4 +- ...migration.py => 0001_initial_migration.py} | 4 +- .../0002_add_booking_exclusion_constraint.py | 35 ++++ .../0003_add_invitee_unique_constraint.py | 25 +++ .../0004_add_start_end_check_constraint.py | 25 +++ backend/src/backend/routers/invitees.py | 9 + backend/src/backend/services/bookings.py | 155 ++++++++++++++- backend/src/backend/services/invitees.py | 34 +++- backend/tests/services/test_bookings.py | 187 ++++++++++++++++++ compose.yml | 1 + 10 files changed, 474 insertions(+), 5 deletions(-) rename backend/migrations/versions/{e6a14f4dae3d_initial_migration.py => 0001_initial_migration.py} (95%) create mode 100644 backend/migrations/versions/0002_add_booking_exclusion_constraint.py create mode 100644 backend/migrations/versions/0003_add_invitee_unique_constraint.py create mode 100644 backend/migrations/versions/0004_add_start_end_check_constraint.py diff --git a/.flake8 b/.flake8 index 1d069559..fe1f1654 100644 --- a/.flake8 +++ b/.flake8 @@ -4,7 +4,9 @@ ignore = E203,E501,RST201,RST203,RST301,W503 max-line-length = 88 max-complexity = 10 docstring-convention = google -per-file-ignores = tests/*:S101,backend/tests/*:S101 +per-file-ignores = + tests/*:S101,B101 + backend/tests/*:S101,B101 rst-roles = class,const,func,meth,mod,ref rst-directives = deprecated exclude = backend/migrations/versions/* diff --git a/backend/migrations/versions/e6a14f4dae3d_initial_migration.py b/backend/migrations/versions/0001_initial_migration.py similarity index 95% rename from backend/migrations/versions/e6a14f4dae3d_initial_migration.py rename to backend/migrations/versions/0001_initial_migration.py index f48ab66e..526d8351 100644 --- a/backend/migrations/versions/e6a14f4dae3d_initial_migration.py +++ b/backend/migrations/versions/0001_initial_migration.py @@ -1,6 +1,6 @@ """Add seed data from seed.sql with separate prepared statements -Revision ID: e6a14f4dae3d +Revision ID: 0001_initial_migration Revises: Create Date: 2025-08-22 23:40:52.885449 """ @@ -13,7 +13,7 @@ from alembic import op # revision identifiers, used by Alembic. -revision: str = "e6a14f4dae3d" +revision: str = "0001" down_revision: Union[str, Sequence[str], None] = None branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None diff --git a/backend/migrations/versions/0002_add_booking_exclusion_constraint.py b/backend/migrations/versions/0002_add_booking_exclusion_constraint.py new file mode 100644 index 00000000..e56635e7 --- /dev/null +++ b/backend/migrations/versions/0002_add_booking_exclusion_constraint.py @@ -0,0 +1,35 @@ +"""Add exclusion constraint to prevent overlapping bookings for the same room + +Revision ID: 0002_add_booking_exclusion_constraint +Revises: 0001_initial_migration +Create Date: 2025-08-26 +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "0002" +down_revision = "0001" +branch_labels = None +depends_on = None + + +def upgrade(): + # Ensure btree_gist extension is enabled + op.execute("CREATE EXTENSION IF NOT EXISTS btree_gist;") + # Add exclusion constraint to prevent overlapping bookings for the same room + op.execute( + """ + ALTER TABLE bookings + ADD CONSTRAINT bookings_no_overlap + EXCLUDE USING gist ( + room_id WITH =, + tsrange(start_time, end_time) WITH && + ); + """ + ) + + +def downgrade(): + op.execute("ALTER TABLE bookings DROP CONSTRAINT IF EXISTS bookings_no_overlap;") diff --git a/backend/migrations/versions/0003_add_invitee_unique_constraint.py b/backend/migrations/versions/0003_add_invitee_unique_constraint.py new file mode 100644 index 00000000..ce47e3f3 --- /dev/null +++ b/backend/migrations/versions/0003_add_invitee_unique_constraint.py @@ -0,0 +1,25 @@ +"""Add unique constraint to prevent duplicate invitees for the same booking + +Revision ID: 0003_add_invitee_unique_constraint +Revises: 0002_add_booking_exclusion_constraint +Create Date: 2025-08-26 +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "0003" +down_revision = "0002" +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_unique_constraint( + "uq_invitees_booking_user", "invitees", ["booking_id", "user_email"] + ) + + +def downgrade(): + op.drop_constraint("uq_invitees_booking_user", "invitees", type_="unique") diff --git a/backend/migrations/versions/0004_add_start_end_check_constraint.py b/backend/migrations/versions/0004_add_start_end_check_constraint.py new file mode 100644 index 00000000..4ea5b4b5 --- /dev/null +++ b/backend/migrations/versions/0004_add_start_end_check_constraint.py @@ -0,0 +1,25 @@ +"""Add start_time < end_time constraint to bookings table + +Revision ID: 0004_add_start_end_check_constraint +Revises: 0003_add_invitee_unique_constraint +Create Date: 2025-08-26 +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "0004" +down_revision = "0003" +branch_labels = None +depends_on = None + + +def upgrade(): + op.create_check_constraint( + "ck_bookings_start_before_end", "bookings", "start_time < end_time" + ) + + +def downgrade(): + op.drop_constraint("ck_bookings_start_before_end", "bookings", type_="check") diff --git a/backend/src/backend/routers/invitees.py b/backend/src/backend/routers/invitees.py index f6f891ea..dac95947 100644 --- a/backend/src/backend/routers/invitees.py +++ b/backend/src/backend/routers/invitees.py @@ -102,6 +102,15 @@ async def add_invitee( status_code=status.HTTP_400_BAD_REQUEST, detail="Database error occurred", ) from e + except ValueError as e: + logger.warning( + f"Duplicate invitee attempted for booking_id {booking_id}" + f" and user {invitee.user_email}: {str(e)}" + ) + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=str(e), + ) from e except Exception as e: logger.error( f"Unexpected error while adding invitee to booking_id {booking_id}: {str(e)}" diff --git a/backend/src/backend/services/bookings.py b/backend/src/backend/services/bookings.py index f59b35d7..b3cd3bb1 100644 --- a/backend/src/backend/services/bookings.py +++ b/backend/src/backend/services/bookings.py @@ -5,18 +5,29 @@ handling queries and logging operations. """ import logging +from datetime import datetime +from datetime import timedelta +from datetime import timezone +from typing import Any +from typing import Awaitable +from typing import Callable +from typing import Sequence from typing import TypedDict from typing import Unpack from typing import cast from sqlalchemy import delete +from sqlalchemy import func from sqlalchemy import select from sqlalchemy import update from sqlalchemy.exc import NoResultFound from sqlalchemy.ext.asyncio import AsyncSession +from backend import config from backend.models import Booking from backend.models import BookingList +from backend.services.invitees import get_invitees_for_booking +from backend.services.rooms import get_room logger = logging.getLogger(__name__) @@ -98,10 +109,73 @@ async def new_booking(session: AsyncSession, booking: Booking) -> Booking: Created booking. Raises: - Exception: If any database error occurs. + ValueError: If any booking constraint is violated. """ logger.debug("Entering new_booking") try: + # Constraint: Room must exist + try: + room = await get_room(session, booking.room_id) + except Exception as e: + logger.warning(f"Attempted to book non-existent room_id {booking.room_id}") + raise ValueError("The selected room does not exist.") from e + + now = datetime.now(timezone.utc) + + # Constraint: No bookings in the past + if booking.start_time < now or booking.end_time < now: + logger.warning("Attempted to create booking in the past.") + raise ValueError("Bookings cannot be made in the past.") + + # Constraint: Start date < end date + if booking.start_time >= booking.end_time: + logger.warning("Attempted to create booking with start_time >= end_time.") + raise ValueError("The booking start time must be before the end time.") + + # Constraint: Max months in future + max_months = config("BOOKING_MAX_MONTHS", default=12, cast=int) + max_future = now + timedelta(days=30 * max_months) + if booking.start_time > max_future or booking.end_time > max_future: + logger.warning("Attempted to create booking too far in the future.") + raise ValueError( + f"Bookings can only be made up to {max_months} months in advance." + ) + + # Check for overlapping bookings for the same room + from sqlalchemy import func + + overlap_stmt = select(Booking).where( + Booking.room_id == booking.room_id, + func.tstzrange(Booking.start_time, Booking.end_time, "[]").op("&&")( + func.tstzrange(booking.start_time, booking.end_time, "[]") + ), + ) + result = await session.scalars(overlap_stmt) + overlapping = result.first() + if overlapping: + logger.warning( + "Attempted to create overlapping booking for room_id %s", + booking.room_id, + ) + raise ValueError( + "Booking times overlap with an existing booking for this room." + ) + + # Constraint: Attendees <= room capacity + invitees = await get_invitees_for_booking( + session, getattr(booking, "id", None) or -1 + ) + num_attendees = len(invitees) if invitees else 0 + if num_attendees > room.capacity: + logger.warning( + f"Attempted to create booking with {num_attendees} attendees" + f" exceeding room capacity {room.capacity}." + ) + raise ValueError( + f"Number of attendees ({num_attendees}) exceeds the room" + f" capacity ({room.capacity})." + ) + session.add(booking) await session.commit() logger.info(f"Successfully created new booking with id: {booking.id}") @@ -114,6 +188,72 @@ async def new_booking(session: AsyncSession, booking: Booking) -> Booking: logger.debug("Exiting new_booking") +async def _validate_room_exists( + session: AsyncSession, + room_id: int, + get_room: Callable[[AsyncSession, int], Awaitable[Any]], +) -> Any: + try: + return await get_room(session, room_id) + except Exception as e: + logger.warning(f"Attempted to update booking to non-existent room_id {room_id}") + raise ValueError("The selected room does not exist.") from e + + +def _validate_time_constraints( + new_start: datetime, + new_end: datetime, + now: datetime, + max_months: int, +) -> None: + if new_start < now or new_end < now: + logger.warning("Attempted to update booking to be in the past.") + raise ValueError("Bookings cannot be made in the past.") + if new_start >= new_end: + logger.warning("Attempted to update booking with start_time >= end_time.") + raise ValueError("The booking start time must be before the end time.") + max_future = now + timedelta(days=30 * max_months) + if new_start > max_future or new_end > max_future: + logger.warning("Attempted to update booking too far in the future.") + raise ValueError( + f"Bookings can only be made up to {max_months} months in advance." + ) + + +async def _validate_no_overlap( + session: AsyncSession, + booking_id: int, + room_id: int, + new_start: datetime, + new_end: datetime, +) -> None: + overlap_stmt = select(Booking).where( + Booking.room_id == room_id, + Booking.id != booking_id, + func.tstzrange(Booking.start_time, Booking.end_time, "[]").op("&&")( + func.tstzrange(new_start, new_end, "[]") + ), + ) + overlapping = (await session.scalars(overlap_stmt)).first() + if overlapping: + logger.warning("Attempted to update booking to overlap for room_id %s", room_id) + raise ValueError( + "Booking times overlap with an existing booking for this room." + ) + + +def _validate_attendee_count(invitees: Sequence[Any], room: Any) -> None: + if len(invitees or []) > room.capacity: + logger.warning( + f"Attempted to update booking with {len(invitees)} attendees" + f" exceeding room capacity {room.capacity}." + ) + raise ValueError( + f"Number of attendees ({len(invitees)}) exceeds the room" + f" capacity ({room.capacity})." + ) + + async def update_booking( session: AsyncSession, booking_id: int, @@ -136,6 +276,19 @@ async def update_booking( f"Entering update_booking with booking_id: {booking_id}, params: {kwargs}" ) try: + current = await get_booking(session, booking_id) + new_room_id = kwargs.get("room_id", current.room_id) + new_start = kwargs.get("start_time", current.start_time) + new_end = kwargs.get("end_time", current.end_time) + + room = await _validate_room_exists(session, new_room_id, get_room) + now = datetime.now(timezone.utc) + max_months = config("BOOKING_MAX_MONTHS", default=12, cast=int) + _validate_time_constraints(new_start, new_end, now, max_months) + await _validate_no_overlap(session, booking_id, new_room_id, new_start, new_end) + invitees = await get_invitees_for_booking(session, booking_id) + _validate_attendee_count(invitees, room) + stmt = update(Booking).where(Booking.id == booking_id).values(**kwargs) result = await session.execute(stmt) if result.rowcount == 0: diff --git a/backend/src/backend/services/invitees.py b/backend/src/backend/services/invitees.py index a04e4986..00d464af 100644 --- a/backend/src/backend/services/invitees.py +++ b/backend/src/backend/services/invitees.py @@ -65,12 +65,44 @@ async def add_invitee_to_booking( The created Invitee object. Raises: - Exception: Any database error encountered during creation is logged and re-raised. + ValueError: If the invitee already exists or if adding the invitee + exceeds room capacity. """ logger.debug( f"Entering add_invitee_to_booking with booking_id: {booking_id}, email: {user_email}" ) try: + # Check for existing invitee for this booking and user + from sqlalchemy import select + + stmt = select(Invitee).where( + Invitee.booking_id == booking_id, Invitee.user_email == user_email + ) + result = await session.scalars(stmt) + existing = result.first() + if existing: + logger.warning( + f"User {user_email} is already invited to booking {booking_id}" + ) + raise ValueError("This user is already invited to this booking.") + + # Enforce attendee count does not exceed room capacity + from backend.services.bookings import get_booking + from backend.services.rooms import get_room + + booking = await get_booking(session, booking_id) + room = await get_room(session, booking.room_id) + invitees = await get_invitees_for_booking(session, booking_id) + if len(invitees) + 1 > room.capacity: + logger.warning( + f"Attempted to add invitee to booking {booking_id} exceeding" + f" room capacity {room.capacity}." + ) + raise ValueError( + f"Cannot add invitee: number of attendees would exceed the" + f" room capacity ({room.capacity})." + ) + invitee = Invitee(booking_id=booking_id, user_email=user_email) session.add(invitee) await session.commit() diff --git a/backend/tests/services/test_bookings.py b/backend/tests/services/test_bookings.py index 776a7df9..4f3eea1f 100644 --- a/backend/tests/services/test_bookings.py +++ b/backend/tests/services/test_bookings.py @@ -1,9 +1,15 @@ """Unit tests for the backend.services.bookings module.""" +import sys +from collections.abc import Awaitable from datetime import datetime +from datetime import timedelta +from datetime import timezone from typing import Any +from typing import Callable from unittest.mock import AsyncMock from unittest.mock import MagicMock +from unittest.mock import patch import pytest from sqlalchemy import delete @@ -234,6 +240,187 @@ async def test_new_booking_success( async_session.commit.assert_called_once() +# --- Constraint tests --- + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "room_exists, start_time, end_time, max_months, expected_error", + [ + # Booking in the past + ( + True, + datetime.now(timezone.utc) - timedelta(days=1), + datetime.now(timezone.utc) + timedelta(hours=1), + 12, + "Bookings cannot be made in the past.", + ), + # Start >= end + ( + True, + datetime.now(timezone.utc) + timedelta(days=1), + datetime.now(timezone.utc) + timedelta(days=1), + 12, + "The booking start time must be before the end time.", + ), + # Too far in future + ( + True, + datetime.now(timezone.utc) + timedelta(days=365 * 2), + datetime.now(timezone.utc) + timedelta(days=365 * 2, hours=1), + 12, + "Bookings can only be made up to 12 months in advance.", + ), + # Non-existent room + ( + False, + datetime.now(timezone.utc) + timedelta(days=1), + datetime.now(timezone.utc) + timedelta(days=2), + 12, + "The selected room does not exist.", + ), + ], + ids=["past", "start>=end", "future", "no_room"], +) +async def test_new_booking_constraints( + async_session: AsyncSession, + mock_logger: MagicMock, + room_exists: bool, + start_time: datetime, + end_time: datetime, + max_months: int, + expected_error: str, +) -> None: + """Test booking constraints: past, start>=end, future, non-existent room. + + Args: + async_session: The asynchronous database session. + mock_logger: The mocked logger instance. + room_exists: Flag indicating if the room exists. + start_time: The proposed start time for the booking. + end_time: The proposed end time for the booking. + max_months: The maximum number of months in advance bookings can be made. + expected_error: The expected error message. + """ + from backend.models import Booking + from backend.services.bookings import new_booking + + booking: Booking = Booking(room_id=1, start_time=start_time, end_time=end_time) + booking.id = 1 + # Patch get_room to simulate room existence + get_room_patch: Callable[[], Awaitable[MagicMock]] = ( + AsyncMock(return_value=MagicMock()) + if room_exists + else AsyncMock(side_effect=Exception("no room")) + ) + with patch("backend.services.bookings.get_room", new=get_room_patch): + # Patch config to set max months + with patch( + "backend.services.bookings.config", wraps=sys.modules["backend"].config + ) as mock_config: + mock_config.side_effect = lambda key: ( # type: ignore + max_months if key == "BOOKING_MAX_MONTHS" else None + ) + with pytest.raises(ValueError) as exc: + await new_booking(async_session, booking) + assert expected_error in str(exc.value) + + +@pytest.mark.asyncio +async def test_new_booking_overlap( + async_session: AsyncSession, mock_logger: MagicMock +) -> None: + """Test overlapping booking constraint. + + Args: + async_session: The asynchronous database session. + mock_logger: The mocked logger instance. + """ + from backend.models import Booking + from backend.services.bookings import new_booking + + # Patch get_room to always exist + with patch( + "backend.services.bookings.get_room", + new=AsyncMock(return_value=MagicMock(capacity=10)), + ): + # Patch config to allow future + with patch( + "backend.services.bookings.config", wraps=sys.modules["backend"].config + ) as mock_config: + mock_config.side_effect = lambda key, **kwargs: ( # type: ignore + 12 + if key == "BOOKING_MAX_MONTHS" + else kwargs.get("default") # type: ignore + ) + # Patch session.scalars to simulate overlap + overlap_booking: Booking = Booking( + room_id=1, + start_time=datetime.now(timezone.utc) + timedelta(days=1), + end_time=datetime.now(timezone.utc) + timedelta(days=2), + ) + overlap_booking.id = 2 + mock_scalars_result = AsyncMock() + mock_scalars_result.first = MagicMock(return_value=overlap_booking) + async_session.scalars = AsyncMock(return_value=mock_scalars_result) + booking: Booking = Booking( + room_id=1, + start_time=datetime.now(timezone.utc) + timedelta(days=1), + end_time=datetime.now(timezone.utc) + timedelta(days=2), + ) + booking.id = 3 + with pytest.raises(ValueError) as exc: + await new_booking(async_session, booking) + assert "overlap" in str(exc.value) + + +@pytest.mark.asyncio +async def test_new_booking_attendees_exceed_capacity( + async_session: AsyncSession, mock_logger: MagicMock +) -> None: + """Test attendee count exceeding room capacity. + + Args: + async_session: The asynchronous database session. + mock_logger: The mocked logger instance. + """ + from backend.models import Booking + from backend.services.bookings import new_booking + + # Patch get_room to have capacity 1 + with patch( + "backend.services.bookings.get_room", + new=AsyncMock(return_value=MagicMock(capacity=1)), + ): + # Patch config to allow future + with patch( + "backend.services.bookings.config", wraps=sys.modules["backend"].config + ) as mock_config: + mock_config.side_effect = lambda key, **kwargs: ( # type: ignore + 12 + if key == "BOOKING_MAX_MONTHS" + else kwargs.get("default") # type: ignore + ) + # Patch session.scalars to simulate no overlap + mock_scalars_result = AsyncMock() + mock_scalars_result.first = MagicMock(return_value=None) + async_session.scalars = AsyncMock(return_value=mock_scalars_result) + # Patch get_invitees_for_booking to return 2 invitees (over capacity) + with patch( + "backend.services.bookings.get_invitees_for_booking", + new=AsyncMock(return_value=[MagicMock(), MagicMock()]), + ): + booking: Booking = Booking( + room_id=1, + start_time=datetime.now(timezone.utc) + timedelta(days=1), + end_time=datetime.now(timezone.utc) + timedelta(days=2), + ) + booking.id = 4 + with pytest.raises(ValueError) as exc: + await new_booking(async_session, booking) + assert "exceeds the room capacity" in str(exc.value) + + @pytest.mark.asyncio @pytest.mark.parametrize("mock_logger", ["backend.services.bookings"], indirect=True) async def test_new_booking_database_error( diff --git a/compose.yml b/compose.yml index c5d45fd2..76dbaa3e 100644 --- a/compose.yml +++ b/compose.yml @@ -11,6 +11,7 @@ services: ENVIRONMENT: production POSTGRES_HOST: postgres LOG_LEVEL: warning + BOOKING_MAX_MONTHS: 12 depends_on: postgres: condition: service_healthy