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

fix: keys migration (document deserialization error) #624

Closed
wants to merge 1 commit into from

Conversation

mikeplotean
Copy link
Contributor

@mikeplotean mikeplotean commented Jul 16, 2024

Description

Introduces keys migration to convert jwk keys documents to new format.

Fixes #625

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • Do we need to implement analytics?
  • Will this be part of a product update? If yes, please write one phrase about this update.
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link

@waltkb
Copy link
Contributor

waltkb commented Jul 18, 2024

Shouldn't it maybe only be executed on demand?

@mikeplotean
Copy link
Contributor Author

Shouldn't it maybe only be executed on demand?

it's executed only on data matching the criteria (in this case, jwk keyType having the jwk field as a string). what do you mean by demand?

@mikeplotean mikeplotean force-pushed the fix/keys-migration branch 2 times, most recently from ee19728 to bdcbb99 Compare August 5, 2024 18:42
@waltkb
Copy link
Contributor

waltkb commented Aug 7, 2024

WalletKeys.selectAll(), each of which it will do some JSON operation on to check if it's JWK document or JWK object

@mikeplotean mikeplotean force-pushed the fix/keys-migration branch 2 times, most recently from 04f0a50 to 0b5f6ac Compare August 14, 2024 11:14
@mikeplotean mikeplotean force-pushed the fix/keys-migration branch 2 times, most recently from d84b1f0 to 7127add Compare September 2, 2024 15:04
@mikeplotean mikeplotean force-pushed the fix/keys-migration branch 2 times, most recently from b05fd41 to 207732d Compare September 20, 2024 07:58
Copy link

sonarcloud bot commented Sep 24, 2024

@waltkb
Copy link
Contributor

waltkb commented Sep 26, 2024

Shouldn't it maybe only be executed on demand?

Migration system:

  • chronological ordered (by monotonic counter) list of migrating functions, where each of the the functions has a sequential ID (e.g. migration 1, 2, 3, 4, 5, 6, 7)
  • a table that holds the latest run migration id (e.g. latest migration 4)
  • a system that on startup runs the migrations with IDs higher than the latest run migration (e.g. 5, 6, 7), in order

e.g.: (explicit types for reference, most not needed)

  1. migration functions (of course names and other stuff can also be given in more advanced version)
val migrations = listOf<() -> Unit>(
    { /* migration function code, or reference thereto */ }, // migration 1
    { /* migration function code, or reference thereto */ }, // migration 2
    { /* migration function code, or reference thereto */ }, // migration 3
    { /* migration function code, or reference thereto */ }, // migration 4
)
  1. Table: standard exposed table
object Migrations : Table("migrations") {
    val latestRunMigration = integer("latest_run_migration")
    override val primaryKey: PrimaryKey = PrimaryKey(latestRunMigration)
}
  1. simple system
val totalMigrations = migrations.size // list from above

// this function is called on startup, after the database is connected, but before the webserver handles connections (in `init = {` at ServiceInitialization)
fun runNeededMigrations() {
    val lastRunMigration = getLatestRunMigration() // select on Migration table
    if (totalMigrations > lastRunMigration) {
        // additional migrations have to run
        migrations.drop(latestMigration).forEachIndexed { index, migration -> // only run migrations that have not yet been run
            // next migration
            migration.invoke()
            setLastestRunMigration(lastRunMigration + index + 1)
        }
    }
}

Copy link
Contributor

@waltkb waltkb left a comment

Choose a reason for hiding this comment

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

see above: Only run migrations when necessary

@mikeplotean
Copy link
Contributor Author

see above: Only run migrations when necessary

should we implement an in-house data migration tool?
cc @philpotisk

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.

Can't use the default onboarding did to claim credentials
3 participants