Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SerialVersionUIDInSerializableClass - Update the error location #6114

Merged
merged 2 commits into from
May 20, 2023
Merged

SerialVersionUIDInSerializableClass - Update the error location #6114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6ca359
Update the error location
atulgpt May 20, 2023
42b580c
Remove `isValid` as that was always `false`
atulgpt May 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 51 additions & 21 deletions ...ain/kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClass.kt
View file
Open in desktop
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,71 @@ 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 = searchSerialVersionUIDFinding(companionObject, klass) ?: return
reportFinding(finding)
}
}
super.visitClass(klass)
}

override fun visitObjectDeclaration(declaration: KtObjectDeclaration) {
if (!declaration.isCompanion() &&
isImplementingSerializable(declaration) &&
!hasCorrectSerialVersionUUID(declaration)
) {
report(
CodeSmell(
issue,
Entity.from(declaration),
"The object ${declaration.nameAsSafeName} " +
"implements the `Serializable` interface and should thus define a `serialVersionUID`."
)
)
}
super.visitObjectDeclaration(declaration)
if (!declaration.isCompanion() && isImplementingSerializable(declaration)) {
val finding = searchSerialVersionUIDFinding(declaration) ?: return
reportFinding(finding)
}
}

private fun reportFinding(finding: SerialVersionUIDFindings) {
report(
CodeSmell(
issue,
Entity.atName(finding.violatingElement),
finding.issueMsg
)
)
}

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 searchSerialVersionUIDFinding(
declaration: KtObjectDeclaration,
parentDeclaration: KtNamedDeclaration = declaration,
): SerialVersionUIDFindings? {
val property = declaration.body?.properties?.firstOrNull { it.name == "serialVersionUID" }
?: return SerialVersionUIDFindings(
parentDeclaration,
parentDeclaration.getIssueMessage(
if (parentDeclaration is KtClass) "class" else "object"
)
)
return if (property.isConstant() && isLongProperty(property) && property.isPrivate()) {
null
} else {
SerialVersionUIDFindings(
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 +131,9 @@ 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 violatingElement: KtNamedDeclaration,
val issueMsg: String,
)
}
128 changes: 123 additions & 5 deletions ...kotlin/io/gitlab/arturbosch/detekt/rules/style/SerialVersionUIDInSerializableClassSpec.kt
View file
Open in desktop
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`"
}
}