From 27957d186453749dc6d85b13d38094969e3b192c Mon Sep 17 00:00:00 2001 From: Hexagon Date: Mon, 10 Jun 2024 00:13:53 +0200 Subject: [PATCH] Cleanup --- CHANGELOG.md | 9 +++- deno.json | 2 +- src/lib/key.ts | 21 ++++++---- src/lib/kv.ts | 9 +--- src/lib/ledger.ts | 4 +- src/lib/transaction.ts | 5 ++- src/lib/utils/file.ts | 8 +++- test/key.test.ts | 95 ++++++++++++++++++++++++++++++++++++++++++ test/kv.bench.ts | 24 +++++++---- test/kv.test.ts | 6 +-- 10 files changed, 148 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ce75bb..5a52493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,11 @@ -## Unrelesed +## 0.15.11 + +- Remove option doLock from `.sync()` +- Remove unused code from `key.ts` +- Add benchmark for getting data that does not exist +- Rename internal function `toAbsolutePath` to `toNormalizedAbsolutePath` and + clarify code with additional comments +- Adds various missing test to reduce the risk for regression bugs ## 0.15.10 diff --git a/deno.json b/deno.json index 3b9351c..15507e1 100644 --- a/deno.json +++ b/deno.json @@ -1,6 +1,6 @@ { "name": "@cross/kv", - "version": "0.15.10", + "version": "0.15.11", "exports": { ".": "./mod.ts", "./cli": "./src/cli/mod.ts" diff --git a/src/lib/key.ts b/src/lib/key.ts index 8eaa6b0..2425925 100644 --- a/src/lib/key.ts +++ b/src/lib/key.ts @@ -67,20 +67,13 @@ export class KVKeyInstance { private key: KVQuery | KVKey; private isQuery: boolean; public byteLength?: number; - hasData: boolean = false; constructor( - key: KVQuery | KVKey | Uint8Array | DataView, + key: KVQuery | KVKey | DataView, isQuery: boolean = false, validate: boolean = true, ) { - if (key instanceof Uint8Array) { - this.key = this.fromUint8Array( - new DataView(key.buffer, key.byteOffset, key.byteLength), - ); - this.hasData = true; - } else if (key instanceof DataView) { + if (key instanceof DataView) { this.key = this.fromUint8Array(key); - this.hasData = true; } else { this.key = key; } @@ -134,6 +127,7 @@ export class KVKeyInstance { keyArray.set(bytes, keyOffset); keyOffset += bytes.length; } + return keyArray; } @@ -225,6 +219,15 @@ export class KVKeyInstance { 'Ranges must have only "from" and/or "to" keys', ); } + + // Check for not mixing number from with string to and vice versa + if ( + (typeof element.from === "number" && + typeof element.to === "string") || + (typeof element.from === "string" && typeof element.to === "number") + ) { + throw new TypeError("Cannot mix string and number in ranges"); + } } if ( diff --git a/src/lib/kv.ts b/src/lib/kv.ts index 7acc87a..dab5f66 100644 --- a/src/lib/kv.ts +++ b/src/lib/kv.ts @@ -291,26 +291,20 @@ export class KV extends EventEmitter { * - Automatically run on adding data * - Can be manually triggered for full consistency before data retrieval (iterate(), listAll(), get()) * - * @param doLock - (Optional) Locks the database before synchronization. Defaults to true. Always true unless called internally. - * * @emits sync - Emits an event with the synchronization result: * - `result`: "ready" | "blocked" | "success" | "ledgerInvalidated" | "error" * - `error`: Error object (if an error occurred) or null * * @throws {Error} If an unexpected error occurs during synchronization. */ - public async sync(doLock = false): Promise { + public async sync(): Promise { // Throw if database isn't open this.ensureOpen(); // Synchronization Logic (with lock if needed) let result: KVSyncResult["result"] = "ready"; let error: Error | null = null; - let lockSucceeded = false; // Keeping track as we only want to unlock the database later, if the locking operation succeeded try { - if (doLock) await this.ledger?.lock(); - lockSucceeded = true; - const newTransactions = await this.ledger?.sync(this.disableIndex); if (newTransactions === null) { // Ledger invalidated @@ -336,7 +330,6 @@ export class KV extends EventEmitter { result = "error"; error = new Error("Error during ledger sync", { cause: syncError }); } finally { - if (doLock && lockSucceeded) await this.ledger?.unlock(); // @ts-ignore .emit exists this.emit("sync", { result, error }); } diff --git a/src/lib/ledger.ts b/src/lib/ledger.ts index fccc205..70c1fc8 100644 --- a/src/lib/ledger.ts +++ b/src/lib/ledger.ts @@ -2,7 +2,7 @@ import { ensureFile, rawOpen, readAtPosition, - toAbsolutePath, + toNormalizedAbsolutePath, writeAtPosition, } from "./utils/file.ts"; import { @@ -71,7 +71,7 @@ export class KVLedger { }; constructor(filePath: string, maxCacheSizeMBytes: number) { - this.dataPath = toAbsolutePath(filePath); + this.dataPath = toNormalizedAbsolutePath(filePath); this.cache = new KVLedgerCache(maxCacheSizeMBytes * 1024 * 1024); } diff --git a/src/lib/transaction.ts b/src/lib/transaction.ts index 27e3366..9dd4c9c 100644 --- a/src/lib/transaction.ts +++ b/src/lib/transaction.ts @@ -133,8 +133,9 @@ export class KVTransaction { this.operation = operation; this.timestamp = timestamp; if (value) { - this.data = new Uint8Array(encode(value)); - this.hash = await sha1(this.data!); + const valueData = new Uint8Array(encode(value)); + this.data = valueData; + this.hash = await sha1(valueData); } } diff --git a/src/lib/utils/file.ts b/src/lib/utils/file.ts index 9c70f64..f27fcea 100644 --- a/src/lib/utils/file.ts +++ b/src/lib/utils/file.ts @@ -3,11 +3,17 @@ import { CurrentRuntime, Runtime } from "@cross/runtime"; import { cwd, isDir, isFile, mkdir } from "@cross/fs"; import { dirname, isAbsolute, join, resolve } from "@std/path"; -export function toAbsolutePath(filename: string): string { +export function toNormalizedAbsolutePath(filename: string): string { let filePath; if (isAbsolute(filename)) { + // Even if the input is already an absolute path, it might not be normalized: + // - It could contain ".." or "." segments that need to be resolved. + // - It might have extra separators that need to be cleaned up. + // - It might not use the correct separator for the current OS. filePath = resolve(filename); } else { + // If the input is relative, combine it with the current working directory + // and then resolve to get a fully normalized absolute path. filePath = resolve(join(cwd(), filename)); } return filePath; diff --git a/test/key.test.ts b/test/key.test.ts index 11a3b0b..fdb71fa 100644 --- a/test/key.test.ts +++ b/test/key.test.ts @@ -212,3 +212,98 @@ test("KVKeyInstance: parse error on invalid range format", () => { KVKeyInstance.parse("users.>=abc.<=123", false); }, TypeError); }); + +test("KVKeyInstance: Throws if trying to use something other than an array for a key", () => { + assertThrows( + () => new KVKeyInstance("users.data.user123" as unknown as KVKey), + TypeError, + "Key must be an array", + ); +}); + +test("KVKeyInstance: Throws if trying to use something other than a string or number for a key element", () => { + assertThrows( + // @ts-ignore expected to be wrong + () => new KVKeyInstance(["users", null]), + TypeError, + "Key ranges are only allowed in queries", + ); +}); + +test("KVKeyInstance: Throws if trying to use an object with extra properties for a key element", () => { + assertThrows( + // @ts-ignore extected to be wrong + () => new KVKeyInstance(["users", { from: 100, extra: "value" }], true), + TypeError, + "Ranges must have only", + ); +}); + +test("KVKeyInstance: Throws if trying to mix strings and numbers in a range for a key element", () => { + assertThrows( + () => new KVKeyInstance(["users", { from: "abc", to: 123 }], true), + TypeError, + "Cannot mix string and number in ranges", + ); +}); + +test("KVKeyInstance: Throws if trying to use an object with invalid range values for a key element", () => { + assertThrows( + () => new KVKeyInstance(["users", { from: 123, to: "abc" }], true, true), + TypeError, + "Cannot mix string and number in ranges", + ); +}); + +test("KVKeyInstance: throws when parsing a key with empty elements", () => { + assertThrows( + () => KVKeyInstance.parse("users..profile", false), + TypeError, + "Key ranges are only allowed in queries", + ); +}); + +test("KVKeyInstance: stringify and parse with empty elements", () => { + // Empty object represents an empty element + const queryWithEmpty: KVQuery = ["users", {}, "profile"]; + const queryWithRangeAndEmpty: KVQuery = ["data", { from: 10, to: 20 }, {}]; + + const parsedKey = KVKeyInstance.parse("users..profile", true); + assertEquals(parsedKey, queryWithEmpty); + + const queryInstance = new KVKeyInstance(queryWithRangeAndEmpty, true); + assertEquals(queryInstance.stringify(), "data.>=#10<=#20."); + + const parsedQuery = KVKeyInstance.parse("data.>=#10<=#20.", true); + assertEquals(parsedQuery, queryWithRangeAndEmpty); +}); + +test("KVKeyInstance: parse with leading dots should throw", () => { + assertThrows( + () => KVKeyInstance.parse(`.data.>=a<=z`, false), + TypeError, + "Ranges are not allowed in keys.", + ); +}); + +test("KVKeyInstance: stringify and parse mixed-type keys", () => { + const mixedKey: KVKey = ["users", 123, "profile"]; + const instance = new KVKeyInstance(mixedKey); + + const stringified = instance.stringify(); + assertEquals(stringified, "users.#123.profile"); + + const parsed = KVKeyInstance.parse(stringified, false); + assertEquals(parsed, mixedKey); +}); + +test("KVKeyInstance: toUint8Array and fromUint8Array roundtrip", () => { + const key: KVKey = ["users", "john_doe", 42]; + const instance = new KVKeyInstance(key); + const uint8Array = instance.toUint8Array(); + + const dataView = new DataView(uint8Array.buffer); + const newKeyInstance = new KVKeyInstance(dataView); + + assertEquals(newKeyInstance.get(), key); +}); diff --git a/test/kv.bench.ts b/test/kv.bench.ts index 18fad01..955c699 100644 --- a/test/kv.bench.ts +++ b/test/kv.bench.ts @@ -18,16 +18,15 @@ async function setupDenoKV() { const crossStore = await setupKV(); const denoStore = await setupDenoKV(); -let crossIterTransaction = 0; -let denoIterTransaction = 0; await Deno.bench("cross_kv_set_100_atomic", async () => { await crossStore.beginTransaction(); for (let i = 0; i < 100; i++) { - await crossStore.set(["testKey", crossIterTransaction++], { + const randomUUID = crypto.randomUUID(); + await crossStore.set(["testKey", randomUUID], { data: { data: "testData", - i: crossIterTransaction, + i: randomUUID, more: { "test": "data", "with1": new Date(), @@ -47,10 +46,11 @@ await Deno.bench("cross_kv_set_100_atomic", async () => { await Deno.bench("deno_kv_set_100_atomic", async () => { const at = denoStore.atomic(); for (let i = 0; i < 100; i++) { - at.set(["testKey", denoIterTransaction++], { + const randomUUID = crypto.randomUUID(); + at.set(["testKey", randomUUID], { data: { data: "testData", - i: denoIterTransaction, + i: randomUUID, more: { "test": "data", "with1": new Date(), @@ -108,9 +108,17 @@ await Deno.bench("deno_kv_set", async () => { }); await Deno.bench("cross_kv_get", async () => { - await crossStore.get(["testKey", 3]); + await crossStore.get(["testKey2", 3]); }); await Deno.bench("deno_kv_get", async () => { - await denoStore.get(["testKey", 3]); + await denoStore.get(["testKey2", 3]); +}); + +await Deno.bench("cross_kv_get_nonexisting", async () => { + await crossStore.get(["testKey2", "eh"]); +}); + +await Deno.bench("deno_kv_get_nonexisting", async () => { + await denoStore.get(["testKey2", "eh"]); }); diff --git a/test/kv.test.ts b/test/kv.test.ts index 17a4951..b608fda 100644 --- a/test/kv.test.ts +++ b/test/kv.test.ts @@ -427,7 +427,7 @@ test("KV: watch functionality - basic matching", async () => { }); await kvStore.set(watchedKey, { name: "Alice", age: 30 }); - await kvStore.sync(true); // Manual sync to trigger the watch callback + await kvStore.sync(); assertEquals(receivedTransaction!.key, watchedKey); assertEquals(receivedTransaction!.data, { name: "Alice", age: 30 }); @@ -451,7 +451,7 @@ test("KV: watch functionality - recursive matching", async () => { await kvStore.set(["users", "user1"], "Alice"); await kvStore.set(["users", "user2"], "Bob"); await kvStore.set(["data", "other"], "Not watched"); - await kvStore.sync(true); // Not needed, but trigger to ensure no duplicate calls occurr + await kvStore.sync(); assertEquals(receivedTransactions.length, 2); assertEquals(receivedTransactions[0].data, "Alice"); @@ -537,7 +537,7 @@ test("KV: watch functionality - no match", async () => { // Add data under a different key await kvStore.set(["data", "something"], "else"); - await kvStore.sync(true); + await kvStore.sync(); assertEquals(callbackCalled, false, "Callback should not have been called");