Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
iuioiua opened this issue Aug 15, 2023 · 6 comments
Closed

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

iuioiua opened this issue Aug 15, 2023 · 6 comments
Assignees
Labels
bug Something isn't working ext/kv Related to "Deno.openKv()" API

Comments

@iuioiua
Copy link
Collaborator

iuioiua commented Aug 15, 2023

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 denoland/saaskit#425

@bartlomieju bartlomieju added bug Something isn't working ext/kv Related to "Deno.openKv()" API labels Aug 15, 2023
@losfair
Copy link
Member

losfair commented Aug 16, 2023

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
Copy link
Collaborator Author

iuioiua commented Aug 16, 2023

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
Copy link
Member

losfair commented Aug 16, 2023

@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
Copy link
Collaborator Author

iuioiua commented Aug 16, 2023

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
Copy link
Member

losfair commented Aug 16, 2023

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
Copy link
Collaborator Author

iuioiua commented Aug 16, 2023

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

@iuioiua iuioiua closed this as completed Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ext/kv Related to "Deno.openKv()" API
Projects
None yet
Development

No branches or pull requests

3 participants