From b87bf1f10e631786dfee9203813835595f615292 Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Sun, 23 Feb 2020 22:34:33 +0000 Subject: [PATCH 1/6] Migrations manager Signed-off-by: Chris Narkiewicz --- build.gradle | 3 + .../android/MockSharedPreferences.kt | 104 +++++++ .../android/TestMigrationsManager.kt | 257 ++++++++++++++++++ .../android/TestMockSharedPreferences.kt | 77 ++++++ .../client/account/UserAccountManager.java | 2 + .../com/nextcloud/client/appinfo/AppInfo.java | 2 + .../nextcloud/client/appinfo/AppInfoImpl.java | 5 + .../client/migrations/MigrationError.kt | 24 ++ .../nextcloud/client/migrations/Migrations.kt | 52 ++++ .../client/migrations/MigrationsManager.kt | 72 +++++ .../migrations/MigrationsManagerImpl.kt | 136 +++++++++ .../nextcloud/client/migrations/Package.md | 6 + .../migrations/MockSharedPreferences.kt | 104 +++++++ .../migrations/TestMigrationsManager.kt | 250 +++++++++++++++++ .../migrations/TestMockSharedPreferences.kt | 77 ++++++ 15 files changed, 1171 insertions(+) create mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt create mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt create mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt create mode 100644 src/main/java/com/nextcloud/client/migrations/MigrationError.kt create mode 100644 src/main/java/com/nextcloud/client/migrations/Migrations.kt create mode 100644 src/main/java/com/nextcloud/client/migrations/MigrationsManager.kt create mode 100644 src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt create mode 100644 src/main/java/com/nextcloud/client/migrations/Package.md create mode 100644 src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt create mode 100644 src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt create mode 100644 src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt diff --git a/build.gradle b/build.gradle index 47552006682c..4450d5646d30 100644 --- a/build.gradle +++ b/build.gradle @@ -387,6 +387,9 @@ dependencies { // androidJacocoAnt "org.jacoco:org.jacoco.agent:${jacocoVersion}" implementation "com.github.stateless4j:stateless4j:2.6.0" + testImplementation "org.robolectric:robolectric:4.3.1" + androidTestImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0" + androidTestImplementation "org.mockito:mockito-android:3.3.0" } configurations.all { diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt new file mode 100644 index 000000000000..81835edca952 --- /dev/null +++ b/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt @@ -0,0 +1,104 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations.android + +import android.content.SharedPreferences +import java.lang.UnsupportedOperationException +import java.util.TreeMap + +@Suppress("TooManyFunctions") +class MockSharedPreferences : SharedPreferences { + + class MockEditor(val store: MutableMap) : SharedPreferences.Editor { + + val editorStore: MutableMap = TreeMap() + + override fun clear(): SharedPreferences.Editor = throw UnsupportedOperationException() + + override fun putLong(key: String?, value: Long): SharedPreferences.Editor = + throw UnsupportedOperationException() + + override fun putInt(key: String?, value: Int): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + + override fun remove(key: String?): SharedPreferences.Editor = throw UnsupportedOperationException() + + override fun putBoolean(key: String?, value: Boolean): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + + override fun putStringSet(key: String?, values: MutableSet?): SharedPreferences.Editor { + editorStore.put(key, values?.toMutableSet()) + return this + } + + override fun commit(): Boolean = true + + override fun putFloat(key: String?, value: Float): SharedPreferences.Editor = + throw UnsupportedOperationException() + + override fun apply() = store.putAll(editorStore) + + override fun putString(key: String?, value: String?): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + } + + val store: MutableMap = TreeMap() + + override fun contains(key: String?): Boolean = store.containsKey(key) + override fun getBoolean(key: String?, defValue: Boolean): Boolean = store.getOrDefault(key, defValue) as Boolean + + override fun unregisterOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = throw UnsupportedOperationException() + + override fun getInt(key: String?, defValue: Int): Int = store.getOrDefault(key, defValue) as Int + + override fun getAll(): MutableMap { + throw UnsupportedOperationException() + } + + override fun edit(): SharedPreferences.Editor { + return MockEditor(store) + } + + override fun getLong(key: String?, defValue: Long): Long { + throw UnsupportedOperationException() + } + + override fun getFloat(key: String?, defValue: Float): Float { + throw UnsupportedOperationException() + } + + override fun getStringSet(key: String?, defValues: MutableSet?): MutableSet? { + return store.getOrDefault(key, defValues) as MutableSet? + } + + override fun registerOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = throw UnsupportedOperationException() + + override fun getString(key: String?, defValue: String?): String? = store.getOrDefault(key, defValue) as String? +} diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt new file mode 100644 index 000000000000..e06bd44ee009 --- /dev/null +++ b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt @@ -0,0 +1,257 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations.android + +import androidx.test.annotation.UiThreadTest +import com.nextcloud.client.account.UserAccountManager +import com.nextcloud.client.appinfo.AppInfo +import com.nextcloud.client.core.ManualAsyncRunner +import com.nextcloud.client.migrations.Migrations +import com.nextcloud.client.migrations.MigrationsManager +import com.nextcloud.client.migrations.MigrationsManagerImpl +import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.never +import com.nhaarman.mockitokotlin2.verify +import com.nhaarman.mockitokotlin2.whenever +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.MockitoAnnotations +import java.lang.RuntimeException +import java.util.LinkedHashSet + +@Suppress("FunctionNaming") +class TestMigrationsManager { + + companion object { + const val OLD_APP_VERSION = 41 + const val NEW_APP_VERSION = 42 + } + + lateinit var migrations: List + + @Mock + lateinit var appInfo: AppInfo + + lateinit var migrationsDb: MockSharedPreferences + + @Mock + lateinit var userAccountManager: UserAccountManager + + lateinit var asyncRunner: ManualAsyncRunner + + internal lateinit var migrationsManager: MigrationsManagerImpl + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + val migrationStep1: Runnable = mock() + val migrationStep2: Runnable = mock() + migrations = listOf( + Migrations.Step(0, "first migration", migrationStep1), + Migrations.Step(1, "second migration", migrationStep2) + ) + asyncRunner = ManualAsyncRunner() + migrationsDb = MockSharedPreferences() + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsManager = MigrationsManagerImpl( + appInfo = appInfo, + migrationsDb = migrationsDb, + asyncRunner = asyncRunner, + migrations = migrations + ) + } + + @Test + fun inital_status_is_unknown() { + // GIVEN + // migration manager has not been used yets + + // THEN + // status is not set + assertEquals(MigrationsManager.Status.UNKNOWN, migrationsManager.status.value) + } + + @Test + fun applied_migrations_are_returned_in_order() { + // GIVEN + // some migrations are marked as applied + // migration ids are stored in random order + val storedMigrationIds = LinkedHashSet() + storedMigrationIds.apply { + add("3") + add("0") + add("2") + add("1") + } + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, storedMigrationIds) + + // WHEN + // applied migration ids are retrieved + val ids = migrationsManager.getAppliedMigrations() + + // THEN + // returned list is sorted + assertEquals(ids, ids.sorted()) + } + + @Test + @Suppress("MagicNumber") + fun registering_new_applied_migration_preserves_old_ids() { + // WHEN + // some applied migrations are registered + val appliedMigrationIds = setOf("0", "1", "2") + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, appliedMigrationIds) + + // WHEN + // new set of migration ids are registered + // some ids are added again + migrationsManager.addAppliedMigration(2, 3, 4) + + // THEN + // new ids are appended to set of existing ids + val expectedIds = setOf("0", "1", "2", "3", "4") + assertEquals(expectedIds, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + } + + @Test + @UiThreadTest + fun migrations_are_scheduled_on_background_thread() { + // GIVEN + // migrations can be applied + assertEquals(0, migrationsManager.getAppliedMigrations().size) + + // WHEN + // migration is started + val count = migrationsManager.startMigration() + + // THEN + // all migrations are scheduled on background thread + // single task is scheduled + assertEquals(migrations.size, count) + assertEquals(1, asyncRunner.size) + assertEquals(MigrationsManager.Status.RUNNING, migrationsManager.status.value) + } + + @Test + @UiThreadTest + fun applied_migrations_are_recorded() { + // GIVEN + // no migrations are applied yet + // current app version is newer then last recorded migrated version + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) + + // WHEN + // migration is run + whenever(userAccountManager.migrateUserId()).thenReturn(true) + val count = migrationsManager.startMigration() + assertTrue(asyncRunner.runOne()) + + // THEN + // total migrations count is returned + // migration functions are called + // applied migrations are recorded + // new app version code is recorded + assertEquals(migrations.size, count) + assertEquals(setOf("0", "1"), migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + assertEquals(NEW_APP_VERSION, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION)) + } + + @Test + @UiThreadTest + fun migration_error_is_recorded() { + // GIVEN + // no migrations applied yet + + // WHEN + // migrations are applied + // one migration throws + val lastMigration = migrations.last() + val errorMessage = "error message" + whenever(lastMigration.function.run()).thenThrow(RuntimeException(errorMessage)) + migrationsManager.startMigration() + assertTrue(asyncRunner.runOne()) + + // THEN + // failure is marked in the migration db + // failure message is recorded + // failed migration id is recorded + assertEquals(MigrationsManager.Status.FAILED, migrationsManager.status.value) + assertTrue(migrationsDb.getBoolean(MigrationsManagerImpl.DB_KEY_FAILED, false)) + assertEquals( + errorMessage, + migrationsDb.getString(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ERROR_MESSAGE, "") + ) + assertEquals( + lastMigration.id, + migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ID, -1) + ) + } + + @Test + @UiThreadTest + fun migrations_are_not_run_if_already_run_for_an_app_version() { + // GIVEN + // migrations were already run for the current app version + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, NEW_APP_VERSION) + + // WHEN + // app is migrated again + val migrationCount = migrationsManager.startMigration() + + // THEN + // migration processing is skipped entirely + // status is set to applied + assertEquals(0, migrationCount) + migrations.forEach { + verify(it.function, never()).run() + } + assertEquals(MigrationsManager.Status.APPLIED, migrationsManager.status.value) + } + + @Test + @UiThreadTest + fun new_app_version_is_marked_as_migrated_if_no_new_migrations_are_available() { + // GIVEN + // migrations were applied in previous version + // new version has no new migrations + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) + val applied = migrations.map { it.id.toString() }.toSet() + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, applied) + + // WHEN + // migration is started + val startedCount = migrationsManager.startMigration() + + // THEN + // no new migrations are run + // new version is marked as migrated + assertEquals(0, startedCount) + assertEquals( + NEW_APP_VERSION, + migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, -1) + ) + } +} diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt new file mode 100644 index 000000000000..659eac6a4f79 --- /dev/null +++ b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt @@ -0,0 +1,77 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations.android + +import org.junit.Before +import org.junit.Test +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertTrue + +@Suppress("MagicNumber", "FunctionNaming") +class TestMockSharedPreferences { + + private lateinit var mock: MockSharedPreferences + + @Before + fun setUp() { + mock = MockSharedPreferences() + } + + @Test + fun get_set_string_set() { + val value = setOf("alpha", "bravo", "charlie") + mock.edit().putStringSet("key", value).apply() + val copy = mock.getStringSet("key", mutableSetOf()) + assertNotSame(value, copy) + assertEquals(value, copy) + } + + @Test + fun get_set_int() { + val value = 42 + val editor = mock.edit() + editor.putInt("key", value) + assertEquals(100, mock.getInt("key", 100)) + editor.apply() + assertEquals(42, mock.getInt("key", 100)) + } + + @Test + fun get_set_boolean() { + val value = true + val editor = mock.edit() + editor.putBoolean("key", value) + assertFalse(mock.getBoolean("key", false)) + editor.apply() + assertTrue(mock.getBoolean("key", false)) + } + + @Test + fun get_set_string() { + val value = "a value" + val editor = mock.edit() + editor.putString("key", value) + assertEquals("default", mock.getString("key", "default")) + editor.apply() + assertEquals("a value", mock.getString("key", "default")) + } +} diff --git a/src/main/java/com/nextcloud/client/account/UserAccountManager.java b/src/main/java/com/nextcloud/client/account/UserAccountManager.java index 4f4d35ecc689..1281ec50f50a 100644 --- a/src/main/java/com/nextcloud/client/account/UserAccountManager.java +++ b/src/main/java/com/nextcloud/client/account/UserAccountManager.java @@ -90,6 +90,8 @@ public interface UserAccountManager extends CurrentAccountProvider { /** * Verifies that every account has userId set + * + * @return true if any account was migrated, false if no account was migrated */ boolean migrateUserId(); diff --git a/src/main/java/com/nextcloud/client/appinfo/AppInfo.java b/src/main/java/com/nextcloud/client/appinfo/AppInfo.java index e3588316576a..60d44ea4ab7d 100644 --- a/src/main/java/com/nextcloud/client/appinfo/AppInfo.java +++ b/src/main/java/com/nextcloud/client/appinfo/AppInfo.java @@ -36,6 +36,8 @@ public interface AppInfo { */ String getFormattedVersionCode(); + int getVersionCode(); + boolean isDebugBuild(); String getAppVersion(Context context); diff --git a/src/main/java/com/nextcloud/client/appinfo/AppInfoImpl.java b/src/main/java/com/nextcloud/client/appinfo/AppInfoImpl.java index 75aa07fd605e..5b85e091400f 100644 --- a/src/main/java/com/nextcloud/client/appinfo/AppInfoImpl.java +++ b/src/main/java/com/nextcloud/client/appinfo/AppInfoImpl.java @@ -33,6 +33,11 @@ public String getFormattedVersionCode() { return Integer.toString(BuildConfig.VERSION_CODE); } + @Override + public int getVersionCode() { + return BuildConfig.VERSION_CODE; + } + @Override public boolean isDebugBuild() { return BuildConfig.DEBUG; diff --git a/src/main/java/com/nextcloud/client/migrations/MigrationError.kt b/src/main/java/com/nextcloud/client/migrations/MigrationError.kt new file mode 100644 index 000000000000..fce1abca5d84 --- /dev/null +++ b/src/main/java/com/nextcloud/client/migrations/MigrationError.kt @@ -0,0 +1,24 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +class MigrationError(val id: Int, message: String, cause: Throwable?) : RuntimeException(message, cause) { + constructor(id: Int, message: String) : this(id, message, null) +} diff --git a/src/main/java/com/nextcloud/client/migrations/Migrations.kt b/src/main/java/com/nextcloud/client/migrations/Migrations.kt new file mode 100644 index 000000000000..73ba8a616bba --- /dev/null +++ b/src/main/java/com/nextcloud/client/migrations/Migrations.kt @@ -0,0 +1,52 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import com.nextcloud.client.account.UserAccountManager +import javax.inject.Inject + +/** + * This class collects all migration steps and provides API to supply those + * steps to [MigrationsManager] for execution. + * + * Note to maintainers: put all migration routines here and export collection of + * opaque [Runnable]s via steps property. + */ +class Migrations @Inject constructor( + private val userAccountManager: UserAccountManager +) { + /** + * @param id Step id; id must be unique + * @param description Human readable migration step description + * @param function Migration runnable object + */ + data class Step(val id: Int, val description: String, val function: Runnable) + + /** + * List of migration steps. Those steps will be loaded and run by [MigrationsManager] + */ + val steps: List = listOf( + Step(0, "migrate user id", Runnable { migrateUserId() }) + ).sortedBy { it.id } + + fun migrateUserId() { + userAccountManager.migrateUserId() + } +} diff --git a/src/main/java/com/nextcloud/client/migrations/MigrationsManager.kt b/src/main/java/com/nextcloud/client/migrations/MigrationsManager.kt new file mode 100644 index 000000000000..1b154d093b5d --- /dev/null +++ b/src/main/java/com/nextcloud/client/migrations/MigrationsManager.kt @@ -0,0 +1,72 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import androidx.annotation.MainThread +import androidx.lifecycle.LiveData + +/** + * This component allows starting and monitoring of application state migrations. + * Migrations are intended to upgrade any existing, persisted application state + * after upgrade to new version, similarly to database migrations. + */ +interface MigrationsManager { + + enum class Status { + /** + * Application migration was not evaluated yet. This is the default + * state just after [android.app.Application] start + */ + UNKNOWN, + + /** + * All migrations applied successfully. + */ + APPLIED, + + /** + * Migration in progress. + */ + RUNNING, + + /** + * Migration failed. Application is in undefined state. + */ + FAILED + } + + /** + * Listenable migration progress. + */ + val status: LiveData + + /** + * Starts application state migration. Migrations will be run in background thread. + * Callers can use [status] to monitor migration progress. + * + * Although the migration process is run in background, status is updated + * immediately and can be accessed immediately after start. + * + * @return Number of migration steps enqueued; 0 if no migrations were started. + */ + @Throws(MigrationError::class) + @MainThread + fun startMigration(): Int +} diff --git a/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt b/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt new file mode 100644 index 000000000000..357fbf40857a --- /dev/null +++ b/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt @@ -0,0 +1,136 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import android.content.SharedPreferences +import androidx.annotation.MainThread +import androidx.lifecycle.LiveData +import androidx.lifecycle.MutableLiveData +import com.nextcloud.client.appinfo.AppInfo +import com.nextcloud.client.core.AsyncRunner +import com.nextcloud.client.migrations.MigrationsManager.Status +import java.util.TreeSet + +internal class MigrationsManagerImpl( + private val appInfo: AppInfo, + private val migrationsDb: SharedPreferences, + private val asyncRunner: AsyncRunner, + private val migrations: Collection +) : MigrationsManager { + + companion object { + const val DB_KEY_LAST_MIGRATED_VERSION = "last_migrated_version" + const val DB_KEY_APPLIED_MIGRATIONS = "applied_migrations" + const val DB_KEY_FAILED = "failed" + const val DB_KEY_FAILED_MIGRATION_ID = "failed_migration_id" + const val DB_KEY_FAILED_MIGRATION_ERROR_MESSAGE = "failed_migration_error" + } + + override val status: LiveData + + init { + this.status = MutableLiveData(Status.UNKNOWN) + } + + fun getAppliedMigrations(): List { + val appliedIdsStr: Set = migrationsDb.getStringSet(DB_KEY_APPLIED_MIGRATIONS, null) ?: TreeSet() + return appliedIdsStr.mapNotNull { + try { + it.toInt() + } catch (_: NumberFormatException) { + null + } + }.sorted() + } + + fun addAppliedMigration(vararg migrations: Int) { + val oldApplied = migrationsDb.getStringSet(DB_KEY_APPLIED_MIGRATIONS, null) ?: TreeSet() + val newApplied = TreeSet().apply { + addAll(oldApplied) + addAll(migrations.map { it.toString() }) + } + migrationsDb.edit().putStringSet(DB_KEY_APPLIED_MIGRATIONS, newApplied).apply() + } + + @Throws(MigrationError::class) + @Suppress("ReturnCount") + override fun startMigration(): Int { + + if (migrationsDb.getBoolean(DB_KEY_FAILED, false)) { + (status as MutableLiveData).value = Status.FAILED + return 0 + } + val lastMigratedVersion = migrationsDb.getInt(DB_KEY_LAST_MIGRATED_VERSION, -1) + if (lastMigratedVersion >= appInfo.versionCode) { + (status as MutableLiveData).value = Status.APPLIED + return 0 + } + val applied = getAppliedMigrations() + val toApply = migrations.filter { !applied.contains(it.id) } + if (toApply.isEmpty()) { + onMigrationSuccess() + return 0 + } + (status as MutableLiveData).value = Status.RUNNING + asyncRunner.post( + task = { asyncApplyMigrations(toApply) }, + onResult = { onMigrationSuccess() }, + onError = { onMigrationFailed(it) } + ) + return toApply.size + } + + /** + * This method calls all pending migrations which can execute long-blocking code. + * It should be run in a background thread. + */ + private fun asyncApplyMigrations(migrations: Collection) { + migrations.forEach { + @Suppress("TooGenericExceptionCaught") // migration code is free to throw anything + try { + it.function.run() + } catch (t: Throwable) { + throw MigrationError(id = it.id, message = t.message ?: t.javaClass.simpleName) + } + addAppliedMigration(it.id) + } + } + + @MainThread + private fun onMigrationFailed(error: Throwable) { + val id = when (error) { + is MigrationError -> error.id + else -> -1 + } + migrationsDb + .edit() + .putBoolean(DB_KEY_FAILED, true) + .putString(DB_KEY_FAILED_MIGRATION_ERROR_MESSAGE, error.message) + .putInt(DB_KEY_FAILED_MIGRATION_ID, id) + .apply() + (status as MutableLiveData).value = Status.FAILED + } + + @MainThread + private fun onMigrationSuccess() { + migrationsDb.edit().putInt(DB_KEY_LAST_MIGRATED_VERSION, appInfo.versionCode).apply() + (status as MutableLiveData).value = Status.APPLIED + } +} diff --git a/src/main/java/com/nextcloud/client/migrations/Package.md b/src/main/java/com/nextcloud/client/migrations/Package.md new file mode 100644 index 000000000000..7443659ddc35 --- /dev/null +++ b/src/main/java/com/nextcloud/client/migrations/Package.md @@ -0,0 +1,6 @@ +# Package com.nextcloud.client.migrations + +This package provides utitilies to migrate application state +during version upgrade. + +Migrations are registered upon run so they can be run only once. diff --git a/src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt b/src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt new file mode 100644 index 000000000000..4c1bf8b03876 --- /dev/null +++ b/src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt @@ -0,0 +1,104 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import android.content.SharedPreferences +import java.lang.UnsupportedOperationException +import java.util.TreeMap + +@Suppress("TooManyFunctions") +class MockSharedPreferences : SharedPreferences { + + class MockEditor(val store: MutableMap) : SharedPreferences.Editor { + + val editorStore: MutableMap = TreeMap() + + override fun clear(): SharedPreferences.Editor = throw UnsupportedOperationException() + + override fun putLong(key: String?, value: Long): SharedPreferences.Editor = + throw UnsupportedOperationException() + + override fun putInt(key: String?, value: Int): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + + override fun remove(key: String?): SharedPreferences.Editor = throw UnsupportedOperationException() + + override fun putBoolean(key: String?, value: Boolean): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + + override fun putStringSet(key: String?, values: MutableSet?): SharedPreferences.Editor { + editorStore.put(key, values?.toMutableSet()) + return this + } + + override fun commit(): Boolean = true + + override fun putFloat(key: String?, value: Float): SharedPreferences.Editor = + throw UnsupportedOperationException() + + override fun apply() = store.putAll(editorStore) + + override fun putString(key: String?, value: String?): SharedPreferences.Editor { + editorStore.put(key, value) + return this + } + } + + val store: MutableMap = TreeMap() + + override fun contains(key: String?): Boolean = store.containsKey(key) + override fun getBoolean(key: String?, defValue: Boolean): Boolean = store.getOrDefault(key, defValue) as Boolean + + override fun unregisterOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = throw UnsupportedOperationException() + + override fun getInt(key: String?, defValue: Int): Int = store.getOrDefault(key, defValue) as Int + + override fun getAll(): MutableMap { + throw UnsupportedOperationException() + } + + override fun edit(): SharedPreferences.Editor { + return MockEditor(store) + } + + override fun getLong(key: String?, defValue: Long): Long { + throw UnsupportedOperationException() + } + + override fun getFloat(key: String?, defValue: Float): Float { + throw UnsupportedOperationException() + } + + override fun getStringSet(key: String?, defValues: MutableSet?): MutableSet? { + return store.getOrDefault(key, defValues) as MutableSet? + } + + override fun registerOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = throw UnsupportedOperationException() + + override fun getString(key: String?, defValue: String?): String? = store.getOrDefault(key, defValue) as String? +} diff --git a/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt b/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt new file mode 100644 index 000000000000..89bb2747e3af --- /dev/null +++ b/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt @@ -0,0 +1,250 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import com.nextcloud.client.account.UserAccountManager +import com.nextcloud.client.appinfo.AppInfo +import com.nextcloud.client.core.ManualAsyncRunner +import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.never +import com.nhaarman.mockitokotlin2.verify +import com.nhaarman.mockitokotlin2.whenever +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.MockitoAnnotations +import org.robolectric.RobolectricTestRunner +import java.lang.RuntimeException +import java.util.LinkedHashSet + +@RunWith(RobolectricTestRunner::class) +class TestMigrationsManager { + + companion object { + const val OLD_APP_VERSION = 41 + const val NEW_APP_VERSION = 42 + } + + lateinit var migrations: List + + @Mock + lateinit var appInfo: AppInfo + + lateinit var migrationsDb: MockSharedPreferences + + @Mock + lateinit var userAccountManager: UserAccountManager + + lateinit var asyncRunner: ManualAsyncRunner + + internal lateinit var migrationsManager: MigrationsManagerImpl + + @Before + fun setUp() { + MockitoAnnotations.initMocks(this) + val migrationStep1: Runnable = mock() + val migrationStep2: Runnable = mock() + migrations = listOf( + Migrations.Step(0, "first migration", migrationStep1), + Migrations.Step(1, "second migration", migrationStep2) + ) + asyncRunner = ManualAsyncRunner() + migrationsDb = MockSharedPreferences() + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsManager = MigrationsManagerImpl( + appInfo = appInfo, + migrationsDb = migrationsDb, + asyncRunner = asyncRunner, + migrations = migrations + ) + } + + @Test + fun `inital status is unknown`() { + // GIVEN + // migration manager has not been used yets + + // THEN + // status is not set + assertEquals(MigrationsManager.Status.UNKNOWN, migrationsManager.status.value) + } + + @Test + fun `applied migrations are returned in order`() { + // GIVEN + // some migrations are marked as applied + // migration ids are stored in random order + val storedMigrationIds = LinkedHashSet() + storedMigrationIds.apply { + add("3") + add("0") + add("2") + add("1") + } + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, storedMigrationIds) + + // WHEN + // applied migration ids are retrieved + val ids = migrationsManager.getAppliedMigrations() + + // THEN + // returned list is sorted + assertEquals(ids, ids.sorted()) + } + + @Test + @Suppress("MagicNumber") + fun `registering new applied migration preserves old ids`() { + // WHEN + // some applied migrations are registered + val appliedMigrationIds = setOf("0", "1", "2") + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, appliedMigrationIds) + + // WHEN + // new set of migration ids are registered + // some ids are added again + migrationsManager.addAppliedMigration(2, 3, 4) + + // THEN + // new ids are appended to set of existing ids + val expectedIds = setOf("0", "1", "2", "3", "4") + assertEquals(expectedIds, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + } + + @Test + fun `migrations are scheduled on background thread`() { + // GIVEN + // migrations can be applied + assertEquals(0, migrationsManager.getAppliedMigrations().size) + + // WHEN + // migration is started + val count = migrationsManager.startMigration() + + // THEN + // all migrations are scheduled on background thread + // single task is scheduled + assertEquals(migrations.size, count) + assertEquals(1, asyncRunner.size) + assertEquals(MigrationsManager.Status.RUNNING, migrationsManager.status.value) + } + + @Test + fun `applied migrations are recorded`() { + // GIVEN + // no migrations are applied yet + // current app version is newer then last recorded migrated version + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) + + // WHEN + // migration is run + whenever(userAccountManager.migrateUserId()).thenReturn(true) + val count = migrationsManager.startMigration() + assertTrue(asyncRunner.runOne()) + + // THEN + // total migrations count is returned + // migration functions are called + // applied migrations are recorded + // new app version code is recorded + assertEquals(migrations.size, count) + assertEquals(setOf("0", "1"), migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + assertEquals(NEW_APP_VERSION, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION)) + } + + @Test + fun `migration error is recorded`() { + // GIVEN + // no migrations applied yet + + // WHEN + // migrations are applied + // one migration throws + val lastMigration = migrations.last() + val errorMessage = "error message" + whenever(lastMigration.function.run()).thenThrow(RuntimeException(errorMessage)) + migrationsManager.startMigration() + assertTrue(asyncRunner.runOne()) + + // THEN + // failure is marked in the migration db + // failure message is recorded + // failed migration id is recorded + assertEquals(MigrationsManager.Status.FAILED, migrationsManager.status.value) + assertTrue(migrationsDb.getBoolean(MigrationsManagerImpl.DB_KEY_FAILED, false)) + assertEquals( + errorMessage, + migrationsDb.getString(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ERROR_MESSAGE, "") + ) + assertEquals( + lastMigration.id, + migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ID, -1) + ) + } + + @Test + fun `migrations are not run if already run for an app version`() { + // GIVEN + // migrations were already run for the current app version + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, NEW_APP_VERSION) + + // WHEN + // app is migrated again + val migrationCount = migrationsManager.startMigration() + + // THEN + // migration processing is skipped entirely + // status is set to applied + assertEquals(0, migrationCount) + migrations.forEach { + verify(it.function, never()).run() + } + assertEquals(MigrationsManager.Status.APPLIED, migrationsManager.status.value) + } + + @Test + fun `new app version is marked as migrated if no new migrations are available`() { + // GIVEN + // migrations were applied in previous version + // new version has no new migrations + whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) + val applied = migrations.map { it.id.toString() }.toSet() + migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, applied) + + // WHEN + // migration is started + val startedCount = migrationsManager.startMigration() + + // THEN + // no new migrations are run + // new version is marked as migrated + assertEquals(0, startedCount) + assertEquals( + NEW_APP_VERSION, + migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, -1) + ) + } +} diff --git a/src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt b/src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt new file mode 100644 index 000000000000..d0156b341e0f --- /dev/null +++ b/src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt @@ -0,0 +1,77 @@ +/* + * Nextcloud Android client application + * + * @author Chris Narkiewicz + * Copyright (C) 2020 Chris Narkiewicz + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package com.nextcloud.client.migrations + +import org.junit.Before +import org.junit.Test +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertTrue + +@Suppress("MagicNumber") +class TestMockSharedPreferences { + + private lateinit var mock: MockSharedPreferences + + @Before + fun setUp() { + mock = MockSharedPreferences() + } + + @Test + fun `get set string set`() { + val value = setOf("alpha", "bravo", "charlie") + mock.edit().putStringSet("key", value).apply() + val copy = mock.getStringSet("key", mutableSetOf()) + assertNotSame(value, copy) + assertEquals(value, copy) + } + + @Test + fun `get set int`() { + val value = 42 + val editor = mock.edit() + editor.putInt("key", value) + assertEquals(100, mock.getInt("key", 100)) + editor.apply() + assertEquals(42, mock.getInt("key", 100)) + } + + @Test + fun `get set boolean`() { + val value = true + val editor = mock.edit() + editor.putBoolean("key", value) + assertFalse(mock.getBoolean("key", false)) + editor.apply() + assertTrue(mock.getBoolean("key", false)) + } + + @Test + fun `get set string`() { + val value = "a value" + val editor = mock.edit() + editor.putString("key", value) + assertEquals("default", mock.getString("key", "default")) + editor.apply() + assertEquals("a value", mock.getString("key", "default")) + } +} From 1ab8d98d8510a0168bc4139e47f7758fc56988d2 Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Tue, 3 Mar 2020 15:54:42 +0000 Subject: [PATCH 2/6] Support for optional migrations Optional migrations care not causing migration failure and can be applied again on next application run. Signed-off-by: Chris Narkiewicz --- .../nextcloud/client/migrations/Migrations.kt | 7 +++- .../migrations/MigrationsManagerImpl.kt | 6 ++- .../migrations/TestMigrationsManager.kt | 42 +++++++++++++++++-- 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/nextcloud/client/migrations/Migrations.kt b/src/main/java/com/nextcloud/client/migrations/Migrations.kt index 73ba8a616bba..dce6eac4fc20 100644 --- a/src/main/java/com/nextcloud/client/migrations/Migrations.kt +++ b/src/main/java/com/nextcloud/client/migrations/Migrations.kt @@ -32,18 +32,21 @@ import javax.inject.Inject class Migrations @Inject constructor( private val userAccountManager: UserAccountManager ) { + /** * @param id Step id; id must be unique * @param description Human readable migration step description * @param function Migration runnable object + * @param mandatory If true, failing migration will cause an exception; if false, it will be skipped and repeated + * again on next startup */ - data class Step(val id: Int, val description: String, val function: Runnable) + data class Step(val id: Int, val description: String, val function: Runnable, val mandatory: Boolean = true) /** * List of migration steps. Those steps will be loaded and run by [MigrationsManager] */ val steps: List = listOf( - Step(0, "migrate user id", Runnable { migrateUserId() }) + Step(0, "migrate user id", Runnable { migrateUserId() }, false) ).sortedBy { it.id } fun migrateUserId() { diff --git a/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt b/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt index 357fbf40857a..d29c42df56db 100644 --- a/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt +++ b/src/main/java/com/nextcloud/client/migrations/MigrationsManagerImpl.kt @@ -106,10 +106,12 @@ internal class MigrationsManagerImpl( @Suppress("TooGenericExceptionCaught") // migration code is free to throw anything try { it.function.run() + addAppliedMigration(it.id) } catch (t: Throwable) { - throw MigrationError(id = it.id, message = t.message ?: t.javaClass.simpleName) + if (it.mandatory) { + throw MigrationError(id = it.id, message = t.message ?: t.javaClass.simpleName) + } } - addAppliedMigration(it.id) } } diff --git a/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt b/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt index 89bb2747e3af..9bdf4994599f 100644 --- a/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt +++ b/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt @@ -27,6 +27,7 @@ import com.nhaarman.mockitokotlin2.never import com.nhaarman.mockitokotlin2.verify import com.nhaarman.mockitokotlin2.whenever import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -64,9 +65,11 @@ class TestMigrationsManager { MockitoAnnotations.initMocks(this) val migrationStep1: Runnable = mock() val migrationStep2: Runnable = mock() + val migrationStep3: Runnable = mock() migrations = listOf( - Migrations.Step(0, "first migration", migrationStep1), - Migrations.Step(1, "second migration", migrationStep2) + Migrations.Step(0, "first migration", migrationStep1, true), + Migrations.Step(1, "second migration", migrationStep2, true), + Migrations.Step(2, "third optional migration", migrationStep3, false) ) asyncRunner = ManualAsyncRunner() migrationsDb = MockSharedPreferences() @@ -169,7 +172,8 @@ class TestMigrationsManager { // applied migrations are recorded // new app version code is recorded assertEquals(migrations.size, count) - assertEquals(setOf("0", "1"), migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + val allAppliedIds = migrations.map { it.id.toString() }.toSet() + assertEquals(allAppliedIds, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) assertEquals(NEW_APP_VERSION, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION)) } @@ -181,7 +185,7 @@ class TestMigrationsManager { // WHEN // migrations are applied // one migration throws - val lastMigration = migrations.last() + val lastMigration = migrations.findLast { it.mandatory } ?: throw IllegalStateException("Test fixture error") val errorMessage = "error message" whenever(lastMigration.function.run()).thenThrow(RuntimeException(errorMessage)) migrationsManager.startMigration() @@ -247,4 +251,34 @@ class TestMigrationsManager { migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, -1) ) } + + @Test + fun `optional migration failure does not trigger a migration failure`() { + // GIVEN + // pending migrations + // mandatory migrations are passing + // one migration is optional and fails + val optionalFailingMigration = migrations.first { !it.mandatory } + whenever(optionalFailingMigration.function.run()).thenThrow(RuntimeException()) + + // WHEN + // migration is started + val startedCount = migrationsManager.startMigration() + asyncRunner.runOne() + assertEquals(migrations.size, startedCount) + + // THEN + // mandatory migrations are marked as applied + // optional failed migration is not marked + // no error + // status is applied + // failed migration is available during next migration + val appliedMigrations = migrations.filter { it.mandatory } + .map { it.id.toString() } + .toSet() + assertTrue("Fixture error", appliedMigrations.isNotEmpty()) + assertEquals(appliedMigrations, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) + assertFalse(migrationsDb.getBoolean(MigrationsManagerImpl.DB_KEY_FAILED, false)) + assertEquals(MigrationsManager.Status.APPLIED, migrationsManager.status.value) + } } From 439dd4ff780fe9501be1a066ea1f99d7884aae2a Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Tue, 3 Mar 2020 23:13:21 +0000 Subject: [PATCH 3/6] Removed MigrationManager Robolectric tests Robolectric tests are removed as we don't want to emulate platform. Renamed instrumentation tests to match expected naming. Updated detekt rule to allow human readable test function names in kt files. This naming convention was adopted in test code to enable human-readable test reports, but DEX does not support spaces in names. Underscore _ is a good replacement providnig similar level of readability. Signed-off-by: Chris Narkiewicz --- build.gradle | 1 - detekt.yml | 1 + .../migrations/MigrationsManagerTest.kt} | 30 +- .../migrations/MockSharedPreferences.kt | 0 .../migrations/MockSharedPreferencesTest.kt} | 10 +- .../android/MockSharedPreferences.kt | 104 ------- .../android/TestMigrationsManager.kt | 257 ------------------ .../android/TestMockSharedPreferences.kt | 77 ------ 8 files changed, 23 insertions(+), 457 deletions(-) rename src/{test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt => androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt} (93%) rename src/{test => androidTest}/java/com/nextcloud/client/migrations/MockSharedPreferences.kt (100%) rename src/{test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt => androidTest/java/com/nextcloud/client/migrations/MockSharedPreferencesTest.kt} (93%) delete mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt delete mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt delete mode 100644 src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt diff --git a/build.gradle b/build.gradle index 4450d5646d30..1b94d1b29720 100644 --- a/build.gradle +++ b/build.gradle @@ -387,7 +387,6 @@ dependencies { // androidJacocoAnt "org.jacoco:org.jacoco.agent:${jacocoVersion}" implementation "com.github.stateless4j:stateless4j:2.6.0" - testImplementation "org.robolectric:robolectric:4.3.1" androidTestImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0" androidTestImplementation "org.mockito:mockito-android:3.3.0" } diff --git a/detekt.yml b/detekt.yml index 1835fe1282ae..16a16e86f1f7 100644 --- a/detekt.yml +++ b/detekt.yml @@ -265,6 +265,7 @@ naming: functionPattern: '^([a-z$][a-zA-Z$0-9]*)|(`.*`)$' excludeClassPattern: '$^' ignoreOverridden: true + excludes: "**/*Test.kt" FunctionParameterNaming: active: true parameterPattern: '[a-z][A-Za-z0-9]*' diff --git a/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt b/src/androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt similarity index 93% rename from src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt rename to src/androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt index 9bdf4994599f..6a2e59561a47 100644 --- a/src/test/java/com/nextcloud/client/migrations/TestMigrationsManager.kt +++ b/src/androidTest/java/com/nextcloud/client/migrations/MigrationsManagerTest.kt @@ -19,6 +19,7 @@ */ package com.nextcloud.client.migrations +import androidx.test.annotation.UiThreadTest import com.nextcloud.client.account.UserAccountManager import com.nextcloud.client.appinfo.AppInfo import com.nextcloud.client.core.ManualAsyncRunner @@ -31,15 +32,12 @@ import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test -import org.junit.runner.RunWith import org.mockito.Mock import org.mockito.MockitoAnnotations -import org.robolectric.RobolectricTestRunner import java.lang.RuntimeException import java.util.LinkedHashSet -@RunWith(RobolectricTestRunner::class) -class TestMigrationsManager { +class MigrationsManagerTest { companion object { const val OLD_APP_VERSION = 41 @@ -83,7 +81,7 @@ class TestMigrationsManager { } @Test - fun `inital status is unknown`() { + fun inital_status_is_unknown() { // GIVEN // migration manager has not been used yets @@ -93,7 +91,7 @@ class TestMigrationsManager { } @Test - fun `applied migrations are returned in order`() { + fun applied_migrations_are_returned_in_order() { // GIVEN // some migrations are marked as applied // migration ids are stored in random order @@ -117,7 +115,7 @@ class TestMigrationsManager { @Test @Suppress("MagicNumber") - fun `registering new applied migration preserves old ids`() { + fun registering_new_applied_migration_preserves_old_ids() { // WHEN // some applied migrations are registered val appliedMigrationIds = setOf("0", "1", "2") @@ -135,7 +133,8 @@ class TestMigrationsManager { } @Test - fun `migrations are scheduled on background thread`() { + @UiThreadTest + fun migrations_are_scheduled_on_background_thread() { // GIVEN // migrations can be applied assertEquals(0, migrationsManager.getAppliedMigrations().size) @@ -153,7 +152,8 @@ class TestMigrationsManager { } @Test - fun `applied migrations are recorded`() { + @UiThreadTest + fun applied_migrations_are_recorded() { // GIVEN // no migrations are applied yet // current app version is newer then last recorded migrated version @@ -178,7 +178,8 @@ class TestMigrationsManager { } @Test - fun `migration error is recorded`() { + @UiThreadTest + fun migration_error_is_recorded() { // GIVEN // no migrations applied yet @@ -208,7 +209,8 @@ class TestMigrationsManager { } @Test - fun `migrations are not run if already run for an app version`() { + @UiThreadTest + fun migrations_are_not_run_if_already_run_for_an_app_version() { // GIVEN // migrations were already run for the current app version whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) @@ -229,7 +231,8 @@ class TestMigrationsManager { } @Test - fun `new app version is marked as migrated if no new migrations are available`() { + @UiThreadTest + fun new_app_version_is_marked_as_migrated_if_no_new_migrations_are_available() { // GIVEN // migrations were applied in previous version // new version has no new migrations @@ -253,7 +256,8 @@ class TestMigrationsManager { } @Test - fun `optional migration failure does not trigger a migration failure`() { + @UiThreadTest + fun optional_migration_failure_does_not_trigger_a_migration_failure() { // GIVEN // pending migrations // mandatory migrations are passing diff --git a/src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/MockSharedPreferences.kt similarity index 100% rename from src/test/java/com/nextcloud/client/migrations/MockSharedPreferences.kt rename to src/androidTest/java/com/nextcloud/client/migrations/MockSharedPreferences.kt diff --git a/src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/MockSharedPreferencesTest.kt similarity index 93% rename from src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt rename to src/androidTest/java/com/nextcloud/client/migrations/MockSharedPreferencesTest.kt index d0156b341e0f..0598bcea8c27 100644 --- a/src/test/java/com/nextcloud/client/migrations/TestMockSharedPreferences.kt +++ b/src/androidTest/java/com/nextcloud/client/migrations/MockSharedPreferencesTest.kt @@ -27,7 +27,7 @@ import org.junit.Assert.assertNotSame import org.junit.Assert.assertTrue @Suppress("MagicNumber") -class TestMockSharedPreferences { +class MockSharedPreferencesTest { private lateinit var mock: MockSharedPreferences @@ -37,7 +37,7 @@ class TestMockSharedPreferences { } @Test - fun `get set string set`() { + fun getSetStringSet() { val value = setOf("alpha", "bravo", "charlie") mock.edit().putStringSet("key", value).apply() val copy = mock.getStringSet("key", mutableSetOf()) @@ -46,7 +46,7 @@ class TestMockSharedPreferences { } @Test - fun `get set int`() { + fun getSetInt() { val value = 42 val editor = mock.edit() editor.putInt("key", value) @@ -56,7 +56,7 @@ class TestMockSharedPreferences { } @Test - fun `get set boolean`() { + fun getSetBoolean() { val value = true val editor = mock.edit() editor.putBoolean("key", value) @@ -66,7 +66,7 @@ class TestMockSharedPreferences { } @Test - fun `get set string`() { + fun getSetString() { val value = "a value" val editor = mock.edit() editor.putString("key", value) diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt deleted file mode 100644 index 81835edca952..000000000000 --- a/src/androidTest/java/com/nextcloud/client/migrations/android/MockSharedPreferences.kt +++ /dev/null @@ -1,104 +0,0 @@ -/* - * Nextcloud Android client application - * - * @author Chris Narkiewicz - * Copyright (C) 2020 Chris Narkiewicz - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ -package com.nextcloud.client.migrations.android - -import android.content.SharedPreferences -import java.lang.UnsupportedOperationException -import java.util.TreeMap - -@Suppress("TooManyFunctions") -class MockSharedPreferences : SharedPreferences { - - class MockEditor(val store: MutableMap) : SharedPreferences.Editor { - - val editorStore: MutableMap = TreeMap() - - override fun clear(): SharedPreferences.Editor = throw UnsupportedOperationException() - - override fun putLong(key: String?, value: Long): SharedPreferences.Editor = - throw UnsupportedOperationException() - - override fun putInt(key: String?, value: Int): SharedPreferences.Editor { - editorStore.put(key, value) - return this - } - - override fun remove(key: String?): SharedPreferences.Editor = throw UnsupportedOperationException() - - override fun putBoolean(key: String?, value: Boolean): SharedPreferences.Editor { - editorStore.put(key, value) - return this - } - - override fun putStringSet(key: String?, values: MutableSet?): SharedPreferences.Editor { - editorStore.put(key, values?.toMutableSet()) - return this - } - - override fun commit(): Boolean = true - - override fun putFloat(key: String?, value: Float): SharedPreferences.Editor = - throw UnsupportedOperationException() - - override fun apply() = store.putAll(editorStore) - - override fun putString(key: String?, value: String?): SharedPreferences.Editor { - editorStore.put(key, value) - return this - } - } - - val store: MutableMap = TreeMap() - - override fun contains(key: String?): Boolean = store.containsKey(key) - override fun getBoolean(key: String?, defValue: Boolean): Boolean = store.getOrDefault(key, defValue) as Boolean - - override fun unregisterOnSharedPreferenceChangeListener( - listener: SharedPreferences.OnSharedPreferenceChangeListener? - ) = throw UnsupportedOperationException() - - override fun getInt(key: String?, defValue: Int): Int = store.getOrDefault(key, defValue) as Int - - override fun getAll(): MutableMap { - throw UnsupportedOperationException() - } - - override fun edit(): SharedPreferences.Editor { - return MockEditor(store) - } - - override fun getLong(key: String?, defValue: Long): Long { - throw UnsupportedOperationException() - } - - override fun getFloat(key: String?, defValue: Float): Float { - throw UnsupportedOperationException() - } - - override fun getStringSet(key: String?, defValues: MutableSet?): MutableSet? { - return store.getOrDefault(key, defValues) as MutableSet? - } - - override fun registerOnSharedPreferenceChangeListener( - listener: SharedPreferences.OnSharedPreferenceChangeListener? - ) = throw UnsupportedOperationException() - - override fun getString(key: String?, defValue: String?): String? = store.getOrDefault(key, defValue) as String? -} diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt deleted file mode 100644 index e06bd44ee009..000000000000 --- a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMigrationsManager.kt +++ /dev/null @@ -1,257 +0,0 @@ -/* - * Nextcloud Android client application - * - * @author Chris Narkiewicz - * Copyright (C) 2020 Chris Narkiewicz - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ -package com.nextcloud.client.migrations.android - -import androidx.test.annotation.UiThreadTest -import com.nextcloud.client.account.UserAccountManager -import com.nextcloud.client.appinfo.AppInfo -import com.nextcloud.client.core.ManualAsyncRunner -import com.nextcloud.client.migrations.Migrations -import com.nextcloud.client.migrations.MigrationsManager -import com.nextcloud.client.migrations.MigrationsManagerImpl -import com.nhaarman.mockitokotlin2.mock -import com.nhaarman.mockitokotlin2.never -import com.nhaarman.mockitokotlin2.verify -import com.nhaarman.mockitokotlin2.whenever -import org.junit.Assert.assertEquals -import org.junit.Assert.assertTrue -import org.junit.Before -import org.junit.Test -import org.mockito.Mock -import org.mockito.MockitoAnnotations -import java.lang.RuntimeException -import java.util.LinkedHashSet - -@Suppress("FunctionNaming") -class TestMigrationsManager { - - companion object { - const val OLD_APP_VERSION = 41 - const val NEW_APP_VERSION = 42 - } - - lateinit var migrations: List - - @Mock - lateinit var appInfo: AppInfo - - lateinit var migrationsDb: MockSharedPreferences - - @Mock - lateinit var userAccountManager: UserAccountManager - - lateinit var asyncRunner: ManualAsyncRunner - - internal lateinit var migrationsManager: MigrationsManagerImpl - - @Before - fun setUp() { - MockitoAnnotations.initMocks(this) - val migrationStep1: Runnable = mock() - val migrationStep2: Runnable = mock() - migrations = listOf( - Migrations.Step(0, "first migration", migrationStep1), - Migrations.Step(1, "second migration", migrationStep2) - ) - asyncRunner = ManualAsyncRunner() - migrationsDb = MockSharedPreferences() - whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) - migrationsManager = MigrationsManagerImpl( - appInfo = appInfo, - migrationsDb = migrationsDb, - asyncRunner = asyncRunner, - migrations = migrations - ) - } - - @Test - fun inital_status_is_unknown() { - // GIVEN - // migration manager has not been used yets - - // THEN - // status is not set - assertEquals(MigrationsManager.Status.UNKNOWN, migrationsManager.status.value) - } - - @Test - fun applied_migrations_are_returned_in_order() { - // GIVEN - // some migrations are marked as applied - // migration ids are stored in random order - val storedMigrationIds = LinkedHashSet() - storedMigrationIds.apply { - add("3") - add("0") - add("2") - add("1") - } - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, storedMigrationIds) - - // WHEN - // applied migration ids are retrieved - val ids = migrationsManager.getAppliedMigrations() - - // THEN - // returned list is sorted - assertEquals(ids, ids.sorted()) - } - - @Test - @Suppress("MagicNumber") - fun registering_new_applied_migration_preserves_old_ids() { - // WHEN - // some applied migrations are registered - val appliedMigrationIds = setOf("0", "1", "2") - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, appliedMigrationIds) - - // WHEN - // new set of migration ids are registered - // some ids are added again - migrationsManager.addAppliedMigration(2, 3, 4) - - // THEN - // new ids are appended to set of existing ids - val expectedIds = setOf("0", "1", "2", "3", "4") - assertEquals(expectedIds, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) - } - - @Test - @UiThreadTest - fun migrations_are_scheduled_on_background_thread() { - // GIVEN - // migrations can be applied - assertEquals(0, migrationsManager.getAppliedMigrations().size) - - // WHEN - // migration is started - val count = migrationsManager.startMigration() - - // THEN - // all migrations are scheduled on background thread - // single task is scheduled - assertEquals(migrations.size, count) - assertEquals(1, asyncRunner.size) - assertEquals(MigrationsManager.Status.RUNNING, migrationsManager.status.value) - } - - @Test - @UiThreadTest - fun applied_migrations_are_recorded() { - // GIVEN - // no migrations are applied yet - // current app version is newer then last recorded migrated version - whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) - - // WHEN - // migration is run - whenever(userAccountManager.migrateUserId()).thenReturn(true) - val count = migrationsManager.startMigration() - assertTrue(asyncRunner.runOne()) - - // THEN - // total migrations count is returned - // migration functions are called - // applied migrations are recorded - // new app version code is recorded - assertEquals(migrations.size, count) - assertEquals(setOf("0", "1"), migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS)) - assertEquals(NEW_APP_VERSION, migrationsDb.store.get(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION)) - } - - @Test - @UiThreadTest - fun migration_error_is_recorded() { - // GIVEN - // no migrations applied yet - - // WHEN - // migrations are applied - // one migration throws - val lastMigration = migrations.last() - val errorMessage = "error message" - whenever(lastMigration.function.run()).thenThrow(RuntimeException(errorMessage)) - migrationsManager.startMigration() - assertTrue(asyncRunner.runOne()) - - // THEN - // failure is marked in the migration db - // failure message is recorded - // failed migration id is recorded - assertEquals(MigrationsManager.Status.FAILED, migrationsManager.status.value) - assertTrue(migrationsDb.getBoolean(MigrationsManagerImpl.DB_KEY_FAILED, false)) - assertEquals( - errorMessage, - migrationsDb.getString(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ERROR_MESSAGE, "") - ) - assertEquals( - lastMigration.id, - migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_FAILED_MIGRATION_ID, -1) - ) - } - - @Test - @UiThreadTest - fun migrations_are_not_run_if_already_run_for_an_app_version() { - // GIVEN - // migrations were already run for the current app version - whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, NEW_APP_VERSION) - - // WHEN - // app is migrated again - val migrationCount = migrationsManager.startMigration() - - // THEN - // migration processing is skipped entirely - // status is set to applied - assertEquals(0, migrationCount) - migrations.forEach { - verify(it.function, never()).run() - } - assertEquals(MigrationsManager.Status.APPLIED, migrationsManager.status.value) - } - - @Test - @UiThreadTest - fun new_app_version_is_marked_as_migrated_if_no_new_migrations_are_available() { - // GIVEN - // migrations were applied in previous version - // new version has no new migrations - whenever(appInfo.versionCode).thenReturn(NEW_APP_VERSION) - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, OLD_APP_VERSION) - val applied = migrations.map { it.id.toString() }.toSet() - migrationsDb.store.put(MigrationsManagerImpl.DB_KEY_APPLIED_MIGRATIONS, applied) - - // WHEN - // migration is started - val startedCount = migrationsManager.startMigration() - - // THEN - // no new migrations are run - // new version is marked as migrated - assertEquals(0, startedCount) - assertEquals( - NEW_APP_VERSION, - migrationsDb.getInt(MigrationsManagerImpl.DB_KEY_LAST_MIGRATED_VERSION, -1) - ) - } -} diff --git a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt b/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt deleted file mode 100644 index 659eac6a4f79..000000000000 --- a/src/androidTest/java/com/nextcloud/client/migrations/android/TestMockSharedPreferences.kt +++ /dev/null @@ -1,77 +0,0 @@ -/* - * Nextcloud Android client application - * - * @author Chris Narkiewicz - * Copyright (C) 2020 Chris Narkiewicz - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ -package com.nextcloud.client.migrations.android - -import org.junit.Before -import org.junit.Test -import org.junit.Assert.assertEquals -import org.junit.Assert.assertFalse -import org.junit.Assert.assertNotSame -import org.junit.Assert.assertTrue - -@Suppress("MagicNumber", "FunctionNaming") -class TestMockSharedPreferences { - - private lateinit var mock: MockSharedPreferences - - @Before - fun setUp() { - mock = MockSharedPreferences() - } - - @Test - fun get_set_string_set() { - val value = setOf("alpha", "bravo", "charlie") - mock.edit().putStringSet("key", value).apply() - val copy = mock.getStringSet("key", mutableSetOf()) - assertNotSame(value, copy) - assertEquals(value, copy) - } - - @Test - fun get_set_int() { - val value = 42 - val editor = mock.edit() - editor.putInt("key", value) - assertEquals(100, mock.getInt("key", 100)) - editor.apply() - assertEquals(42, mock.getInt("key", 100)) - } - - @Test - fun get_set_boolean() { - val value = true - val editor = mock.edit() - editor.putBoolean("key", value) - assertFalse(mock.getBoolean("key", false)) - editor.apply() - assertTrue(mock.getBoolean("key", false)) - } - - @Test - fun get_set_string() { - val value = "a value" - val editor = mock.edit() - editor.putString("key", value) - assertEquals("default", mock.getString("key", "default")) - editor.apply() - assertEquals("a value", mock.getString("key", "default")) - } -} From 9ef27576e1742ff5fdabc99bfd52baddf53b818b Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Tue, 3 Mar 2020 23:13:58 +0000 Subject: [PATCH 4/6] Enabled migration manager in MainApp Migration manager added to DI. Migration manager called in place of legacy migration code. Signed-off-by: Chris Narkiewicz --- .../com/nextcloud/client/di/AppModule.java | 21 +++++++++++++++++++ .../java/com/owncloud/android/MainApp.java | 15 +++++++------ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/nextcloud/client/di/AppModule.java b/src/main/java/com/nextcloud/client/di/AppModule.java index 033199561d53..c689adef5567 100644 --- a/src/main/java/com/nextcloud/client/di/AppModule.java +++ b/src/main/java/com/nextcloud/client/di/AppModule.java @@ -25,6 +25,7 @@ import android.app.NotificationManager; import android.content.ContentResolver; import android.content.Context; +import android.content.SharedPreferences; import android.content.res.Resources; import android.os.Handler; import android.media.AudioManager; @@ -32,6 +33,7 @@ import com.nextcloud.client.account.CurrentAccountProvider; import com.nextcloud.client.account.UserAccountManager; import com.nextcloud.client.account.UserAccountManagerImpl; +import com.nextcloud.client.appinfo.AppInfo; import com.nextcloud.client.core.AsyncRunner; import com.nextcloud.client.core.Clock; import com.nextcloud.client.core.ClockImpl; @@ -41,6 +43,9 @@ import com.nextcloud.client.logger.Logger; import com.nextcloud.client.logger.LoggerImpl; import com.nextcloud.client.logger.LogsRepository; +import com.nextcloud.client.migrations.Migrations; +import com.nextcloud.client.migrations.MigrationsManager; +import com.nextcloud.client.migrations.MigrationsManagerImpl; import com.nextcloud.client.network.ClientFactory; import com.owncloud.android.datamodel.ArbitraryDataProvider; import com.owncloud.android.datamodel.UploadsStorageManager; @@ -172,4 +177,20 @@ AudioManager audioManager(Context context) { EventBus eventBus() { return EventBus.getDefault(); } + + @Provides + @Singleton + Migrations migrations(UserAccountManager userAccountManager) { + return new Migrations(userAccountManager); + } + + @Provides + @Singleton + MigrationsManager migrationsManager(Application application, + AppInfo appInfo, + AsyncRunner asyncRunner, + Migrations migrations) { + SharedPreferences migrationsDb = application.getSharedPreferences("migrations", Context.MODE_PRIVATE); + return new MigrationsManagerImpl(appInfo, migrationsDb, asyncRunner, migrations.getSteps()); + } } diff --git a/src/main/java/com/owncloud/android/MainApp.java b/src/main/java/com/owncloud/android/MainApp.java index 37ec76bc0258..8b3545c804c5 100644 --- a/src/main/java/com/owncloud/android/MainApp.java +++ b/src/main/java/com/owncloud/android/MainApp.java @@ -53,6 +53,7 @@ import com.nextcloud.client.jobs.BackgroundJobManager; import com.nextcloud.client.logger.LegacyLoggerAdapter; import com.nextcloud.client.logger.Logger; +import com.nextcloud.client.migrations.MigrationsManager; import com.nextcloud.client.network.ConnectivityService; import com.nextcloud.client.onboarding.OnboardingService; import com.nextcloud.client.preferences.AppPreferences; @@ -93,6 +94,7 @@ import java.util.Arrays; import java.util.HashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.concurrent.TimeUnit; @@ -170,6 +172,9 @@ public class MainApp extends MultiDexApplication implements HasAndroidInjector { @Inject EventBus eventBus; + @Inject + MigrationsManager migrationsManager; + private PassCodeManager passCodeManager; @SuppressWarnings("unused") @@ -261,14 +266,8 @@ public void onCreate() { registerActivityLifecycleCallbacks(new ActivityInjector()); - Thread t = new Thread(() -> { - // best place, before any access to AccountManager or database - if (!preferences.isUserIdMigrated()) { - final boolean migrated = accountManager.migrateUserId(); - preferences.setMigratedUserId(migrated); - } - }); - t.start(); + int startedMigrationsCount = migrationsManager.startMigration(); + logger.i(TAG, String.format(Locale.US, "Started %d migrations", startedMigrationsCount)); JobManager.create(this).addJobCreator( new NCJobCreator( From 1ba288e5e68a1918baffda2cec45fe6a4b310333 Mon Sep 17 00:00:00 2001 From: Chris Narkiewicz Date: Wed, 4 Mar 2020 06:07:42 +0000 Subject: [PATCH 5/6] Properly signal error when user id migration fails Previous implementation was returning success status when *any* account was migrated, despite possible failures. Return error when any account fails to be migrated. The migration remines idempotent and can be re-run again, until if finally succeeds. Signed-off-by: Chris Narkiewicz --- .../com/nextcloud/client/account/UserAccountManager.java | 6 ++++-- .../nextcloud/client/account/UserAccountManagerImpl.java | 9 ++++----- .../java/com/nextcloud/client/migrations/Migrations.kt | 6 +++++- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/nextcloud/client/account/UserAccountManager.java b/src/main/java/com/nextcloud/client/account/UserAccountManager.java index 1281ec50f50a..2c3de3779ae3 100644 --- a/src/main/java/com/nextcloud/client/account/UserAccountManager.java +++ b/src/main/java/com/nextcloud/client/account/UserAccountManager.java @@ -89,9 +89,11 @@ public interface UserAccountManager extends CurrentAccountProvider { boolean exists(Account account); /** - * Verifies that every account has userId set + * Verifies that every account has userId set and sets the user id if not. + * This migration is idempotent and can be run multiple times until + * all accounts are migrated. * - * @return true if any account was migrated, false if no account was migrated + * @return true if migration was successful, false if any account failed to be migrated */ boolean migrateUserId(); diff --git a/src/main/java/com/nextcloud/client/account/UserAccountManagerImpl.java b/src/main/java/com/nextcloud/client/account/UserAccountManagerImpl.java index 49d695a2f7d5..4784b9441211 100644 --- a/src/main/java/com/nextcloud/client/account/UserAccountManagerImpl.java +++ b/src/main/java/com/nextcloud/client/account/UserAccountManagerImpl.java @@ -342,12 +342,11 @@ public boolean accountOwnsFile(OCFile file, Account account) { } public boolean migrateUserId() { - boolean success = false; Account[] ocAccounts = accountManager.getAccountsByType(MainApp.getAccountType(context)); String userId; String displayName; GetUserInfoRemoteOperation remoteUserNameOperation = new GetUserInfoRemoteOperation(); - + int failed = 0; for (Account account : ocAccounts) { String storedUserId = accountManager.getUserData(account, com.owncloud.android.lib.common.accounts.AccountUtils.Constants.KEY_USER_ID); @@ -370,10 +369,12 @@ public boolean migrateUserId() { } else { // skip account, try it next time Log_OC.e(TAG, "Error while getting username for account: " + account.name); + failed++; continue; } } catch (Exception e) { Log_OC.e(TAG, "Error while getting username: " + e.getMessage()); + failed++; continue; } @@ -383,11 +384,9 @@ public boolean migrateUserId() { accountManager.setUserData(account, com.owncloud.android.lib.common.accounts.AccountUtils.Constants.KEY_USER_ID, userId); - - success = true; } - return success; + return failed == 0; } private String getAccountType() { diff --git a/src/main/java/com/nextcloud/client/migrations/Migrations.kt b/src/main/java/com/nextcloud/client/migrations/Migrations.kt index dce6eac4fc20..366e09fcb30e 100644 --- a/src/main/java/com/nextcloud/client/migrations/Migrations.kt +++ b/src/main/java/com/nextcloud/client/migrations/Migrations.kt @@ -21,6 +21,7 @@ package com.nextcloud.client.migrations import com.nextcloud.client.account.UserAccountManager import javax.inject.Inject +import kotlin.IllegalStateException /** * This class collects all migration steps and provides API to supply those @@ -50,6 +51,9 @@ class Migrations @Inject constructor( ).sortedBy { it.id } fun migrateUserId() { - userAccountManager.migrateUserId() + val allAccountsHaveUserId = userAccountManager.migrateUserId() + if (!allAccountsHaveUserId) { + throw IllegalStateException("Failed to set user id for all accounts") + } } } From 5872a973a799e088501f4e19d08c231488d9ad19 Mon Sep 17 00:00:00 2001 From: tobiasKaminsky Date: Fri, 6 Mar 2020 10:36:10 +0100 Subject: [PATCH 6/6] Bump after rebase Signed-off-by: tobiasKaminsky --- scripts/analysis/lint-results.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/analysis/lint-results.txt b/scripts/analysis/lint-results.txt index 2d857a44dcc9..4cb8c8d70724 100644 --- a/scripts/analysis/lint-results.txt +++ b/scripts/analysis/lint-results.txt @@ -1,2 +1,2 @@ DO NOT TOUCH; GENERATED BY DRONE - Lint Report: 92 warnings + Lint Report: 94 warnings