oliver-oloughlin / kvdex

A high-level abstraction layer for Deno KV with zero third-party dependencies by default 🦕🗝️
https://jsr.io/@olli/kvdex
MIT License
192 stars 6 forks source link

findBySecondaryIndex returns array not ordered by versionstamp #126

Closed juliendorra closed 10 months ago

juliendorra commented 10 months ago

My expectation was that using findBySecondaryIndex would return an array ordered by versionstamp, but it is not the case in my test on local Deno KV (untested on Deploy)

This:

const db = kvdex(kv, {
    apps: collection(GroupsModel, {
        indices: {
            urlid: "secondary",
            timestamp: "secondary",
        },
        serialized: true
    })
});

  const allGroups = await db.apps.findBySecondaryIndex(
      'urlid',
      urlid,
      {
          reverse: true
      });

returns the groups in this order (not the latest group is in the middle):

[[
   {
    id: "fabf1625-9fb4-4d8a-adb6-269b4b3278ab",
    versionstamp: "00000000000002330000",
    value: {
      groups: [],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:17.791Z"
    }
  },
   {
    id: "97416ee8-921b-4b3d-8239-ac786d5321e2",
    versionstamp: "00000000000001ee0000",
    value: {
      groups: [ ],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:08.877Z"
    }
  },
   {
    id: "9449716e-d7a8-4659-9d9d-2ad14c3bac23",
    versionstamp: "00000000000001f70000",
    value: {
      groups: [],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:12.372Z"
    }
  },
   {
    id: "81fdf6e0-a55d-4eab-8001-55ec37d4987b",
    versionstamp: "00000000000002270000",
    value: {
      groups: [ ],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:16.462Z"
    }
  },
   {
    id: "81831af8-4f1e-4699-adfc-31b2a8bd9d77",
    versionstamp: "000000000000024a0000",
    value: {
      groups: [],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:52:14.424Z"
    }
  },
   {
    id: "2fe987e3-512f-4960-ab69-8c0bacc8aeef",
    versionstamp: "000000000000021a0000",
    value: {
      groups: [  ],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:15.293Z"
    }
  },
   {
    id: "2d2bbea6-47e0-481e-b131-e6e80175b902",
    versionstamp: "00000000000002090000",
    value: {
      groups: [ ],
      urlid: "gt3h48wks7dc1m",
      timestamp: "2023-11-26T09:37:14.091Z"
    }
  }
]

Each entry is added with add().

This makes it impossible to use limit: 1, reverse: true consistently as the order is not (as far as I can see) predictable.

Is there something I'm missing?

I will use the workaround to retrieve everything and filter compare the timestamps, but it will be costly when users have created many versions.

oliver-oloughlin commented 10 months ago

This is expected behaviour, and I'll explain why:

Almost all methods that handle multiple documents use kv.list under the hood (except for findMany()). This means the documents you get back when using getMany or findBySecondaryIndex() are ordered exactly like they are stored in KV. Values in KV are not ordered by versionstamp, but by their key. The default id setter for kvdex when using add() is crypto.randomUUID(). So the documents are actually ordered by random at default. If you want to have them ordered by insertion timestamp you can easily set a custom id generator in the collection options. I recommend using ulid, which you can find in the standard library for Deno. I'll provide an example:

import { collection, kvdex, model } from "https://deno.land/x/kvdex/mod.ts"
import { ulid } from "https://deno.land/std@0.208.0/ulid/mod.ts"

const db = kvdex(await Deno.openKv(), {
  strings: collection(model<string>(), {
    idGenerator: () => ulid(),
  }),
})
juliendorra commented 10 months ago

Thanks a lot for the explanation and detailed solution! I now recall seeing ulid() used in examples 😅

It would be good to add this expected random ordering behavior to the readme, as it might surprise others. Edit: mentioning in the Add() paragraph that "The default id setter for kvdex when using add() is crypto.randomUUID()" would also be a very useful addition to the Readme!

oliver-oloughlin commented 10 months ago

Good point, I'll do that.