mirror of
https://github.com/simonw/datasette.git
synced 2026-10-18 15:26:57 +02:00
Keep an empty-string keyset pagination cursor, closes #2967
path_from_row_pks() returns "" for a row whose only primary key value is
the empty string, and "" is a legitimate _next= cursor. Both the site
that emits the token and the site that reads it back tested it for
truthiness, so the token was discarded.
Emitting:
"next": next_value and str(next_value) or None
turns "" into None, while next_url is still built a few lines earlier
and stays non-null. docs/json_api.rst:99 says both are null on the final
page, so the response contradicted its own documentation. Reading back,
"if _next:" then ignored the cursor and the query returned page 1
again, so a client following next_url looped on the first row forever.
CREATE TABLE t (id TEXT PRIMARY KEY);
INSERT INTO t (id) VALUES ('a'), (''), ('c'), ('d'), 'e');
/d/t.json?_size=1 rows [''] next=None next_url=...&_next=
following it rows [''] next=None next_url=...&_next=
None is the no-cursor sentinel: it is initialised to None at
table.py:2508 and nothing assigns "" to it deliberately. The same
function already asks this correctly at table.py:2536
("if prefix is None: prefix = '\'").
A bare ?_next= with no value now means "rows after the empty primary
key" rather than being ignored, which is the only coherent reading of
an empty cursor. _next=a and _next=zzz are unchanged.
No docs change: json_api.rst already describes the fixed behaviour.
Test: tests/test_table_api.py::test_paginate_table_with_empty_string_primary_key
walks the pages and fails on cec5e6b2 with
assert '_next=None' in '...?_size=1&_next='
pytest -n auto -m 'not serial' 2782 passed, pytest -m serial 40 passed.
black, ruff, cog --check and codespell clean.
This commit is contained in:
parent
cec5e6b2ef
commit
3bc7e7517a
2 changed files with 46 additions and 3 deletions
|
|
@ -2140,10 +2140,10 @@ async def table_view_data(
|
|||
count_sql = f"select count(*) {from_sql}"
|
||||
|
||||
# Handle pagination driven by ?_next=
|
||||
_next = _next or request.args.get("_next")
|
||||
_next = _next if _next is not None else request.args.get("_next")
|
||||
|
||||
offset = ""
|
||||
if _next:
|
||||
if _next is not None:
|
||||
sort_value = None
|
||||
if is_view:
|
||||
# _next is an offset
|
||||
|
|
@ -2397,7 +2397,7 @@ async def table_view_data(
|
|||
|
||||
data = {
|
||||
"ok": True,
|
||||
"next": next_value and str(next_value) or None,
|
||||
"next": None if next_value is None else str(next_value),
|
||||
"next_url": next_url,
|
||||
}
|
||||
data.update(
|
||||
|
|
|
|||
|
|
@ -366,6 +366,49 @@ async def test_page_size_zero(ds_client):
|
|||
assert None is response.json()["next_url"]
|
||||
|
||||
|
||||
def test_paginate_table_with_empty_string_primary_key():
|
||||
# path_from_row_pks() returns "" for a row whose only primary key
|
||||
# value is the empty string, and "" is a legitimate _next= cursor.
|
||||
# The "next" key and the _next= handling both used a truthiness
|
||||
# check, so the token was dropped and pagination never advanced.
|
||||
with make_app_client(
|
||||
memory=True,
|
||||
extra_databases={
|
||||
"empty_pk": (
|
||||
"CREATE TABLE t (id TEXT PRIMARY KEY);"
|
||||
"INSERT INTO t (id) VALUES ('a'), (''), ('c'), ('d'), ('e');"
|
||||
)
|
||||
},
|
||||
) as client:
|
||||
fetched = []
|
||||
path = "/empty_pk/t.json?_size=1"
|
||||
page = 0
|
||||
while path:
|
||||
page += 1
|
||||
response = client.get(path)
|
||||
assert response.status == 200
|
||||
data = response.json
|
||||
fetched.extend(row["id"] for row in data["rows"])
|
||||
if data["next_url"]:
|
||||
# next_url and next agree, per docs/json_api.rst
|
||||
assert (
|
||||
urllib.parse.urlencode({"_next": data["next"]}) in data["next_url"]
|
||||
)
|
||||
path = data["next_url"]
|
||||
if path:
|
||||
path = path.replace("http://localhost", "")
|
||||
assert page < 30, "Possible infinite loop detected"
|
||||
|
||||
# Every row exactly once
|
||||
assert 5 == len(fetched)
|
||||
assert ["", "a", "c", "d", "e"] == sorted(fetched)
|
||||
assert 5 == page
|
||||
# And the final page has both keys null, as documented
|
||||
last = client.get("/empty_pk/t.json?_size=100").json
|
||||
assert None is last["next"]
|
||||
assert None is last["next_url"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_paginate_compound_keys(ds_client):
|
||||
fetched = []
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue