From a776551e7eeb3434c450aaa7acc6ca87ee3b419e Mon Sep 17 00:00:00 2001 From: Sergey Pak Date: Wed, 25 Sep 2024 15:32:30 +0200 Subject: [PATCH] IJPL-158288 settingsSync shouldn't sync non-roamable files GitOrigin-RevId: 2ac91c173c3067f4d4a5af3a4831af2c7633c3aa --- .../settingsSync/git/record/ChangeRecord.kt | 4 +- .../settingsSync/SettingsSyncFiltering.kt | 26 +++++--- .../settingsSync/SettingsFilteringTest.kt | 7 ++ .../settingsSync/SettingsSyncRealIdeTest.kt | 66 ++++++++++++------- .../SettingsSyncRealIdeTestBase.kt | 40 ++++++++--- 5 files changed, 101 insertions(+), 42 deletions(-) diff --git a/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt b/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt index 38c048ef83d7..7b83c4c2273f 100644 --- a/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt +++ b/plugins/settings-sync/git/src/com/intellij/settingsSync/git/record/ChangeRecord.kt @@ -5,7 +5,7 @@ import com.intellij.openapi.diagnostic.logger import com.intellij.openapi.vcs.changes.Change import com.intellij.settingsSync.GitSettingsLog import com.intellij.settingsSync.SettingsSyncBundle -import com.intellij.settingsSync.getCategory +import com.intellij.settingsSync.getRoamableCategory import com.intellij.vcs.log.VcsFullCommitDetails import java.text.DateFormat import java.util.* @@ -129,7 +129,7 @@ internal class ChangeRecord(commitId: Int, if (fileName == GitSettingsLog.PLUGINS_FILE) return SettingsCategory.PLUGINS //workaround empty category - return getCategory(fileName)?.first ?: SettingsCategory.OTHER + return getRoamableCategory(fileName)?.first ?: SettingsCategory.OTHER } val changesCategories = changes.map { getChangeCategory(it) }.distinct().sortedBy { getCategoryOrder(it) }.map { toString(it) } diff --git a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt index 82c7606cac9b..30e1247662d4 100644 --- a/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt +++ b/plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncFiltering.kt @@ -16,7 +16,7 @@ internal fun isSyncCategoryEnabled(fileSpec: String): Boolean { if (rawFileSpec == SettingsSyncSettings.FILE_SPEC) return true - val (category, subCategory) = getSchemeCategory(rawFileSpec) ?: getCategory(rawFileSpec) ?: return false + val (category, subCategory) = getSchemeCategory(rawFileSpec) ?: getRoamableCategory(rawFileSpec) ?: return false if (category != SettingsCategory.OTHER && SettingsSyncSettings.getInstance().isCategoryEnabled(category)) { if (subCategory != null) { @@ -33,23 +33,29 @@ private fun removeOsPrefix(fileSpec: String): String { return if (fileSpec.startsWith(osPrefix)) StringUtil.trimStart(fileSpec, osPrefix) else fileSpec } -private fun getCategory(fileName: String, componentClasses: List>>): Pair { +private fun getRoamableCategory(fileName: String, componentClasses: List>>): Pair { componentClasses.forEach { val category = ComponentCategorizer.getCategory(it) + if (category == SettingsCategory.OTHER) return@forEach - if (category != SettingsCategory.OTHER) { - // Once found, ignore any other possibly conflicting definitions - return (category to getSubCategory(fileName)) + val state = it.getAnnotation(State::class.java) + val storage = state.storages.find { it.value == fileName } + if (storage == null || !storage.roamingType.isRoamable) { + return@forEach } + + // Once found, ignore any other possibly conflicting definitions + return (category to getSubCategory(fileName)) } + return SettingsCategory.OTHER to null } -private val categoryCache: ConcurrentHashMap> = ConcurrentHashMap() +private val roamambleCategoryCache: ConcurrentHashMap?> = ConcurrentHashMap() -fun getCategory(fileName: String): Pair? { - categoryCache[fileName]?.let { cachedCategory -> +fun getRoamableCategory(fileName: String): Pair? { + roamambleCategoryCache[fileName]?.let { cachedCategory -> return cachedCategory } @@ -59,9 +65,9 @@ fun getCategory(fileName: String): Pair? { return null } - val category = getSchemeCategory(fileName) ?: getCategory(fileName, componentClasses) + val category = getSchemeCategory(fileName) ?: getRoamableCategory(fileName, componentClasses) - categoryCache[fileName] = category + roamambleCategoryCache[fileName] = category return category } diff --git a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsFilteringTest.kt b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsFilteringTest.kt index aea609d507b6..97bffef9e8c4 100644 --- a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsFilteringTest.kt +++ b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsFilteringTest.kt @@ -1,6 +1,7 @@ package com.intellij.settingsSync import com.intellij.configurationStore.getPerOsSettingsStorageFolderName +import com.intellij.idea.TestFor import com.intellij.openapi.components.RoamingType import com.intellij.openapi.components.SettingsCategory import com.intellij.openapi.editor.colors.impl.EditorColorsManagerImpl @@ -72,4 +73,10 @@ class SettingsFilteringTest : LightPlatformTestCase() { fun `test settings sync settings always synchronized` () { assertTrue(isSyncCategoryEnabled("settingsSync.xml")) } + + @TestFor(issues = ["IJPL-162877"]) + fun `test dont sync non-roamable components`() { + assertFalse(isSyncCategoryEnabled("advancedSettings.xml")) + assertFalse(isSyncCategoryEnabled("trustedPaths.xml")) + } } \ No newline at end of file diff --git a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTest.kt b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTest.kt index 93d175531105..506c6c25dadc 100644 --- a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTest.kt +++ b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTest.kt @@ -3,6 +3,7 @@ package com.intellij.settingsSync import com.intellij.configurationStore.getPerOsSettingsStorageFolderName import com.intellij.ide.GeneralSettings import com.intellij.ide.ui.UISettings +import com.intellij.idea.TestFor import com.intellij.openapi.Disposable import com.intellij.openapi.components.* import com.intellij.openapi.editor.ex.EditorSettingsExternalizable @@ -14,6 +15,7 @@ import com.intellij.util.toByteArray import com.intellij.util.xmlb.annotations.Attribute import kotlinx.coroutines.runBlocking import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.BeforeAll import org.junit.jupiter.api.Test import java.nio.charset.Charset import java.time.Instant @@ -293,12 +295,53 @@ internal class SettingsSyncRealIdeTest : SettingsSyncRealIdeTestBase() { testVariousComponentsShouldBeSyncedOrNot(Roamable(), expectedToBeSynced = true) } + + @Test + @TestFor(issues = ["IJPL-162877"]) + fun `don't sync non-roamable files`() = timeoutRunBlockingAndStopBridge { + val nonRoamable = ExportableNonRoamable() + + nonRoamable.init() + val generalSettings = GeneralSettings.getInstance().init() + initSettingsSync(SettingsSyncBridge.InitMode.JustInit) + + val generalSettingsState = GeneralSettings().apply { + isSaveOnFrameDeactivation = false + }.toFileState() + val nonRoamableState = ExportableNonRoamable().apply { + aState.foo = "bar" + }.toFileState() + + remoteCommunicator.prepareFileOnServer(SettingsSnapshot( + MetaInfo(Instant.now(), getLocalApplicationInfo()), + setOf(generalSettingsState, nonRoamableState), + null, emptyMap(), emptySet() + )) + + waitForSettingsToBeApplied(generalSettings) { + SettingsSyncEvents.getInstance().fireSettingsChanged(SyncSettingsEvent.SyncRequest) + } + Assertions.assertFalse(generalSettings.isSaveOnFrameDeactivation) + Assertions.assertTrue(nonRoamable.aState.foo == "") + + bridge.waitForAllExecuted() + + val state = nonRoamable::class.annotations.find { it is State } as State + val file = state.storages.first().value + + // we shouldn't drop non-roamable files as they might be used by other IDEs + val syncCopy = settingsSyncStorage.resolve("options").resolve(file) + val localCopy = configDir.resolve("options").resolve(file) + Assertions.assertTrue(syncCopy.exists(), "File should be copied from server") + Assertions.assertFalse(localCopy.exists(), "File should not be updated (it's not roamamble)") + } + + private suspend fun testVariousComponentsShouldBeSyncedOrNot(component: BaseComponent, expectedToBeSynced: Boolean) { component.aState.foo = "bar" runBlocking { application.componentStore.saveComponent(component) } - application.registerComponentImplementation(component.javaClass, component.javaClass, false) initSettingsSync(SettingsSyncBridge.InitMode.JustInit) @@ -315,27 +358,6 @@ internal class SettingsSyncRealIdeTest : SettingsSyncRealIdeTestBase() { } } - private data class AState(@Attribute var foo: String = "") - - @State(name = "SettingsSyncTestExportableNonRoamable", - storages = [Storage("settings-sync-test.exportable-non-roamable.xml", roamingType = RoamingType.DISABLED, exportable = true)]) - private class ExportableNonRoamable : BaseComponent() - - @State(name = "SettingsSyncTestRoamable", - storages = [Storage("settings-sync-test.roamable.xml", roamingType = RoamingType.DEFAULT)], - category = SettingsCategory.UI) - private class Roamable : BaseComponent() - - private open class BaseComponent : PersistentStateComponent { - var aState = AState() - - override fun getState() = aState - - override fun loadState(state: AState) { - this.aState = state - } - } - @Test fun `local and remote changes in different files are both applied`() = timeoutRunBlockingAndStopBridge { val generalSettings = GeneralSettings.getInstance().init() diff --git a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTestBase.kt b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTestBase.kt index 3386cdcecf67..945c6551fe59 100644 --- a/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTestBase.kt +++ b/plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncRealIdeTestBase.kt @@ -4,21 +4,17 @@ import com.intellij.configurationStore.ApplicationStoreImpl import com.intellij.configurationStore.StateLoadPolicy import com.intellij.ide.plugins.PluginManagerCore import com.intellij.openapi.application.ApplicationManager -import com.intellij.openapi.components.PersistentStateComponent -import com.intellij.openapi.components.StateStorage +import com.intellij.openapi.components.* import com.intellij.openapi.components.impl.stores.IComponentStore +import com.intellij.openapi.extensions.DefaultPluginDescriptor import com.intellij.openapi.progress.currentThreadCoroutineScope import com.intellij.openapi.util.Disposer import com.intellij.openapi.util.io.FileUtil.createTempDirectory -import com.intellij.settingsSync.GitSettingsLog.Companion -import com.intellij.settingsSync.plugins.SettingsSyncPluginManager import com.intellij.testFramework.common.timeoutRunBlocking import com.intellij.testFramework.replaceService -import com.intellij.util.progress.sleepCancellable +import com.intellij.util.xmlb.annotations.Attribute import kotlinx.coroutines.runBlocking import kotlinx.coroutines.yield -import org.eclipse.jgit.api.Git -import org.eclipse.jgit.storage.file.FileRepositoryBuilder import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.BeforeAll @@ -27,25 +23,53 @@ import java.lang.reflect.Constructor import java.nio.file.Path import java.time.Instant import java.util.concurrent.CountDownLatch -import kotlin.io.path.exists import kotlin.time.Duration.Companion.seconds internal abstract class SettingsSyncRealIdeTestBase : SettingsSyncTestBase() { protected lateinit var componentStore: TestComponentStore + val testPluginDescriptor: DefaultPluginDescriptor = DefaultPluginDescriptor("test") @BeforeEach fun setupComponentStore() { componentStore = TestComponentStore(configDir) application.replaceService(IComponentStore::class.java, componentStore, disposable) //warm up + application.registerService(ExportableNonRoamable::class.java, ExportableNonRoamable::class.java, testPluginDescriptor, false) + application.registerService(Roamable::class.java, Roamable::class.java, testPluginDescriptor, false) + //application.registerService(Roamable::class.java, Roamable::class.java, false) application.processAllImplementationClasses { componentClass, plugin -> // do nothing } } + internal data class AState(@Attribute var foo: String = "") + + @State(name = "SettingsSyncTestExportableNonRoamable", + category = SettingsCategory.TOOLS, + storages = [Storage("settings-sync-test.exportable-non-roamable.xml", roamingType = RoamingType.DISABLED, exportable = true)]) + internal class ExportableNonRoamable : BaseComponent() + + @State(name = "SettingsSyncTestRoamable", + storages = [Storage("settings-sync-test.roamable.xml", roamingType = RoamingType.DEFAULT)], + category = SettingsCategory.UI) + internal class Roamable : BaseComponent() + + internal open class BaseComponent : PersistentStateComponent { + var aState = AState() + + override fun getState() = aState + + override fun loadState(state: AState) { + this.aState = state + } + } + + @AfterEach fun resetComponentStatesToDefault() { + application.unregisterService(ExportableNonRoamable::class.java) + application.unregisterService(Roamable::class.java) componentStore.resetComponents() }