diff --git a/README.md b/README.md index 783bd91..2e57683 100644 --- a/README.md +++ b/README.md @@ -38,6 +38,34 @@ java.lang.IllegalArgumentException: SavedStateProvider with the given key is alr Avoid using the `defaultComponentContext` function inside Composable functions. +### SerializableDiscriminatorRule + +Decompose uses the kotlinx.serialization library to serialize component configurations. +A common mistake is naming a property the same as the class discriminator (default: "type"). For example: + +```kotlin +import kotlinx.serialization.Serializable + +@Serializable +sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + val type: String // DON'T DO THIS + ) : ScreenConfig() +} +``` + +If Decompose attempts to serialize this component configuration, you will encounter the following exception: +``` +java.lang.IllegalStateException: Sealed class 'ScreenConfig.Details' cannot be serialized as base class 'ScreenConfig' because it has property name that conflicts with JSON class discriminator 'type'. +``` + +Do not name properties in polymorphic serializable classes as a class discriminator. + ## Enabling rules By default, all rules are enabled, but you can configure them in your `detekt.yml` configuration file: @@ -45,4 +73,7 @@ By default, all rules are enabled, but you can configure them in your `detekt.ym DecomposeRuleSet: DecomposeComponentContextRule: active: true + SerializableDiscriminatorRule: + active: true + classDiscriminator: 'type' # optional ``` diff --git a/gradle.properties b/gradle.properties index 99bf1b5..54af6b4 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ kotlin.code.style=official GROUP=io.github.ajiekcx.detekt -VERSION_NAME=0.1.0 +VERSION_NAME=0.2.0 diff --git a/src/main/kotlin/io/github/ajiekcx/detekt/DecomposeRuleSetProvider.kt b/src/main/kotlin/io/github/ajiekcx/detekt/DecomposeRuleSetProvider.kt index c061ae6..b3a4390 100644 --- a/src/main/kotlin/io/github/ajiekcx/detekt/DecomposeRuleSetProvider.kt +++ b/src/main/kotlin/io/github/ajiekcx/detekt/DecomposeRuleSetProvider.kt @@ -11,7 +11,8 @@ class DecomposeRuleSetProvider : RuleSetProvider { return RuleSet( ruleSetId, listOf( - DecomposeComponentContextRule(config) + DecomposeComponentContextRule(config), + SerializableDiscriminatorRule(config) ), ) } diff --git a/src/main/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRule.kt b/src/main/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRule.kt new file mode 100644 index 0000000..1f10a59 --- /dev/null +++ b/src/main/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRule.kt @@ -0,0 +1,156 @@ +package io.github.ajiekcx.detekt + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtFile +import org.jetbrains.kotlin.psi.KtNamedDeclaration +import org.jetbrains.kotlin.psi.psiUtil.findPropertyByName + +/** + * A Detekt rule that checks for properties named as class discriminators + * in polymorphic serializable classes. It prevents naming properties + * using a reserved discriminator (e.g., "type") to avoid serialization issues. + * + * @see https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md + */ +class SerializableDiscriminatorRule(config: Config) : Rule(config) { + // Retrieves the forbidden property name from the configuration (default is "type") + private val classDiscriminator = config + .subConfig("SerializableDiscriminatorRule") + .valueOrDefault("classDiscriminator", "type") + + override val issue = Issue( + javaClass.simpleName, + Severity.CodeSmell, + "Do not name properties in polymorphic serializable classes as a class discriminator: $classDiscriminator", + Debt.FIVE_MINS + ) + + /** + * Visits the entire Kotlin file and checks for serializable sealed class hierarchies. + * It ensures that subclasses of sealed serializable classes do not contain a property + * named as the forbidden class discriminator. + */ + override fun visitKtFile(file: KtFile) { + super.visitKtFile(file) + + // Skip processing if the file does not import kotlinx.serialization.Serializable + if (!file.hasSerializableImport()) return + + // Iterate through all declarations in the file, filtering only sealed serializable classes + file.declarations + .filterIsInstance() + .filter { it.isSealed() && it.isSerializable() } + .forEach { sealedClass -> + // Find all serializable subclasses of the sealed class and check for violations + file.declarations + .filterIsInstance() + .filter { it.isSubclassOf(sealedClass) && it.isSerializable() } + .forEach { it.findAndReportClassDiscriminatorProperty() } + } + } + + /** + * Visits each class declaration and checks for forbidden properties if the class + * is a sealed and contains nested serializable classes. + */ + override fun visitClass(klass: KtClass) { + super.visitClass(klass) + + // Skip processing if the containing file does not import kotlinx.serialization.Serializable + if (!klass.containingKtFile.hasSerializableImport()) return + + // If the class is a sealed class and is marked as @Serializable + if (klass.isSealed() && klass.isSerializable()) { + // Check nested classes inside the sealed class + klass.declarations + .filterIsInstance() + .filter { it.isSerializable() } + .forEach { nestedClass -> + nestedClass.findAndReportClassDiscriminatorProperty() + } + } + } + + /** + * Checks whether a Kotlin file has the required import for kotlinx.serialization.Serializable. + */ + private fun KtFile.hasSerializableImport(): Boolean { + return hasImport("import kotlinx.serialization.Serializable") + } + + /** + * Finds and reports properties that match the forbidden class discriminator name. + */ + private fun KtClass.findAndReportClassDiscriminatorProperty() { + // If a property is explicitly annotated with @SerialName(classDiscriminator), report an issue + if (findPropertyWithSerialName(classDiscriminator)) { + reportIssue() + + return + } + + // Otherwise, check if the property exists by name + val property = findPropertyByName(classDiscriminator) ?: return + + // If the property has a different @SerialName annotation, do not report it + if (property.hasSerialNameWithoutClassDiscriminator()) return + + // Report an issue if the property is named as the class discriminator + reportIssue() + } + + /** + * Checks if a class has a primary constructor parameter with @SerialName matching the forbidden name. + */ + private fun KtClass.findPropertyWithSerialName(name: String): Boolean { + // Ensure the necessary import for @SerialName exists + val hasImport = containingKtFile.hasSerialNameImport() + + return hasImport && primaryConstructorParameters.any { param -> + param.hasValOrVar() && param.annotationEntries.any { it.text == "@SerialName(\"$name\")" } + } + } + + /** + * Checks if a named property has an @SerialName annotation that does not match the forbidden discriminator. + */ + private fun KtNamedDeclaration.hasSerialNameWithoutClassDiscriminator(): Boolean { + val hasImport = containingKtFile.hasSerialNameImport() + if (!hasImport) return false + + val serialNameAnnotation = annotationEntries.find { it.text.startsWith("@SerialName") } + + return serialNameAnnotation != null + && serialNameAnnotation.text != "@SerialName(\"$classDiscriminator\")" + } + + /** + * Checks whether the Kotlin file has an import statement for kotlinx.serialization.SerialName. + */ + private fun KtFile.hasSerialNameImport(): Boolean { + return hasImport("import kotlinx.serialization.SerialName") + } + + /** + * Reports a code smell issue when a forbidden class discriminator property is found. + */ + private fun KtClass.reportIssue() { + report( + CodeSmell( + issue, + Entity.from(this), + """ + Do not name properties in polymorphic serializable classes as a class discriminator: $classDiscriminator" + See https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/polymorphism.md + """.trimIndent() + ) + ) + } +} diff --git a/src/main/kotlin/io/github/ajiekcx/detekt/Utils.kt b/src/main/kotlin/io/github/ajiekcx/detekt/Utils.kt index 91ff670..821ff7d 100644 --- a/src/main/kotlin/io/github/ajiekcx/detekt/Utils.kt +++ b/src/main/kotlin/io/github/ajiekcx/detekt/Utils.kt @@ -55,6 +55,22 @@ internal fun KtClass.isFragment(): Boolean { return hasSuperTypeEndingWith("Fragment") } +/** + * Determines if a class has the @Serializable annotation. + */ +internal fun KtClass.isSerializable(): Boolean { + return annotationEntries.any { it.text.startsWith("@Serializable") } +} + +/** + * Checks if a class is a direct subclass of a given class. + */ +internal fun KtClass.isSubclassOf(klass: KtClass): Boolean { + return superTypeListEntries + .mapNotNull { it.typeReference?.text } + .any { it == klass.name } +} + /** * Checks if the class has a superclass that ends with a given suffix. * diff --git a/src/main/resources/config/config.yml b/src/main/resources/config/config.yml index 1c181a0..fb641c8 100644 --- a/src/main/resources/config/config.yml +++ b/src/main/resources/config/config.yml @@ -1,3 +1,6 @@ DecomposeRuleSet: DecomposeComponentContextRule: active: true + SerializableDiscriminatorRule: + active: true + classDiscriminator: 'type' diff --git a/src/test/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRuleTest.kt b/src/test/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRuleTest.kt new file mode 100644 index 0000000..43558ac --- /dev/null +++ b/src/test/kotlin/io/github/ajiekcx/detekt/SerializableDiscriminatorRuleTest.kt @@ -0,0 +1,339 @@ +package io.github.ajiekcx.detekt + +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.rules.KotlinCoreEnvironmentTest +import io.gitlab.arturbosch.detekt.test.compileAndLintWithContext +import io.kotest.matchers.collections.shouldHaveSize +import org.jetbrains.kotlin.cli.jvm.compiler.KotlinCoreEnvironment +import org.junit.jupiter.api.Test + +@KotlinCoreEnvironmentTest +internal class SerializableDiscriminatorRuleTest(private val env: KotlinCoreEnvironment) { + private val defaultRule = SerializableDiscriminatorRule(Config.empty) + + @Test + fun `reports type property in serializable class`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + val type: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property in serializable private class`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + val type: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property in serializable class with custom config`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + val customDiscriminator: String + ) : ScreenConfig() + } + """ + val rule = SerializableDiscriminatorRule(CustomConfig("customDiscriminator")) + val findings = rule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property in serializable interface in file`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed interface ScreenConfig + + @Serializable + data object InputConfig : ScreenConfig + + @Serializable + data class DetailsConfig( + val message: String, + val type: String + ) : ScreenConfig + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property in serializable class in file`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig + + @Serializable + data object InputConfig : ScreenConfig() + + @Serializable + data class DetailsConfig( + val message: String, + val type: String + ) : ScreenConfig() + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property in serializable interface`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed interface ScreenConfig { + @Serializable + data object Input : ScreenConfig + + @Serializable + data class Details( + val message: String, + val type: String + ) : ScreenConfig + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `reports type property with the same SerialName in serializable class`() { + val code = """ + import kotlinx.serialization.Serializable + import kotlinx.serialization.SerialName + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + @SerialName("type") + val type: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `doesn't report type property in serializable class in file without supertype`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig + + @Serializable + data object InputConfig : ScreenConfig() + + @Serializable + data class DetailsConfig( + val message: String, + val type: String + ) + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `doesn't report without type property in serializable class`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `doesn't report without type property in serializable class in file`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed class ScreenConfig + + @Serializable + data object InputConfig : ScreenConfig() + + @Serializable + data class DetailsConfig( + val message: String + ) : ScreenConfig() + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `doesn't report without type property in serializable interface`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + sealed interface ScreenConfig { + @Serializable + data object Input : ScreenConfig + + @Serializable + data class Details( + val message: String + ) : ScreenConfig + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `doesn't report type property with different SerialName in serializable class`() { + val code = """ + import kotlinx.serialization.Serializable + import kotlinx.serialization.SerialName + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + @SerialName("t") + val type: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `reports property with type SerialName in serializable class`() { + val code = """ + import kotlinx.serialization.Serializable + import kotlinx.serialization.SerialName + + @Serializable + sealed class ScreenConfig { + @Serializable + data object Input : ScreenConfig() + + @Serializable + data class Details( + val message: String, + @SerialName("type") + val messageType: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 1 + } + + @Test + fun `doesn't report type property in serializable data class`() { + val code = """ + import kotlinx.serialization.Serializable + + @Serializable + data class Details( + val message: String, + val type: String + ) + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + @Test + fun `doesn't report type property in non serializable class`() { + val code = """ + sealed class ScreenConfig { + data object Input : ScreenConfig() + + data class Details( + val message: String, + val type: String + ) : ScreenConfig() + } + """ + val findings = defaultRule.compileAndLintWithContext(env, code) + findings shouldHaveSize 0 + } + + private class CustomConfig(private val classDiscriminator: String) : Config { + override fun subConfig(key: String): CustomConfig = this + + @Suppress("UNCHECKED_CAST") + override fun valueOrNull(key: String): T? = when (key) { + Config.ACTIVE_KEY -> true as? T + else -> null + } + + override fun valueOrDefault(key: String, default: T): T { + if (key == "classDiscriminator") { + return classDiscriminator as T + } + + return super.valueOrDefault(key, default) + } + } +}