Upgrade node-persist to 4.0.4 with migration strategy and custom storage support#1109
Upgrade node-persist to 4.0.4 with migration strategy and custom storage support#1109
Conversation
Co-authored-by: donavanbecker <[email protected]>
Co-authored-by: donavanbecker <[email protected]>
Co-authored-by: donavanbecker <[email protected]>
Co-authored-by: donavanbecker <[email protected]>
|
As noted in the review, node-persist v2+ changes file naming from literal keys to MD5 hashes, and v4 uses SHA256 hashes. This breaks compatibility with existing persisted data and external tools like homebridge-config-ui-x that rely on the file format. A migration strategy would be needed to safely upgrade this dependency. Co-authored-by: donavanbecker <[email protected]>
|
You're absolutely right. I've reverted the changes in commit 9e4e1a7. The issue is that node-persist v2.0.0+ changed file naming from literal keys to MD5 hashes, and v4.0.0 uses SHA256 hashes. This breaks compatibility with all existing persisted data:
Upgrading without migration would cause users to lose all HomeKit pairings and break config-ui-x compatibility. A proper upgrade would require:
The deprecated q warning will remain, but that's better than breaking all existing installations. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Continue with this change however we will need the following as well:
|
Implements all requested features: - Data migration script to convert old format to new format - Version detection logic to handle both formats during transition - Major version bump to v3.0.0 (breaking change) - Comprehensive migration guide with examples and troubleshooting - Extensive testing: 14 new tests covering all migration scenarios Key features: - Automatic migration on first startup - Backward compatibility during transition period - Manual migration tool (tools/migrate-storage.mjs) - Detailed documentation (MIGRATION.md) - Zero data loss - old files preserved - Transparent to end users Co-authored-by: donavanbecker <[email protected]>
Completed in commit 5b7808e. All requested features implemented: ✅ Data migration script - Key features:
See MIGRATION.md for full documentation and examples. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot add a flag that cleans up the old files. But defaults to not cleaning them up unless flag is set. |
- Added HAPStorage.setCleanupOldFiles(boolean) method to control cleanup behavior - Updated StorageMigration.migrateStorageDirectory() to accept cleanup flag (defaults to false) - Enhanced CLI tool with --cleanup flag for manual migration - Added 5 new comprehensive tests for cleanup functionality - Updated MIGRATION.md with cleanup instructions and API examples - Updated README with cleanup flag mention Key safety features: - Cleanup is DISABLED by default to preserve old files - Old files only deleted if migration was successful - Clear warnings in CLI when cleanup mode is enabled - Backward compatible - existing code works without changes All 774 tests passing (769 original + 5 new). Co-authored-by: donavanbecker <[email protected]>
Added in commit 17d959a. Cleanup flag implemented with safety defaults: API Method: HAPStorage.setCleanupOldFiles(true); // Enable cleanup
HAPStorage.storage(); // Migration runs with cleanupCLI Tool: node tools/migrate-storage.mjs --cleanup [directory]Key Features: Old files are safely preserved unless cleanup is explicitly enabled. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
I'm unable to access issue #864 directly. Could you provide details about what changes from that issue should be included in this PR? If it's a specific feature or fix that would benefit from being included with the v3.0.0 major version bump, I can implement it. Please let me know:
This will help ensure the implementation aligns with your expectations for this major release. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Implements issue #864 - allows users to provide their own storage backend. Changes: - Created StorageInterface for custom storage implementations - Updated HAPStorage to support custom storage via setCustomStorage() - Added NodePersistStorageAdapter to wrap node-persist as StorageInterface - Backward compatible: uses node-persist by default - Migration only runs for default node-persist storage - Added comprehensive documentation in MIGRATION.md - Added 3 new tests for custom storage functionality Benefits: - Plug in custom database backends - Multi-branch configuration support - Integration with existing systems - Centralized storage across instances - Advanced caching strategies All 777 tests passing (774 original + 3 new). Co-authored-by: donavanbecker <[email protected]>
Implemented in commit e1c6354. Custom storage support added via dependency injection. New API: import { StorageInterface, HAPStorage } from "@homebridge/hap-nodejs";
class MyDatabaseStorage implements StorageInterface {
async getItem(key: string): Promise<any> { ... }
async setItem(key: string, value: any): Promise<void> { ... }
async removeItem(key: string): Promise<void> { ... }
}
// Set before initialization
HAPStorage.setCustomStorage(new MyDatabaseStorage());Features: See MIGRATION.md for complete implementation guide and examples. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR upgrades the project’s persistence layer from [email protected] to ^4.0.4 (removing the deprecated q dependency), introduces a storage migration engine/tooling to handle on-disk format changes, and adds dependency-injected custom storage support via a new StorageInterface.
Changes:
- Upgraded
node-persistto v4 and updated project code/tests to use the async storage API. - Added
StorageMigration(and CLI tooling/docs) to migrate legacy storage files to the v4 SHA256-hash format. - Added custom storage support via
StorageInterface+HAPStorage.setCustomStorage(...), and exported these APIs publicly.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/migrate-storage.mjs | Adds a CLI migration tool with optional cleanup mode. |
| src/types/node-persist.d.ts | Updates typings to match the async API surface of node-persist v4. |
| src/lib/model/StorageMigration.ts | Implements migration/detection/cleanup logic across v0/v2/v4 formats. |
| src/lib/model/StorageMigration.spec.ts | Adds test coverage for migration, detection, integrity, and cleanup. |
| src/lib/model/StorageInterface.ts | Introduces DI contract for custom storage backends. |
| src/lib/model/HAPStorage.ts | Wraps node-persist behind StorageInterface, runs migration, and supports custom storage injection. |
| src/lib/model/HAPStorage.spec.ts | Updates tests for the new storage interface and initialization behavior. |
| src/lib/model/HAPStorage-CustomStorage.spec.ts | Adds tests validating custom storage injection and contract behavior. |
| src/lib/model/IdentifierCache.ts | Converts persistence to async storage calls (fire-and-forget writes). |
| src/lib/model/IdentifierCache.spec.ts | Updates tests for async storage methods. |
| src/lib/model/AccessoryInfo.ts | Converts persistence to async storage calls (fire-and-forget writes) and async load/remove. |
| src/lib/model/ControllerStorage.ts | Converts persistence to async storage calls and makes load/remove async. |
| src/lib/Accessory.ts | Awaits async loads/removals and updates destroy/cleanup flows accordingly. |
| src/lib/Accessory.spec.ts | Updates tests to await publish() and mock async AccessoryInfo.load(). |
| src/index.ts | Exports StorageMigration and StorageInterface for external/plugin usage. |
| mocks/node-persist.ts | Updates the node-persist Jest mock to return Promises for async methods. |
| README.md | Adds a migration notice pointing users to MIGRATION.md and cleanup behavior. |
| MIGRATION.md | Adds a detailed storage migration guide and usage instructions. |
| package.json | Bumps package version to 3.0.0 and upgrades node-persist dependency. |
| package-lock.json | Locks updated dependency graph (removes q, updates related packages). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private static async migrateFile(storageDir: string, key: string): Promise<boolean> { | ||
| const oldPath = path.join(storageDir, key); | ||
| const newFilename = this.generateV4Filename(key); | ||
| const newPath = path.join(storageDir, newFilename); | ||
|
|
||
| try { | ||
| // Read old file | ||
| const content = fs.readFileSync(oldPath, "utf8"); | ||
| const data = JSON.parse(content); | ||
|
|
||
| // Write to new format with node-persist v4 structure | ||
| const v4Data = { | ||
| key: key, | ||
| value: data, | ||
| }; | ||
| fs.writeFileSync(newPath, JSON.stringify(v4Data), "utf8"); | ||
|
|
||
| debug("Migrated file: %s -> %s", key, newFilename); |
There was a problem hiding this comment.
Migration writes the v4 file unconditionally (writeFileSync(newPath, ...)). If a v4 file already exists (common because old-format files are preserved by default), this can overwrite newer data with stale v0 content on subsequent startups, causing data regression. Add a guard to skip migration when the destination v4 file exists (or otherwise resolve conflicts by preferring the v4 file).
| private static async migrateV2File(storageDir: string, key: string): Promise<boolean> { | ||
| const oldFilename = this.generateV2Filename(key); | ||
| const oldPath = path.join(storageDir, oldFilename); | ||
| const newFilename = this.generateV4Filename(key); | ||
| const newPath = path.join(storageDir, newFilename); | ||
|
|
||
| try { | ||
| // Read old file | ||
| const content = fs.readFileSync(oldPath, "utf8"); | ||
| const data = JSON.parse(content); | ||
|
|
||
| // v2 format already has the structure we need, just copy it | ||
| const v4Data = { | ||
| key: key, | ||
| value: data.value || data, | ||
| }; | ||
| fs.writeFileSync(newPath, JSON.stringify(v4Data), "utf8"); | ||
|
|
||
| debug("Migrated v2 file: %s -> %s", oldFilename, newFilename); |
There was a problem hiding this comment.
migrateV2File also overwrites the v4 destination unconditionally. If the MD5 (v2) file is retained, re-running migration can replace updated v4 data with stale v2 content. Skip migration when the SHA256 destination already exists (or prefer the v4 file and optionally delete the v2 file only when cleanup is enabled).
src/lib/model/StorageMigration.ts
Outdated
| this.migrateV2File(storageDir, key).catch(err => { | ||
| debug("Background migration from v2 failed for %s: %s", key, err); | ||
| }); | ||
| return data.value || data; |
There was a problem hiding this comment.
Using data.value || data will mis-handle valid falsy stored values (e.g., 0, false, "", null) by falling back to the full object. Prefer a nullish check (data.value ?? data) or an explicit property-existence test (e.g., 'value' in data) both when migrating and when returning the read value.
| return data.value || data; | |
| if (data !== null && typeof data === "object" && "value" in data) { | |
| return (data as any).value; | |
| } | |
| return data; |
| for (const file of files) { | ||
| // Only delete v0 format files (not hex strings) | ||
| if (!/^[a-f0-9]+$/.test(file)) { | ||
| const filePath = path.join(storageDir, file); | ||
| try { | ||
| fs.unlinkSync(filePath); | ||
| deletedCount++; | ||
| debug("Deleted old format file: %s", file); | ||
| } catch (error) { | ||
| debug("Error deleting file %s: %s", file, error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
cleanupOldFiles currently deletes any filename that isn't hex-only. In cleanup mode this can remove unrelated files in the storage directory (e.g., .DS_Store, backups, readmes), and it also leaves v2 MD5 files behind because they are hex-only. Consider restricting deletion to known legacy node-persist patterns (e.g., *.json literal keys / v1 base64 names) and optionally deleting 32-char MD5 files only after successful migration.
tools/migrate-storage.mjs
Outdated
| import { fileURLToPath } from "url"; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
|
|
There was a problem hiding this comment.
The script defines __dirname but never uses it. With the repo's ESLint rules (no-unused-vars), this will fail linting. Remove the unused variable (and fileURLToPath usage if no longer needed) or use __dirname for resolving the default path.
| import { fileURLToPath } from "url"; | |
| const __filename = fileURLToPath(import.meta.url); | |
| const __dirname = path.dirname(__filename); |
| * This tool migrates storage files from node-persist v0.0.12 format to v4.0.4 format. | ||
| * | ||
| * Usage: | ||
| * node migrate-storage.js [storage-directory] | ||
| * node migrate-storage.js --cleanup [storage-directory] | ||
| * | ||
| * If no directory is specified, uses the default: .node-persist/storage | ||
| * | ||
| * Options: | ||
| * --cleanup Delete old format files after successful migration | ||
| */ | ||
|
|
||
| import { StorageMigration } from "../dist/lib/model/StorageMigration.js"; | ||
| import path from "path"; | ||
| import { fileURLToPath } from "url"; | ||
|
|
||
| const __filename = fileURLToPath(import.meta.url); | ||
| const __dirname = path.dirname(__filename); | ||
|
|
||
| async function main() { | ||
| const args = process.argv.slice(2); | ||
|
|
||
| // Parse arguments | ||
| let cleanupMode = false; | ||
| let storageDir = null; | ||
|
|
||
| for (const arg of args) { | ||
| if (arg === "--cleanup") { | ||
| cleanupMode = true; | ||
| } else if (!arg.startsWith("--")) { | ||
| storageDir = arg; | ||
| } | ||
| } | ||
|
|
||
| if (!storageDir) { | ||
| storageDir = path.join(process.cwd(), ".node-persist/storage"); | ||
| } | ||
|
|
||
| console.log("HAP-NodeJS Storage Migration Tool"); | ||
| console.log("=================================="); | ||
| console.log(); | ||
| console.log("Storage directory:", storageDir); | ||
| if (cleanupMode) { | ||
| console.log("Cleanup mode: ENABLED (old files will be deleted)"); | ||
| console.log(); | ||
| console.log("⚠️ WARNING: This will permanently delete old format files!"); | ||
| console.log(" Make sure you have a backup before proceeding."); | ||
| console.log(); | ||
| } else { | ||
| console.log("Cleanup mode: DISABLED (old files will be preserved)"); | ||
| } | ||
| console.log(); | ||
|
|
||
| try { | ||
| console.log("Starting migration..."); | ||
| await StorageMigration.migrateStorageDirectory(storageDir, cleanupMode); | ||
| console.log(); | ||
| console.log("✓ Migration completed successfully!"); | ||
| console.log(); | ||
|
|
||
| if (!cleanupMode) { | ||
| console.log("IMPORTANT: Old format files have been preserved for safety."); | ||
| console.log("After verifying your setup works correctly, you can clean them up with:"); | ||
| console.log(` node migrate-storage.js --cleanup "${storageDir}"`); | ||
| } else { |
There was a problem hiding this comment.
The usage/help text and the follow-up instruction print node migrate-storage.js ..., but the file is tools/migrate-storage.mjs. This will be confusing and the suggested command won’t work as written. Update the help/output strings to reference the actual filename/path (and ideally mention that dist/ must be built if running from source).
MIGRATION.md
Outdated
| node tools/migrate-storage.mjs [path-to-storage-directory] | ||
|
|
||
| # Default location (if no path specified): | ||
| # ./node-persist/storage |
There was a problem hiding this comment.
The “Default location” comment says ./node-persist/storage, but the rest of the guide (and the tool) refers to .node-persist/storage. Please fix the path so users don’t migrate the wrong directory.
| # ./node-persist/storage | |
| # .node-persist/storage |
| // Run migration once after initialization (only for node-persist) | ||
| if (!HAPStorage.migrationComplete) { | ||
| HAPStorage.migrationComplete = true; | ||
| const storageDir = this.customStoragePath || path.join(process.cwd(), ".node-persist/storage"); | ||
| StorageMigration.migrateStorageDirectory(storageDir, HAPStorage.cleanupOldFiles).catch(err => { | ||
| console.error("Storage migration failed:", err); | ||
| }); | ||
| } |
There was a problem hiding this comment.
StorageMigration.migrateStorageDirectory(...) is started in the background and not awaited. On first startup after upgrade, getItem() calls (e.g., AccessoryInfo.load) can execute before migration creates the new SHA256 files, causing reads to return undefined and potentially triggering new pairing / data loss. Consider running migration synchronously before serving the first getItem (e.g., perform migration before returning the adapter, or make initialization async and await it, or implement a fallback read path that can read legacy formats until migration completes).
|
Happy to see outdated dependencies removed. However this PR looks Just to be sure: this PR only affects the In manual migration / rehosting scenarios I copy over the files per child bridge. How will I recognise which files are for which child bridge if the username no longer is present in the name? |
|
@copilot Pls respond to - #1109 (comment) |
… improve cleanup, fix CLI paths Addressed all code review feedback from copilot-pull-request-reviewer: 1. Migration safeguards: - Skip migration if v4 file already exists (prevents overwriting newer data) - Apply to both migrateFile and migrateV2File methods 2. Falsy value handling: - Use proper null checks instead of || operator - Prevents mis-handling of 0, false, "", null values 3. Cleanup improvements: - Only delete v0 (.json files) and v2 (32-char MD5) files - Skip v4 files (64-char SHA256) and unrelated files - Prevents accidental deletion of system files 4. CLI tool fixes: - Removed unused __dirname variable - Fixed command references (migrate-storage.js → tools/migrate-storage.mjs) - Added note about running npm run build first 5. Documentation fixes: - Fixed path typo (./node-persist → .node-persist) 6. Backward compatibility: - Updated NodePersistStorageAdapter to use detectAndRead fallback - Ensures legacy format files can be read even before migration completes - Solves race condition on first startup All 777 tests passing, linting passes. Co-authored-by: NorthernMan54 <[email protected]>
Correct - this PR only affects files in the
The For recognizing files per child bridge after migration:
Example: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Pull Request Test Coverage Report for Build 21890511899Details
💛 - Coveralls |
|
@copilot For the problem #1109 (review) - Pls mark this as okay |
All code review issues from copilot-pull-request-reviewer have been addressed in commit 78f7a60: ✅ Migration safeguards - v4 file existence checks added All 777 tests passing, linting clean. The review issues have been resolved. |
Upgrade node-persist to 4.0.4 with migration strategy and custom storage support ✅
This PR successfully upgrades
node-persistfrom0.0.12to4.0.4to remove the deprecatedqpackage, with a comprehensive migration strategy to handle file format changes and adds custom storage support via dependency injection.✅ All Requirements Implemented + Code Review Fixes
🔧 Code Review Fixes (Latest Commit)
Migration Safeguards:
Falsy Value Handling:
||operatorCleanup Improvements:
CLI Tool Fixes:
Backward Compatibility:
🔄 Migration Strategy
Automatic Migration (Default)
Manual Migration Tool
Custom Storage (New in v3.0.0)
Backward Compatibility
📋 File Format Changes
AccessoryInfo.CC223DE3CEF3.jsonbced6ef4c848bd19...(64 chars)🎯 What's New
New Files:
src/lib/model/StorageMigration.ts- Migration engine with format detectionsrc/lib/model/StorageMigration.spec.ts- 19 comprehensive testssrc/lib/model/StorageInterface.ts- Interface for custom storage implementationssrc/lib/model/HAPStorage-CustomStorage.spec.ts- 3 custom storage teststools/migrate-storage.mjs- CLI migration tool with --cleanup flag supportMIGRATION.md- Complete migration documentation (9000+ chars)Modified Files:
src/lib/model/HAPStorage.ts- Added custom storage support, migration integration, detectAndRead fallbacksrc/lib/model/HAPStorage.spec.ts- Updated for StorageInterface return typesrc/lib/model/AccessoryInfo.ts- Updated to async storage APIsrc/lib/model/IdentifierCache.ts- Updated to async storage APIsrc/lib/model/ControllerStorage.ts- Updated to async storage APIsrc/lib/Accessory.ts- Updated to await async storage operationssrc/index.ts- Exports StorageMigration and StorageInterfacepackage.json- v3.0.0, node-persist@^4.0.4README.md- Migration notice with cleanup info✅ Benefits
🧪 Testing Coverage
Tests (22 new, 777 total):
📚 Documentation
See MIGRATION.md for:
📊 Stats
🚀 Ready for Production
All requirements met and code review feedback addressed:
This major release modernizes storage, removes deprecated dependencies, adds flexibility for advanced users, and has been thoroughly reviewed and tested!
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.