From 23be24ca4a4a0a3e3132553f573472bd4dfcbda1 Mon Sep 17 00:00:00 2001 From: Asher Gomez Date: Fri, 8 Sep 2023 11:36:45 +1000 Subject: [PATCH] refactor: use ULIDs for items (#514) Please pay attention to the migration script, as it's responsible for migrating all items and votes. I've also changed all instances of `monotonicUlid()` to `ulid()` at there appears to be a bug that prevents `monotonicUlid()` from respecting the `seedTime` parameter. The migration can be simulated as follows: 1. Switch to **main** branch. 2. Run `deno run db:reset`. 3. Run `deno run db:seed`. 4. Run `deno task start` and navigate to `http://localhost:8000`. 6. Vote some items. 7. Note the voted items and the order of the items feed. 8. Switch to **ulid-items** branch. 9. Run `deno run db:migrate`. 10. Run `deno task start` and navigate to `http://localhost:8000`. 11. Check that the voted items and the order of the items feed are identical to those previously noted. Closes #475 Closes #476 --- components/ItemSummary.tsx | 3 +- routes/api/comments.ts | 6 +-- routes/api/items/[id]/vote.ts | 4 +- routes/api/items/index.ts | 10 ++-- tasks/db_migrate.ts | 66 +++++++++++++++++------- tasks/db_seed.ts | 10 ++-- utils/db.ts | 65 ++++++++++++------------ utils/db_test.ts | 96 ++++++++++++++++------------------- 8 files changed, 140 insertions(+), 120 deletions(-) diff --git a/components/ItemSummary.tsx b/components/ItemSummary.tsx index 8131abba952c..692d7579cacd 100644 --- a/components/ItemSummary.tsx +++ b/components/ItemSummary.tsx @@ -2,6 +2,7 @@ import VoteButton from "@/islands/VoteButton.tsx"; import type { Item } from "@/utils/db.ts"; import UserPostedAt from "./UserPostedAt.tsx"; +import { decodeTime } from "std/ulid/mod.ts"; export interface ItemSummaryProps { item: Item; @@ -33,7 +34,7 @@ export default function ItemSummary(props: ItemSummaryProps) {

diff --git a/routes/api/comments.ts b/routes/api/comments.ts index 7881c323bc01..672785e1a111 100644 --- a/routes/api/comments.ts +++ b/routes/api/comments.ts @@ -4,7 +4,7 @@ import { errors } from "std/http/http_errors.ts"; import { createComment, createNotification, getItem } from "@/utils/db.ts"; import { redirect } from "@/utils/http.ts"; import { assertSignedIn, State } from "@/middleware/session.ts"; -import { monotonicUlid } from "std/ulid/mod.ts"; +import { ulid } from "std/ulid/mod.ts"; export const handler: Handlers = { async POST(req, ctx) { @@ -25,7 +25,7 @@ export const handler: Handlers = { const { sessionUser } = ctx.state; await createComment({ - id: monotonicUlid(), + id: ulid(), userLogin: sessionUser.login, itemId: itemId, text, @@ -33,7 +33,7 @@ export const handler: Handlers = { if (item.userLogin !== sessionUser.login) { await createNotification({ - id: monotonicUlid(), + id: ulid(), userLogin: item.userLogin, type: "comment", text: `${sessionUser.login} commented on your post: ${item.title}`, diff --git a/routes/api/items/[id]/vote.ts b/routes/api/items/[id]/vote.ts index fcf097f2e377..5e5af4e2f375 100644 --- a/routes/api/items/[id]/vote.ts +++ b/routes/api/items/[id]/vote.ts @@ -8,7 +8,7 @@ import { getItem, newVoteProps, } from "@/utils/db.ts"; -import { monotonicUlid } from "std/ulid/mod.ts"; +import { ulid } from "std/ulid/mod.ts"; import { errors } from "std/http/http_errors.ts"; export const handler: Handlers = { @@ -28,7 +28,7 @@ export const handler: Handlers = { if (item.userLogin !== sessionUser.login) { await createNotification({ - id: monotonicUlid(), + id: ulid(), userLogin: item.userLogin, type: "vote", text: `${sessionUser.login} upvoted your post: ${item.title}`, diff --git a/routes/api/items/index.ts b/routes/api/items/index.ts index f02afab78376..cfdd66aad2cd 100644 --- a/routes/api/items/index.ts +++ b/routes/api/items/index.ts @@ -1,16 +1,17 @@ -import { collectValues, listItemsByTime } from "@/utils/db.ts"; +import { collectValues, listItems } from "@/utils/db.ts"; import { getCursor } from "@/utils/http.ts"; import { type Handlers } from "$fresh/server.ts"; -import { createItem, type Item, newItemProps } from "@/utils/db.ts"; +import { createItem, type Item } from "@/utils/db.ts"; import { redirect } from "@/utils/http.ts"; import { assertSignedIn, State } from "@/middleware/session.ts"; import { errors } from "std/http/http_errors.ts"; +import { ulid } from "std/ulid/mod.ts"; // Copyright 2023 the Deno authors. All rights reserved. MIT license. export const handler: Handlers = { async GET(req) { const url = new URL(req.url); - const iter = listItemsByTime({ + const iter = listItems({ cursor: getCursor(url), limit: 10, reverse: true, @@ -33,10 +34,11 @@ export const handler: Handlers = { } const item: Item = { + id: ulid(), userLogin: ctx.state.sessionUser.login, title, url, - ...newItemProps(), + score: 0, }; await createItem(item); return redirect("/items/" + item.id); diff --git a/tasks/db_migrate.ts b/tasks/db_migrate.ts index e02c078ca22e..bbcd21f47c30 100644 --- a/tasks/db_migrate.ts +++ b/tasks/db_migrate.ts @@ -12,32 +12,60 @@ * deno task db:migrate * ``` */ -import { type Comment, createComment, kv } from "@/utils/db.ts"; -import { monotonicUlid } from "std/ulid/mod.ts"; +import { + createItem, + createVote, + deleteVote, + type Item, + kv, + User, +} from "@/utils/db.ts"; +import { ulid } from "std/ulid/mod.ts"; -interface OldComment extends Comment { +interface OldItem extends Item { createdAt: Date; } if (!confirm("WARNING: The database will be migrated. Continue?")) Deno.exit(); -const promises = []; - -const iter = kv.list({ prefix: ["comments_by_item"] }); -for await (const { key, value } of iter) { - if (!value.createdAt) continue; - promises.push(kv.delete(key)); - promises.push(createComment({ - id: monotonicUlid(value.createdAt.getTime()), - userLogin: value.userLogin, - itemId: value.itemId, - text: value.text, - })); +const iter1 = kv.list({ prefix: ["items"] }); +for await (const oldItemEntry of iter1) { + if (oldItemEntry.value.createdAt) { + const newItem = { + id: ulid(new Date(oldItemEntry.value.createdAt).getTime()), + userLogin: oldItemEntry.value.userLogin, + url: oldItemEntry.value.url, + title: oldItemEntry.value.title, + score: oldItemEntry.value.score, + }; + await createItem(newItem); + const iter2 = kv.list({ + prefix: ["users_voted_for_item", oldItemEntry.value.id], + }); + for await (const userEntry of iter2) { + await deleteVote({ + itemId: oldItemEntry.value.id, + userLogin: userEntry.value.login, + }); + await deleteVote({ + itemId: newItem.id, + userLogin: userEntry.value.login, + }); + await createVote({ + itemId: newItem.id, + userLogin: userEntry.value.login, + createdAt: new Date(), + }); + } + await kv.delete(oldItemEntry.key); + } } -const results = await Promise.allSettled(promises); -results.forEach((result) => { - if (result.status === "rejected") console.error(result); -}); +const iter3 = kv.list({ prefix: ["items_by_user"] }); +const promises = []; +for await (const { key, value } of iter3) { + if (value.createdAt) promises.push(kv.delete(key)); +} +await Promise.all(promises); kv.close(); diff --git a/tasks/db_seed.ts b/tasks/db_seed.ts index 3856a30a7d40..cf5283a68df5 100644 --- a/tasks/db_seed.ts +++ b/tasks/db_seed.ts @@ -1,11 +1,7 @@ // Copyright 2023 the Deno authors. All rights reserved. MIT license. // Description: Seeds the kv db with Hacker News stories -import { - createItem, - createUser, - newItemProps, - newUserProps, -} from "@/utils/db.ts"; +import { createItem, createUser, newUserProps } from "@/utils/db.ts"; +import { ulid } from "std/ulid/mod.ts"; // Reference: https://github.com/HackerNews/API const API_BASE_URL = `https://hacker-news.firebaseio.com/v0`; @@ -36,7 +32,7 @@ const stories = await Promise.all( storiesResponses.map((r) => r.json()), ) as Story[]; const items = stories.map(({ by: userLogin, title, url, score, time }) => ({ - ...newItemProps(), + id: ulid(), userLogin, title, url, diff --git a/utils/db.ts b/utils/db.ts index 37e1ffe46c2d..6b57cb340335 100644 --- a/utils/db.ts +++ b/utils/db.ts @@ -1,4 +1,5 @@ // Copyright 2023 the Deno authors. All rights reserved. MIT license. +import { decodeTime } from "std/ulid/mod.ts"; import { chunk } from "std/collections/chunk.ts"; const KV_PATH_KEY = "KV_PATH"; @@ -54,69 +55,70 @@ export function formatDate(date: Date) { // Item export interface Item { + // Uses ULID + id: string; userLogin: string; title: string; url: string; - // The below properties can be automatically generated upon item creation - id: string; - createdAt: Date; score: number; } -export function newItemProps(): Pick { - return { - id: crypto.randomUUID(), - score: 0, - createdAt: new Date(), - }; -} - /** * Creates a new item in KV. Throws if the item already exists in one of the indexes. * * @example * ```ts - * import { newItemProps, createItem } from "@/utils/db.ts"; + * import { createItem } from "@/utils/db.ts"; + * import { ulid } from "std/ulid/mod.ts"; * * await createItem({ + * id: ulid(), * userLogin: "john_doe", * title: "example-title", * url: "https://example.com", - * ...newItemProps(), + * score: 0, * }); * ``` */ export async function createItem(item: Item) { const itemsKey = ["items", item.id]; - const itemsByTimeKey = ["items_by_time", item.createdAt.getTime(), item.id]; const itemsByUserKey = ["items_by_user", item.userLogin, item.id]; - const itemsCountKey = ["items_count", formatDate(item.createdAt)]; + const itemsCountKey = [ + "items_count", + formatDate(new Date(decodeTime(item.id))), + ]; const res = await kv.atomic() .check({ key: itemsKey, versionstamp: null }) - .check({ key: itemsByTimeKey, versionstamp: null }) .check({ key: itemsByUserKey, versionstamp: null }) .set(itemsKey, item) - .set(itemsByTimeKey, item) .set(itemsByUserKey, item) .sum(itemsCountKey, 1n) .commit(); - if (!res.ok) throw new Error(`Failed to create item: ${item}`); + if (!res.ok) throw new Error("Failed to create item"); } export async function deleteItem(item: Item) { const itemsKey = ["items", item.id]; - const itemsByTimeKey = ["items_by_time", item.createdAt.getTime(), item.id]; const itemsByUserKey = ["items_by_user", item.userLogin, item.id]; + const [itemsRes, itemsByUserRes] = await kv.getMany([ + itemsKey, + itemsByUserKey, + ]); + if (itemsRes.value === null) throw new Deno.errors.NotFound("Item not found"); + if (itemsByUserRes.value === null) { + throw new Deno.errors.NotFound("Item by user not found"); + } const res = await kv.atomic() + .check(itemsRes) + .check(itemsByUserRes) .delete(itemsKey) - .delete(itemsByTimeKey) .delete(itemsByUserKey) .commit(); - if (!res.ok) throw new Error(`Failed to delete item: ${item}`); + if (!res.ok) throw new Error("Failed to delete item"); } export async function getItem(id: string) { @@ -124,6 +126,10 @@ export async function getItem(id: string) { return res.value; } +export function listItems(options?: Deno.KvListOptions) { + return kv.list({ prefix: ["items"] }, options); +} + export function listItemsByUser( userLogin: string, options?: Deno.KvListOptions, @@ -131,10 +137,6 @@ export function listItemsByUser( return kv.list({ prefix: ["items_by_user", userLogin] }, options); } -export function listItemsByTime(options?: Deno.KvListOptions) { - return kv.list({ prefix: ["items_by_time"] }, options); -} - // Notification export interface Notification { // Uses ULID @@ -151,10 +153,10 @@ export interface Notification { * @example * ```ts * import { createNotification } from "@/utils/db.ts"; - * import { monotonicUlid } from "std/ulid/mod.ts"; + * import { ulid } from "std/ulid/mod.ts"; * * await createNotification({ - * id: monotonicUlid(), + * id: ulid(), * userLogin: "john_doe", * type: "example-type", * text: "Hello, world!", @@ -309,7 +311,6 @@ export async function createVote(vote: Vote) { vote.itemId, vote.userLogin, ]; - const itemByTimeKey = ["items_by_time", item.createdAt.getTime(), item.id]; const itemByUserKey = ["items_by_user", item.userLogin, item.id]; const votesCountKey = ["votes_count", formatDate(vote.createdAt)]; @@ -321,7 +322,6 @@ export async function createVote(vote: Vote) { .check({ key: itemVotedByUserKey, versionstamp: null }) .check({ key: userVotedForItemKey, versionstamp: null }) .set(itemKey, item) - .set(itemByTimeKey, item) .set(itemByUserKey, item) .set(itemVotedByUserKey, item) .set(userVotedForItemKey, user) @@ -352,14 +352,14 @@ export async function deleteVote(vote: Omit) { const user = userRes.value; if (item === null) throw new Deno.errors.NotFound("Item not found"); if (user === null) throw new Deno.errors.NotFound("User not found"); - if (itemVotedByUserRes.value === null) { + /** @todo Uncomment after ULID-items migration */ + /* if (itemVotedByUserRes.value === null) { throw new Deno.errors.NotFound("Item voted by user not found"); } if (userVotedForItemRes.value === null) { throw new Deno.errors.NotFound("User voted for item not found"); - } + } */ - const itemByTimeKey = ["items_by_time", item.createdAt.getTime(), item.id]; const itemByUserKey = ["items_by_user", item.userLogin, item.id]; item.score--; @@ -370,7 +370,6 @@ export async function deleteVote(vote: Omit) { .check(itemVotedByUserRes) .check(userVotedForItemRes) .set(itemKey, item) - .set(itemByTimeKey, item) .set(itemByUserKey, item) .delete(itemVotedByUserKey) .delete(userVotedForItemKey) diff --git a/utils/db_test.ts b/utils/db_test.ts index acd68f376483..17529cdd623b 100644 --- a/utils/db_test.ts +++ b/utils/db_test.ts @@ -26,11 +26,10 @@ import { incrVisitsCountByDay, type Item, listCommentsByItem, - listItemsByTime, + listItems, listItemsByUser, listItemsVotedByUser, listNotifications, - newItemProps, newUserProps, newVoteProps, Notification, @@ -38,17 +37,16 @@ import { type User, } from "./db.ts"; import { - assert, assertArrayIncludes, assertEquals, assertRejects, } from "std/assert/mod.ts"; import { DAY } from "std/datetime/constants.ts"; -import { monotonicUlid } from "std/ulid/mod.ts"; +import { ulid } from "std/ulid/mod.ts"; export function genNewComment(): Comment { return { - id: monotonicUlid(), + id: ulid(), itemId: crypto.randomUUID(), userLogin: crypto.randomUUID(), text: crypto.randomUUID(), @@ -57,10 +55,11 @@ export function genNewComment(): Comment { export function genNewItem(): Item { return { + id: ulid(), userLogin: crypto.randomUUID(), title: crypto.randomUUID(), url: `http://${crypto.randomUUID()}.com`, - ...newItemProps(), + score: 0, }; } @@ -75,7 +74,7 @@ export function genNewUser(): User { export function genNewNotification(): Notification { return { - id: monotonicUlid(), + id: ulid(), userLogin: crypto.randomUUID(), type: crypto.randomUUID(), text: crypto.randomUUID(), @@ -83,53 +82,44 @@ export function genNewNotification(): Notification { }; } -Deno.test("[db] newItemProps()", () => { - const itemProps = newItemProps(); - assert(itemProps.createdAt.getTime() <= Date.now()); - assertEquals(typeof itemProps.id, "string"); - assertEquals(itemProps.score, 0); -}); - -Deno.test("[db] getAllItems()", async () => { - const item1 = genNewItem(); - const item2 = genNewItem(); +Deno.test("[db] items", async () => { + const user = genNewUser(); + const item1: Item = { + ...genNewItem(), + id: ulid(), + userLogin: user.login, + }; + const item2: Item = { + ...genNewItem(), + id: ulid(Date.now() + 1_000), + userLogin: user.login, + }; - assertEquals(await collectValues(listItemsByTime()), []); + assertEquals(await getItem(item1.id), null); + assertEquals(await getItem(item2.id), null); + assertEquals(await collectValues(listItems()), []); + assertEquals(await collectValues(listItemsByUser(user.login)), []); + await assertRejects(async () => await deleteItem(item1), "Item not found"); await createItem(item1); await createItem(item2); - assertArrayIncludes(await collectValues(listItemsByTime()), [item1, item2]); -}); - -Deno.test("[db] (get/create/delete)Item()", async () => { - const item = genNewItem(); - - assertEquals(await getItem(item.id), null); + await assertRejects(async () => await createItem(item1)); - const dates = [new Date()]; - const [itemsCount] = await getManyMetrics("items_count", dates); - await createItem(item); - assertEquals(await getManyMetrics("items_count", dates), [itemsCount + 1n]); - await assertRejects(async () => await createItem(item)); - assertEquals(await getItem(item.id), item); - - await deleteItem(item); - assertEquals(await getItem(item.id), null); -}); - -Deno.test("[db] getItemsByUser()", async () => { - const userLogin = crypto.randomUUID(); - const item1 = { ...genNewItem(), userLogin }; - const item2 = { ...genNewItem(), userLogin }; - - assertEquals(await collectValues(listItemsByUser(userLogin)), []); - - await createItem(item1); - await createItem(item2); - assertArrayIncludes(await collectValues(listItemsByUser(userLogin)), [ + assertEquals(await getItem(item1.id), item1); + assertEquals(await getItem(item2.id), item2); + assertEquals(await collectValues(listItems()), [item1, item2]); + assertEquals(await collectValues(listItemsByUser(user.login)), [ item1, item2, ]); + + await deleteItem(item1); + await deleteItem(item2); + + assertEquals(await getItem(item1.id), null); + assertEquals(await getItem(item1.id), null); + assertEquals(await collectValues(listItems()), []); + assertEquals(await collectValues(listItemsByUser(user.login)), []); }); Deno.test("[db] user", async () => { @@ -245,7 +235,11 @@ Deno.test("[db] getDatesSince()", () => { Deno.test("[db] notifications", async () => { const userLogin = crypto.randomUUID(); const notification1 = { ...genNewNotification(), userLogin }; - const notification2 = { ...genNewNotification(), userLogin }; + const notification2 = { + ...genNewNotification(), + userLogin, + id: ulid(Date.now() + 1_000), + }; assertEquals(await getNotification(notification1), null); assertEquals(await getNotification(notification2), null); @@ -286,24 +280,24 @@ Deno.test("[db] notifications", async () => { Deno.test("[db] compareScore()", () => { const item1: Item = { + id: ulid(), userLogin: crypto.randomUUID(), title: crypto.randomUUID(), url: `http://${crypto.randomUUID()}.com`, - ...newItemProps(), score: 1, }; const item2: Item = { + id: ulid(), userLogin: crypto.randomUUID(), title: crypto.randomUUID(), url: `http://${crypto.randomUUID()}.com`, - ...newItemProps(), - score: 2, + score: 3, }; const item3: Item = { + id: ulid(), userLogin: crypto.randomUUID(), title: crypto.randomUUID(), url: `http://${crypto.randomUUID()}.com`, - ...newItemProps(), score: 5, };