Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate data from umami old version to new version #2215

Conversation

ajinkyaraj-23
Copy link
Collaborator

Proposed changes

(Specific to umami desktop version)
Due to addition of custom protocol in apps/desktop/public/electron.js, the localstorage key structure changed from file:// to app://assets thus the data of accounts etc. was not being visible in the upgraded version (2.3.4)

Added a logic to migrate the existing data from old key to new keys in leveldb.

Types of changes

  • Bugfix
  • New feature
  • Refactor
  • Breaking change
  • UI fix

Steps to reproduce

Upgrade to 2.3.4 and data from previous version disappers.

| Before | Now |

To test build a production grade packaged app and replace it over existing 2.3.3 version app. The data should be visible.

Checklist

  • Tests that prove my fix is effective or that my feature works have been added
  • Documentation has been added (if appropriate)
  • Screenshots are added (if any UI changes have been made)
  • All TODOs have a corresponding task created (and the link is attached to it)

Copy link

vercel bot commented Dec 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
umami-embed-iframe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 3:10pm
umami-embed-iframe-ghostnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 3:10pm
umami-v2-web 🛑 Canceled (Inspect) Jan 9, 2025 3:10pm
umami-v2-web-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 3:10pm

@@ -223,7 +247,10 @@ function start() {
// This method will be called when Electron has finished its initialization and
// is ready to create the browser windows.
// Some APIs can only be used after this event occurs.
app.whenReady().then(createWindow);
app.whenReady().then(async () => {
await readAndCopyValues();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called every time the app is open?
Would it be possible to check if the user data is absent & only then copy it (so readAndCopyValues() only runs once)?

Will log out work properly?
We should make sure to clear both dbs on logout to avoid repopulating db with the old data

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now its called everytime the app is open. I believe that will rewrite old data again and again. I can change it like follows:
once the data is copied i can delete old data. Then there is no problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider creating a backup for old data. This allows for recovery in case the migration encounters unforeseen issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once the data is copied i can delete old data. Then there is no problem?

Good idea, @ajinkyaraj-23
However, migrating back to the older version would result in no user data - we should check with Aswin if that's something we're OK with

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right that's a good question.

async function readAndCopyValues() {
try {
const accountsValue = await db.get("_file://\x00\x01persist:accounts");
const rootValue = await db.get("_file://\x00\x01persist:root");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should migrate all the data present in the existing (old) DB, not just root and accounts

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?
following are the keys which are generated:

Key: b'_devtools://devtools\x00\x01experiments', Value : b'\x01{}'
Key: b'_devtools://devtools\x00\x01localInspectorVersion', Value : b'\x0137'
Key: b'_file://\x00\x01beacon:matrix-selected-node', Value : b'\x01beacon-node-1.hope-5.papers.tech'
Key: b'_file://\x00\x01beacon:sdk-matrix-preserved-state', Value : b'\x01{"syncToken":"s44195095_1_0_1_1_1_1_2556620_1","rooms":{}}'
Key: b'_file://\x00\x01beacon:sdk-secret-seed', Value : b'\x019eca6480-5621-54a9-7c26-9d5cc062bc9f'
Key: b'_file://\x00\x01beacon:sdk_version', Value : b'\x014.2.2'
Key: b'_file://\x00\x01chakra-ui-color-mode', Value : b'\x01dark'
Key: b'_file://\x00\x01loglevel:broadcast-channel', Value : b'\x01ERROR'
Key: b'_file://\x00\x01loglevel:customauth', Value : b'\x01SILENT'
Key: b'_file://\x00\x01loglevel:fnd', Value : b'\x01SILENT'
Key: b'_file://\x00\x01loglevel:http-helpers', Value : b'\x01INFO'
Key: b'_file://\x00\x01loglevel:torus.js', Value : b'\x01SILENT'
Key: b'_file://\x00\x01persist:accounts'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the same functionality as the previous version and lose nothing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only backup these two keys in backup so i guess, this is good enough, other keys need not be copied and we can still keep the same functionality. Rather trying to copy everything causes more errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about Key: b'_file://\x00\x01loglevel:customauth', Value : b'\x01SILENT' - I believe that's dApp connections, they should be preserved on update I think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will be enough for this fix, but any other flags or keys on the client side will be lost

@OKendigelyan OKendigelyan force-pushed the 2195-updating-umami-should-not-lead-to-re-iimporting-all-my-accounts branch from b8779cc to 570c4d8 Compare December 9, 2024 13:08
@ajinkyaraj-23 ajinkyaraj-23 force-pushed the 2195-updating-umami-should-not-lead-to-re-iimporting-all-my-accounts branch from 570c4d8 to 5e7b427 Compare December 11, 2024 23:18
Copy link
Contributor

@asiia-trilitech asiia-trilitech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @OKendigelyan & @ajinkyaraj-23 - amazing work 💪 💪 💪

LGTM - but please check the comments first

"@taquito/signer": "^20.1.2",
"@taquito/taquito": "^20.1.2",
"@taquito/utils": "^20.1.2",
"@taquito/ledger-signer": "^21.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check ledger on Win, Linux & Mac before publishing this release just in case

@@ -13,7 +13,8 @@
"isolatedModules": true,
"noEmit": true,
"jsx": "react-jsx",
"customConditions": ["@umami/source"]
"customConditions": ["@umami/source"],
"preserveSymlinks": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed?

async function createBackupFromPrevDB() {
const dbPath = path.normalize(path.join(app.getPath("userData"), "Local Storage", "leveldb"));
const backupPath = path.normalize(
path.join(app.getPath("userData"), "Local Storage", "backup_leveldb.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe umami_backup_leveldb.json ?

@@ -20,6 +20,10 @@ export const accountsInitialState: AccountsState = {
seedPhrases: {},
secretKeys: {},
password: "",
alerts: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if alerts should be accounts slice, but happy for it to stay here for now.

We can do migration to a different / separate slice later if needed

@@ -144,6 +148,15 @@ export const accountsSlice = createSlice({
setPassword: (state, { payload }: { payload: string }) => {
state.password = payload;
},
setAlerts: (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you please add tests for this?

@@ -16,6 +16,13 @@ export const logout = (persistor: Persistor) =>
.catch(() => {})
.finally(() => {
persistor.pause();
const migrationCompleted = localStorage.getItem("migration_2_3_3_to_2_3_4_completed");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments on migration flag setup here

@@ -80,6 +80,7 @@ export const restoreV2BackupFile = async (
persistor.pause();

localStorage.clear();
localStorage.setItem("migration_2_3_3_to_2_3_4_completed", "true");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe migration_to_2_3_5_completed?

app.whenReady().then(async () => {
// Execute createBackupFromPrevDB at the beginning
try {
await createBackupFromPrevDB();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do somewhat of a version check - like if it's a migration from 2.4 & earlier, then do the backup

To avoid creating backup on the 2.6+ versions when not needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed offline with @OKendigelyan - there is no simple way to check the version


ipcMain.handle("getBackupData", () => {
const backupPath = path.normalize(
path.join(app.getPath("userData"), "Local Storage", "backup_leveldb.json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe this should be a separate function used in both getBackup & createBackup?

return;
}

if (fs.existsSync(backupPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some security-related concerns regarding not deleting the backup file after we already used it.

Discussed offline with @OKendigelyan - seems like there is no simple solution for this.

If we're planning to remove this data migration bit in the nearest future (& clean up the backup with it 🙂 ) - should be OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating Umami should not lead to re-iimporting all my accounts
4 participants