From 14fd635dfb7abfe82684309c1f130b2f02c82c74 Mon Sep 17 00:00:00 2001 From: Abhijit Sarkar Date: Sun, 1 Jul 2018 12:08:01 -0700 Subject: [PATCH] [#5] Write rules individually (#6) * [#5] Write rules individually --- src/main/kotlin/Migrator.kt | 109 ++++++------------------- src/main/kotlin/RulesetValidator.kt | 18 ++-- src/test/kotlin/MigratorTest.kt | 19 +++++ src/test/kotlin/RulesetVerifier.kt | 37 +++------ src/test/resources/ruleset-issue-5.xml | 10 +++ 5 files changed, 76 insertions(+), 117 deletions(-) create mode 100644 src/test/resources/ruleset-issue-5.xml diff --git a/src/main/kotlin/Migrator.kt b/src/main/kotlin/Migrator.kt index a62d5a9..0526470 100644 --- a/src/main/kotlin/Migrator.kt +++ b/src/main/kotlin/Migrator.kt @@ -10,7 +10,6 @@ import net.sourceforge.pmd.Properties import net.sourceforge.pmd.Rule import net.sourceforge.pmd.Ruleset import org.slf4j.LoggerFactory -import java.util.Objects import java.util.concurrent.TimeUnit @@ -29,12 +28,12 @@ object Migrator { .toMap() } - private fun split(rule: String): Pair? { - return "(.*\\.xml)(?:/)?(.*)?".toRegex().matchEntire(rule).let { + internal fun split(ref: String): Pair? { + return "(.*\\.xml)(?:/)?(.*)?".toRegex().matchEntire(ref).let { when (it?.groups?.size) { 3 -> it.groups[1]!!.value to it.groups[2]!!.value else -> { - LOGGER.warn("No match found for rule: $rule") + LOGGER.warn("No match found for rule: $ref") null } } @@ -65,7 +64,8 @@ object Migrator { } private fun Rule.toRule(): Rule? { - val name = this.ref?.let { split(it) }?.second ?: return this + val name = if (RULE_MAP.containsKey(this.name)) this.name + else this.ref?.let { split(it) }?.second ?: return this if (this.name != null && this.name != name) { LOGGER.warn("Rule: {} has been renamed to: {}", this.name, name) @@ -75,47 +75,29 @@ object Migrator { name = name, ref = "${RULE_MAP[name]}/$name", properties = this.properties - ) + ).also { + it.description = description + it.priority = priority + it.language = language + it.minimumLanguageVersion = minimumLanguageVersion + it.maximumLanguageVersion = maximumLanguageVersion + it.since = since + it.message = message + it.externalInfoUrl = externalInfoUrl + it.clazz = clazz + if (isDfa == true) it.isDfa = true + if (isTypeResolution) it.isTypeResolution = true + } } - private fun Rule.toRules() = async { - val objectFactory = ObjectFactory() + private fun Rule.toRules(): Deferred> = async { val outer = this@toRules if (outer.ref?.isRuleset() == true) { LOGGER.debug("Found ruleset: {}", outer.ref) - - val categoryMap = PMD.ruleset(outer.ref) - .rule - .map { - split(it.ref)?.apply { - if (it.name != null && it.name != second) { - LOGGER.warn("Rule: {} has been renamed to: {}", it.name, second) - } - } - ?.let { it.first } - } - .filter { it != null } - .distinct() - .map { it to emptyList() } - .toMap() - - val excludeMap = (outer.exclude ?: emptyList()) - .map { it.name } - .groupBy { RULE_MAP[it] } - - (categoryMap.keys + excludeMap.keys) - .map { it to ((categoryMap[it] ?: emptyList()) + (excludeMap[it] ?: emptyList())) } - .map { (c, ex) -> - if (ex.isNotEmpty()) { - LOGGER.debug("Found exclusions: $ex") - } - createRule( - ref = c, - exclude = ex - .map { objectFactory.createExclude().apply { this.name = it } } - ) - } + PMD.ruleset(outer.ref) + .rule.mapNotNull { it.toRule() } + .filter { outer.exclude.none { e -> e.name == it.name } } } else { LOGGER.debug("Found rule: {}", outer.ref) outer.toRule()?.let { @@ -124,7 +106,7 @@ object Migrator { } } - private fun String.isRuleset() = isNotMigrated() && this.endsWith(".xml") + private fun String.isRuleset(): Boolean = isNotMigrated() && this.endsWith(".xml") private fun awaitRules(deferred: Deferred>): List { return runBlocking { @@ -155,36 +137,6 @@ object Migrator { .toList() .flatMap { it.second } } - .groupBy { RuleWrapper(it) } - .map { - val ref = it.key.rule.ref - createRule( - ref = ref, - name = if (ref == null) it.key.rule.name else null, - exclude = it.value.flatMap { it.exclude }.distinctBy { it.name }, - properties = it.value.flatMap { it.properties?.property ?: emptyList() } - .let { - objectFactory.createProperties().apply { - property.addAll(it) - } - } - ).also { rule -> - it.value.firstOrNull { it.name == rule.name }?.also { orig -> - rule.description = orig.description - rule.priority = orig.priority - rule.language = orig.language - rule.minimumLanguageVersion = orig.minimumLanguageVersion - rule.maximumLanguageVersion = orig.maximumLanguageVersion - rule.since = orig.since - rule.message = orig.message - rule.externalInfoUrl = orig.externalInfoUrl - rule.clazz = orig.clazz - rule.isDfa = orig.isDfa - rule.isTypeResolution = orig.isTypeResolution - rule.isDeprecated = orig.isDeprecated - } - } - } .let { objectFactory.createRuleset().apply { rule.addAll(it.sortedWith(compareBy(nullsLast()) { it.ref })) @@ -193,19 +145,4 @@ object Migrator { } } } - - class RuleWrapper(val rule: Rule) { - private val key = rule.ref ?: rule.name - - override fun equals(other: Any?): Boolean { - if (this === other) return true - if (javaClass != other?.javaClass) return false - - return key == (other as RuleWrapper).key - } - - override fun hashCode(): Int { - return Objects.hashCode(key) - } - } } \ No newline at end of file diff --git a/src/main/kotlin/RulesetValidator.kt b/src/main/kotlin/RulesetValidator.kt index 7170a81..1b4bf9a 100644 --- a/src/main/kotlin/RulesetValidator.kt +++ b/src/main/kotlin/RulesetValidator.kt @@ -14,13 +14,17 @@ object RulesetValidator { }) } - val ex1 = first.flatMap { it.exclude.map { it.name } }.toSet() - val ex2 = second.flatMap { it.exclude.map { it.name } } - (ex1 to ex2).apply { - assert(first.size == second.size, { - "Old ruleset had ${first.size} exclusions, new one has ${second.size}. Diff: ${first - second}" - }) - } + val excludes = first + .flatMap { it.exclude.map { it.name } } + .toSet() + val oldNames = first + .mapNotNull { it.name ?: it.ref?.let { Migrator.split(it) }?.second } + .toSet() + val newNames = second.mapNotNull { it.name } + .toSet() + // excluded rules shouldn't be in new ruleset unless they'd been redefined + val exclusions = excludes.intersect(newNames) - oldNames + assert(exclusions.isEmpty(), { "Found rules that're supposed to be excluded: $exclusions" }) } } } \ No newline at end of file diff --git a/src/test/kotlin/MigratorTest.kt b/src/test/kotlin/MigratorTest.kt index 0c41ccb..bcdc439 100644 --- a/src/test/kotlin/MigratorTest.kt +++ b/src/test/kotlin/MigratorTest.kt @@ -41,4 +41,23 @@ class MigratorTest { RulesetValidator.validate(ruleset, migrated) } + + @Test + fun `should migrate rules individually`() { + val ruleset = unmarshal(Paths.get(javaClass.getResource("/ruleset-issue-5.xml").toURI())) + val migrated = Migrator.migrate(ruleset) + + assertNotNull(migrated) + assertEquals(4, migrated.rule.size) + + assertEquals(listOf( + "LooseCoupling", + "CouplingBetweenObjects", + "LawOfDemeter", + "LoosePackageCoupling"), + migrated.rule.map { it.name } + ) + + RulesetValidator.validate(ruleset, migrated) + } } \ No newline at end of file diff --git a/src/test/kotlin/RulesetVerifier.kt b/src/test/kotlin/RulesetVerifier.kt index 9015725..60e3e98 100644 --- a/src/test/kotlin/RulesetVerifier.kt +++ b/src/test/kotlin/RulesetVerifier.kt @@ -1,6 +1,5 @@ package org.abhijitsarkar.pmd -import net.sourceforge.pmd.Rule import net.sourceforge.pmd.Ruleset import kotlin.test.assertEquals import kotlin.test.assertNotNull @@ -12,31 +11,21 @@ object RulesetVerifier { fun verify(ruleset: Ruleset) { assertNotNull(ruleset) - assertEquals(11, ruleset.rule.size) + val refMap = ruleset.rule + .groupBy { Migrator.split(it.ref)?.first ?: it.name } + .mapValues { it.value.flatMap { it?.properties?.property ?: emptyList() }.size } + .toMap() + + assertEquals(7, refMap.size) ruleset.apply { - assertRule(findByRef(this, "category/java/bestpractices.xml")) - assertRule(findByRef(this, "category/java/codestyle.xml"), excludeCount = 9) - assertRule(findByRef(this, "category/java/codestyle.xml/LongVariable"), propertyCount = 1) - assertRule(findByRef(this, "category/java/codestyle.xml/ShortVariable"), propertyCount = 1) - assertRule(findByRef(this, "category/java/codestyle.xml/TooManyStaticImports"), propertyCount = 1) - assertRule(findByRef(this, "category/java/design.xml"), excludeCount = 5) - assertRule(findByRef(this, "category/java/documentation.xml"), excludeCount = 1) - assertRule(findByRef(this, "category/java/errorprone.xml"), excludeCount = 5) - assertRule(findByRef(this, "category/java/errorprone.xml/AvoidDuplicateLiterals"), propertyCount = 1) - assertRule(findByRef(this, "category/java/multithreading.xml")) - assertRule(findByRef(this, "category/java/performance.xml")) + assertEquals(0, refMap["category/java/bestpractices.xml"]) + assertEquals(3, refMap["category/java/codestyle.xml"]) + assertEquals(0, refMap["category/java/design.xml"]) + assertEquals(0, refMap["category/java/documentation.xml"]) + assertEquals(1, refMap["category/java/errorprone.xml"]) + assertEquals(0, refMap["category/java/multithreading.xml"]) + assertEquals(0, refMap["category/java/performance.xml"]) } } - - private fun assertRule(rule: Rule?, excludeCount: Int = 0, propertyCount: Int = 0) { - assertNotNull(rule) - assertEquals(excludeCount, rule?.exclude?.size ?: 0) - assertEquals(propertyCount, rule?.properties?.property?.size ?: 0) - } - - private fun findByRef(ruleset: Ruleset, ref: String): Rule? { - return ruleset.rule - .find { it.ref == ref } - } } \ No newline at end of file diff --git a/src/test/resources/ruleset-issue-5.xml b/src/test/resources/ruleset-issue-5.xml new file mode 100644 index 0000000..361102b --- /dev/null +++ b/src/test/resources/ruleset-issue-5.xml @@ -0,0 +1,10 @@ + + + https://github.com/asarkar/pmd-migration-tool/issues/5 + + + + +