IDEA-144052 When "Settings repository" is enabled changes in 'Path Variables' aren't saved to default path.macros.xml file causing errors in build process

platform/configuration-store-impl/src/ApplicationStoreImpl.kt

@@ -16,8 +16,8 @@
 package com.intellij.configurationStore
 
 import com.intellij.application.options.PathMacrosImpl
+import com.intellij.openapi.application.Application
 import com.intellij.openapi.application.PathManager
-import com.intellij.openapi.application.impl.ApplicationImpl
 import com.intellij.openapi.application.invokeAndWaitIfNeed
 import com.intellij.openapi.components.PathMacroManager
 import com.intellij.openapi.components.StateStorageOperation
@@ -25,38 +25,21 @@ import com.intellij.openapi.components.StoragePathMacros
 import com.intellij.openapi.components.impl.BasePathMacroManager
 import com.intellij.openapi.components.impl.ServiceManagerImpl
 import com.intellij.openapi.components.impl.stores.FileStorageCoreUtil
+import com.intellij.openapi.util.JDOMUtil
 import com.intellij.openapi.util.NamedJDOMExternalizable
+import com.intellij.openapi.util.io.FileUtilRt
 import com.intellij.openapi.vfs.LocalFileSystem
 import com.intellij.openapi.vfs.VfsUtil
+import org.jdom.Element
 
 class ApplicationPathMacroManager : BasePathMacroManager(null)
 
-class ApplicationStoreImpl(private val application: ApplicationImpl, pathMacroManager: PathMacroManager) : ComponentStoreImpl() {
-  override val storageManager = object : StateStorageManagerImpl("application", pathMacroManager.createTrackingSubstitutor(), application) {
-    override fun getOldStorageSpec(component: Any, componentName: String, operation: StateStorageOperation): String? {
-      if (component is NamedJDOMExternalizable) {
-        return "${StoragePathMacros.APP_CONFIG}/${component.getExternalFileName()}${FileStorageCoreUtil.DEFAULT_EXT}"
-      }
-      else {
-        return DEFAULT_STORAGE_SPEC
-      }
-    }
-
-    override fun getMacroSubstitutor(fileSpec: String) = if (fileSpec == "${StoragePathMacros.APP_CONFIG}/${PathMacrosImpl.EXT_FILE_NAME}${FileStorageCoreUtil.DEFAULT_EXT}") null else super.getMacroSubstitutor(fileSpec)
-
-    override protected val isUseXmlProlog: Boolean
-      get() = false
-  }
-
-  companion object {
-    private val DEFAULT_STORAGE_SPEC = "${StoragePathMacros.APP_CONFIG}/${PathManager.DEFAULT_OPTIONS_FILE_NAME}${FileStorageCoreUtil.DEFAULT_EXT}"
-
-    private val FILE_STORAGE_DIR = "options"
-  }
+class ApplicationStoreImpl(private val application: Application, pathMacroManager: PathMacroManager) : ComponentStoreImpl() {
+  override val storageManager = ApplicationStorageManager(application, pathMacroManager)
 
   override fun setPath(path: String) {
     storageManager.addMacro(ROOT_CONFIG, path)
-    storageManager.addMacro(StoragePathMacros.APP_CONFIG, "$path/$FILE_STORAGE_DIR")
+    storageManager.addMacro(StoragePathMacros.APP_CONFIG, "$path/${ApplicationStorageManager.FILE_STORAGE_DIR}")
 
     val configDir = LocalFileSystem.getInstance().refreshAndFindFileByPath(path)
     if (configDir != null) {
@@ -68,7 +51,7 @@ class ApplicationStoreImpl(private val application: ApplicationImpl, pathMacroMa
         val refreshAll = ServiceManagerImpl.isUseReadActionToInitService()
 
         VfsUtil.markDirtyAndRefresh(false, refreshAll, true, configDir)
-        val optionsDir = configDir.findChild(FILE_STORAGE_DIR)
+        val optionsDir = configDir.findChild(ApplicationStorageManager.FILE_STORAGE_DIR)
         if (!refreshAll && optionsDir != null) {
           // not recursive, options directory contains only files
           VfsUtil.markDirtyAndRefresh(false, false, true, optionsDir)
@@ -76,4 +59,42 @@ class ApplicationStoreImpl(private val application: ApplicationImpl, pathMacroMa
       }
     }
   }
+}
+
+class ApplicationStorageManager(private val application: Application, pathMacroManager: PathMacroManager? = null) : StateStorageManagerImpl("application", pathMacroManager?.createTrackingSubstitutor(), application) {
+  companion object {
+    private val DEFAULT_STORAGE_SPEC = "${StoragePathMacros.APP_CONFIG}/${PathManager.DEFAULT_OPTIONS_FILE_NAME}${FileStorageCoreUtil.DEFAULT_EXT}"
+
+    val FILE_STORAGE_DIR = "options"
+  }
+
+  override fun getOldStorageSpec(component: Any, componentName: String, operation: StateStorageOperation): String? {
+    if (component is NamedJDOMExternalizable) {
+      return "${StoragePathMacros.APP_CONFIG}/${component.getExternalFileName()}${FileStorageCoreUtil.DEFAULT_EXT}"
+    }
+    else {
+      return DEFAULT_STORAGE_SPEC
+    }
+  }
+
+  override fun getMacroSubstitutor(fileSpec: String) = if (fileSpec == "${StoragePathMacros.APP_CONFIG}/${PathMacrosImpl.EXT_FILE_NAME}${FileStorageCoreUtil.DEFAULT_EXT}") null else super.getMacroSubstitutor(fileSpec)
+
+  override protected val isUseXmlProlog: Boolean
+    get() = false
+
+  override fun dataLoadedFromProvider(storage: FileBasedStorage, element: Element?) {
+    // IDEA-144052 When "Settings repository" is enabled changes in 'Path Variables' aren't saved to default path.macros.xml file causing errors in build process
+    try {
+      if (element == null) {
+        storage.file.delete()
+      }
+      else {
+        FileUtilRt.createParentDirs(storage.file)
+        JDOMUtil.writeElement(element, storage.file.writer(), "\n")
+      }
+    }
+    catch (e: Throwable) {
+      LOG.error(e)
+    }
+  }
 }

platform/configuration-store-impl/src/FileBasedStorage.kt

@@ -37,7 +37,7 @@ import java.io.File
 import java.io.IOException
 import java.nio.ByteBuffer
 
-open class FileBasedStorage(private volatile var file: File,
+open class FileBasedStorage(file: File,
                             fileSpec: String,
                             rootElementName: String,
                             pathMacroManager: TrackingPathMacroSubstitutor? = null,
@@ -47,6 +47,9 @@ open class FileBasedStorage(private volatile var file: File,
   private var lineSeparator: LineSeparator? = null
   private var blockSavingTheContent = false
 
+  volatile var file = file
+    private set
+
   init {
     if (ApplicationManager.getApplication().isUnitTestMode() && file.getPath().startsWith('$')) {
       throw AssertionError("It seems like some macros were not expanded for path: $file")
@@ -97,8 +100,6 @@ open class FileBasedStorage(private volatile var file: File,
     return cachedVirtualFile
   }
 
-  override fun getFile() = file
-
   override fun loadLocalData(): Element? {
     blockSavingTheContent = false
     try {

platform/configuration-store-impl/src/ProjectStoreImpl.kt

@@ -33,7 +33,6 @@ import com.intellij.openapi.util.Pair
 import com.intellij.openapi.util.io.FileUtil
 import com.intellij.openapi.util.io.FileUtilRt
 import com.intellij.openapi.util.io.systemIndependentPath
-import com.intellij.openapi.util.text.StringUtil
 import com.intellij.openapi.vfs.*
 import com.intellij.util.PathUtilRt
 import com.intellij.util.SmartList
@@ -109,18 +108,10 @@ open class ProjectStoreImpl(override val project: ProjectImpl, private val pathM
   }
 
   override fun getProjectBasePath(): String? {
-    val path = getProjectFilePath()
-    if (!StringUtil.isEmptyOrSpaces(path)) {
-      return getBasePath(File(path))
-    }
-
-    //we are not yet initialized completely ("open directory", etc)
-    val storage = storageManager.getStateStorage(StoragePathMacros.PROJECT_FILE, RoamingType.PER_USER)
-    return if (storage is FileBasedStorage) getBasePath(storage.getFile()) else null
+    val path = PathUtilRt.getParentPath(getProjectFilePath())
+    return if (scheme == StorageScheme.DEFAULT) path else PathUtilRt.getParentPath(path)
   }
 
-  private fun getBasePath(file: File) = if (scheme == StorageScheme.DEFAULT) file.getParent() else file.getParentFile()?.getParent()
-
   override fun getProjectName(): String {
     if (scheme == StorageScheme.DIRECTORY_BASED) {
       val baseDir = getProjectBaseDir()

platform/configuration-store-impl/src/StateStorageManagerImpl.kt

@@ -155,7 +155,6 @@ open class StateStorageManagerImpl(private val rootTagName: String,
     }
   }
 
-
   fun getCachedFileStorages(changed: Collection<String>, deleted: Collection<String>) = storageLock.withLock { Pair(getCachedFileStorages(changed), getCachedFileStorages(deleted)) }
 
   fun getCachedFileStorages(fileSpecs: Collection<String>): Collection<StateStorage> {
@@ -233,6 +232,10 @@ open class StateStorageManagerImpl(private val rootTagName: String,
       storageManager.beforeElementLoaded(element)
       super<FileBasedStorage>.beforeElementLoaded(element)
     }
+
+    override fun dataLoadedFromProvider(element: Element?) {
+      storageManager.dataLoadedFromProvider(this, element)
+    }
   }
 
   private fun String.normalizePath(): String {
@@ -247,6 +250,9 @@ open class StateStorageManagerImpl(private val rootTagName: String,
   protected open fun beforeElementLoaded(element: Element) {
   }
 
+  protected open fun dataLoadedFromProvider(storage: FileBasedStorage, element: Element?) {
+  }
+
   override final fun rename(path: String, newName: String) {
     storageLock.withLock {
       val storage = getOrCreateStorage(collapseMacros(path), RoamingType.PER_USER) as FileBasedStorage
@@ -256,7 +262,7 @@ open class StateStorageManagerImpl(private val rootTagName: String,
         if (file != null) {
           file.rename(storage, newName)
         }
-        else if (storage.getFile().getName() != newName) {
+        else if (storage.file.getName() != newName) {
           // old file didn't exist or renaming failed
           val expandedPath = expandMacros(path)
           val parentPath = PathUtilRt.getParentPath(expandedPath)

platform/configuration-store-impl/src/XmlElementStorage.kt

@@ -47,6 +47,7 @@ abstract class XmlElementStorage protected constructor(protected val fileSpec: S
     if (provider != null && provider.enabled) {
       try {
         element = loadDataFromProvider()
+        dataLoadedFromProvider(element)
       }
       catch (e: Exception) {
         LOG.error(e)
@@ -59,6 +60,9 @@ abstract class XmlElementStorage protected constructor(protected val fileSpec: S
     return if (element == null) StateMap.EMPTY else loadState(element)
   }
 
+  protected open fun dataLoadedFromProvider(element: Element?) {
+  }
+
   private fun loadDataFromProvider() = JDOMUtil.load(provider!!.loadContent(fileSpec, roamingType))
 
   private fun loadState(element: Element): StateMap {

platform/configuration-store-impl/testSrc/ApplicationStoreTest.kt

@@ -15,7 +15,7 @@
  */
 package com.intellij.configurationStore
 
-import com.intellij.application.options.PathMacrosImpl
+import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.application.runWriteAction
 import com.intellij.openapi.components.*
 import com.intellij.openapi.components.impl.stores.StreamProvider
@@ -26,8 +26,7 @@ import com.intellij.testFramework.*
 import com.intellij.util.SmartList
 import com.intellij.util.xmlb.XmlSerializerUtil
 import gnu.trove.THashMap
-import org.hamcrest.CoreMatchers.equalTo
-import org.hamcrest.MatcherAssert.assertThat
+import org.assertj.core.api.Assertions.assertThat
 import org.intellij.lang.annotations.Language
 import org.junit.Before
 import org.junit.ClassRule
@@ -35,6 +34,7 @@ import org.junit.Rule
 import org.junit.Test
 import java.io.ByteArrayInputStream
 import java.io.InputStream
+import java.nio.file.Paths
 import kotlin.properties.Delegates
 
 class ApplicationStoreTest {
@@ -56,7 +56,7 @@ class ApplicationStoreTest {
     componentStore = MyComponentStore(FileUtilRt.toSystemIndependentName(testAppConfig.getPath()))
   }
 
-  public Test fun `stream provider save if several storages configured`() {
+  @Test fun `stream provider save if several storages configured`() {
     val component = SeveralStoragesConfigured()
 
     val streamProvider = MyStreamProvider()
@@ -66,27 +66,30 @@ class ApplicationStoreTest {
     component.foo = "newValue"
     componentStore.save(SmartList())
 
-    assertThat<String>(streamProvider.data.get(RoamingType.PER_USER)!!.get(StoragePathMacros.APP_CONFIG + "/proxy.settings.xml"), equalTo("<application>\n" + "  <component name=\"HttpConfigurable\">\n" + "    <option name=\"foo\" value=\"newValue\" />\n" + "  </component>\n" + "</application>"))
+    assertThat(streamProvider.data.get(RoamingType.PER_USER)!!.get("${StoragePathMacros.APP_CONFIG}/proxy.settings.xml")).isEqualTo("<application>\n" + "  <component name=\"HttpConfigurable\">\n" + "    <option name=\"foo\" value=\"newValue\" />\n" + "  </component>\n" + "</application>")
   }
 
-  public Test fun testLoadFromStreamProvider() {
+  @Test fun testLoadFromStreamProvider() {
     val component = SeveralStoragesConfigured()
 
     val streamProvider = MyStreamProvider()
     val map = THashMap<String, String>()
-    map.put(StoragePathMacros.APP_CONFIG + "/proxy.settings.xml", "<application>\n" + "  <component name=\"HttpConfigurable\">\n" + "    <option name=\"foo\" value=\"newValue\" />\n" + "  </component>\n" + "</application>")
+    val fileSpec = "${StoragePathMacros.APP_CONFIG}/proxy.settings.xml"
+    map.put(fileSpec, "<application>\n  <component name=\"HttpConfigurable\">\n    <option name=\"foo\" value=\"newValue\" />\n  </component>\n</application>")
     streamProvider.data.put(RoamingType.PER_USER, map)
 
     componentStore.storageManager.setStreamProvider(streamProvider)
     componentStore.initComponent(component, false)
-    assertThat(component.foo, equalTo("newValue"))
+    assertThat(component.foo).isEqualTo("newValue")
+
+    assertThat(Paths.get(componentStore.storageManager.expandMacros(fileSpec))).exists()
   }
 
-  public @Test @RunsInEdt fun `remove deprecated storage on write`() {
+  @Test @RunsInEdt fun `remove deprecated storage on write`() {
     doRemoveDeprecatedStorageOnWrite(SeveralStoragesConfigured())
   }
 
-  public @Test @RunsInEdt fun `remove deprecated storage on write 2`() {
+  @Test @RunsInEdt fun `remove deprecated storage on write 2`() {
     doRemoveDeprecatedStorageOnWrite(ActualStorageLast())
   }
 
@@ -95,12 +98,12 @@ class ApplicationStoreTest {
     writeConfig("proxy.settings.xml", "<application><component name=\"HttpConfigurable\"><option name=\"foo\" value=\"new\" /></component></application>")
 
     componentStore.initComponent(component, false)
-    assertThat(component.foo, equalTo("new"))
+    assertThat(component.foo).isEqualTo("new")
 
     component.foo = "new2"
     componentStore.save(SmartList())
 
-    assertThat(oldFile.exists(), equalTo(false))
+    assertThat(oldFile.exists()).isFalse()
   }
 
   private fun writeConfig(fileName: String, Language("XML") data: String) = runWriteAction { testAppConfig.writeChild(fileName, data) }
@@ -132,14 +135,7 @@ class ApplicationStoreTest {
   }
 
   class MyComponentStore(testAppConfigPath: String) : ComponentStoreImpl() {
-    override val storageManager = object : StateStorageManagerImpl("application") {
-      override fun getMacroSubstitutor(fileSpec: String): TrackingPathMacroSubstitutor? {
-        if (fileSpec == "${StoragePathMacros.APP_CONFIG}/${PathMacrosImpl.EXT_FILE_NAME}.xml") {
-          return null
-        }
-        return super.getMacroSubstitutor(fileSpec)
-      }
-    }
+    override val storageManager = ApplicationStorageManager(ApplicationManager.getApplication())
 
     init {
       setPath(testAppConfigPath)
@@ -154,7 +150,7 @@ class ApplicationStoreTest {
     public var foo: String = "defaultValue"
   }
 
-  State(name = "HttpConfigurable", storages = arrayOf(Storage(file = StoragePathMacros.APP_CONFIG + "/proxy.settings.xml"), Storage(file = StoragePathMacros.APP_CONFIG + "/other.xml", deprecated = true)))
+  @State(name = "HttpConfigurable", storages = arrayOf(Storage(file = StoragePathMacros.APP_CONFIG + "/proxy.settings.xml"), Storage(file = StoragePathMacros.APP_CONFIG + "/other.xml", deprecated = true)))
   class SeveralStoragesConfigured : Foo(), PersistentStateComponent<SeveralStoragesConfigured> {
     override fun getState(): SeveralStoragesConfigured? {
       return this
@@ -165,7 +161,7 @@ class ApplicationStoreTest {
     }
   }
 
-  State(name = "HttpConfigurable", storages = arrayOf(Storage(file = "${StoragePathMacros.APP_CONFIG}/other.xml", deprecated = true), Storage(file = "${StoragePathMacros.APP_CONFIG}/proxy.settings.xml")))
+  @State(name = "HttpConfigurable", storages = arrayOf(Storage(file = "${StoragePathMacros.APP_CONFIG}/other.xml", deprecated = true), Storage(file = "${StoragePathMacros.APP_CONFIG}/proxy.settings.xml")))
   class ActualStorageLast : Foo(), PersistentStateComponent<ActualStorageLast> {
     override fun getState() = this
 
@@ -173,9 +169,4 @@ class ApplicationStoreTest {
       XmlSerializerUtil.copyBean(state, this)
     }
   }
-}
-
-fun VirtualFile.writeChild(relativePath: String, data: String) = VfsTestUtil.createFile(this, relativePath, data)
-
-val VirtualFile.path: String
-  get() = getPath()
+}

platform/configuration-store-impl/testSrc/ModuleStoreRenameTest.kt

@@ -81,7 +81,7 @@ class ModuleStoreRenameTest {
   public Test fun `rename module using model`() {
     runInEdtAndWait { module.saveStore() }
     val storage = module.stateStore.getStateStorageManager().getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage
-    val oldFile = storage.getFile()
+    val oldFile = storage.file
     assertThat(oldFile).isFile()
 
     val oldName = module.getName()
@@ -95,7 +95,7 @@ class ModuleStoreRenameTest {
   public Test fun `rename module using rename virtual file`() {
     runInEdtAndWait { module.saveStore() }
     var storage = module.stateStore.getStateStorageManager().getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage
-    val oldFile = storage.getFile()
+    val oldFile = storage.file
     assertThat(oldFile).isFile()
 
     val oldName = module.getName()
@@ -109,7 +109,7 @@ class ModuleStoreRenameTest {
 
   private fun assertRename(newName: String, oldFile: File) {
     val storageManager = moduleFixture.getModule().stateStore.getStateStorageManager()
-    val newFile = (storageManager.getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage).getFile()
+    val newFile = (storageManager.getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage).file
     assertThat(newFile.getName()).isEqualTo("$newName${ModuleFileType.DOT_DEFAULT_EXTENSION}")
     assertThat(oldFile)
       .doesNotExist()
@@ -125,7 +125,7 @@ class ModuleStoreRenameTest {
     val storageManager = module.stateStore.getStateStorageManager()
     val storage = storageManager.getStateStorage(StoragePathMacros.MODULE_FILE, RoamingType.PER_USER) as FileBasedStorage
 
-    val oldFile = storage.getFile()
+    val oldFile = storage.file
     val parentVirtualDir = storage.getVirtualFile()!!.getParent()
     runInEdtAndWait { runWriteAction { parentVirtualDir.rename(null, UUID.randomUUID().toString()) } }
 

platform/configuration-store-impl/testSrc/ProjectStoreTest.kt

@@ -24,10 +24,7 @@ import com.intellij.openapi.project.ex.ProjectManagerEx
 import com.intellij.openapi.project.impl.ProjectManagerImpl
 import com.intellij.openapi.util.io.systemIndependentPath
 import com.intellij.openapi.vfs.VirtualFile
-import com.intellij.testFramework.ProjectRule
-import com.intellij.testFramework.RuleChain
-import com.intellij.testFramework.TemporaryDirectory
-import com.intellij.testFramework.runInEdtAndWait
+import com.intellij.testFramework.*
 import org.assertj.core.api.Assertions.assertThat
 import org.intellij.lang.annotations.Language
 import org.junit.ClassRule

platform/platform-impl/src/com/intellij/openapi/components/impl/stores/FileStorage.java

@@ -16,7 +16,6 @@
 package com.intellij.openapi.components.impl.stores;
 
 import com.intellij.openapi.vfs.VirtualFile;
-import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 
 import java.io.File;
@@ -25,9 +24,6 @@ public interface FileStorage {
   @Nullable
   VirtualFile getVirtualFile();
 
-  @NotNull
-  File getFile();
-
   // todo remove
   void setFile(@Nullable VirtualFile file, @Nullable File ioFileIfChanged);
 }

platform/testFramework/src/com/intellij/testFramework/TemporaryDirectory.kt

@@ -120,4 +120,9 @@ public val Path.systemIndependentPath: String
 public val Path.parentSystemIndependentPath: String
   get() = getParent()!!.toString().replace(File.separatorChar, '/')
 
-public fun Path.readText(): String = Files.readAllBytes(this).toString(Charsets.UTF_8)
+public fun Path.readText(): String = Files.readAllBytes(this).toString(Charsets.UTF_8)
+
+fun VirtualFile.writeChild(relativePath: String, data: String) = VfsTestUtil.createFile(this, relativePath, data)
+
+val VirtualFile.path: String
+  get() = getPath()