fix blinking SchemeManagerTest "different extensions" — problem was in file order

2026-09-27 10:03:11 +07:00 · 2018-11-08 13:53:31 +01:00
parent 41bb1afe0f
commit 130ffc0ce7
3 changed files with 120 additions and 75 deletions
@@ -181,7 +181,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
      // isDuringLoad is true even if loadSchemes called not first time, but on reload,
      // because scheme processor should use cumulative event `reloaded` to update runtime state/caches
      val schemeLoader = createSchemeLoader(isDuringLoad = true)
-      if (provider != null && provider.processChildren(fileSpec, roamingType, { canRead(it) }) { name, input, readOnly ->
+      val isLoadOnlyFromProvider = provider != null && provider.processChildren(fileSpec, roamingType, { canRead(it) }) { name, input, readOnly ->
        catchAndLog(name) {
          val scheme = schemeLoader.loadScheme(name, input)
          if (readOnly && scheme != null) {
@@ -189,9 +189,9 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
          }
        }
        true
-      }) {
      }
-      else {
+
+      if (!isLoadOnlyFromProvider) {
        ioDirectory.directoryStreamIfExists({ canRead(it.fileName.toString()) }) { directoryStream ->
          for (file in directoryStream) {
            if (file.isDirectory()) {
@@ -282,17 +282,24 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
      }
    }

+    val filesToDelete = THashSet(filesToDelete)
    for (scheme in changedSchemes) {
      try {
-        saveScheme(scheme, nameGenerator)
+        saveScheme(scheme, nameGenerator, filesToDelete)
      }
      catch (e: Throwable) {
        errors.add(RuntimeException("Cannot save scheme $fileSpec/$scheme", e))
      }
    }

-    val filesToDelete = THashSet(filesToDelete)
    if (!filesToDelete.isEmpty) {
+      val iterator = schemeToInfo.values.iterator()
+      for (info in iterator) {
+        if (filesToDelete.contains(info.fileName)) {
+          iterator.remove()
+        }
+      }
+
      this.filesToDelete.removeAll(filesToDelete)
      deleteFiles(errors, filesToDelete)
      // remove empty directory only if some file was deleted - avoid check on each save
@@ -335,12 +342,12 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
    }
  }

-  private fun saveScheme(scheme: MUTABLE_SCHEME, nameGenerator: UniqueNameGenerator) {
+  private fun saveScheme(scheme: MUTABLE_SCHEME, nameGenerator: UniqueNameGenerator, filesToDelete: MutableSet<String>) {
    var externalInfo: ExternalInfo? = schemeToInfo.get(scheme)
    val currentFileNameWithoutExtension = externalInfo?.fileNameWithoutExtension
    val element = processor.writeScheme(scheme)?.let { it as? Element ?: (it as Document).detachRootElement() }
    if (element.isEmpty()) {
-      externalInfo?.scheduleDelete()
+      externalInfo?.scheduleDelete(filesToDelete)
      return
    }

@@ -352,11 +359,13 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
    val newDigest = element!!.digest()
    when {
      externalInfo != null && currentFileNameWithoutExtension === fileNameWithoutExtension && externalInfo.isDigestEquals(newDigest) -> return
-      isEqualToBundledScheme(externalInfo, newDigest, scheme) -> return
+      isEqualToBundledScheme(externalInfo, newDigest, scheme, filesToDelete) -> return

      // we must check it only here to avoid delete old scheme just because it is empty (old idea save -> new idea delete on open)
      processor is LazySchemeProcessor && processor.isSchemeDefault(scheme, newDigest) -> {
-        externalInfo?.scheduleDelete()
+        if (externalInfo != null) {
+          filesToDelete.add(externalInfo.fileName)
+        }
        return
      }
    }
@@ -400,7 +409,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
              file = oldFile
            }
            else {
-              externalInfo.scheduleDelete()
+              externalInfo.scheduleDelete(filesToDelete)
            }
          }
        }
@@ -415,14 +424,14 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
      }
      else {
        if (renamed) {
-          externalInfo!!.scheduleDelete()
+          filesToDelete.add(externalInfo!!.fileName)
        }
        ioDirectory.resolve(fileName).write(byteOut.internalBuffer, 0, byteOut.size())
      }
    }
    else {
      if (renamed) {
-        externalInfo!!.scheduleDelete()
+        externalInfo!!.scheduleDelete(filesToDelete)
      }
      provider!!.write(providerPath, byteOut.internalBuffer, byteOut.size(), roamingType)
    }
@@ -438,7 +447,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
    externalInfo.schemeKey = processor.getSchemeKey(scheme)
  }

-  private fun isEqualToBundledScheme(externalInfo: ExternalInfo?, newDigest: ByteArray, scheme: MUTABLE_SCHEME): Boolean {
+  private fun isEqualToBundledScheme(externalInfo: ExternalInfo?, newDigest: ByteArray, scheme: MUTABLE_SCHEME, filesToDelete: MutableSet<String>): Boolean {
    fun serializeIfPossible(scheme: T): Element? {
      LOG.runAndLogException {
        @Suppress("UNCHECKED_CAST")
@@ -451,7 +460,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
    val bundledScheme = schemeListManager.readOnlyExternalizableSchemes.get(processor.getSchemeKey(scheme))
    if (bundledScheme == null) {
      if ((processor as? LazySchemeProcessor)?.isSchemeEqualToBundled(scheme) == true) {
-        externalInfo?.scheduleDelete()
+        externalInfo?.scheduleDelete(filesToDelete)
        return true
      }
      return false
@@ -464,20 +473,12 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
      } ?: return false
    }
    if (bundledExternalInfo.isDigestEquals(newDigest)) {
-      externalInfo?.scheduleDelete()
+      externalInfo?.scheduleDelete(filesToDelete)
      return true
    }
    return false
  }

-  private fun ExternalInfo.scheduleDelete() {
-    filesToDelete.add(fileName)
-  }
-
-  internal fun scheduleDelete(info: ExternalInfo) {
-    info.scheduleDelete()
-  }
-
  private fun isRenamed(scheme: T): Boolean {
    val info = schemeToInfo.get(scheme)
    return info != null && processor.getSchemeKey(scheme) != info.schemeKey
@@ -550,7 +551,7 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,
      }

      iterator.remove()
-      info.scheduleDelete()
+      info.scheduleDelete(filesToDelete)
    }
  }

@@ -558,36 +559,35 @@ class SchemeManagerImpl<T : Any, MUTABLE_SCHEME : T>(val fileSpec: String,

  override fun findSchemeByName(schemeName: String) = schemes.firstOrNull { processor.getSchemeKey(it) == schemeName }

-  override fun removeScheme(name: String) = removeFirstScheme(schemes, this) {processor.getSchemeKey(it) == name }
+  override fun removeScheme(name: String) = removeFirstScheme {processor.getSchemeKey(it) == name }

-  override fun removeScheme(scheme: T) = removeFirstScheme(schemes, this) { it == scheme } != null
+  override fun removeScheme(scheme: T) = removeFirstScheme { it == scheme } != null

  override fun isMetadataEditable(scheme: T) = !schemeListManager.readOnlyExternalizableSchemes.containsKey(processor.getSchemeKey(scheme))

  override fun toString() = fileSpec
-}

-// static method to ensure that receiver state is not used
-private fun <T : Any> removeFirstScheme(schemes: MutableList<T>, schemeManager: SchemeManagerImpl<T, *>, condition: (T) -> Boolean): T? {
-  val iterator = schemes.iterator()
-  for (scheme in iterator) {
-    if (!condition(scheme)) {
-      continue
+  private fun removeFirstScheme(condition: (T) -> Boolean): T? {
+    val iterator = schemes.iterator()
+    for (scheme in iterator) {
+      if (!condition(scheme)) {
+        continue
+      }
+
+      if (activeScheme === scheme) {
+        activeScheme = null
+      }
+
+      iterator.remove()
+
+      if (processor.isExternalizable(scheme)) {
+        schemeToInfo.remove(scheme)?.scheduleDelete(filesToDelete)
+      }
+      return scheme
    }

-    if (schemeManager.activeScheme === scheme) {
-      schemeManager.activeScheme = null
-    }
-
-    iterator.remove()
-
-    if (schemeManager.processor.isExternalizable(scheme)) {
-      schemeManager.schemeToInfo.remove(scheme)?.let(schemeManager::scheduleDelete)
-    }
-    return scheme
+    return null
  }
-
-  return null
 }

 internal fun nameIsMissed(bytes: ByteArray): RuntimeException {
@@ -46,12 +46,6 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
    return schemeToInfo.get(existingScheme) ?: schemeManager.schemeToInfo.get(existingScheme)
  }

-  private fun isFromFileWithOldExtension(existingScheme: T): Boolean {
-    val info = getInfoForExistingScheme(existingScheme)
-    // scheme from file with old extension, so, we must ignore it
-    return info != null && schemeManager.schemeExtension != info.fileExtension
-  }
-
  private fun isFromFileWithNewExtension(existingScheme: T, fileNameWithoutExtension: String): Boolean {
    return getInfoForExistingScheme(existingScheme)?.fileNameWithoutExtension == fileNameWithoutExtension
  }
@@ -64,6 +58,8 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
    schemeManager.filesToDelete.addAll(filesToDelete)
    schemeManager.filesToDelete.addAll(preScheduledFilesToDelete)

+
+
    schemeManager.schemeToInfo.putAll(schemeToInfo)

    val result = schemes.subList(newSchemesOffset, schemes.size)
@@ -105,26 +101,35 @@ internal class SchemeLoader<T : Any, MUTABLE_SCHEME : T>(private val schemeManag
      // not added to filesToDelete because it is only shadowed
      return true
    }
-    else if (processor.isExternalizable(existingScheme) && isFromFileWithOldExtension(existingScheme)) {
-      schemes.removeAt(existingSchemeIndex)
-      if (existingSchemeIndex < newSchemesOffset) {
-        newSchemesOffset--
+
+    if (processor.isExternalizable(existingScheme)) {
+      val existingInfo = getInfoForExistingScheme(existingScheme)
+      // is from file with old extension
+      if (existingInfo != null && schemeManager.schemeExtension != existingInfo.fileExtension) {
+        schemeToInfo.remove(existingScheme)
+        filesToDelete.add(existingInfo.fileName)
+
+        schemes.removeAt(existingSchemeIndex)
+        if (existingSchemeIndex < newSchemesOffset) {
+          newSchemesOffset--
+        }
+
+        // when existing loaded scheme removed, we need to remove it from schemeManager.schemeToInfo,
+        // but SchemeManager will correctly remove info on save, no need to complicate
+        return true
      }
+    }
+
+    if (schemeManager.schemeExtension != extension && isFromFileWithNewExtension(existingScheme, fileNameWithoutExtension)) {
+      // 1.oldExt is loading after 1.newExt - we should delete 1.oldExt
      filesToDelete.add(fileName)
    }
    else {
-      if (schemeManager.schemeExtension != extension && isFromFileWithNewExtension(existingScheme, fileNameWithoutExtension)) {
-        // 1.oldExt is loading after 1.newExt - we should delete 1.oldExt
-        filesToDelete.add(fileName)
-      }
-      else {
-        // We don't load scheme with duplicated name - if we generate unique name for it, it will be saved then with new name.
-        // It is not what all can expect. Such situation in most cases indicates error on previous level, so, we just warn about it.
-        LOG.warn("Scheme file \"$fileName\" is not loaded because defines duplicated name \"$schemeKey\"")
-      }
-      return false
+      // We don't load scheme with duplicated name - if we generate unique name for it, it will be saved then with new name.
+      // It is not what all can expect. Such situation in most cases indicates error on previous level, so, we just warn about it.
+      LOG.warn("Scheme file \"$fileName\" is not loaded because defines duplicated name \"$schemeKey\"")
    }
-    return true
+    return false
  }

  fun loadScheme(fileName: String, input: InputStream): MUTABLE_SCHEME? {
@@ -277,6 +282,10 @@ internal class ExternalInfo(var fileNameWithoutExtension: String, var fileExtens

  fun isDigestEquals(newDigest: ByteArray) = Arrays.equals(digest, newDigest)

+  fun scheduleDelete(filesToDelete: MutableSet<String>) {
+    filesToDelete.add(fileName)
+  }
+
  override fun toString() = fileName
 }

@@ -4,6 +4,7 @@ package com.intellij.configurationStore
 import com.intellij.configurationStore.schemeManager.SchemeFileTracker
 import com.intellij.configurationStore.schemeManager.SchemeManagerImpl
 import com.intellij.openapi.application.ApplicationManager
+import com.intellij.openapi.components.RoamingType
 import com.intellij.openapi.options.ExternalizableScheme
 import com.intellij.openapi.options.SchemeManagerFactory
 import com.intellij.openapi.util.io.FileUtil
@@ -13,10 +14,7 @@ import com.intellij.testFramework.PlatformTestUtil
 import com.intellij.testFramework.ProjectRule
 import com.intellij.testFramework.TemporaryDirectory
 import com.intellij.testFramework.runInEdtAndWait
-import com.intellij.util.io.createDirectories
-import com.intellij.util.io.directoryStreamIfExists
-import com.intellij.util.io.readText
-import com.intellij.util.io.write
+import com.intellij.util.io.*
 import com.intellij.util.loadElement
 import com.intellij.util.toByteArray
 import com.intellij.util.xmlb.annotations.Tag
@@ -27,6 +25,7 @@ import org.junit.ClassRule
 import org.junit.Rule
 import org.junit.Test
 import java.io.File
+import java.io.InputStream
 import java.nio.file.Path
 import java.util.function.Function

@@ -137,7 +136,15 @@ internal class SchemeManagerTest {
    file.write(serialize()!!.toByteArray())
  }

-  @Test fun `different extensions`() {
+  @Test fun `different extensions - old, new`() {
+    doDifferentExtensionTest(listOf("1.xml", "1.icls"))
+  }
+
+  @Test fun `different extensions - new, old`() {
+    doDifferentExtensionTest(listOf("1.icls", "1.xml"))
+  }
+
+  private fun doDifferentExtensionTest(fileNames: List<String>) {
    val dir = tempDirManager.newPath()

    val scheme = TestScheme("local", "true")
@@ -148,15 +155,44 @@ internal class SchemeManagerTest {
      override val schemeExtension = ".icls"
    }

-    val schemesManager = SchemeManagerImpl(FILE_SPEC, ATestSchemesProcessor(), null, dir)
-    schemesManager.loadSchemes()
-    assertThat(schemesManager.allSchemes).containsOnly(scheme)
+    // use provider to specify exact order of files (it is critical to test both variants - old, new or new, old)
+    val schemeManager = SchemeManagerImpl(FILE_SPEC, ATestSchemesProcessor(), object : StreamProvider {
+      override fun write(fileSpec: String, content: ByteArray, size: Int, roamingType: RoamingType) {
+        getFile(fileSpec).write(content, 0, size)
+      }
+
+      override fun read(fileSpec: String, roamingType: RoamingType, consumer: (InputStream?) -> Unit): Boolean {
+        getFile(fileSpec).inputStream().use(consumer)
+        return true
+      }
+
+      override fun processChildren(path: String,
+                                   roamingType: RoamingType,
+                                   filter: (name: String) -> Boolean,
+                                   processor: (name: String, input: InputStream, readOnly: Boolean) -> Boolean): Boolean {
+        for (name in fileNames) {
+          dir.resolve(name).inputStream().use {
+            processor(name, it, false)
+          }
+        }
+        return true
+      }
+
+      override fun delete(fileSpec: String, roamingType: RoamingType): Boolean {
+        getFile(fileSpec).delete()
+        return true
+      }
+
+      private fun getFile(fileSpec: String) = dir.resolve(fileSpec.substring(FILE_SPEC.length + 1))
+    }, dir)
+    schemeManager.loadSchemes()
+    assertThat(schemeManager.allSchemes).containsOnly(scheme)

    assertThat(dir.resolve("1.icls")).isRegularFile()
    assertThat(dir.resolve("1.xml")).isRegularFile()

    scheme.data = "newTrue"
-    schemesManager.save()
+    schemeManager.save()

    assertThat(dir.resolve("1.icls")).isRegularFile()
    assertThat(dir.resolve("1.xml")).doesNotExist()