diff --git a/platform/configuration-store-impl/src/schemeManager/SchemeManagerImpl.kt b/platform/configuration-store-impl/src/schemeManager/SchemeManagerImpl.kt index e1c26cf07b1d..fef027a4aee7 100644 --- a/platform/configuration-store-impl/src/schemeManager/SchemeManagerImpl.kt +++ b/platform/configuration-store-impl/src/schemeManager/SchemeManagerImpl.kt @@ -181,7 +181,7 @@ class SchemeManagerImpl(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(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(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(val fileSpec: String, } } - private fun saveScheme(scheme: MUTABLE_SCHEME, nameGenerator: UniqueNameGenerator) { + private fun saveScheme(scheme: MUTABLE_SCHEME, nameGenerator: UniqueNameGenerator, filesToDelete: MutableSet) { 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(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(val fileSpec: String, file = oldFile } else { - externalInfo.scheduleDelete() + externalInfo.scheduleDelete(filesToDelete) } } } @@ -415,14 +424,14 @@ class SchemeManagerImpl(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(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): Boolean { fun serializeIfPossible(scheme: T): Element? { LOG.runAndLogException { @Suppress("UNCHECKED_CAST") @@ -451,7 +460,7 @@ class SchemeManagerImpl(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(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(val fileSpec: String, } iterator.remove() - info.scheduleDelete() + info.scheduleDelete(filesToDelete) } } @@ -558,36 +559,35 @@ class SchemeManagerImpl(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 removeFirstScheme(schemes: MutableList, schemeManager: SchemeManagerImpl, 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 { diff --git a/platform/configuration-store-impl/src/schemeManager/schemeLoader.kt b/platform/configuration-store-impl/src/schemeManager/schemeLoader.kt index be3c6a74fcbe..3e763fd6d3b9 100644 --- a/platform/configuration-store-impl/src/schemeManager/schemeLoader.kt +++ b/platform/configuration-store-impl/src/schemeManager/schemeLoader.kt @@ -46,12 +46,6 @@ internal class SchemeLoader(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(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(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) { + filesToDelete.add(fileName) + } + override fun toString() = fileName } diff --git a/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt b/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt index b5563cf4f551..4734b5673b20 100644 --- a/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt +++ b/platform/configuration-store-impl/testSrc/SchemeManagerTest.kt @@ -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) { 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()