denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
94.69k stars 5.26k forks source link

bug(kv): `iter.cursor` returns non-empty string when `limit` is defined #20173

Closed iuioiua closed 1 year ago

iuioiua commented 1 year ago

The following:

const kv = await Deno.openKv(":memory:");

const limit = 5;

for (let i = 0; i < limit; i++) {
  await kv.set(["posts", crypto.randomUUID()], undefined);
}

const iter = kv.list({ prefix: ["posts"] });
for await (const _entry of iter) console.log(iter.cursor);
console.log(iter.cursor);

Produces (notice the empty last line):

AjU5NzVkZjQ2LTM4ZjAtNDYwNC04Yjk0LWU5OGZjY2U4ZTg3YQA=
AjVlYjI0ZWI0LTk3ODgtNDMxYy05MjNmLTZjMGIyZGJkMjE3OAA=
Ajc4ZTI0OTU2LWU5NDAtNDIxYy05MGQ5LTM4MDI4YTM4N2NjOQA=
AjkxZGVhMmY0LTk5NTMtNGRlZi05OTViLTRhYmVlNGNlOTRiMgA=
AmFiNWQyZDdmLWQ1MGQtNDI2ZC05MDA5LTIyYjgxZWI3NzQwNwA=

However, when you set const iter = kv.list({ prefix: ["posts"] }, { limit });, the following output is produced (notice the non-empty last line):

AjVhMWM5OWFkLTBkNDYtNGQ3OS05ZmQ5LTNlOTA2ZWQ2YjE5ZAA=
AjkzNTE3MWViLWUzMmYtNDI3ZS05M2ZkLThhOWU2ZDA0NDFjOQA=
AmE5NzYyNjU1LTM2NGUtNGNiMC1hMzVjLTFjODczYzMxNWRkMwA=
AmI4MmFiY2MxLWJhNzItNDQxNi04NTdkLTJkMjkwN2E5MDdiZgA=
AmU2ZjE5MTAwLWM1OTctNDM2Yi04OGVmLWFhYzg1ZTIyOGI1NAA=
AmU2ZjE5MTAwLWM1OTctNDM2Yi04OGVmLWFhYzg1ZTIyOGI1NAA=

The consequence of using the last line as the cursor property for kv.list() is that the next value is empty. In other words, the next page is blank. Instead, the last line should be blank. I hope my understanding is correct.

Related https://github.com/denoland/saaskit/pull/425

losfair commented 1 year ago

What happens if you do const iter = kv.list({ prefix: ["posts"] }, { limit: limit + 1 });?

Returning the empty cursor requires looking ahead one entry past the limit, which is not done by default due to the overhead.

iuioiua commented 1 year ago

That just increases the limit by 1, which doesn't fix the issue. If my previous setup had 10 posts per page, the setup now has 11. So when there are 11 posts, my setup will still point towards an empty second page. PTAL at my workaround in https://github.com/denoland/saaskit/pull/425/files#diff-fda30a2409d24fac1c4c1b607c96124b0329fe42338d678ec91f40c6a64efd57

losfair commented 1 year ago

@iuioiua When you use limit + 1, the 11th entry should always be dropped. If the result size is less than 11, it's then known that this is the last page.

iuioiua commented 1 year ago

Ok, I see. Is this the recommended method for pagination? The behaviour still surprises me, and this is the first time I've seen this method being documented. I may not fully understand the performance implications.

losfair commented 1 year ago

Is this the recommended method for pagination?

Yes, this is the only way to handle the corner case where total_records % records_per_page == 0 without letting the client see an empty last page. The performance implication is that one more entry needs to be read for every page - the overhead is usually negligible but we'd like to make it explicit.

iuioiua commented 1 year ago

Closing as this is expected behaviour. Thanks, @losfair!