AccountSettings: allow to create new instances during migrations (#…

…1195)

AccountSettings: allow to create new instances during migrations

app/src/androidTest/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration17Test.kt

@@ -18,7 +18,6 @@ import at.bitfire.davdroid.sync.account.setAndVerifyUserData
 import dagger.hilt.android.qualifiers.ApplicationContext
 import dagger.hilt.android.testing.HiltAndroidRule
 import dagger.hilt.android.testing.HiltAndroidTest
-import io.mockk.mockk
 import okhttp3.HttpUrl.Companion.toHttpUrl
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
@@ -83,7 +82,7 @@ class AccountSettingsMigration17Test {
                 )
 
                 // run migration
-                migration.migrate(account, mockk())
+                migration.migrate(account)
 
                 // migration renames address book, update account
                 addressBookAccount = accountManager.getAccountsByType(addressBookAccountType).filter {

app/src/androidTest/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration18Test.kt

@@ -66,7 +66,7 @@ class AccountSettingsMigration18Test {
         every { accountManager.getUserData(addressBookAccount, LocalAddressBook.USER_DATA_COLLECTION_ID) } returns "123"
 
         val account = Account("test", "test")
-        migration.migrate(account, mockk())
+        migration.migrate(account)
 
         verify(exactly = 0) {
             accountManager.setUserData(addressBookAccount, any(), any())
@@ -88,7 +88,7 @@ class AccountSettingsMigration18Test {
         every { accountManager.getUserData(addressBookAccount, LocalAddressBook.USER_DATA_COLLECTION_ID) } returns "123"
 
         val account = Account("test", "test")
-        migration.migrate(account, mockk())
+        migration.migrate(account)
 
         verify(exactly = 0) {
             accountManager.setUserData(addressBookAccount, any(), any())
@@ -124,7 +124,7 @@ class AccountSettingsMigration18Test {
         every { accountManager.getAccountsByType(addressBookAccountType) } returns arrayOf(addressBookAccount)
         every { accountManager.getUserData(addressBookAccount, LocalAddressBook.USER_DATA_COLLECTION_ID) } returns "100"
 
-        migration.migrate(account, mockk())
+        migration.migrate(account)
 
         verify {
             accountManager.setUserData(addressBookAccount, LocalAddressBook.USER_DATA_ACCOUNT_NAME, account.name)

app/src/main/kotlin/at/bitfire/davdroid/settings/AccountSettings.kt

@@ -26,6 +26,7 @@ import dagger.assisted.AssistedFactory
 import dagger.assisted.AssistedInject
 import dagger.hilt.android.qualifiers.ApplicationContext
 import net.openid.appauth.AuthState
+import java.util.Collections
 import java.util.logging.Level
 import java.util.logging.Logger
 import javax.inject.Provider
@@ -79,26 +80,25 @@ class AccountSettings @AssistedInject constructor(
             throw IllegalArgumentException("Invalid account type for AccountSettings(): ${account.type}")
 
         // synchronize because account migration must only be run one time
-        synchronized(AccountSettings::class.java) {
-            val versionStr = accountManager.getUserData(account, KEY_SETTINGS_VERSION) ?: throw InvalidAccountException(account)
-            var version = 0
-            try {
-                version = Integer.parseInt(versionStr)
-            } catch (e: NumberFormatException) {
-                logger.log(Level.SEVERE, "Invalid account version: $versionStr", e)
-            }
-            logger.fine("Account ${account.name} has version $version, current version: $CURRENT_VERSION")
-
-            if (version < CURRENT_VERSION) {
-                if (currentlyUpdating) {
-                    logger.severe("Redundant call: migration created AccountSettings(). This must never happen.")
-                    throw IllegalStateException("Redundant call: migration created AccountSettings()")
-                } else {
-                    currentlyUpdating = true
+        synchronized(currentlyUpdating) {
+            if (currentlyUpdating.contains(account))
+                logger.warning("AccountSettings created during migration of $account – not running update()")
+            else {
+                val versionStr = accountManager.getUserData(account, KEY_SETTINGS_VERSION) ?: throw InvalidAccountException(account)
+                var version = 0
+                try {
+                    version = Integer.parseInt(versionStr)
+                } catch (e: NumberFormatException) {
+                    logger.log(Level.SEVERE, "Invalid account version: $versionStr", e)
+                }
+                logger.fine("Account ${account.name} has version $version, current version: $CURRENT_VERSION")
+
+                if (version < CURRENT_VERSION) {
+                    currentlyUpdating += account
                     try {
                         update(version, abortOnMissingMigration)
                     } finally {
-                        currentlyUpdating = false
+                        currentlyUpdating -= account
                     }
                 }
             }
@@ -362,7 +362,7 @@ class AccountSettings @AssistedInject constructor(
                     throw IllegalArgumentException("Missing AccountSettings migration $fromVersion → $toVersion")
             } else {
                 try {
-                    migration.get().migrate(account, this)
+                    migration.get().migrate(account)
 
                     logger.info("Account settings version update to $toVersion successful")
                     accountManager.setAndVerifyUserData(account, KEY_SETTINGS_VERSION, toVersion.toString())
@@ -373,6 +373,7 @@ class AccountSettings @AssistedInject constructor(
         }
     }
 
+
     companion object {
 
         const val CURRENT_VERSION = 18
@@ -433,10 +434,8 @@ class AccountSettings @AssistedInject constructor(
 
         const val SYNC_INTERVAL_MANUALLY = -1L
 
-        /** Static property to indicate whether AccountSettings migration is currently running.
-         * **Access must be `synchronized` with `AccountSettings::class.java`.** */
-        @Volatile
-        var currentlyUpdating = false
+        /** Static property to remember which AccountSettings updates/migrations are currently running */
+        val currentlyUpdating = Collections.synchronizedSet(mutableSetOf<Account>())
 
         fun initialUserData(credentials: Credentials?): Bundle {
             val bundle = Bundle()

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration.kt

@@ -12,17 +12,14 @@ interface AccountSettingsMigration {
     /**
      * Migrate the account settings from the old version to the new version.
      *
-     * The target version number is registered in the Hilt module as [Int] key of the multi-binding of [AccountSettings].
-     *
-     * @param   account          The account to migrate.
-     * @param   accountSettings  The account settings object that initiated the migration.
-     *
-     * This method _must not_ create [AccountSettings] itself! This would cause an infinite loop. Use [accountSettings] instead.
+     * **The new (target) version number is registered in the Hilt module as [Int] key of the multi-binding of [AccountSettings].**
      *
      * This method should depend on current architecture of [AccountSettings] as little as possible. Methods of [AccountSettings]
      * may change in future and it shouldn't be necessary to change migrations as well. So it's better to operate "low-level"
      * directly on the account user-data – which is also better testable.
+     *
+     * @param   account          The account to migrate
      */
-    fun migrate(account: Account, accountSettings: AccountSettings)
+    fun migrate(account: Account)
 
 }

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration10.kt

@@ -11,7 +11,6 @@ import android.content.pm.PackageManager
 import android.provider.CalendarContract
 import androidx.core.content.ContextCompat
 import at.bitfire.davdroid.resource.LocalTask
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.ical4android.AndroidCalendar
 import at.bitfire.ical4android.TaskProvider
 import at.techbee.jtx.JtxContract.asSyncAdapter
@@ -36,7 +35,7 @@ class AccountSettingsMigration10 @Inject constructor(
     @ApplicationContext private val context: Context
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         TaskProvider.acquire(context, TaskProvider.ProviderName.OpenTasks)?.use { provider ->
             val tasksUri = provider.tasksUri().asSyncAdapter(account)
             val emptyETag = ContentValues(1)

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration11.kt

@@ -24,17 +24,17 @@ import javax.inject.Inject
  * again when the tasks provider is switched.
  */
 class AccountSettingsMigration11 @Inject constructor(
+    private val accountSettingsFactory: AccountSettings.Factory,
     @ApplicationContext private val context: Context,
     private val tasksAppManager: TasksAppManager
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         val accountManager: AccountManager = AccountManager.get(context)
         tasksAppManager.currentProvider()?.let { provider ->
-            val interval = accountSettings.getSyncInterval(provider.authority)
+            val interval = accountSettingsFactory.create(account).getSyncInterval(provider.authority)
             if (interval != null)
-                accountManager.setAndVerifyUserData(account,
-                    AccountSettings.KEY_SYNC_INTERVAL_TASKS, interval.toString())
+                accountManager.setAndVerifyUserData(account, AccountSettings.KEY_SYNC_INTERVAL_TASKS, interval.toString())
         }
     }
 

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration12.kt

@@ -12,7 +12,6 @@ import android.content.pm.PackageManager
 import android.provider.CalendarContract
 import android.util.Base64
 import androidx.core.content.ContextCompat
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.ical4android.AndroidEvent
 import at.bitfire.ical4android.UnknownProperty
 import at.techbee.jtx.JtxContract.asSyncAdapter
@@ -41,7 +40,7 @@ class AccountSettingsMigration12 @Inject constructor(
     private val logger: Logger
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         if (ContextCompat.checkSelfPermission(context, android.Manifest.permission.WRITE_CALENDAR) == PackageManager.PERMISSION_GRANTED) {
             context.contentResolver.acquireContentProviderClient(CalendarContract.AUTHORITY)?.use { provider ->
                 // Attention: CalendarProvider does NOT limit the results of the ExtendedProperties query

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration13.kt

@@ -7,7 +7,6 @@ package at.bitfire.davdroid.settings.migration
 import android.accounts.Account
 import android.content.Context
 import androidx.preference.PreferenceManager
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.settings.Settings
 import dagger.Binds
 import dagger.Module
@@ -28,7 +27,7 @@ class AccountSettingsMigration13 @Inject constructor(
     @ApplicationContext private val context: Context
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         // proxy settings are managed by SharedPreferencesProvider
         val preferences = PreferenceManager.getDefaultSharedPreferences(context)
 

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration14.kt

@@ -26,11 +26,12 @@ import javax.inject.Inject
  * corresponding PeriodicSyncWorkers
  */
 class AccountSettingsMigration14 @Inject constructor(
+    private val accountSettingsFactory: AccountSettings.Factory,
     @ApplicationContext private val context: Context,
     private val logger: Logger
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         // Cancel any potentially running syncs for this account (sync framework)
         ContentResolver.cancelSync(account, null)
 
@@ -42,6 +43,7 @@ class AccountSettingsMigration14 @Inject constructor(
             TaskProvider.ProviderName.TasksOrg.authority
         )
 
+        val accountSettings = accountSettingsFactory.create(account)
         for (authority in authorities) {
             // Enable PeriodicSyncWorker (WorkManager), with known intervals
             enableWorkManager(account, authority, accountSettings)

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration15.kt

@@ -22,11 +22,13 @@ import javax.inject.Inject
  * the new [BaseSyncWorker.exists] and [at.bitfire.davdroid.ui.AccountsActivity.Model].
  */
 class AccountSettingsMigration15 @Inject constructor(
+    private val accountSettingsFactory: AccountSettings.Factory,
     private val syncWorkerManager: SyncWorkerManager
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         for (authority in syncWorkerManager.syncAuthorities()) {
+            val accountSettings = accountSettingsFactory.create(account)
             val interval = accountSettings.getSyncInterval(authority)
             accountSettings.setSyncInterval(authority, interval ?: AccountSettings.SYNC_INTERVAL_MANUALLY)
         }

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration16.kt

@@ -25,12 +25,14 @@ import javax.inject.Inject
  * name and no new workers were enqueued). Here we enqueue all periodic sync workers again with the correct class name.
  */
 class AccountSettingsMigration16 @Inject constructor(
+    private val accountSettingsFactory: AccountSettings.Factory,
     @ApplicationContext private val context: Context,
     private val logger: Logger,
     private val syncWorkerManager: SyncWorkerManager
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
+        val accountSettings = accountSettingsFactory.create(account)
         for (authority in syncWorkerManager.syncAuthorities()) {
             logger.info("Re-enqueuing periodic sync workers for $account/$authority, if necessary")
 

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration17.kt

@@ -14,7 +14,6 @@ import at.bitfire.davdroid.repository.DavCollectionRepository
 import at.bitfire.davdroid.repository.DavServiceRepository
 import at.bitfire.davdroid.resource.LocalAddressBook
 import at.bitfire.davdroid.resource.LocalAddressBookStore
-import at.bitfire.davdroid.settings.AccountSettings
 import dagger.Binds
 import dagger.Module
 import dagger.hilt.InstallIn
@@ -40,7 +39,7 @@ class AccountSettingsMigration17 @Inject constructor(
     private val serviceRepository: DavServiceRepository
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         val addressBookAccountType = context.getString(R.string.account_type_address_book)
         try {
             context.contentResolver.acquireContentProviderClient(ContactsContract.AUTHORITY)

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration18.kt

@@ -11,7 +11,6 @@ import at.bitfire.davdroid.R
 import at.bitfire.davdroid.db.AppDatabase
 import at.bitfire.davdroid.db.Service
 import at.bitfire.davdroid.resource.LocalAddressBook
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.sync.account.setAndVerifyUserData
 import dagger.Binds
 import dagger.Module
@@ -38,7 +37,7 @@ class AccountSettingsMigration18 @Inject constructor(
     private val db: AppDatabase
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         val accountManager = AccountManager.get(context)
         db.serviceDao().getByAccountAndType(account.name, Service.TYPE_CARDDAV)?.let { service ->
             db.collectionDao().getByService(service.id).forEach { collection ->

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration7.kt

@@ -25,7 +25,7 @@ class AccountSettingsMigration7 @Inject constructor(
     @ApplicationContext private val context: Context
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         // add calendar colors
         context.contentResolver.acquireContentProviderClient(CalendarContract.AUTHORITY)?.use { provider ->
             AndroidCalendar.insertColors(provider, account)

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration8.kt

@@ -8,7 +8,6 @@ import android.accounts.Account
 import android.content.ContentUris
 import android.content.ContentValues
 import android.content.Context
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.ical4android.TaskProvider
 import at.techbee.jtx.JtxContract.asSyncAdapter
 import dagger.Binds
@@ -33,7 +32,7 @@ class AccountSettingsMigration8 @Inject constructor(
      * There is a mistake in this method. [TaskContract.Tasks.SYNC_VERSION] is used to store the
      * SEQUENCE and should not be used for the eTag.
      */
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         TaskProvider.acquire(context, TaskProvider.ProviderName.OpenTasks)?.use { provider ->
             // ETag is now in sync_version instead of sync1
             // UID  is now in _uid         instead of sync2

app/src/main/kotlin/at/bitfire/davdroid/settings/migration/AccountSettingsMigration9.kt

@@ -8,7 +8,6 @@ import android.accounts.Account
 import android.content.ContentResolver
 import at.bitfire.davdroid.db.AppDatabase
 import at.bitfire.davdroid.db.Service
-import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.ical4android.TaskProvider
 import dagger.Binds
 import dagger.Module
@@ -28,7 +27,7 @@ class AccountSettingsMigration9 @Inject constructor(
     private val logger: Logger
 ): AccountSettingsMigration {
 
-    override fun migrate(account: Account, accountSettings: AccountSettings) {
+    override fun migrate(account: Account) {
         val hasCalDAV = db.serviceDao().getByAccountAndType(account.name, Service.TYPE_CALDAV) != null
         if (!hasCalDAV && ContentResolver.getIsSyncable(account, TaskProvider.ProviderName.OpenTasks.authority) != 0) {
             logger.info("Disabling OpenTasks sync for $account")