Skip to content

Commit

Permalink
Update the error location
Browse files Browse the repository at this point in the history 
Error will be highlighted only at class location when `serialVersionUID` is missing. In case of wrong type, or modifier of `serialVersionUID` error will be highlighted at property declaration
Add TC asserting location and error msg
Add TC for nested classes
  • Loading branch information
@atulgpt
atulgpt committed May 20, 2023
1 parent 01fa957 commit cc494e7
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 21 deletions.
67 changes: 51 additions & 16 deletions ...ain/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import io.gitlab.arturbosch.detekt.rules.isConstant
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.KtClassOrObject
import org.jetbrains.kotlin.psi.KtConstantExpression
import org.jetbrains.kotlin.psi.KtNamedDeclaration
import org.jetbrains.kotlin.psi.KtObjectDeclaration
import org.jetbrains.kotlin.psi.KtPrefixExpression
import org.jetbrains.kotlin.psi.KtProperty
Expand Down Expand Up @@ -54,47 +55,75 @@ class SerialVersionUIDInSerializableClass(config: Config = Config.empty) : Rule(
)

override fun visitClass(klass: KtClass) {
super.visitClass(klass)
if (!klass.isInterface() && isImplementingSerializable(klass)) {
val companionObject = klass.companionObject()
if (companionObject == null || !hasCorrectSerialVersionUUID(companionObject)) {
if (companionObject == null) {
report(
CodeSmell(
issue,
Entity.from(klass),
"The class ${klass.nameAsSafeName} implements" +
" the `Serializable` interface and should thus define a `serialVersionUID`."
Entity.atName(klass),
klass.getIssueMessage("class")
)
)
} else {
val finding = hasCorrectSerialVersionUID(companionObject, klass) ?: return
searchForSerialVersionUIDAndReport(finding)
}
}
super.visitClass(klass)
}

override fun visitObjectDeclaration(declaration: KtObjectDeclaration) {
if (!declaration.isCompanion() &&
isImplementingSerializable(declaration) &&
!hasCorrectSerialVersionUUID(declaration)
) {
super.visitObjectDeclaration(declaration)
if (!declaration.isCompanion() && isImplementingSerializable(declaration)) {
val finding = hasCorrectSerialVersionUID(declaration) ?: return
searchForSerialVersionUIDAndReport(finding)
}
}

private fun searchForSerialVersionUIDAndReport(finding: SerialVersionUIDFindings) {
if (finding.isValid.not()) {
report(
CodeSmell(
issue,
Entity.from(declaration),
"The object ${declaration.nameAsSafeName} " +
"implements the `Serializable` interface and should thus define a `serialVersionUID`."
Entity.atName(finding.violatingElement),
finding.issueMsg
)
)
}
super.visitObjectDeclaration(declaration)
}

private fun isImplementingSerializable(classOrObject: KtClassOrObject) =
classOrObject.superTypeListEntries.any { it.text == "Serializable" }

private fun hasCorrectSerialVersionUUID(declaration: KtObjectDeclaration): Boolean {
val property = declaration.body?.properties?.firstOrNull { it.name == "serialVersionUID" } ?: return false
return property.isConstant() && isLongProperty(property) && property.isPrivate()
private fun hasCorrectSerialVersionUID(
declaration: KtObjectDeclaration,
parentDeclaration: KtNamedDeclaration = declaration,
): SerialVersionUIDFindings? {
val property = declaration.body?.properties?.firstOrNull { it.name == "serialVersionUID" }
?: return SerialVersionUIDFindings(
false,
parentDeclaration,
parentDeclaration.getIssueMessage(
if (parentDeclaration is KtClass) "class" else "object"
)
)
return if (property.isConstant() && isLongProperty(property) && property.isPrivate()) {
null
} else {
SerialVersionUIDFindings(
false,
property,
"The property `serialVersionUID` signature is not correct. `serialVersionUID` should be " +
"`private` and `constant` and its type should be `Long`"
)
}
}

private fun KtNamedDeclaration.getIssueMessage(typeOfDeclaration: String) =
"The $typeOfDeclaration ${this.nameAsSafeName} " +
"implements the `Serializable` interface and should thus define a `serialVersionUID`."

private fun isLongProperty(property: KtProperty) = hasLongType(property) || hasLongAssignment(property)

private fun hasLongType(property: KtProperty) = property.typeReference?.text == "Long"
Expand All @@ -106,4 +135,10 @@ class SerialVersionUIDInSerializableClass(config: Config = Config.empty) : Rule(
return assignmentText != null && assignmentText.last() == 'L' &&
assignmentText.substring(0, assignmentText.length - 1).toLongOrNull() != null
}

private data class SerialVersionUIDFindings(
val isValid: Boolean,
val violatingElement: KtNamedDeclaration,
val issueMsg: String,
)
}
128 changes: 123 additions & 5 deletions ...kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClassSpec.kt
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.SourceLocation
import io.gitlab.arturbosch.detekt.test.assertThat
import io.gitlab.arturbosch.detekt.test.compileAndLint
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
Expand All @@ -15,7 +17,16 @@ class SerialVersionUIDInSerializableClassSpec {
class C : Serializable
""".trimIndent()
assertThat(subject.compileAndLint(code)).hasSize(1)
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(1)
.hasStartSourceLocation(3, 7)
.hasEndSourceLocation(3, 8)
assertThat(findings[0])
.hasMessage(
"The class C implements the `Serializable` interface and should thus define " +
"a `serialVersionUID`."
)
}

@Test
Expand All @@ -29,7 +40,14 @@ class SerialVersionUIDInSerializableClassSpec {
}
}
""".trimIndent()
assertThat(subject.compileAndLint(code)).hasSize(1)
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(1)
.hasStartSourceLocation(5, 27)
.hasEndSourceLocation(5, 43)
assertThat(findings[0]).hasMessage(
WRONG_SERIAL_VERSION_UID_MESSAGE
)
}

@Test
Expand All @@ -43,7 +61,13 @@ class SerialVersionUIDInSerializableClassSpec {
}
}
""".trimIndent()
assertThat(subject.compileAndLint(code)).hasSize(1)
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(1)
.hasStartSourceLocation(5, 27)
.hasEndSourceLocation(5, 43)

assertThat(findings[0]).hasMessage(WRONG_SERIAL_VERSION_UID_MESSAGE)
}

@Test
Expand All @@ -57,7 +81,13 @@ class SerialVersionUIDInSerializableClassSpec {
}
}
""".trimIndent()
assertThat(subject.compileAndLint(code)).hasSize(1)
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(1)
.hasStartSourceLocation(5, 19)
.hasEndSourceLocation(5, 35)

assertThat(findings[0]).hasMessage(WRONG_SERIAL_VERSION_UID_MESSAGE)
}

@Test
Expand All @@ -75,7 +105,89 @@ class SerialVersionUIDInSerializableClassSpec {
}
}
""".trimIndent()
assertThat(subject.compileAndLint(code)).hasSize(2)
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(2)
.hasStartSourceLocations(SourceLocation(3, 7), SourceLocation(8, 12))
.hasEndSourceLocations(SourceLocation(3, 8), SourceLocation(8, 43))
assertThat(findings.map { it.message }).containsOnly(
"The class C implements the `Serializable` interface and should thus define " +
"a `serialVersionUID`.",
"The object NestedIncorrectSerialVersionUID implements the `Serializable` interface and should thus " +
"define a `serialVersionUID`."
)
}

@Test
fun `reports nested object without const modifier`() {
val code = """
import java.io.Serializable
object A : Serializable {
object B : Serializable {
private val serialVersionUID = 1L
}
private val serialVersionUID = 1L
}
""".trimIndent()
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(2)
.hasStartSourceLocations(SourceLocation(5, 21), SourceLocation(8, 17))
.hasEndSourceLocations(SourceLocation(5, 37), SourceLocation(8, 33))
assertThat(findings.map { it.message }).containsOnly(
WRONG_SERIAL_VERSION_UID_MESSAGE,
WRONG_SERIAL_VERSION_UID_MESSAGE
)
}

@Test
fun `reports nested class without const modifier`() {
val code = """
import java.io.Serializable
class A : Serializable {
class B : Serializable {
companion object {
private val serialVersionUID = 1L
}
}
companion object {
private val serialVersionUID = 1L
}
}
""".trimIndent()
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(2)
.hasStartSourceLocations(SourceLocation(6, 25), SourceLocation(11, 21))
.hasEndSourceLocations(SourceLocation(6, 41), SourceLocation(11, 37))
assertThat(findings.map { it.message }).containsOnly(
WRONG_SERIAL_VERSION_UID_MESSAGE,
WRONG_SERIAL_VERSION_UID_MESSAGE
)
}

@Test
fun `reports nested class without serialVersionUID property`() {
val code = """
import java.io.Serializable
class A : Serializable {
class B : Serializable
}
""".trimIndent()
val findings = subject.compileAndLint(code)
assertThat(findings)
.hasSize(2)
.hasStartSourceLocations(SourceLocation(3, 7), SourceLocation(4, 11))
.hasEndSourceLocations(SourceLocation(3, 8), SourceLocation(4, 12))
assertThat(findings.map { it.message }).containsOnly(
"The class A implements the `Serializable` interface and should thus define a `serialVersionUID`.",
"The class B implements the `Serializable` interface and should thus define a `serialVersionUID`."
)
}

@Test
Expand Down Expand Up @@ -135,4 +247,10 @@ class SerialVersionUIDInSerializableClassSpec {
""".trimIndent()
assertThat(subject.compileAndLint(code)).isEmpty()
}

companion object {
private const val WRONG_SERIAL_VERSION_UID_MESSAGE =
"The property `serialVersionUID` signature is not correct. `serialVersionUID` should be " +
"`private` and `constant` and its type should be `Long`"
}
}

0 comments on commit cc494e7

Please sign in to comment.