Merge pull request #79 from HarperDB/bug-fixes

Bug fixes

Author: cb1kenobi

README.md

@@ -302,6 +302,17 @@ await db.put('foo', 'bar');
 console.log(await db.get('foo'));
 ```
 
+> [!IMPORTANT]
+> If your custom store overrides `putSync()` without calling `super.putSync()`
+> and it performs its own `this.encodeKey(key)`, then you MUST encode the VALUE
+> before you encode the KEY.
+>
+> Keys are encoded into a shared buffer. If the database is opened with the
+> `sharedStructuresKey` option, encoding the value will load and save the
+> structures which encodes the `sharedStructuresKey` overwriting the encoded
+> key in the shared key buffer, so it's ultra important that you encode the
+> value first!
+
 ## Interfaces
 
 ### `RocksDBOptions`
@@ -343,19 +354,19 @@ This package requires Node.js 18 or higher, pnpm, and a C++ compiler.
 > pnpm config set stream true
 > ```
 
-### Building the Native Binding
-
-To compile everything including the native binding and the TypeScript source, run:
-
-```bash
-pnpm build
-```
+### Building
 
-To configure and compile only the native binding, run:
+There are two things being built: the native binding and the TypeScript code.
+Each of those can be built to be debug friendly.
 
-```bash
-pnpm rebuild
-```
+| Description | Command |
+| --- | --- |
+| Production build (minified + native binding) | `pnpm build` |
+| TypeScript only (minified) | `pnpm build:bundle` |
+| TypeScript only (unminified) | `pnpm build:debug` |
+| Native binding only (prod) | `pnpm rebuild` |
+| Native binding only (with debug logging) | `pnpm rebuild:debug` |
+| Debug build everything | `pnpm build:debug && pnpm rebuild:debug` |
 
 When building the native binding, it will download the appropriate prebuilt
 RocksDB library for your platform and architecture from the

package.json

@@ -16,7 +16,7 @@
   "scripts": {
     "build": "pnpm build:bundle && pnpm rebuild",
     "build:bundle": "rimraf dist && rollup -c rollup.config.ts --configPlugin typescript && pnpm build:types",
-    "build:debug": "pnpm build:bundle && pnpm rebuild:debug",
+    "build:debug": "SKIP_MINIFY=1 pnpm build:bundle",
     "build:types": "pnpm build:types:temp && pnpm build:types:roll && pnpm build:types:check",
     "build:types:temp": "tsc --declaration --emitDeclarationOnly --outDir temp --project tsconfig.build.json",
     "build:types:roll": "rollup --config rollup.dts.config.ts --configPlugin typescript && rimraf temp",

rollup.config.ts

@@ -25,7 +25,7 @@ function generateConfig(format: 'es' | 'cjs') {
 			entryFileNames: format === 'es' ? 'index.js' : 'index.cjs'
 		},
 		plugins: [
-			esbuildMinifyPlugin({
+			process.env.SKIP_MINIFY ? null : esbuildMinifyPlugin({
 				minify: true,
 				minifySyntax: true
 			}),

src/binding/db_registry.cpp

@@ -62,22 +62,23 @@ std::unique_ptr<DBHandle> DBRegistry::OpenDB(const std::string& path, const DBOp
 			}
 
 			DEBUG_LOG("%p DBRegistry::OpenDB Database %s already open\n", instance.get(), path.c_str())
+			DEBUG_LOG("%p DBRegistry::OpenDB Checking for column family %s\n", instance.get(), name.c_str())
 
 			dbExists = true;
 
 			// manually copy the columns because we don't know which ones are valid
 			bool columnExists = false;
 			for (auto& column : descriptor->columns) {
-				std::shared_ptr<rocksdb::ColumnFamilyHandle> existingColumn = column.second;
-				if (existingColumn) {
-					columns[column.first] = existingColumn;
-					if (column.first == name) {
-						columnExists = true;
-					}
+				columns[column.first] = column.second;
+				if (column.first == name) {
+					DEBUG_LOG("%p DBRegistry::OpenDB Column family %s already exists\n", instance.get(), name.c_str())
+					columnExists = true;
 				}
 			}
 			if (!columnExists) {
+				DEBUG_LOG("DBRegistry::OpenDB Creating column family \"%s\"\n", name.c_str())
 				columns[name] = createColumn(descriptor->db, name);
+				descriptor->columns[name] = columns[name];
 			}
 		}
 	}

src/binding/transaction.cpp

@@ -1,11 +1,12 @@
+#include <sstream>
+#include <thread>
 #include "database.h"
 #include "db_handle.h"
 #include "db_iterator.h"
 #include "macros.h"
 #include "transaction.h"
 #include "transaction_handle.h"
 #include "util.h"
-#include <sstream>
 
 #define UNWRAP_TRANSACTION_HANDLE(fnName) \
 	std::shared_ptr<TransactionHandle>* txnHandle = nullptr; \
@@ -315,6 +316,19 @@ napi_value Transaction::PutSync(napi_env env, napi_callback_info info) {
 	rocksdb::Slice keySlice(key + keyStart, keyEnd - keyStart);
 	rocksdb::Slice valueSlice(value + valueStart, valueEnd - valueStart);
 
+#ifdef DEBUG
+	fprintf(stderr, "[%04zu] Transaction::PutSync() Key:", std::hash<std::thread::id>{}(std::this_thread::get_id()) % 10000);
+	for (size_t i = 0; i < keySlice.size(); i++) {
+		fprintf(stderr, " %02x", (unsigned char)keySlice.data()[i]);
+	}
+	fprintf(stderr, "\n");
+	fprintf(stderr, "[%04zu] Transaction::PutSync() Value:", std::hash<std::thread::id>{}(std::this_thread::get_id()) % 10000);
+	for (size_t i = 0; i < valueSlice.size(); i++) {
+		fprintf(stderr, " %02x", (unsigned char)valueSlice.data()[i]);
+	}
+	fprintf(stderr, "\n");
+#endif
+
 	ROCKSDB_STATUS_THROWS_ERROR_LIKE((*txnHandle)->putSync(keySlice, valueSlice), "Transaction put failed")
 
 	NAPI_RETURN_UNDEFINED()

src/database.ts

@@ -4,9 +4,9 @@ import { Store, type StoreOptions } from './store.js';
 import { config, type TransactionOptions, type RocksDatabaseConfig } from './load-binding.js';
 import * as orderedBinary from 'ordered-binary';
 import { Encoder as MsgpackEncoder } from 'msgpackr';
-import type { Key } from './encoding.js';
+import type { EncoderFunction, Key } from './encoding.js';
 
-interface RocksDatabaseOptions extends StoreOptions {
+export interface RocksDatabaseOptions extends StoreOptions {
 	/**
 	 * The column family name.
 	 *
@@ -183,8 +183,11 @@ export class RocksDatabase extends DBI<DBITransactional> {
 		 * 4. encoding === `ordered-binary`
 		 * 5. encoder.writeKey()
 		 */
-		let EncoderClass = store.encoder?.Encoder;
-		if (store.encoding === false || typeof EncoderClass === 'function') {
+		let EncoderClass: EncoderFunction | undefined = store.encoder?.Encoder;
+		if (store.encoding === false) {
+			store.encoder = null;
+			EncoderClass = undefined;
+		} else if (typeof EncoderClass === 'function') {
 			store.encoder = null;
 		} else if (
 			typeof store.encoder?.encode !== 'function' &&
@@ -206,8 +209,12 @@ export class RocksDatabase extends DBI<DBITransactional> {
 				};
 				opts.saveStructures = (structures: any, isCompatible: boolean | ((existingStructures: any) => boolean)) => {
 					this.transactionSync((txn: Transaction) => {
-						const existingStructuresBuffer = txn.getBinarySync(sharedStructuresKey);
-						const existingStructures = existingStructuresBuffer && store.decoder?.decode ? store.decoder.decode(existingStructuresBuffer) : undefined;
+						// note: we need to get a fresh copy of the shared structures,
+						// so we don't want to use the transaction's getBinarySync()
+						const existingStructuresBuffer = this.getBinarySync(sharedStructuresKey);
+						const existingStructures = existingStructuresBuffer && store.decoder?.decode
+							? store.decoder.decode(existingStructuresBuffer)
+							: undefined;
 						if (typeof isCompatible == 'function') {
 							if (!isCompatible(existingStructures)) {
 								return false;
@@ -228,7 +235,6 @@ export class RocksDatabase extends DBI<DBITransactional> {
 			if (!store.decoder) {
 				store.decoder = store.encoder;
 			}
-			store.decoderCopies = !store.encoder.needsStableBuffer;
 		} else if (store.encoding === 'ordered-binary') {
 			store.encoder = {
 				readKey: orderedBinary.readKey,
@@ -246,6 +252,11 @@ export class RocksDatabase extends DBI<DBITransactional> {
 					return store.encodeBuffer.subarray(0, bytesWritten);
 				}
 			};
+			store.encoder.copyBuffers = true;
+		}
+
+		if (store.decoder?.needsStableBuffer !== true) {
+			store.decoderCopies = true;
 		}
 
 		if (store.decoder?.readKey && !store.decoder.decode) {
@@ -261,11 +272,15 @@ export class RocksDatabase extends DBI<DBITransactional> {
 		return this;
 	}
 
+	get path() {
+		return this.store.path;
+	}
+
 	/**
 	 * Executes all operations in the callback as a single transaction.
 	 *
 	 * @param callback - A async function that receives the transaction as an argument.
-	 * @returns A promise that resolves when the transaction is committed or aborted.
+	 * @returns A promise that resolves the `callback` return value.
 	 *
 	 * @example
 	 * ```ts
@@ -275,7 +290,7 @@ export class RocksDatabase extends DBI<DBITransactional> {
 	 * });
 	 * ```
 	 */
-	async transaction(callback: (txn: Transaction) => Promise<any>, options?: TransactionOptions) {
+	async transaction(callback: (txn: Transaction) => PromiseLike<any>, options?: TransactionOptions) {
 		if (typeof callback !== 'function') {
 			throw new TypeError('Callback must be a function');
 		}
@@ -292,7 +307,24 @@ export class RocksDatabase extends DBI<DBITransactional> {
 		}
 	}
 
-	transactionSync(callback: (txn: Transaction) => void, options?: TransactionOptions) {
+	/**
+	 * Executes all operations in the callback as a single transaction.
+	 *
+	 * @param callback - A function that receives the transaction as an
+	 * argument. If the callback return promise-like value, it is awaited
+	 * before committing the transaction. Otherwise, the callback is treated as
+	 * synchronous.
+	 * @returns The `callback` return value.
+	 *
+	 * @example
+	 * ```ts
+	 * const db = RocksDatabase.open('/path/to/database');
+	 * await db.transaction(async (txn) => {
+	 *   await txn.put('key', 'value');
+	 * });
+	 * ```
+	 */
+	transactionSync<T>(callback: (txn: Transaction) => T | PromiseLike<T>, options?: TransactionOptions): T | PromiseLike<T> {
 		if (typeof callback !== 'function') {
 			throw new TypeError('Callback must be a function');
 		}
@@ -301,6 +333,20 @@ export class RocksDatabase extends DBI<DBITransactional> {
 
 		try {
 			const result = callback(txn);
+			let committed = false;
+
+			// despite being 'sync', we need to support async operations
+			if (result && typeof result === 'object' && 'then' in result && typeof result.then === 'function') {
+				return result.then((value) => {
+					if (committed) {
+						throw new Error('Transaction already committed');
+					}
+					committed = true;
+					txn.commitSync();
+					return value as T;
+				});
+			}
+
 			txn.commitSync();
 			return result;
 		} catch (error) {

src/dbi.ts

@@ -1,6 +1,6 @@
-import { getTxnId, type GetOptions, type PutOptions, type Store } from './store.js';
 import { when, withResolvers, type MaybePromise } from './util.js';
 import { NativeDatabase, NativeTransaction } from './load-binding.js';
+import type { GetOptions, PutOptions, Store } from './store.js';
 import type { Key } from './encoding.js';
 import type { Transaction } from './transaction.js';
 
@@ -173,7 +173,17 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 		if (this.store.decoderCopies) {
 			return when(
 				() => this.getBinaryFast(key, options),
-				result => result === undefined ? undefined : this.store.decodeValue(result as Buffer)
+				result => {
+					if (result === undefined) {
+						return undefined;
+					}
+
+					if (options?.skipDecode) {
+						return result;
+					}
+
+					return this.store.decodeValue(result as Buffer);
+				}
 			);
 		}
 
@@ -242,7 +252,7 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 				error = err;
 				reject?.(err);
 			},
-			getTxnId(options)
+			this.store.getTxnId(options)
 		);
 
 		if (error) {
@@ -300,7 +310,7 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 				error = err;
 				reject?.(err);
 			},
-			getTxnId(options)
+			this.store.getTxnId(options)
 		);
 
 		if (error) {
@@ -327,10 +337,9 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 
 		return this.store.getSync(
 			this.#context,
-			this.store.encodeKey(key),
+			key,
 			options
 		);
-		// TODO: return UNMODIFIED if the value is not modified
 	}
 
 	/**
@@ -356,17 +365,7 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 	 * ```
 	 */
 	getKeysCount(options?: RangeOptions & T): number {
-		const startKey = options?.start ? this.store.encodeKey(options?.start) : undefined;
-		const start = startKey ? Buffer.from(startKey.subarray(startKey.start, startKey.end)) : undefined;
-
-		const endKey = options?.end ? this.store.encodeKey(options.end) : undefined;
-		const end = endKey ? Buffer.from(endKey.subarray(endKey.start, endKey.end)) : undefined;
-
-		return this.store.getCount(this.#context, {
-			...options,
-			start,
-			end,
-		}, getTxnId(options));
+		return this.store.getCount(this.#context, options);
 	}
 
 	/**
@@ -438,7 +437,7 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 	 * ```
 	 */
 	async remove(key: Key, options?: T): Promise<void> {
-		return this.store.removeSync(this.#context, key, options);
+		return this.store.removeSync(this.#context, key, options as DBITransactional);
 	}
 
 	/**
@@ -455,6 +454,6 @@ export class DBI<T extends DBITransactional | unknown = unknown> {
 	 * ```
 	 */
 	removeSync(key: Key, options?: T): void {
-		return this.store.removeSync(this.#context, key, options);
+		return this.store.removeSync(this.#context, key, options as DBITransactional);
 	}
 }