Bugfix/ogc 3388 non partly reserved slots

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)
 ~~~~~~~~~~~~~~~~~~~
 

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

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,15 +1751,17 @@ def add_blocker(
         *,
         group: UUID,
         reason: str | None = ...,
-        token: UUID | None = ...
+        token: UUID | None = ...,
+        id: int | None = ...
     ) -> list[ReservationBlocker]: ...
 
     def add_blocker(
         self,
         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)

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)

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: