fix(pagination): clamp page_from to ES max_result_window to prevent 500
issue #746: requesting a page beyond the 10000-hit boundary returned a 500 to the user. for example, page 67 of a 9981-video channel with page_size=150 produced from=9900 + size=150 = 10050, which ES rejects with 'Result window is too large' (HTTP 400 from ES, surfaces as 500). first_guess() now clamps page_from to MAX_RESULT_WINDOW - page_size when the request would overflow, and sets max_hits=True so the UI can flag the cap. validate() uses the same constant for consistency. adds unit tests covering the bug report scenario, exact-boundary case, and a different page_size to guard the behavior. Signed-off-by: Ian Schwartz <168640+studiozeroseven@users.noreply.github.com>
This commit is contained in:
parent
5477ca97f1
commit
64e1d61a81
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
Loading…
Reference in New Issue