diff --git a/HISTORY.rst b/HISTORY.rst index 1548234..1047b6f 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -1,6 +1,21 @@ Changelog --------- +1.1.3 (unreleased) +~~~~~~~~~~~~~~~~~~~ + +- Records the owning reservation/blocker id on each reserved slot + + ReservedSlot gains a ``source_id`` column holding the id of its owning + reservation or blocker (see ``source_type``). A slot can now be attributed to + its exact object directly, instead of inferring it from the allocation and + time range. This fixes ``reserved_slots_by_reservation`` dropping the slot on + non-partly_available allocations (where the slot spans the whole allocation + and is wider than a narrower reservation), which left orphaned slots behind on + removal. Consumers must add the column and backfill it (see the onegov + ``add_source_id_to_reserved_slots`` upgrade). + [Tschuppi81] + 1.1.2 (16.06.2026) ~~~~~~~~~~~~~~~~~~~ diff --git a/src/libres/db/models/reserved_slot.py b/src/libres/db/models/reserved_slot.py index 63351d1..26a80e7 100644 --- a/src/libres/db/models/reserved_slot.py +++ b/src/libres/db/models/reserved_slot.py @@ -68,6 +68,9 @@ class ReservedSlot(TimestampMixin, ORMBase): reservation_token: Mapped[UUID] + # id of the owning Reservation or ReservationBlocker (see source_type) + source_id: Mapped[int] = mapped_column(index=True) + __table_args__ = ( Index('reservation_resource_ix', 'reservation_token', 'resource'), # NOTE: GiST index for temporal queries on reserved slots diff --git a/src/libres/db/scheduler.py b/src/libres/db/scheduler.py index c569e21..3f2c043 100644 --- a/src/libres/db/scheduler.py +++ b/src/libres/db/scheduler.py @@ -9,7 +9,7 @@ from sqlalchemy import exc from sqlalchemy import func from sqlalchemy.orm import selectinload -from sqlalchemy.sql import and_, not_, or_ +from sqlalchemy.sql import and_, not_ from uuid import uuid4 as new_uuid, UUID from libres.context.core import ContextServicesMixin @@ -1491,6 +1491,7 @@ def _approve_reservation_record( slot.resource = allocation.resource slot.reservation_token = reservation.token slot.source_type = 'reservation' + slot.source_id = reservation.id # the slots are written with the allocation allocation.reserved_slots.append(slot) @@ -1729,7 +1730,8 @@ def add_blocker( dates: _dtrange | Collection[_dtrange], group: None = ..., reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... @overload @@ -1738,7 +1740,8 @@ def add_blocker( dates: None, group: UUID, reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... @overload @@ -1748,7 +1751,8 @@ def add_blocker( *, group: UUID, reason: str | None = ..., - token: UUID | None = ... + token: UUID | None = ..., + id: int | None = ... ) -> list[ReservationBlocker]: ... def add_blocker( @@ -1756,7 +1760,8 @@ def add_blocker( dates: _dtrange | Collection[_dtrange] | None = None, group: UUID | None = None, reason: str | None = None, - token: UUID | None = None + token: UUID | None = None, + id: int | None = None ) -> list[ReservationBlocker]: """ Adds a blocker to one or many allocations. @@ -1827,7 +1832,6 @@ def add_blocker( elif not allocation.contains(start, end): raise errors.TimerangeTooLong - # ok, we're good to go if token is None: token = new_uuid() reserved_slots = [] @@ -1836,6 +1840,7 @@ def create_reserved_slots( allocation: Allocation, start: datetime, end: datetime, + source_id: int, including_mirrors: bool = True ) -> None: for slot_start, slot_end in allocation.all_slots(start, end): @@ -1845,6 +1850,7 @@ def create_reserved_slots( slot.resource = allocation.resource slot.reservation_token = token slot.source_type = 'blocker' + slot.source_id = source_id # the slots are written with the allocation allocation.reserved_slots.append(slot) @@ -1859,7 +1865,7 @@ def create_reserved_slots( for mirror in self.allocation_mirrors_by_master(allocation): create_reserved_slots( - mirror, start, end, + mirror, start, end, source_id, including_mirrors=False ) @@ -1877,12 +1883,18 @@ def new_blockers_by_group( blocker.target_type = 'group' blocker.resource = self.resource blocker.reason = reason + if id is not None: + blocker.id = id + # flush to assign the blocker id before its slots reference it + self.session.add(blocker) + self.session.flush() for allocation in self.allocations_by_group(group): create_reserved_slots( allocation, allocation._start, - allocation._end + allocation._end, + blocker.id ) yield blocker @@ -1919,8 +1931,15 @@ def new_blockers_by_dates( blocker.target_type = 'allocation' blocker.resource = self.resource blocker.reason = reason - - create_reserved_slots(allocation, start, end) + if id is not None: + blocker.id = id + + # flush to assign the id before its slots reference it + self.session.add(blocker) + self.session.flush() + create_reserved_slots( + allocation, start, end, blocker.id + ) yield blocker @@ -1936,9 +1955,6 @@ def new_blockers_by_dates( if not reserved_slots: raise errors.NotReservableError - for blocker in blockers: - self.session.add(blocker) - return blockers def remove_blocker( @@ -2018,9 +2034,9 @@ def change_blocker( new_blocker, = self.add_blocker( dates=(new_start, new_end), reason=old_reason, - token=token + token=token, + id=id ) - new_blocker.id = id return new_blocker @@ -2297,24 +2313,7 @@ def reserved_slots_by_reservation( if id is None: return query - # allocation_id is ambiguous when multiple reservations share a token - # on a partly_available allocation; filter by time range instead. - # start is None for group reservations — the or_ includes all their - # slots. - return ( - query - .join(Reservation, and_( - Reservation.token == ReservedSlot.reservation_token, - Reservation.id == id - )) - .filter(or_( - Reservation.start.is_(None), - and_( - ReservedSlot.start >= Reservation.start, - ReservedSlot.end <= Reservation.end, - ) - )) - ) + return query.filter(ReservedSlot.source_id == id) def reserved_slots_by_blocker( self, @@ -2331,21 +2330,7 @@ def reserved_slots_by_blocker( if id is None: return query - # Same rationale as reserved_slots_by_reservation. - return ( - query - .join(ReservationBlocker, and_( - ReservationBlocker.token == ReservedSlot.reservation_token, - ReservationBlocker.id == id - )) - .filter(or_( - ReservationBlocker.start.is_(None), - and_( - ReservedSlot.start >= ReservationBlocker.start, - ReservedSlot.end <= ReservationBlocker.end, - ) - )) - ) + return query.filter(ReservedSlot.source_id == id) def reservations_by_group(self, group: UUID) -> Query[Reservation]: tokens = self.managed_reservations().with_entities(Reservation.token) diff --git a/tests/test_allocation.py b/tests/test_allocation.py index 68fabff..a1e8c39 100644 --- a/tests/test_allocation.py +++ b/tests/test_allocation.py @@ -255,6 +255,7 @@ def add_reservation( slot.allocation = allocation slot.reservation_token = reservation slot.source_type = 'reservation' + slot.source_id = 1 # synthetic slot, no real owner scheduler.session.add(slot) scheduler.session.flush() scheduler.session.refresh(allocation) @@ -281,6 +282,7 @@ def add_blocker( slot.allocation = allocation slot.reservation_token = blocker slot.source_type = 'blocker' + slot.source_id = 1 # synthetic slot, no real owner scheduler.session.add(slot) scheduler.session.flush() scheduler.session.refresh(allocation) diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index 966a413..bef3e32 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -691,6 +691,77 @@ def test_remove_reservation_does_not_affect_sibling_reservations( assert remaining_count == slots_a_count +def test_remove_reservation_on_non_partly_allocation_removes_slot( + scheduler: Scheduler, +) -> None: + """On a non-partly allocation the reserved slot spans the whole + allocation, even if the reservation was made for a narrower (but + contained) range. Matching slots by time range would then drop the slot, + leaving an orphaned ReservedSlot behind that keeps showing up on the + calendar after the reservation was removed (OGC-3388).""" + dates = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0)) + scheduler.allocate(dates, partly_available=False) + + # a contained sub-range is accepted on a whole-only allocation, but the + # slot still covers the entire allocation + sub = (datetime(2014, 3, 7, 9, 0), datetime(2014, 3, 7, 17, 0)) + token = scheduler.reserve('user@example.org', sub) + scheduler.commit() + scheduler.approve_reservations(token) + scheduler.commit() + + reservation = scheduler.reservations_by_token(token).one() + assert reservation.start is not None + assert reservation.end is not None + slot = scheduler.reserved_slots_by_reservation(token).one() + # the slot is wider than the reservation range + assert slot.start < reservation.start or slot.end > reservation.end + + # the slot must still be attributed to this reservation ... + assert scheduler.reserved_slots_by_reservation( + token, reservation.id + ).count() == 1 + + # ... and removing the reservation must not leave an orphaned slot + scheduler.remove_reservation(token, reservation.id) + scheduler.commit() + assert scheduler.reserved_slots_by_type(token, 'reservation').count() == 0 + + +def test_reserved_slots_store_source_id(scheduler: Scheduler) -> None: + """Every reserved slot records the id of its owning reservation/blocker + (source_id), so it can be attributed to its exact object directly.""" + dates = (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 18, 0)) + scheduler.allocate(dates, partly_available=True) + + token = scheduler.reserve( + 'user@example.org', + (datetime(2014, 3, 7, 8, 0), datetime(2014, 3, 7, 10, 0)) + ) + scheduler.commit() + scheduler.approve_reservations(token) + scheduler.commit() + + reservation = scheduler.reservations_by_token(token).one() + slots = scheduler.reserved_slots_by_reservation(token).all() + assert slots + for slot in slots: + assert slot.source_type == 'reservation' + assert slot.source_id == reservation.id + + blocker = scheduler.add_blocker( + (datetime(2014, 3, 7, 10, 0), datetime(2014, 3, 7, 12, 0)) + )[0] + assert blocker.id is not None + scheduler.commit() + + blocker_slots = scheduler.reserved_slots_by_blocker(blocker.token).all() + assert blocker_slots + for slot in blocker_slots: + assert slot.source_type == 'blocker' + assert slot.source_id == blocker.id + + def test_remove_blocker_does_not_affect_sibling_blockers( scheduler: Scheduler, ) -> None: