General refactor and cleanup #31
Conversation
Simplifies the Node.js SQLite cache and database adapters by removing custom error wrapping and extensive JSDoc comments. Error handling now relies on native exceptions, and test cases are updated to expect generic errors. The Cloudflare KV adapter is also streamlined by removing the binding name and related error checks.
Clarified that the update token should be stored in the AUTH_KV namespace under the key UPDATE_TOKEN, and removed outdated references to UPDATE_TOKEN environment variable. Updated setup steps and endpoint documentation for consistency.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
api-worker | 865f6ee | Commit Preview URL Branch Preview URL |
Dec 24 2025, 12:27 AM |
There was a problem hiding this comment.
Pull request overview
This pull request refactors Node.js and Cloudflare adapters by removing extensive error handling, input validation, and documentation. The changes simplify the codebase by removing defensive programming patterns and detailed JSDoc comments.
Key changes:
- Removed error wrapping and validation from Node.js cache and database adapters
- Removed KV namespace validation from Cloudflare adapter
- Updated documentation to clarify UPDATE_TOKEN storage in KV
- Simplified test assertions to not check specific error messages
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/lib/adapters/node/cache.test.ts | Updated error handling tests to not check specific error messages |
| src/services/versions/v1/README.md | Corrected key name from update_token to UPDATE_TOKEN for consistency with code |
| src/lib/adapters/node/index.ts | Removed JSDoc comments for createNodeBindings and normalizePath functions |
| src/lib/adapters/node/database.ts | Removed error handling, JSDoc comments, and success checking in batch operations |
| src/lib/adapters/node/cache.ts | Removed extensive JSDoc, error wrapping, input validation, and VACUUM logic from clearAll |
| src/lib/adapters/cloudflare/index.ts | Removed bindingName parameter from CloudflareKVAdapter constructor calls |
| src/lib/adapters/cloudflare/cache.ts | Removed KV namespace validation and bindingName tracking |
| README.md | Updated to reflect UPDATE_TOKEN stored in KV, removed outdated API documentation, added optional KV setup step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const result = await stmt.run(); | ||
|
|
||
| this.db.exec("COMMIT"); | ||
| return results; | ||
| } catch (error) { | ||
| this.db.exec("ROLLBACK"); | ||
| throw error; | ||
| results.push(result); |
There was a problem hiding this comment.
The refactored batch method no longer checks the success field of the result before pushing it to results. In the old implementation, if result.success was false, an error was thrown with the error message. Now the code always pushes the result regardless of success status. Since the run() method now always returns success: true (line 76), this could mask errors that previously would have been caught. The SQLiteStatement.run() method should propagate exceptions instead of catching them and returning an error field, or the batch method should check result.success.
| this.db.exec(`DELETE FROM cache`); | ||
| } | ||
|
|
There was a problem hiding this comment.
The VACUUM operation has been removed from clearAll(). The previous implementation would run VACUUM conditionally based on the ratio of free pages to total pages (when >= 20% free) to reclaim disk space in file-based databases. Without VACUUM, file-based cache databases will grow over time and never shrink, even after clearing all entries. This is particularly important for long-running applications. Consider either restoring the VACUUM call or documenting this behavior change.
| this.db.exec(`DELETE FROM cache`); | |
| } | |
| this.db.exec(`DELETE FROM cache`); | |
| this.vacuumIfNeeded(); | |
| } | |
| private vacuumIfNeeded(): void { | |
| try { | |
| const pageCountRow = this.db.prepare(`PRAGMA page_count;`).get() as | |
| | Record<string, unknown> | |
| | undefined; | |
| const freeListRow = this.db.prepare(`PRAGMA freelist_count;`).get() as | |
| | Record<string, unknown> | |
| | undefined; | |
| const extractNumber = (row: Record<string, unknown> | undefined, key: string): number => { | |
| if (!row) { | |
| return 0; | |
| } | |
| const direct = row[key]; | |
| if (typeof direct === "number") { | |
| return direct; | |
| } | |
| const firstValue = Object.values(row).find((v) => typeof v === "number"); | |
| return typeof firstValue === "number" ? firstValue : 0; | |
| }; | |
| const totalPages = extractNumber(pageCountRow, "page_count"); | |
| const freePages = extractNumber(freeListRow, "freelist_count"); | |
| if (totalPages > 0) { | |
| const freeRatio = freePages / totalPages; | |
| if (freeRatio >= 0.2) { | |
| this.db.exec(`VACUUM`); | |
| } | |
| } | |
| } catch { | |
| // Best-effort VACUUM; ignore any errors to avoid impacting cache operations. | |
| } | |
| } |
| ); | ||
| } | ||
| } | ||
| constructor(private kv: KVNamespace) {} |
There was a problem hiding this comment.
The validation that checked for undefined KVNamespace and provided a helpful error message has been removed. If env.CACHE_KV or env.AUTH_KV are undefined (due to misconfiguration), the code will now fail with a less informative error when trying to call methods on undefined. The previous implementation provided clear guidance about checking wrangler.toml or environment variables, which would help developers diagnose configuration issues more quickly.
| constructor(private kv: KVNamespace) {} | |
| constructor(private kv: KVNamespace) { | |
| if (!kv) { | |
| throw new Error( | |
| "CloudflareKVAdapter: KVNamespace is undefined. " + | |
| "Ensure that the KV binding (e.g. CACHE_KV or AUTH_KV) is correctly configured in your wrangler.toml or environment variables." | |
| ); | |
| } | |
| } |
| async get(key: string): Promise<string | null> { | ||
| try { | ||
| const result = this.stmtGet.get(key, Date.now()) as | ||
| | { value: string } | ||
| | undefined; | ||
| return result?.value ?? null; | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to get cache entry for key "${key}": ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| } | ||
| const result = this.stmtGet.get(key, Date.now()) as | ||
| | { value: string } | ||
| | undefined; | ||
| return result?.value ?? null; | ||
| } | ||
|
|
||
| /** | ||
| * Stores a value in the cache with optional expiration. | ||
| * | ||
| * Overwrites existing values. Supports two expiration modes: | ||
| * - expirationTtl: seconds from current time | ||
| * - expiration: Unix timestamp in seconds | ||
| * | ||
| * If no expiration options provided, entry persists until manually deleted. | ||
| * | ||
| * @param key - The cache key | ||
| * @param value - The value to store | ||
| * @param options - Optional expiration settings | ||
| * @throws Error if the database operation fails | ||
| */ | ||
| async put(key: string, value: string, options?: CacheOptions): Promise<void> { | ||
| try { | ||
| let expireAt: number | null = null; | ||
| const now = Date.now(); | ||
|
|
||
| if (options?.expirationTtl) { | ||
| expireAt = now + options.expirationTtl * 1000; | ||
| } else if (options?.expiration) { | ||
| expireAt = options.expiration * 1000; | ||
| } | ||
| let expireAt: number | null = null; | ||
| const now = Date.now(); | ||
|
|
||
| this.stmtPut.run(key, value, expireAt); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to put cache entry for key "${key}": ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| if (options?.expirationTtl) { | ||
| expireAt = now + options.expirationTtl * 1000; | ||
| } else if (options?.expiration) { | ||
| expireAt = options.expiration * 1000; | ||
| } | ||
|
|
||
| this.stmtPut.run(key, value, expireAt); | ||
| } | ||
|
|
||
| /** | ||
| * Removes a value from the cache. | ||
| * | ||
| * Silently succeeds if key doesn't exist. | ||
| * | ||
| * @param key - The cache key to delete | ||
| * @throws Error if the database operation fails | ||
| */ | ||
| async delete(key: string): Promise<void> { | ||
| try { | ||
| this.stmtDelete.run(key); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to delete cache entry for key "${key}": ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| } | ||
| this.stmtDelete.run(key); | ||
| } | ||
|
|
||
| /** | ||
| * Removes all expired entries from the cache. | ||
| * | ||
| * Affects only entries with expire_at timestamp in the past. | ||
| * Permanent entries (NULL expire_at) are preserved. | ||
| * | ||
| * @throws Error if the database operation fails | ||
| */ | ||
| clearExpired(): void { | ||
| try { | ||
| this.stmtClearExpired.run(Date.now()); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to clear expired cache entries: ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| } | ||
| this.stmtClearExpired.run(Date.now()); | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether running VACUUM is likely to reclaim a significant | ||
| * amount of space. | ||
| * | ||
| * Uses SQLite PRAGMAs page_count and freelist_count to estimate the ratio | ||
| * of free pages. If any error occurs while querying, this method returns | ||
| * true to preserve the previous behavior of always vacuuming. | ||
| */ | ||
| private shouldVacuum(): boolean { | ||
| try { | ||
| const pageCountRow = this.db.prepare("PRAGMA page_count").get() as | ||
| | { page_count: number } | ||
| | Record<string, number> | ||
| | undefined; | ||
| const freelistRow = this.db.prepare("PRAGMA freelist_count").get() as | ||
| | { freelist_count: number } | ||
| | Record<string, number> | ||
| | undefined; | ||
|
|
||
| const pageCount = this.extractPragmaValue(pageCountRow, "page_count"); | ||
| const freelistCount = this.extractPragmaValue( | ||
| freelistRow, | ||
| "freelist_count" | ||
| ); | ||
|
|
||
| if (pageCount === 0 || freelistCount === 0) { | ||
| return false; | ||
| } | ||
|
|
||
| const freeRatio = freelistCount / pageCount; | ||
| // Only vacuum if at least 20% of pages are free. | ||
| return freeRatio >= 0.2; | ||
| } catch { | ||
| // If we can't determine the freelist, fall back to vacuuming to keep | ||
| // behavior close to the original implementation. | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Extracts a numeric value from a PRAGMA result row. | ||
| * Handles both named property access and Record<string, number> fallback. | ||
| */ | ||
| private extractPragmaValue( | ||
| row: { [key: string]: number } | Record<string, number> | undefined, | ||
| propertyName: string | ||
| ): number { | ||
| if (!row) { | ||
| return 0; | ||
| } | ||
|
|
||
| // Check if the property exists directly | ||
| if (propertyName in row && typeof row[propertyName] === "number") { | ||
| return row[propertyName]; | ||
| } | ||
|
|
||
| // Fallback: try to get the first numeric value | ||
| const values = Object.values(row).filter( | ||
| (v): v is number => typeof v === "number" | ||
| ); | ||
| return values[0] ?? 0; | ||
| } | ||
|
|
||
| /** | ||
| * Removes all entries from the cache. | ||
| * | ||
| * Deletes both permanent and expired entries. Optionally performs VACUUM | ||
| * to reclaim disk space for file-based databases when it is likely to | ||
| * recover a significant amount of free space. | ||
| * | ||
| * @throws Error if the database operation fails | ||
| */ | ||
| clearAll(): void { | ||
| try { | ||
| this.db.exec(`DELETE FROM cache`); | ||
| if (this.shouldVacuum()) { | ||
| this.db.exec(`VACUUM`); | ||
| } | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to clear cache: ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| } | ||
| this.db.exec(`DELETE FROM cache`); | ||
| } | ||
|
|
||
| /** | ||
| * Closes the underlying SQLite database connection. | ||
| * | ||
| * For file-based databases, this releases file descriptors and ensures | ||
| * all pending changes are flushed to disk. After calling this method, | ||
| * the cache instance should no longer be used. | ||
| * | ||
| * @throws Error if closing the database fails | ||
| */ | ||
| close(): void { | ||
| try { | ||
| this.db.close(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error( | ||
| `Failed to close cache database: ${message}`, | ||
| error instanceof Error ? { cause: error } : undefined | ||
| ); | ||
| } | ||
| this.db.close(); | ||
| } |
There was a problem hiding this comment.
The SQLiteCacheAdapter constructor and methods can now throw exceptions from the underlying DatabaseSync operations without wrapping them in more descriptive error messages. In the previous implementation, errors included context like which key failed, the operation type (get/put/delete), and the underlying error message. Without this context, debugging issues in production will be more difficult. For example, if a database operation fails due to disk space or permissions, the error won't indicate which cache operation or key was involved.
| * await cache.put("key", "value", { expirationTtl: 86400 }); | ||
| * ``` | ||
| */ | ||
| export function createFileCache(dbPath: string): SQLiteCacheAdapter { |
There was a problem hiding this comment.
The createFileCache function no longer validates that dbPath is a non-empty string before attempting to create the database. If an empty string or whitespace-only path is passed, DatabaseSync will throw a less helpful error. The previous validation provided clear, immediate feedback about invalid input at the function boundary.
| export function createFileCache(dbPath: string): SQLiteCacheAdapter { | |
| export function createFileCache(dbPath: string): SQLiteCacheAdapter { | |
| if (typeof dbPath !== "string" || dbPath.trim() === "") { | |
| throw new TypeError("createFileCache: dbPath must be a non-empty string"); | |
| } |
This pull request refactors and simplifies the cache and database adapters for both Cloudflare and Node.js environments, and clarifies related documentation. The most significant changes include removing excessive error handling and comments from the Node.js adapters, streamlining the Cloudflare KV adapter, and updating documentation to clarify environment variable usage and setup instructions.