feat(server): KebabCaseInPathSegmentsRule generalized to CaseCheckerR…

…ule.checkPathSegments() zalando#813
promood · Oct 4, 2018 · 53115f2 · 53115f2
1 parent 14c505b
commit 53115f2
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 77 deletions.
diff --git a/cli/zally/integration_test.go b/cli/zally/integration_test.go
@@ -67,7 +67,7 @@ func TestIntegrationWithSomeOtherLocalYamlFile(t *testing.T) {
 		out, e := RunAppAndCaptureOutput([]string{"", "lint", "../../server/src/test/resources/fixtures/api_tinbox.yaml"})
 
 		assert.Contains(t, out, "Provide API Specification using OpenAPI")
-		assert.Contains(t, out, "MUST violations: 54")
+		assert.Contains(t, out, "MUST violations: 57")
 		assert.Contains(t, out, "SHOULD violations: 23")
 		assert.Contains(t, out, "MAY violations: 11")
 		assert.Contains(t, out, "HINT violations: 0")

diff --git a/server/src/main/java/de/zalando/zally/rule/CaseChecker.kt b/server/src/main/java/de/zalando/zally/rule/CaseChecker.kt
@@ -3,6 +3,7 @@ package de.zalando.zally.rule
 import com.typesafe.config.Config
 import de.zalando.zally.rule.api.Context
 import de.zalando.zally.rule.api.Violation
+import de.zalando.zally.util.PatternUtil
 import de.zalando.zally.util.getAllHeaders
 import de.zalando.zally.util.getAllParameters
 import de.zalando.zally.util.getAllProperties
@@ -11,6 +12,7 @@ import io.github.config4k.extract
 class CaseChecker(
     val cases: Map<String, Regex>,
     val propertyNames: CaseCheck? = null,
+    val pathSegments: CaseCheck? = null,
     val pathParameterNames: CaseCheck? = null,
     val queryParameterNames: CaseCheck? = null,
     val headerNames: CaseCheck? = null
@@ -31,49 +33,69 @@ class CaseChecker(
         }
     }
 
-    fun checkPropertyNames(context: Context): List<Violation> {
-        return context.api
-            .getAllProperties()
-            .flatMap { (name, schema) ->
-                check("Property", "Properties", propertyNames, name)
-                    ?.let { context.violations(it, schema) }
-                    .orEmpty()
-            }
-    }
+    fun checkPathSegments(context: Context): List<Violation> = context.api
+        .paths?.entries.orEmpty()
+        .flatMap { (path, item) ->
+            val inputs = path
+                .split("/")
+                .filterNot { it.isEmpty() }
+                .filterNot { PatternUtil.isPathVariable(it) }
+            check("Path segment", "Path segments", pathSegments, inputs)
+                ?.let { context.violations(it, item) }
+                .orEmpty()
+        }
 
-    fun checkHeadersNames(context: Context): List<Violation> {
-        return context.api
-            .getAllHeaders()
-            .flatMap { header ->
-                check("Header", "Headers", headerNames, header.name)
-                    ?.let { context.violations(it, header.element) }
-                    .orEmpty()
-            }
-    }
+    fun checkPropertyNames(context: Context): List<Violation> = context.api
+        .getAllProperties()
+        .flatMap { (name, schema) ->
+            check("Property", "Properties", propertyNames, name)
+                ?.let { context.violations(it, schema) }
+                .orEmpty()
+        }
 
-    fun checkPathParameterNames(context: Context): List<Violation> {
-        return checkParameterNames(context, "Path", pathParameterNames)
-    }
+    fun checkHeadersNames(context: Context): List<Violation> = context.api
+        .getAllHeaders()
+        .flatMap { header ->
+            check("Header", "Headers", headerNames, header.name)
+                ?.let { context.violations(it, header.element) }
+                .orEmpty()
+        }
 
-    fun checkQueryParameterNames(context: Context): List<Violation> {
-        return checkParameterNames(context, "Query", queryParameterNames)
-    }
+    fun checkPathParameterNames(context: Context): List<Violation> =
+        checkParameterNames(context, "Path", pathParameterNames)
 
-    fun checkParameterNames(
+    fun checkQueryParameterNames(context: Context): List<Violation> =
+        checkParameterNames(context, "Query", queryParameterNames)
+
+    private fun checkParameterNames(
         context: Context,
         type: String,
         check: CaseCheck?
-    ): List<Violation> {
-        return context.api.getAllParameters().values
-            .filter { type.toLowerCase() == it.`in` }
-            .flatMap { param ->
-                check("$type parameter", "$type parameters", check, param.name)
-                    ?.let { context.violations(it, param) }
-                    .orEmpty()
-            }
-    }
+    ): List<Violation> = context.api
+        .getAllParameters().values
+        .filter { type.toLowerCase() == it.`in` }
+        .flatMap { param ->
+            check("$type parameter", "$type parameters", check, param.name)
+                ?.let { context.violations(it, param) }
+                .orEmpty()
+        }
+
+    internal fun check(prefix: String, prefixes: String, check: CaseChecker.CaseCheck?, input: String): String? =
+        check(prefix, prefixes, check, listOf(input))
+
+    internal fun check(
+        prefix: String,
+        prefixes: String,
+        check: CaseChecker.CaseCheck?,
+        vararg inputs: String
+    ): String? = check(prefix, prefixes, check, inputs.asIterable())
 
-    fun check(prefix: String, prefixes: String, check: CaseChecker.CaseCheck?, vararg inputs: String): String? {
+    internal fun check(
+        prefix: String,
+        prefixes: String,
+        check: CaseChecker.CaseCheck?,
+        inputs: Iterable<String>
+    ): String? {
         if (check == null) {
             return null
         }
@@ -119,8 +141,8 @@ class CaseChecker(
         }
     }
 
-    private fun appendSuggestions(message: StringBuilder, inputs: Array<out String>) {
-        val suggestions = inputs.asIterable()
+    private fun appendSuggestions(message: StringBuilder, inputs: Iterable<String>) {
+        val suggestions = inputs
             .map { input ->
                 cases.filterValues { it.matches(input) }.keys
             }
@@ -133,14 +155,4 @@ class CaseChecker(
             suggestions.isNotEmpty() -> message.append(" but seems to be one of ").append(suggestions.joinToString())
         }
     }
-
-    private fun caseToString(case: CaseChecker.CaseCheck): String {
-        val matches = cases.filterValues { it.pattern == case.toString() }
-        return if (matches.isEmpty()) {
-            "regex $case"
-        } else {
-            val entry = matches.iterator().next()
-            "${entry.key} (${entry.value})"
-        }
-    }
 }
diff --git a/server/src/main/java/de/zalando/zally/rule/zalando/KebabCaseInPathSegmentsRule.kt b/server/src/main/java/de/zalando/zally/rule/zalando/KebabCaseInPathSegmentsRule.kt
@@ -1,29 +1,27 @@
 package de.zalando.zally.rule.zalando
 
+import com.typesafe.config.Config
+import de.zalando.zally.rule.CaseChecker
 import de.zalando.zally.rule.api.Check
 import de.zalando.zally.rule.api.Context
 import de.zalando.zally.rule.api.Rule
 import de.zalando.zally.rule.api.Severity
 import de.zalando.zally.rule.api.Violation
-import de.zalando.zally.util.PatternUtil
 
 @Rule(
     ruleSet = ZalandoRuleSet::class,
     id = "129",
     severity = Severity.MUST,
     title = "Lowercase words with hyphens"
 )
-class KebabCaseInPathSegmentsRule {
+class KebabCaseInPathSegmentsRule(config: Config) {
+
+    private val checker = CaseChecker.load(config)
 
     private val description = "Use lowercase separate words with hyphens for path segments"
-    internal val lowerCaseHyphenSeparatedRegex = """^[a-z-]+$""".toRegex()
+    internal val lowerCaseHyphenSeparatedRegex = "^[a-z-]+$".toRegex()
 
     @Check(severity = Severity.MUST)
     fun checkKebabCaseInPathSegments(context: Context): List<Violation> =
-        context.api.paths.orEmpty().entries
-            .filter { (path, _) ->
-                path.split("/").filterNot { it.isEmpty() }
-                    .any { segment -> !PatternUtil.isPathVariable(segment) && !segment.matches(lowerCaseHyphenSeparatedRegex) }
-            }
-            .map { (_, pathEntry) -> context.violation(description, pathEntry) }
+        checker.checkPathSegments(context).map { Violation(description, it.pointer!!) }
 }
diff --git a/server/src/main/java/de/zalando/zally/rule/zally/CaseCheckerRule.kt b/server/src/main/java/de/zalando/zally/rule/zally/CaseCheckerRule.kt
@@ -34,4 +34,8 @@ class CaseCheckerRule(@Autowired config: Config) {
     @Check(severity = Severity.MUST)
     fun checkHeaderNames(context: Context): List<Violation> =
         checker.checkHeadersNames(context)
+
+    @Check(severity = Severity.MUST)
+    fun checkPathSegments(context: Context): List<Violation> =
+        checker.checkPathSegments(context)
 }
diff --git a/server/src/main/java/de/zalando/zally/util/PatternUtil.kt b/server/src/main/java/de/zalando/zally/util/PatternUtil.kt
@@ -8,7 +8,6 @@ object PatternUtil {
     private val CAMEL_CASE_PATTERN = "^[a-z]+(?:[A-Z][a-z]+)*$".toRegex()
     private val PASCAL_CASE_PATTERN = "^[A-Z][a-z]+(?:[A-Z][a-z]+)*$".toRegex()
     private val HYPHENATED_CAMEL_CASE_PATTERN = "^[a-z]+(?:-[A-Z][a-z]+)*$".toRegex()
-    private val KEBAB_CASE_PATTERN = "^[a-z]+(?:-[a-z]+)*$".toRegex()
     private val HYPHENATED_PATTERN = "^[A-Za-z0-9.]+(-[A-Za-z0-9.]+)*$".toRegex()
     private val PATH_VARIABLE_PATTERN = "\\{.+}$".toRegex()
     private val APPLICATION_PROBLEM_JSON_PATTERN = "^application/(problem\\+)?json$".toRegex()
@@ -24,8 +23,6 @@ object PatternUtil {
 
     fun isHyphenatedCamelCase(input: String): Boolean = input.matches(HYPHENATED_CAMEL_CASE_PATTERN)
 
-    fun isKebabCase(input: String): Boolean = input.matches(KEBAB_CASE_PATTERN)
-
     fun isHyphenated(input: String): Boolean = input.matches(HYPHENATED_PATTERN)
 
     fun isApplicationJsonOrProblemJson(mediaType: String): Boolean =

diff --git a/server/src/main/resources/rules-config.conf b/server/src/main/resources/rules-config.conf
@@ -1,8 +1,12 @@
 CaseChecker {
   cases {
     snake_case: "[a-z0-9]+(_[a-z0-9]+)*"
+    kebab-case: "[a-z]+(?:-[a-z]+)*"
     Hyphenated-Pascal-Case: "[A-Z]+[a-z0-9]*(-[A-Z]+[a-z0-9]*)*"
   }
+  pathSegments {
+    regex: ${CaseChecker.cases.kebab-case}
+  }
   propertyNames {
     regex: ${CaseChecker.cases.snake_case}
     whitelist: [ _links ]

diff --git a/server/src/test/java/de/zalando/zally/rule/CaseCheckerParameterizedTest.kt b/server/src/test/java/de/zalando/zally/rule/CaseCheckerParameterizedTest.kt
@@ -7,7 +7,7 @@ import org.junit.runner.RunWith
 import org.junit.runners.Parameterized
 
 @RunWith(Parameterized::class)
-class CaseCheckerParameterizedTest(val param: TestParam) {
+class CaseCheckerParameterizedTest(private val param: TestParam) {
 
     class TestParam(val case: String, val term: String, val expectation: Boolean) {
         operator fun not(): TestParam = TestParam(case, term, !expectation)
@@ -30,7 +30,20 @@ class CaseCheckerParameterizedTest(val param: TestParam) {
             ) + parameters(
                 "snake_case",
                 listOf("test_case", "test", "v1_id", "0_1_2_3"),
-                listOf("test__case", "TestCase", "Test_Case", "", "_", "customer-number", "_customer_number", "CUSTOMER_NUMBER")
+                listOf(
+                    "test__case",
+                    "TestCase",
+                    "Test_Case",
+                    "",
+                    "_",
+                    "customer-number",
+                    "_customer_number",
+                    "CUSTOMER_NUMBER"
+                )
+            ) + parameters(
+                "kebab-case",
+                listOf("test-case"),
+                listOf("test-Case", "testCase")
             )
 
             val excess = parameters.map { it.case }.toSet() - checker.cases.keys
@@ -61,12 +74,7 @@ class CaseCheckerParameterizedTest(val param: TestParam) {
         }
 
         private fun parameters(case: String, matches: List<String>, mismatches: List<String>): List<TestParam> {
-            return matches.map {
-                TestParam(case, it, true)
-            } +
-                   mismatches.map {
-               TestParam(case, it, false)
-           }
+            return matches.map { TestParam(case, it, true) } + mismatches.map { TestParam(case, it, false) }
         }
     }
 

diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/KebabCaseInPathSegmentsRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/KebabCaseInPathSegmentsRuleTest.kt
@@ -1,13 +1,14 @@
 package de.zalando.zally.rule.zalando
 
 import de.zalando.zally.getOpenApiContextFromContent
+import de.zalando.zally.testConfig
 import org.assertj.core.api.Assertions.assertThat
 import org.intellij.lang.annotations.Language
 import org.junit.Test
 
 class KebabCaseInPathSegmentsRuleTest {
 
-    private val rule = KebabCaseInPathSegmentsRule()
+    private val rule = KebabCaseInPathSegmentsRule(testConfig)
 
     @Test
     fun `checkKebabCaseInPathSegments should return violation for path segments which are not lowercase separate words with hyphens`() {

diff --git a/server/src/test/java/de/zalando/zally/rule/zally/CaseCheckerRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zally/CaseCheckerRuleTest.kt
@@ -92,4 +92,22 @@ class CaseCheckerRuleTest {
             .descriptionsAllMatch("Header 'InVaLiD!' does not match .*".toRegex())
             .pointersEqualTo("/paths/~1things/post/parameters/0")
     }
+
+    @Test
+    fun `checkPathSegments returns violations`() {
+        @Language("YAML")
+        val context = getSwaggerContextFromContent("""
+            swagger: '2.0'
+            paths:
+              /things/{param}//InVaLiD:
+                post:
+            """.trimIndent())
+
+        val violations = cut.checkPathSegments(context)
+
+        ZallyAssertions
+            .assertThat(violations)
+            .descriptionsAllMatch("Path segment 'InVaLiD' does not match .*".toRegex())
+            .pointersEqualTo("/paths/~1things~1{param}~1~1InVaLiD")
+    }
 }
diff --git a/server/src/test/java/de/zalando/zally/util/PatternUtilTest.kt b/server/src/test/java/de/zalando/zally/util/PatternUtilTest.kt
@@ -4,7 +4,6 @@ import de.zalando.zally.util.PatternUtil.hasTrailingSlash
 import de.zalando.zally.util.PatternUtil.isCamelCase
 import de.zalando.zally.util.PatternUtil.isHyphenated
 import de.zalando.zally.util.PatternUtil.isHyphenatedCamelCase
-import de.zalando.zally.util.PatternUtil.isKebabCase
 import de.zalando.zally.util.PatternUtil.isPascalCase
 import de.zalando.zally.util.PatternUtil.isPathVariable
 import org.junit.Assert.assertFalse
@@ -52,13 +51,6 @@ class PatternUtilTest {
         assertFalse(isHyphenatedCamelCase("TestCase"))
     }
 
-    @Test
-    fun checkIsKebabCase() {
-        assertTrue(isKebabCase("test-case"))
-        assertFalse(isKebabCase("test-Case"))
-        assertFalse(isKebabCase("testCase"))
-    }
-
     @Test
     fun checkIsHyphenated() {
         // uncontroversial positive cases