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

Feature/bca/migrate rust in db #8405

Merged

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented May 6, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

`

Perform the legcay to rust migration by using a new CryptoDB migeration (MigrateCryptoTo022), instead of in the DI layer.
Main change is that the former code was migrating a Realminstance, but now as it's in a migration it has to do it with a DynamicRealm. I kept the former code as a utility (could be deleted?), but only the dynamic migration is used now.
Some of the database migration test have been updated as the rust migration is now down seamlessly.

Notice that now, reverting from rust to kotlin crypto will fail due to incompatible db. Anyhow such back migration is not technically possible atm due to lack of support in vodozemac. (will fail with Illegal Argument: Provided schema version 21 is less than last set version 22.)

Motivation and context

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@BillCarsonFr BillCarsonFr requested a review from bmarty May 6, 2023 07:58
@BillCarsonFr BillCarsonFr force-pushed the feature/bma/crypto_rust_default branch from bb31a2d to f1036d4 Compare May 8, 2023 13:37
@BillCarsonFr BillCarsonFr force-pushed the feature/bca/migrate_rust_in_db branch from d13521f to 5e28631 Compare May 9, 2023 07:27
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

One comment about the migration, else LGTM

migrateOperation.dynamicExecute(realm, rustDirectory, rustEncryptionConfiguration.getDatabasePassphrase())

// wa can't delete all for now, but we can do some cleaning
realm.schema.get("OlmSessionEntity")?.transform {
Copy link
Member

Choose a reason for hiding this comment

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

I think you must also delete the file OlmSessionEntity to be able to do that, because else it will be added again to the schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an assertion to the test to ensure that entities are removed. But yeah we keep the table for now (albeit empty)

@sonarcloud
Copy link

sonarcloud bot commented May 9, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@BillCarsonFr BillCarsonFr merged commit 8baa485 into feature/bma/crypto_rust_default May 9, 2023
@BillCarsonFr BillCarsonFr deleted the feature/bca/migrate_rust_in_db branch May 9, 2023 13:44
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.

2 participants