Fix more issues with pagination

Author: matti-lamppu

api/graphql/types/reservation_unit/filtersets.py

@@ -211,8 +211,8 @@ def get_filter_reservable(self, qs: ReservationUnitQuerySet, name: str, value: d
         if not calculate_first_reservable_time:
             return qs
 
-        # Calculate a cache key for pagination
-        # Pagination does not affect the calculated results (note: ordering does!).
+        # Calculate a cache key caching results for pagination.
+        # Pagination arguments do not affect the calculated results (note: ordering does!).
         unaffecting = {"first", "last", "offset", "after", "before"}
         args = sorted(f"{key}={value}" for key, value in self.data.items() if key not in unaffecting)
         cache_key: str = base64.b64encode(",".join(args).encode()).decode()

reservation_units/utils/first_reservable_time_helper/first_reservable_time_helper.py

@@ -211,7 +211,7 @@ def __init__(  # noqa: PLR0915
         ##############
 
         if pagination_args is not None:
-            pagination_args["size"] = graphene_settings.RELAY_CONNECTION_MAX_LIMIT
+            pagination_args["size"] = reservation_unit_queryset.count()
             qs_slice = calculate_queryset_slice(**pagination_args)
             self.start_offset = qs_slice.start
             self.stop_offset = qs_slice.stop
@@ -245,7 +245,7 @@ def __init__(  # noqa: PLR0915
 
         self.first_reservable_times = {}
         self.reservation_unit_closed_statuses = {}
-        self.cached_value_validity = {}
+        self.cached_value_validity: dict[int, datetime.datetime] = {}
 
         # Closed time spans that are shared by all ReservationUnits
         self.shared_hard_closed_time_spans = self._get_shared_hard_closed_time_spans()
@@ -277,20 +277,23 @@ def calculate_all_first_reservable_times(self) -> None:
             # If we already have cached FRT results for enough reservation units to fill the page
             # AND all the previous pages, then we don't need to calculate anything.
             # NOTE: This does not support different orderings of the same queryset!
-            required = (
+            # TODO: This doesn't work for the last page if it's not full!
+            cached = (
                 sum(1 for frt in self.first_reservable_times.values() if frt is not None)
                 if self.show_only_reservable
                 else len(self.first_reservable_times)
             )
-            if required >= self.stop_offset:
+            if cached >= self.stop_offset:
                 return
 
-            # Otherwise, we can still skip looping through the previous pages.
-            qs = qs[self.start_offset :]
-            self.start_offset = 0  # Remove offset since we skip the previous pages
+            # Otherwise, we should still have valid results for the previous pages.
+            # We can start calculating after the last cached result.
+            qs = qs[len(self.first_reservable_times) :]
+            # We also don't need to skip any results that are not already cached.
+            self.start_offset = 0
 
         # If we don't have valid results, and this is not the first page,
-        # we should fetch the current and previous pages in on chunk. The next chunk
+        # we should fetch the current and previous pages in one chunk. The next chunk
         # will also be bigger (if needed), but likely small enough (<100) not to cause any problems.
         elif self.start_offset > 0:
             self.chunk_size = self.stop_offset
@@ -303,15 +306,14 @@ def calculate_all_first_reservable_times(self) -> None:
             self.reservation_unit_closed_statuses[reservation_unit.pk] = is_closed
             self.first_reservable_times[reservation_unit.pk] = first_reservable_time
 
-            # Start offset should exist only if we need to recalculate previous pages.
-            # -> Don't count the result for the current page.
-            if self.start_offset > 0:
-                self.start_offset -= 1
-                continue
-
             # If we should only show reservable reservation units, then we should count the result for the
             # current page only if there is a first reservable time.
             if not self.show_only_reservable or first_reservable_time is not None:
+                # Start offset should exist only if we need to recalculate previous pages.
+                # -> Don't count the result for the current page.
+                if self.start_offset > 0:
+                    self.start_offset -= 1
+                    continue
                 results += 1
 
             # This only really matters when showing only reservable reservation units,
@@ -327,9 +329,9 @@ def _read_cached_results(self) -> bool:
 
         Check that we have valid results for all the previous pages.
         If not, we must recalculate results for all previous pages, since they might be different
-        if one FRT has changed to None or from None to a valid value.
+        if one FRT has changed from a datetime to None or vice versa.
         """
-        cached_data: dict[ReservationUnitPK, dict[str, Any]] = json.loads(cache.get(self.cache_key, "{}"))
+        cached_data: dict[str, dict[str, Any]] = json.loads(cache.get(self.cache_key, "{}"))
 
         has_valid_results_for_previous_pages = bool(cached_data)
         for pk, item in cached_data.items():
@@ -339,21 +341,22 @@ def _read_cached_results(self) -> bool:
                 self.first_reservable_times.clear()
                 return False
 
-            self.reservation_unit_closed_statuses[pk] = cached_result.closed
-            self.first_reservable_times[pk] = cached_result.frt
-            self.cached_value_validity[pk] = cached_result.valid_until
+            self.reservation_unit_closed_statuses[int(pk)] = cached_result.closed
+            self.first_reservable_times[int(pk)] = cached_result.frt
+            self.cached_value_validity[int(pk)] = cached_result.valid_until
 
         return has_valid_results_for_previous_pages
 
     def _cache_results(self) -> None:
-        """Save the calculated results to the cache."""
-        cached_data: dict[ReservationUnitPK, dict[str, Any]] = {}
+        """Save the calculated FRT results to the cache."""
+        cached_data: dict[str, dict[str, Any]] = {}
         new_valid_until = self.now + datetime.timedelta(minutes=2)
 
         for pk, frt in self.first_reservable_times.items():
             is_closed = self.reservation_unit_closed_statuses[pk]
+            # If FRT was not calculated in this request, use the previous 'valid_until' value.
             valid_until = self.cached_value_validity.get(pk, new_valid_until)
-            cached_data[pk] = CachedReservableTime(frt=frt, closed=is_closed, valid_until=valid_until).to_dict()
+            cached_data[str(pk)] = CachedReservableTime(frt=frt, closed=is_closed, valid_until=valid_until).to_dict()
 
         cache.set(self.cache_key, json.dumps(cached_data), timeout=120)
 

tests/test_graphql_api/test_reservation_unit/test_query_first_reservable_time.py

@@ -74,7 +74,7 @@ def dt(*, year: int = NEXT_YEAR, month: int = 1, day: int = 1, hour: int = 0, mi
 
 @pytest.fixture(autouse=True)
 def _clear_cache() -> None:
-    # Cache needs to be cleared between tests do that
+    # Cache needs to be cleared between tests so that
     # FRT calculations don't reuse results between runs.
     try:
         cache.clear()
@@ -2513,14 +2513,15 @@ def test__reservation_unit__first_reservable_time__remove_not_reservable(graphql
     # Check that we fetch reservation units in a big chunk which is then filtered
     # instead of fetching chunks of the expected page size.
     # Queries:
-    #  1) Fetch reservation units for FRT calculation
-    #  2) Fetch hauki resources for FRT calculation
-    #  3) Fetch reservable time spans for FRT calculation
-    #  4) Fetch application rounds for FRT calculation
-    #  5) Fetch affecting time spans for FRT calculation
-    #  6) Count reservation units for response
-    #  7) Fetch reservation units for response
-    response.assert_query_count(7)
+    #  1) Count reservation units for FRT calculation
+    #  2) Fetch reservation units for FRT calculation
+    #  3) Fetch hauki resources for FRT calculation
+    #  4) Fetch reservable time spans for FRT calculation
+    #  5) Fetch application rounds for FRT calculation
+    #  6) Fetch affecting time spans for FRT calculation
+    #  7) Count reservation units for response
+    #  8) Fetch reservation units for response
+    response.assert_query_count(8)
 
 
 ########################################################################################################################
@@ -2580,9 +2581,9 @@ def test__reservation_unit__first_reservable_time__previous_page_cached(graphql,
     assert cached_value[str(reservation_unit.pk)]["closed"] == "False"
 
     # Check that there was no additional queries
-    response_2.assert_query_count(7)
+    response_2.assert_query_count(8)
     # ...and that we were able to skip iterating through the previous pages due to cached results.
-    assert "OFFSET 1" in response_2.queries[0]
+    assert "OFFSET 1" in response_2.queries[1]
 
 
 ########################################################################################################################
@@ -2631,9 +2632,9 @@ def test__reservation_unit__first_reservable_time__previous_page_not_cached(grap
     assert cached_value[str(reservation_unit.pk)]["closed"] == "False"
 
     # Check that there was no additional queries
-    response.assert_query_count(7)
+    response.assert_query_count(8)
     # We also cannot skip previous pages due to missing cached results.
-    assert "OFFSET 1" not in response.queries[0]
+    assert "OFFSET 1" not in response.queries[1]
 
 
 ########################################################################################################################
@@ -2693,7 +2694,7 @@ def test__reservation_unit__first_reservable_time__different_filters_dont_share_
     assert len(cached_value) == 1
 
     # We couldn't use the cached results, so make database queries as usual.
-    response_2.assert_query_count(7)
+    response_2.assert_query_count(8)
 
 
 ########################################################################################################################
@@ -2743,9 +2744,10 @@ def test__reservation_unit__first_reservable_time__use_cached_results(graphql, r
 
     # Since we used cached results, we didn't need to make database queries.
     # Only make queries for:
-    #  1) Count reservation units for response
-    #  2) Fetch reservation units for response
-    response_2.assert_query_count(2)
+    #  1) Count reservation units for FRT calculation
+    #  2) Count reservation units for response
+    #  3) Fetch reservation units for response
+    response_2.assert_query_count(3)
 
 
 ########################################################################################################################
@@ -2800,9 +2802,10 @@ def test__reservation_unit__first_reservable_time__use_cached_results__not_first
 
     # Since we used cached results, we didn't need to make database queries.
     # Only make queries for:
-    #  1) Count reservation units for response
-    #  2) Fetch reservation units for response
-    response_3.assert_query_count(2)
+    #  1) Count reservation units for FRT calculation
+    #  2) Count reservation units for response
+    #  3) Fetch reservation units for response
+    response_3.assert_query_count(3)
 
 
 ########################################################################################################################
@@ -2860,4 +2863,4 @@ def test__reservation_unit__first_reservable_time__cached_results_not_valid_anym
 
     # Since we couldn't use all the cached results,
     # we needed to fetch data from the database for re-calculation.
-    response.assert_query_count(7)
+    response.assert_query_count(8)