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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

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 Dec 19, 2024 8:59pm
umami-embed-iframe-ghostnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 8:59pm
umami-v2-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 8:59pm
umami-v2-web-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 8:59pm

@@ -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
try {
await createBackupFromPrevDB();
createWindow();
} catch (error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about putting createWindow outside the try and catch block. we dont want people to encouter error if the restoration of backup fails. We just want them to see clear welcome screen.

@OKendigelyan OKendigelyan force-pushed the 2195-updating-umami-should-not-lead-to-re-iimporting-all-my-accounts branch from 9b9a783 to 2c3ded3 Compare December 19, 2024 14:56
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