IJPL-158288 settingsSync shouldn't sync non-roamable files

GitOrigin-RevId: 2ac91c173c3067f4d4a5af3a4831af2c7633c3aa

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) }

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<Class<PersistentStateComponent<Any>>>): Pair<SettingsCategory, String?> {
+private fun getRoamableCategory(fileName: String, componentClasses: List<Class<PersistentStateComponent<Any>>>): Pair<SettingsCategory, String?> {
   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<String, Pair<SettingsCategory, String?>> = ConcurrentHashMap()
+private val roamambleCategoryCache: ConcurrentHashMap<String, Pair<SettingsCategory, String?>?> = ConcurrentHashMap()
 
-fun getCategory(fileName: String): Pair<SettingsCategory, String?>? {
-  categoryCache[fileName]?.let { cachedCategory ->
+fun getRoamableCategory(fileName: String): Pair<SettingsCategory, String?>? {
+  roamambleCategoryCache[fileName]?.let { cachedCategory ->
     return cachedCategory
   }
 
@@ -59,9 +65,9 @@ fun getCategory(fileName: String): Pair<SettingsCategory, String?>? {
     return null
   }
 
-  val category = getSchemeCategory(fileName) ?: getCategory(fileName, componentClasses)
+  val category = getSchemeCategory(fileName) ?: getRoamableCategory(fileName, componentClasses)
 
-  categoryCache[fileName] = category
+  roamambleCategoryCache[fileName] = category
 
   return category
 }

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"))
+  }
 }

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<AState> {
-    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()

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<AState> {
+    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()
   }