Skip to content

Commit 7fc464c

Browse files
NordCoderdNordCoderd
committed
reproducing and fixing issues with rules against exceptions and configuration properties
1 parent eb6f659 commit 7fc464c

14 files changed

Lines changed: 221 additions & 63 deletions

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ Maven Central: https://central.sonatype.com/artifact/dev.protsenko/spring-boot-c
1111
Add dependency to your `build.gradle.kts` or `build.gradle`:
1212

1313
```kotlin
14-
implementation("dev.protsenko:spring-boot-code-guard:1.0.7")
14+
implementation("dev.protsenko:spring-boot-code-guard:1.0.8")
1515
```
1616

1717
## Usage

‎build.gradle.kts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ plugins {
1111
}
1212

1313
group = "dev.protsenko"
14-
version = "1.0.7"
14+
version = "1.0.8"
1515

1616
repositories {
1717
mavenCentral()

‎src/main/kotlin/dev/protsenko/codeguard/rules/general/GeneralRules.kt‎

Lines changed: 23 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ object CoreRules {
4747
val propNames = mutableProperties.joinToString(", ") { it.name }
4848
throw AssertionError(
4949
"${klass.name} has mutable properties: $propNames. " +
50-
"@Configuration classes should be stateless.",
50+
"@Configuration classes should be stateless.",
5151
)
5252
}
5353
}
@@ -80,7 +80,7 @@ object CoreRules {
8080
if (!isInConfigClass) {
8181
val message =
8282
"@Bean method ${function.name} in $containingClassName " +
83-
"should be in a @Configuration class"
83+
"should be in a @Configuration class"
8484
throw AssertionError(message)
8585
}
8686
}
@@ -105,39 +105,24 @@ object CoreRules {
105105
"NestedRuntimeException",
106106
)
107107

108-
scope
109-
.notSuppressedClasses(suppressKey)
110-
.filter { it.name.endsWith("Exception") }
111-
.filterNot { klass ->
112-
val parentNameFromModel = klass.parentClass?.name
113-
val parentNameFromText =
114-
Regex(""":\s*([A-Za-z0-9_.]+)""")
115-
.find(klass.text)
116-
?.groupValues
117-
?.getOrNull(1)
118-
?.substringAfterLast('.')
119-
val parentName = parentNameFromModel ?: parentNameFromText ?: "Any"
108+
val violations =
109+
scope
110+
.notSuppressedClasses(suppressKey)
111+
.filter { it.name.endsWith("Exception") }
112+
.map { it to it.parents(indirectParents = true).map { parent -> parent.name } }
113+
.filterNot { (_, parentNames) ->
114+
parentNames.any { it in validExceptionBases }
115+
}
120116

121-
validExceptionBases.contains(parentName) ||
122-
(parentName.endsWith("Exception") && parentName != "Exception")
123-
}.also { violations ->
124-
if (violations.isNotEmpty()) {
125-
val violatingClasses =
126-
violations.joinToString(", ") {
127-
val parentNameFromModel = it.parentClass?.name
128-
val parentNameFromText =
129-
Regex(""":\s*([A-Za-z0-9_.]+)""")
130-
.find(it.text)
131-
?.groupValues
132-
?.getOrNull(1)
133-
?.substringAfterLast('.')
134-
"${it.name} extends ${parentNameFromModel ?: parentNameFromText ?: "Any"}"
135-
}
136-
throw AssertionError(
137-
"Custom exception classes should extend RuntimeException or proper Spring exceptions: $violatingClasses",
138-
)
117+
if (violations.isNotEmpty()) {
118+
val violatingClasses =
119+
violations.joinToString(", ") { (klass, parentName) ->
120+
"${klass.name} extends $parentName"
139121
}
140-
}
122+
throw AssertionError(
123+
"Custom exception classes should extend RuntimeException or proper Spring exceptions: $violatingClasses",
124+
)
125+
}
141126
}
142127
}
143128

@@ -164,7 +149,7 @@ object CoreRules {
164149

165150
throw AssertionError(
166151
"Found ${properties.size} field(s) with @Autowired/@Inject in: $violatingClasses. " +
167-
"Use constructor injection instead.",
152+
"Use constructor injection instead.",
168153
)
169154
}
170155
}
@@ -187,8 +172,8 @@ object CoreRules {
187172
val annotationName = annotation.substringAfterLast(".")
188173
throw AssertionError(
189174
"${function.containingDeclaration}.${function.name} is private and " +
190-
"annotated with @$annotationName — Spring proxy cannot intercept private methods, " +
191-
"the annotation will be silently ignored.",
175+
"annotated with @$annotationName — Spring proxy cannot intercept private methods, " +
176+
"the annotation will be silently ignored.",
192177
)
193178
}
194179
}
@@ -213,7 +198,7 @@ object CoreRules {
213198
val methodNames = violations.joinToString(", ") { it.name }
214199
throw AssertionError(
215200
"${klass.name} uses println/print in method(s): $methodNames. " +
216-
"Use proper logging (SLF4J) instead of console output.",
201+
"Use proper logging (SLF4J) instead of console output.",
217202
)
218203
}
219204
}
@@ -240,7 +225,7 @@ object CoreRules {
240225
val methodNames = violations.joinToString(", ") { it.name }
241226
throw AssertionError(
242227
"${klass.name} prints stack traces in method(s): $methodNames. " +
243-
"Use proper logging instead of printStackTrace().",
228+
"Use proper logging instead of printStackTrace().",
244229
)
245230
}
246231
}

‎src/main/kotlin/dev/protsenko/codeguard/rules/packages/PackageRules.kt‎

Lines changed: 49 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dev.protsenko.codeguard.rules.packages
22

33
import com.lemonappdev.konsist.api.container.KoScope
4+
import com.lemonappdev.konsist.api.declaration.KoAnnotationDeclaration
45
import com.lemonappdev.konsist.api.ext.list.withAnnotationNamed
56
import com.lemonappdev.konsist.api.ext.list.withoutAnnotationNamed
67
import dev.protsenko.codeguard.core.SpringBootRule
@@ -13,16 +14,42 @@ import dev.protsenko.codeguard.rules.isSpringDataRepository
1314
* Rules for package structure and naming conventions.
1415
*/
1516
object PackageRules {
16-
private val configurationPropertiesPrefixRegex =
17-
Regex("""ConfigurationProperties\s*\(\s*(?:prefix\s*=\s*)?"([^"]+)"""")
1817
private val kebabCasePrefixRegex =
1918
Regex("""^[a-z0-9]+(?:-[a-z0-9]+)*(?:\.[a-z0-9]+(?:-[a-z0-9]+)*)*$""")
19+
private val constReferenceRegex = Regex("""\$\{?([A-Za-z_][A-Za-z0-9_]*)}?""")
20+
21+
private fun configurationPropertiesPrefix(
22+
annotation: KoAnnotationDeclaration,
23+
stringConstants: Map<String, String>,
24+
): String? =
25+
annotation.arguments
26+
.firstOrNull { it.name.isEmpty() || it.name == "prefix" }
27+
?.value
28+
?.let { resolveConstReferences(it, stringConstants) }
29+
30+
private fun stringConstants(scope: KoScope): Map<String, String> =
31+
scope
32+
.properties(includeNested = false)
33+
.filter { it.hasConstModifier }
34+
.mapNotNull { property ->
35+
property.value?.let { property.name to it }
36+
}.toMap()
37+
38+
private fun resolveConstReferences(
39+
value: String,
40+
stringConstants: Map<String, String>,
41+
): String? {
42+
var resolved = true
43+
val prefix =
44+
constReferenceRegex.replace(value) { match ->
45+
stringConstants[match.groupValues[1]] ?: run {
46+
resolved = false
47+
match.value
48+
}
49+
}
2050

21-
private fun configurationPropertiesPrefix(annotationText: String): String? =
22-
configurationPropertiesPrefixRegex
23-
.find(annotationText)
24-
?.groupValues
25-
?.getOrNull(1)
51+
return prefix.takeIf { resolved }
52+
}
2653

2754
private fun idClassReference(argumentValue: String?): String? =
2855
argumentValue
@@ -196,20 +223,22 @@ object PackageRules {
196223
override val suppressKey = "CodeGuard:configurationPropertiesPrefixKebabCase"
197224

198225
override fun verify(scope: KoScope) {
226+
val stringConstants = stringConstants(scope)
227+
199228
scope
200229
.notSuppressedClasses(suppressKey)
201230
.withAnnotationNamed(SpringAnnotations.configurationPropertiesAnnotations)
202231
.forEach { klass ->
203232
val prefix =
204233
klass.annotations
205234
.firstNotNullOfOrNull { annotation ->
206-
configurationPropertiesPrefix(annotation.text)
235+
configurationPropertiesPrefix(annotation, stringConstants)
207236
} ?: return@forEach
208237

209238
if (!kebabCasePrefixRegex.matches(prefix)) {
210239
throw AssertionError(
211240
"@ConfigurationProperties prefix should use lowercase kebab-case segments: " +
212-
"${klass.name} has prefix '$prefix'",
241+
"${klass.name} has prefix '$prefix'",
213242
)
214243
}
215244
}
@@ -403,7 +432,7 @@ object PackageRules {
403432
.filterValues { classesInFile ->
404433
classesInFile.none {
405434
it.hasAnnotationWithName(SpringAnnotations.CONTROLLER) ||
406-
it.hasAnnotationWithName(SpringAnnotations.REST_CONTROLLER)
435+
it.hasAnnotationWithName(SpringAnnotations.REST_CONTROLLER)
407436
}
408437
}.values
409438
.flatten()
@@ -443,7 +472,7 @@ object PackageRules {
443472
klass.annotations
444473
.filter { annotation ->
445474
annotation.fullyQualifiedName in SpringAnnotations.idClassAnnotations ||
446-
annotation.name == "IdClass"
475+
annotation.name == "IdClass"
447476
}.flatMap { annotation ->
448477
annotation.arguments
449478
.mapNotNull { argument ->
@@ -465,13 +494,13 @@ object PackageRules {
465494
SpringAnnotations.entityAnnotations.any { annotationName ->
466495
klass.hasAnnotationWithName(annotationName)
467496
} ||
468-
klass.hasParentClass(indirectParents = true) { parent ->
469-
SpringAnnotations.entityAnnotations.any { annotationName ->
470-
parent.sourceDeclaration
471-
?.asClassDeclaration()
472-
?.hasAnnotationWithName(annotationName) == true
497+
klass.hasParentClass(indirectParents = true) { parent ->
498+
SpringAnnotations.entityAnnotations.any { annotationName ->
499+
parent.sourceDeclaration
500+
?.asClassDeclaration()
501+
?.hasAnnotationWithName(annotationName) == true
502+
}
473503
}
474-
}
475504
}
476505

477506
classesInFile.filterNot { klass ->
@@ -491,9 +520,9 @@ object PackageRules {
491520
SpringAnnotations.entityPackageAnnotations.any { annotationName ->
492521
klass.hasAnnotationWithName(annotationName)
493522
} ||
494-
inheritsFromEntity ||
495-
isReferencedIdClass ||
496-
hasEntityAnchorInFile
523+
inheritsFromEntity ||
524+
isReferencedIdClass ||
525+
hasEntityAnchorInFile
497526
}
498527
}
499528

‎src/test/kotlin/dev/protsenko/codeguard/coverage/DependencyInjectionAndCoreRulesViolationTest.kt‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ class DependencyInjectionAndCoreRulesViolationTest {
348348
}
349349
assertEquals(
350350
"@ConfigurationProperties prefix should use lowercase kebab-case segments: " +
351-
"AppProperties has prefix 'appMail'",
351+
"AppProperties has prefix 'appMail'",
352352
error.message,
353353
)
354354
}
@@ -375,6 +375,58 @@ class DependencyInjectionAndCoreRulesViolationTest {
375375
PackageRules.configurationPropertiesPrefixKebabCaseRule.verify(positiveScope)
376376
}
377377

378+
@Test
379+
fun `configurationPropertiesPrefixKebabCaseRule passes for const val string template prefix`() {
380+
val positiveScope =
381+
Konsist.scopeFromFiles(
382+
listOf(
383+
"src/test/kotlin/fixtures/violations/core/config/ConfigurationPropertiesPrefixKebabCaseConstTemplatePositive.kt",
384+
),
385+
)
386+
PackageRules.configurationPropertiesPrefixKebabCaseRule.verify(positiveScope)
387+
}
388+
389+
@Test
390+
fun `configurationPropertiesPrefixKebabCaseRule passes for const val only string template prefix`() {
391+
val positiveScope =
392+
Konsist.scopeFromFiles(
393+
listOf(
394+
"src/test/kotlin/fixtures/violations/core/config/ConfigurationPropertiesPrefixKebabCaseConstOnlyTemplatePositive.kt",
395+
),
396+
)
397+
PackageRules.configurationPropertiesPrefixKebabCaseRule.verify(positiveScope)
398+
}
399+
400+
@Test
401+
fun `configurationPropertiesPrefixKebabCaseRule skips unresolved string template prefix`() {
402+
val positiveScope =
403+
Konsist.scopeFromFiles(
404+
listOf(
405+
"src/test/kotlin/fixtures/violations/core/config/ConfigurationPropertiesPrefixKebabCaseUnresolvedConstTemplatePositive.kt",
406+
),
407+
)
408+
PackageRules.configurationPropertiesPrefixKebabCaseRule.verify(positiveScope)
409+
}
410+
411+
@Test
412+
fun `configurationPropertiesPrefixKebabCaseRule detects resolved const val string template prefix with uppercase segment`() {
413+
val negativeScope =
414+
Konsist.scopeFromFiles(
415+
listOf(
416+
"src/test/kotlin/fixtures/violations/core/config/ConfigurationPropertiesPrefixKebabCaseConstTemplateNegative.kt",
417+
),
418+
)
419+
val error =
420+
assertFailsWith<AssertionError> {
421+
PackageRules.configurationPropertiesPrefixKebabCaseRule.verify(negativeScope)
422+
}
423+
assertEquals(
424+
"@ConfigurationProperties prefix should use lowercase kebab-case segments: " +
425+
"UppercaseFooProperties has prefix 'MYAPP.foo'",
426+
error.message,
427+
)
428+
}
429+
378430
@Test
379431
fun `configurationPropertiesPrefixKebabCaseRule skips annotations without prefix`() {
380432
val positiveScope =

‎src/test/kotlin/dev/protsenko/codeguard/coverage/ExceptionHandlingRulesViolationTest.kt‎

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class ExceptionHandlingRulesViolationTest {
4747
customExceptionStructureRule.verify(negativeScope)
4848
}
4949
assertEquals(
50-
"Custom exception classes should extend RuntimeException or proper Spring exceptions: InvalidException extends Throwable, BadCustomException extends Any",
50+
"Custom exception classes should extend RuntimeException or proper Spring exceptions: InvalidException extends [Throwable], BadCustomException extends [Any]",
5151
error.message,
5252
)
5353
}
@@ -61,6 +61,40 @@ class ExceptionHandlingRulesViolationTest {
6161
customExceptionStructureRule.verify(positiveScope)
6262
}
6363

64+
@Test
65+
fun `customExceptionStructureRule passes for exception extending runtime exception through custom parent`() {
66+
val positiveScope =
67+
Konsist.scopeFromFiles(
68+
listOf("src/test/kotlin/fixtures/violations/exception/security/CustomExceptionStructureSecurityParentPositive.kt"),
69+
)
70+
customExceptionStructureRule.verify(positiveScope)
71+
}
72+
73+
@Test
74+
fun `customExceptionStructureRule uses declared supertype instead of constructor parameter type`() {
75+
val positiveScope =
76+
Konsist.scopeFromFiles(
77+
listOf("src/test/kotlin/fixtures/violations/exception/CustomExceptionStructureConstructorParameterPositive.kt"),
78+
)
79+
customExceptionStructureRule.verify(positiveScope)
80+
}
81+
82+
@Test
83+
fun `customExceptionStructureRule detects checked exception parent named exception`() {
84+
val negativeScope =
85+
Konsist.scopeFromFiles(
86+
listOf("src/test/kotlin/fixtures/violations/exception/CustomExceptionCheckedNamedExceptionNegative.kt"),
87+
)
88+
val error =
89+
assertFailsWith<AssertionError> {
90+
customExceptionStructureRule.verify(negativeScope)
91+
}
92+
assertEquals(
93+
"Custom exception classes should extend RuntimeException or proper Spring exceptions: ImportException extends [IOException]",
94+
error.message,
95+
)
96+
}
97+
6498
@Test
6599
fun `customExceptionStructureRule detects checked exception inheritance`() {
66100
val negativeScope =
@@ -72,7 +106,7 @@ class ExceptionHandlingRulesViolationTest {
72106
customExceptionStructureRule.verify(negativeScope)
73107
}
74108
assertEquals(
75-
"Custom exception classes should extend RuntimeException or proper Spring exceptions: CheckedBusinessException extends Exception",
109+
"Custom exception classes should extend RuntimeException or proper Spring exceptions: CheckedBusinessException extends [Exception]",
76110
error.message,
77111
)
78112
}

0 commit comments

Comments
 (0)