diff --git a/backend/common/src/index_generic.py b/backend/common/src/index_generic.py index 25c098b9..dbb000dd 100644 --- a/backend/common/src/index_generic.py +++ b/backend/common/src/index_generic.py @@ -89,6 +89,11 @@ class Pagination: figure out the pagination based on page size and total_hits """ + # elasticsearch's default index.max_result_window; ES rejects any + # ``from + size`` request that exceeds this with a 400 (becomes a + # 500 to the user). see issue #746. + MAX_RESULT_WINDOW = 10000 + def __init__(self, request): self.request = request self.page_get = False @@ -121,12 +126,21 @@ class Pagination: i for i in range(page_get - 1, page_get - 6, -1) if i > 1 ] prev_pages.reverse() + # clamp page_from so the ES request stays within max_result_window. + # without this, large channel pages that cross the 10000 hit + # boundary (e.g. page 67 of a 9981-video channel with page_size=150) + # return 500. see issue #746. + safe_size = self.page_size if isinstance(self.page_size, int) else 0 + max_hits = False + if page_from + safe_size > self.MAX_RESULT_WINDOW: + page_from = self.MAX_RESULT_WINDOW - safe_size + max_hits = True pagination = { "page_size": self.page_size, "page_from": page_from, "prev_pages": prev_pages, "current_page": page_get, - "max_hits": False, + "max_hits": max_hits, "params": self.params, } @@ -136,7 +150,7 @@ class Pagination: """validate pagination with total_hits after making api call""" page_get = self.page_get max_pages = math.ceil(total_hits / self.page_size) - if total_hits >= 10000: + if total_hits >= self.MAX_RESULT_WINDOW: # es returns maximal 10000 results self.pagination["max_hits"] = True max_pages = max_pages - 1 diff --git a/backend/common/tests/test_src/test_pagination.py b/backend/common/tests/test_src/test_pagination.py new file mode 100644 index 00000000..b6214302 --- /dev/null +++ b/backend/common/tests/test_src/test_pagination.py @@ -0,0 +1,103 @@ +"""tests for Pagination class to guard against ES max_result_window overflow. + +regression test for issue #746: requesting a page that would push +``from + size`` past the 10000 hit boundary returned a 500 to the user. +the fix clamps ``page_from`` so the ES query fits in one request and +sets ``max_hits`` so the UI can flag the cap. +""" + +from unittest.mock import patch + +from common.src.index_generic import Pagination + + +class _StubRequest: + """minimal request stand-in; only ``request.GET`` and ``request.user.id`` + are used by ``Pagination.__init__``.""" + + def __init__(self, page): + self._page = page + self._user = type("_User", (), {"id": 1})() + + @property + def user(self): + return self._user + + class _GET: + def __init__(self, page): + self._page = str(page) + + def copy(self): + return _StubRequest._GET(self._page) + + def get(self, key, default=None): + if key == "page": + return self._page + return default + + def pop(self, key, default=None): + return default + + def urlencode(self): + return "" + + GET = property(lambda self: self._GET(self._page)) + + +def _make_pagination(page, page_size): + """build a Pagination without touching UserConfig / ES.""" + request = _StubRequest(page) + with patch( + "common.src.index_generic.UserConfig", + return_value=type( + "_UC", (), {"get_value": staticmethod(lambda *a, **k: page_size)} + )(), + ): + return Pagination(request) + + +def test_first_page_within_window_is_unchanged(): + """page 1: from=0, no clamp, max_hits=False.""" + p = _make_pagination(page=1, page_size=150) + assert p.pagination["page_from"] == 0 + assert p.pagination["page_size"] == 150 + assert p.pagination["max_hits"] is False + + +def test_middle_page_within_window_is_unchanged(): + """a normal mid-page is unaffected by the clamp.""" + p = _make_pagination(page=10, page_size=150) + assert p.pagination["page_from"] == 9 * 150 + assert p.pagination["max_hits"] is False + + +def test_overflow_page_is_clamped(): + """the bug: page 67 of a 9981-video channel with page_size=150. + raw from = 66 * 150 = 9900, + 150 = 10050 > 10000. + expected: from is clamped to 10000 - 150 = 9850, max_hits=True. + """ + p = _make_pagination(page=67, page_size=150) + assert p.pagination["page_from"] == 9850 + assert p.pagination["page_size"] == 150 + assert p.pagination["max_hits"] is True + + +def test_overflow_page_at_exact_boundary_is_unchanged(): + """the last page that just fits stays unchanged (no clamp).""" + # 10000 / 150 = 66.66..., so page 66 → from=9750, +150=9900, fits. + p = _make_pagination(page=66, page_size=150) + assert p.pagination["page_from"] == 9750 + assert p.pagination["max_hits"] is False + + +def test_overflow_with_different_page_size(): + """bug surfaces with any page_size; sanity-check a different value.""" + # 10000 / 50 = 200 exactly, so page 200 is fine, page 201 overflows + p = _make_pagination(page=201, page_size=50) + assert p.pagination["page_from"] == 10000 - 50 + assert p.pagination["max_hits"] is True + + +def test_max_result_window_is_documented_constant(): + """the constant should be exposed and equal to the ES default.""" + assert Pagination.MAX_RESULT_WINDOW == 10000