From 644c7c340b1483d1ddf227be82023d25d64df294 Mon Sep 17 00:00:00 2001 From: Konstantin Date: Wed, 24 Apr 2024 16:47:05 +0200 Subject: [PATCH] [gradle] Add validation checks on invalid xml or item type. (#4680) If a project has invalid value xml files (empty/broken content or duplicated keys) then gradle will show an error. fixes https://github.com/JetBrains/compose-multiplatform/issues/4663 --- .github/workflows/gradle-plugin.yml | 2 +- components/gradle.properties | 2 +- .../resources/ComposeResourcesGeneration.kt | 2 +- .../compose/resources/GenerateResClassTask.kt | 62 +++++++------ .../GenerateResourceAccessorsTask.kt | 74 ++++++++-------- .../resources/PrepareComposeResources.kt | 61 +++++++++---- .../test/tests/integration/ResourcesTest.kt | 86 +++++++++++++++++-- 7 files changed, 195 insertions(+), 94 deletions(-) diff --git a/.github/workflows/gradle-plugin.yml b/.github/workflows/gradle-plugin.yml index b7ad41c330..0a0e81b92a 100644 --- a/.github/workflows/gradle-plugin.yml +++ b/.github/workflows/gradle-plugin.yml @@ -16,7 +16,7 @@ jobs: strategy: fail-fast: false matrix: - os: [ubuntu-20.04, macos-12, windows-2022] + os: [ubuntu-20.04, macos-14, windows-2022] gradle: [7.4, 8.7] agp: [7.3.1, 8.3.1] runs-on: ${{ matrix.os }} diff --git a/components/gradle.properties b/components/gradle.properties index 9b1f014bb7..8604e6cf0e 100644 --- a/components/gradle.properties +++ b/components/gradle.properties @@ -8,7 +8,7 @@ android.useAndroidX=true #Versions kotlin.version=1.9.23 -compose.version=1.6.10-dev1596 +compose.version=1.6.10-beta02 agp.version=8.2.2 #Compose diff --git a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/ComposeResourcesGeneration.kt b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/ComposeResourcesGeneration.kt index 56b989ebdd..3304eea4a7 100644 --- a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/ComposeResourcesGeneration.kt +++ b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/ComposeResourcesGeneration.kt @@ -68,7 +68,7 @@ internal fun Project.configureComposeResourcesGeneration( //setup task execution during IDE import tasks.configureEach { importTask -> if (importTask.name == "prepareKotlinIdeaImport") { - importTask.dependsOn(tasks.withType(CodeGenerationTask::class.java)) + importTask.dependsOn(tasks.withType(IdeaImportTask::class.java)) } } } diff --git a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResClassTask.kt b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResClassTask.kt index e0202be20b..bc4155b4e0 100644 --- a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResClassTask.kt +++ b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResClassTask.kt @@ -2,18 +2,38 @@ package org.jetbrains.compose.resources import org.gradle.api.DefaultTask import org.gradle.api.file.DirectoryProperty -import org.gradle.api.file.RegularFile import org.gradle.api.provider.Property import org.gradle.api.provider.Provider -import org.gradle.api.tasks.* +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.Optional +import org.gradle.api.tasks.OutputDirectory +import org.gradle.api.tasks.TaskAction import java.io.File /** * This task should be FAST and SAFE! Because it is being run during IDE import. */ -internal abstract class CodeGenerationTask : DefaultTask() +internal abstract class IdeaImportTask : DefaultTask() { + @get:Input + val ideaIsInSync: Provider = project.provider { + System.getProperty("idea.sync.active", "false").toBoolean() + } + + @TaskAction + fun run() { + try { + safeAction() + } catch (e: Exception) { + //message must contain two ':' symbols to be parsed by IDE UI! + logger.error("e: $name task was failed:", e) + if (!ideaIsInSync.get()) throw e + } + } -internal abstract class GenerateResClassTask : CodeGenerationTask() { + abstract fun safeAction() +} + +internal abstract class GenerateResClassTask : IdeaImportTask() { companion object { private const val RES_FILE_NAME = "Res" } @@ -34,26 +54,20 @@ internal abstract class GenerateResClassTask : CodeGenerationTask() { @get:OutputDirectory abstract val codeDir: DirectoryProperty - @TaskAction - fun generate() { - try { - val dir = codeDir.get().asFile - dir.deleteRecursively() - dir.mkdirs() - - if (shouldGenerateCode.get()) { - logger.info("Generate $RES_FILE_NAME.kt") - - val pkgName = packageName.get() - val moduleDirectory = packagingDir.getOrNull()?.let { it.invariantSeparatorsPath + "/" } ?: "" - val isPublic = makeAccessorsPublic.get() - getResFileSpec(pkgName, RES_FILE_NAME, moduleDirectory, isPublic).writeTo(dir) - } else { - logger.info("Generation Res class is disabled") - } - } catch (e: Exception) { - //message must contain two ':' symbols to be parsed by IDE UI! - logger.error("e: $name task was failed:", e) + override fun safeAction() { + val dir = codeDir.get().asFile + dir.deleteRecursively() + dir.mkdirs() + + if (shouldGenerateCode.get()) { + logger.info("Generate $RES_FILE_NAME.kt") + + val pkgName = packageName.get() + val moduleDirectory = packagingDir.getOrNull()?.let { it.invariantSeparatorsPath + "/" } ?: "" + val isPublic = makeAccessorsPublic.get() + getResFileSpec(pkgName, RES_FILE_NAME, moduleDirectory, isPublic).writeTo(dir) + } else { + logger.info("Generation Res class is disabled") } } } diff --git a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt index fd997ed03b..18940b68eb 100644 --- a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt +++ b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/GenerateResourceAccessorsTask.kt @@ -14,7 +14,7 @@ import java.io.File import java.nio.file.Path import kotlin.io.path.relativeTo -internal abstract class GenerateResourceAccessorsTask : CodeGenerationTask() { +internal abstract class GenerateResourceAccessorsTask : IdeaImportTask() { @get:Input abstract val packageName: Property @@ -39,51 +39,45 @@ internal abstract class GenerateResourceAccessorsTask : CodeGenerationTask() { @get:OutputDirectory abstract val codeDir: DirectoryProperty - @TaskAction - fun generate() { - try { - val kotlinDir = codeDir.get().asFile - val rootResDir = resDir.get() - val sourceSet = sourceSetName.get() + override fun safeAction() { + val kotlinDir = codeDir.get().asFile + val rootResDir = resDir.get() + val sourceSet = sourceSetName.get() - logger.info("Clean directory $kotlinDir") - kotlinDir.deleteRecursively() - kotlinDir.mkdirs() + logger.info("Clean directory $kotlinDir") + kotlinDir.deleteRecursively() + kotlinDir.mkdirs() - if (shouldGenerateCode.get()) { - logger.info("Generate accessors for $rootResDir") + if (shouldGenerateCode.get()) { + logger.info("Generate accessors for $rootResDir") - //get first level dirs - val dirs = rootResDir.listNotHiddenFiles() + //get first level dirs + val dirs = rootResDir.listNotHiddenFiles() - dirs.forEach { f -> - if (!f.isDirectory) { - error("${f.name} is not directory! Raw files should be placed in '${rootResDir.name}/files' directory.") - } + dirs.forEach { f -> + if (!f.isDirectory) { + error("${f.name} is not directory! Raw files should be placed in '${rootResDir.name}/files' directory.") } - - //type -> id -> resource item - val resources: Map>> = dirs - .flatMap { dir -> - dir.listNotHiddenFiles() - .mapNotNull { it.fileToResourceItems(rootResDir.toPath()) } - .flatten() - } - .groupBy { it.type } - .mapValues { (_, items) -> items.groupBy { it.name } } - - val pkgName = packageName.get() - val moduleDirectory = packagingDir.getOrNull()?.let { it.invariantSeparatorsPath + "/" } ?: "" - val isPublic = makeAccessorsPublic.get() - getAccessorsSpecs( - resources, pkgName, sourceSet, moduleDirectory, isPublic - ).forEach { it.writeTo(kotlinDir) } - } else { - logger.info("Generation accessors for $rootResDir is disabled") } - } catch (e: Exception) { - //message must contain two ':' symbols to be parsed by IDE UI! - logger.error("e: $name task was failed:", e) + + //type -> id -> resource item + val resources: Map>> = dirs + .flatMap { dir -> + dir.listNotHiddenFiles() + .mapNotNull { it.fileToResourceItems(rootResDir.toPath()) } + .flatten() + } + .groupBy { it.type } + .mapValues { (_, items) -> items.groupBy { it.name } } + + val pkgName = packageName.get() + val moduleDirectory = packagingDir.getOrNull()?.let { it.invariantSeparatorsPath + "/" } ?: "" + val isPublic = makeAccessorsPublic.get() + getAccessorsSpecs( + resources, pkgName, sourceSet, moduleDirectory, isPublic + ).forEach { it.writeTo(kotlinDir) } + } else { + logger.info("Generation accessors for $rootResDir is disabled") } } diff --git a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/PrepareComposeResources.kt b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/PrepareComposeResources.kt index a6e5460ab0..07cdc50093 100644 --- a/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/PrepareComposeResources.kt +++ b/gradle-plugins/compose/src/main/kotlin/org/jetbrains/compose/resources/PrepareComposeResources.kt @@ -1,14 +1,23 @@ package org.jetbrains.compose.resources -import org.gradle.api.DefaultTask import org.gradle.api.Project -import org.gradle.api.file.* +import org.gradle.api.file.DirectoryProperty +import org.gradle.api.file.FileSystemOperations +import org.gradle.api.file.FileTree import org.gradle.api.provider.Property import org.gradle.api.provider.Provider -import org.gradle.api.tasks.* +import org.gradle.api.tasks.IgnoreEmptyDirectories +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.Internal +import org.gradle.api.tasks.OutputDirectory +import org.gradle.api.tasks.OutputFiles +import org.gradle.api.tasks.SkipWhenEmpty +import org.gradle.api.tasks.TaskProvider import org.jetbrains.compose.internal.utils.uppercaseFirstChar import org.jetbrains.kotlin.gradle.plugin.KotlinSourceSet import org.w3c.dom.Node +import org.xml.sax.SAXParseException import java.io.File import java.util.* import javax.inject.Inject @@ -60,7 +69,7 @@ internal fun Project.getPreparedComposeResourcesDir(sourceSet: KotlinSourceSet): private fun getPrepareComposeResourcesTaskName(sourceSet: KotlinSourceSet) = "prepareComposeResourcesTaskFor${sourceSet.name.uppercaseFirstChar()}" -internal abstract class CopyNonXmlValueResourcesTask : DefaultTask() { +internal abstract class CopyNonXmlValueResourcesTask : IdeaImportTask() { @get:Inject abstract val fileSystem: FileSystemOperations @@ -82,8 +91,7 @@ internal abstract class CopyNonXmlValueResourcesTask : DefaultTask() { dir.asFileTree.matching { it.exclude("values*/*.${XmlValuesConverterTask.CONVERTED_RESOURCE_EXT}") } } - @TaskAction - fun run() { + override fun safeAction() { realOutputFiles.get().forEach { f -> f.delete() } fileSystem.copy { copy -> copy.includeEmptyDirs = false @@ -95,7 +103,7 @@ internal abstract class CopyNonXmlValueResourcesTask : DefaultTask() { } } -internal abstract class PrepareComposeResourcesTask : DefaultTask() { +internal abstract class PrepareComposeResourcesTask : IdeaImportTask() { @get:InputFiles @get:SkipWhenEmpty @get:IgnoreEmptyDirectories @@ -109,8 +117,7 @@ internal abstract class PrepareComposeResourcesTask : DefaultTask() { @get:OutputDirectory abstract val outputDir: DirectoryProperty - @TaskAction - fun run() = Unit + override fun safeAction() = Unit } internal data class ValueResourceRecord( @@ -135,7 +142,7 @@ internal data class ValueResourceRecord( } } -internal abstract class XmlValuesConverterTask : DefaultTask() { +internal abstract class XmlValuesConverterTask : IdeaImportTask() { companion object { const val CONVERTED_RESOURCE_EXT = "cvr" //Compose Value Resource private const val FORMAT_VERSION = 0 @@ -163,8 +170,7 @@ internal abstract class XmlValuesConverterTask : DefaultTask() { dir.asFileTree.matching { it.include("values*/*.$suffix.$CONVERTED_RESOURCE_EXT") } } - @TaskAction - fun run() { + override fun safeAction() { val outDir = outputDir.get().asFile val suffix = fileSuffix.get() realOutputFiles.get().forEach { f -> f.delete() } @@ -176,7 +182,13 @@ internal abstract class XmlValuesConverterTask : DefaultTask() { .resolve(f.parentFile.name) .resolve(f.nameWithoutExtension + ".$suffix.$CONVERTED_RESOURCE_EXT") output.parentFile.mkdirs() - convert(f, output) + try { + convert(f, output) + } catch (e: SAXParseException) { + error("XML file ${f.absolutePath} is not valid. Check the file content.") + } catch (e: Exception) { + error("XML file ${f.absolutePath} is not valid. ${e.message}") + } } } } @@ -186,17 +198,28 @@ internal abstract class XmlValuesConverterTask : DefaultTask() { private fun convert(original: File, converted: File) { val doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(original) val items = doc.getElementsByTagName("resources").item(0).childNodes - val records = List(items.length) { items.item(it) }.mapNotNull { getItemRecord(it)?.getAsString() } + val records = List(items.length) { items.item(it) } + .filter { it.hasAttributes() } + .map { getItemRecord(it) } + + //check there are no duplicates type + key + records.groupBy { it.key } + .filter { it.value.size > 1 } + .forEach { (key, records) -> + val allTypes = records.map { it.type } + require(allTypes.size == allTypes.toSet().size) { "Duplicated key '$key'." } + } + val fileContent = buildString { appendLine("version:$FORMAT_VERSION") - records.sorted().forEach { appendLine(it) } + records.map { it.getAsString() }.sorted().forEach { appendLine(it) } } converted.writeText(fileContent) } - private fun getItemRecord(node: Node): ValueResourceRecord? { - val type = ResourceType.fromString(node.nodeName) ?: return null - val key = node.attributes.getNamedItem("name").nodeValue + private fun getItemRecord(node: Node): ValueResourceRecord { + val type = ResourceType.fromString(node.nodeName) ?: error("Unknown resource type: '${node.nodeName}'.") + val key = node.attributes.getNamedItem("name")?.nodeValue ?: error("Attribute 'name' not found.") val value: String when (type) { ResourceType.STRING -> { @@ -225,7 +248,7 @@ internal abstract class XmlValuesConverterTask : DefaultTask() { } } - else -> error("Unknown string resource type: '$type'") + else -> error("Unknown string resource type: '$type'.") } return ValueResourceRecord(type, key, value) } diff --git a/gradle-plugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/ResourcesTest.kt b/gradle-plugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/ResourcesTest.kt index 91de2bbb88..8dfbd0c527 100644 --- a/gradle-plugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/ResourcesTest.kt +++ b/gradle-plugins/compose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/ResourcesTest.kt @@ -12,6 +12,24 @@ import kotlin.io.path.relativeTo import kotlin.test.* class ResourcesTest : GradlePluginTestBase() { + @Test + fun testSafeImport() { + with(testProject("misc/commonResources")) { + file("src/commonMain/composeResources/drawable-en").renameTo( + file("src/commonMain/composeResources/drawable-rent") + ) + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("e: generateResourceAccessorsForCommonMain task was failed:") + check.logContains("contains unknown qualifier: 'rent'.") + } + + gradle("prepareKotlinIdeaImport", "-Didea.sync.active=true").checks { + check.logContains("e: generateResourceAccessorsForCommonMain task was failed:") + check.logContains("contains unknown qualifier: 'rent'.") + } + } + } + @Test fun testGeneratedAccessors(): Unit = with(testProject("misc/commonResources")) { //check generated resource's accessors @@ -36,7 +54,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable-en").renameTo( file("src/commonMain/composeResources/drawable-rent") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ contains unknown qualifier: 'rent'. @@ -47,7 +65,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable-rent").renameTo( file("src/commonMain/composeResources/drawable-rUS-en") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ Region qualifier must be declared after language: 'en-rUS'. @@ -58,7 +76,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable-rUS-en").renameTo( file("src/commonMain/composeResources/drawable-rUS") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ Region qualifier must be used only with language. @@ -69,7 +87,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable-rUS").renameTo( file("src/commonMain/composeResources/drawable-en-fr") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ contains repetitive qualifiers: 'en' and 'fr'. @@ -80,7 +98,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable-en-fr").renameTo( file("src/commonMain/composeResources/image") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ Unknown resource type: 'image' @@ -91,7 +109,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/image").renameTo( file("src/commonMain/composeResources/files-de") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ The 'files' directory doesn't support qualifiers: 'files-de'. @@ -102,7 +120,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/files-de").renameTo( file("src/commonMain/composeResources/strings") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ Unknown resource type: 'strings'. @@ -113,7 +131,7 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/strings").renameTo( file("src/commonMain/composeResources/string-us") ) - gradle("prepareKotlinIdeaImport").checks { + gradleFailure("prepareKotlinIdeaImport").checks { check.logContains( """ Forbidden directory name 'string-us'! String resources should be declared in 'values/strings.xml'. @@ -129,6 +147,58 @@ class ResourcesTest : GradlePluginTestBase() { file("src/commonMain/composeResources/drawable/vector_2.xml") ) + val testXml = file("src/commonMain/composeResources/values/test.xml") + testXml.writeText("") + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Check the file content.") + } + + testXml.writeText("invalid") + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Check the file content.") + } + + testXml.writeText(""" + + aaa + + """.trimIndent()) + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Unknown resource type: 'aaa'.") + } + + testXml.writeText(""" + + aaa + + """.trimIndent()) + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Unknown string resource type: 'drawable'.") + } + + testXml.writeText(""" + + aaa + aaa + aaa + aaa + + """.trimIndent()) + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Duplicated key 'v1'.") + } + + testXml.writeText(""" + + aaa + aaa + + """.trimIndent()) + gradleFailure("prepareKotlinIdeaImport").checks { + check.logContains("${testXml.name} is not valid. Attribute 'name' not found.") + } + testXml.delete() + file("build.gradle.kts").modify { txt -> txt + """ compose.resources {