From 65ccbe8758ae66c280d7bb65ea805d842486f5a6 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Thu, 27 Sep 2018 00:37:39 +0100 Subject: [PATCH 1/4] test(server): Removed unnecessary tests from ExtractBasePathRuleTest #834 --- .../rule/zally/ExtractBasePathRuleTest.kt | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt index faf4053a1..0c08dc3ab 100644 --- a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt @@ -1,9 +1,7 @@ package de.zalando.zally.rule.zally -import de.zalando.zally.getFixture import de.zalando.zally.rule.api.Violation import de.zalando.zally.swaggerWithPaths -import io.swagger.models.Swagger import org.assertj.core.api.Assertions.assertThat import org.junit.Test @@ -12,11 +10,6 @@ class ExtractBasePathRuleTest { private val rule = ExtractBasePathRule() - @Test - fun validateEmptyPath() { - assertThat(rule.validate(Swagger())).isNull() - } - @Test fun simplePositiveCase() { val swagger = swaggerWithPaths("/orders/{order_id}", "/orders/{updates}", "/merchants") @@ -66,16 +59,4 @@ class ExtractBasePathRuleTest { ) assertThat(rule.validate(swagger)).isNull() } - - @Test - fun positiveCaseSpp() { - val swagger = getFixture("api_spp.json") - assertThat(rule.validate(swagger)).isNull() - } - - @Test - fun positiveCaseTinbox() { - val swagger = getFixture("api_tinbox.yaml") - assertThat(rule.validate(swagger)).isNull() - } } From 27ab796a3b1b0cd54fbdc6107414dd88e8d5f605 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Thu, 27 Sep 2018 00:56:01 +0100 Subject: [PATCH 2/4] refactor(server): Converted ExtractBasePathRule to use Context #834 --- .../zally/rule/zally/ExtractBasePathRule.kt | 17 ++- .../rule/zally/ExtractBasePathRuleTest.kt | 109 +++++++++++------- 2 files changed, 78 insertions(+), 48 deletions(-) diff --git a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt index 0a63c75f3..f71163e8b 100644 --- a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt +++ b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt @@ -1,10 +1,11 @@ package de.zalando.zally.rule.zally +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.rule.api.Context import de.zalando.zally.rule.api.Check import de.zalando.zally.rule.api.Rule import de.zalando.zally.rule.api.Severity import de.zalando.zally.rule.api.Violation -import io.swagger.models.Swagger @Rule( ruleSet = ZallyRuleSet::class, @@ -17,15 +18,13 @@ class ExtractBasePathRule { private val description = "All paths start with prefix '%s'. This prefix could be part of base path." @Check(severity = Severity.HINT) - fun validate(swagger: Swagger): Violation? { - val paths = swagger.paths.orEmpty().keys - if (paths.size < 2) { - return null + fun validate(context: Context): List { + val paths = context.api.paths?.keys.orEmpty() + val prefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } + return when { + paths.size < 2 || prefix.isEmpty() -> emptyList() + else -> listOf(Violation(description.format(prefix), emptyList(), JsonPointer.compile("/paths"))) } - val commonPrefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } - return if (commonPrefix.isNotEmpty()) - Violation(description.format(commonPrefix), emptyList()) - else null } private fun findCommonPrefix(s1: String, s2: String): String { diff --git a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt index 0c08dc3ab..469d01780 100644 --- a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt @@ -1,62 +1,93 @@ package de.zalando.zally.rule.zally -import de.zalando.zally.rule.api.Violation -import de.zalando.zally.swaggerWithPaths -import org.assertj.core.api.Assertions.assertThat +import de.zalando.zally.getSwaggerContextFromContent +import de.zalando.zally.rule.ZallyAssertions +import org.intellij.lang.annotations.Language import org.junit.Test class ExtractBasePathRuleTest { - val DESC_PATTERN = "All paths start with prefix '%s'. This prefix could be part of base path." private val rule = ExtractBasePathRule() @Test - fun simplePositiveCase() { - val swagger = swaggerWithPaths("/orders/{order_id}", "/orders/{updates}", "/merchants") - assertThat(rule.validate(swagger)).isNull() + fun `validate swagger with no common first segments returns no violations`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + paths: + /orders/{order_id}: {} + /orders/{updates}: {} + /merchants: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .isEmpty() } @Test - fun singlePathShouldPass() { - val swagger = swaggerWithPaths("/orders/{order_id}") - assertThat(rule.validate(swagger)).isNull() + fun `validate swagger with single path returns no violations`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + paths: + /orders/{order_id}: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .isEmpty() } @Test - fun simpleNegativeCase() { - val swagger = swaggerWithPaths( - "/shipment/{shipment_id}", - "/shipment/{shipment_id}/status", - "/shipment/{shipment_id}/details" - ) - val rule = rule - val expected = Violation(DESC_PATTERN.format("/shipment"), - emptyList()) - assertThat(rule.validate(swagger)).isEqualTo(expected) + fun `validate swagger with common first segment returns violation`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + paths: + /shipment/{shipment_id}: {} + /shipment/{shipment_id}/status: {} + /shipment/{shipment_id}/details: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .descriptionsEqualTo("All paths start with prefix '/shipment'. This prefix could be part of base path.") + .pointersEqualTo("/paths") } @Test - fun multipleResourceNegativeCase() { - val swagger = swaggerWithPaths( - "/queue/models/configs/{config-id}", - "/queue/models/", - "/queue/models/{model-id}", - "/queue/models/summaries" - ) - val rule = rule - val expected = Violation(DESC_PATTERN.format("/queue/models"), - emptyList()) - assertThat(rule.validate(swagger)).isEqualTo(expected) + fun `validate swagger with multiple common first segments returns violation`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + paths: + /queue/models/configs/{config-id}: {} + /queue/models/: {} + /queue/models/{model-id}: {} + /queue/models/summaries: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .descriptionsEqualTo("All paths start with prefix '/queue/models'. This prefix could be part of base path.") + .pointersEqualTo("/paths") } @Test - fun shouldMatchWholeSubresource() { - val swagger = swaggerWithPaths( - "/api/{api_id}/deployments", - "/api/{api_id}/", - "/applications/{app_id}", - "/applications/" - ) - assertThat(rule.validate(swagger)).isNull() + fun `validate swagger with common prefix but no common first segments returns no violations`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + paths: + /api/{api_id}/deployments: {} + /api/{api_id}/: {} + /applications/{app_id}: {} + /applications/: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .isEmpty() } } From ad63cfeea55109348827254711275d5c8a19ebc2 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Thu, 27 Sep 2018 01:08:15 +0100 Subject: [PATCH 3/4] feat(server): ExtractBasePathRule violation messages differ for Swagger vs OpenAPI specs #834 --- .../zally/rule/zally/ExtractBasePathRule.kt | 21 ++++++++++++------ .../rule/zally/ExtractBasePathRuleTest.kt | 22 +++++++++++++++++-- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt index f71163e8b..72e0da5be 100644 --- a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt +++ b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt @@ -8,25 +8,32 @@ import de.zalando.zally.rule.api.Severity import de.zalando.zally.rule.api.Violation @Rule( - ruleSet = ZallyRuleSet::class, - id = "H001", - severity = Severity.HINT, - title = "Base path can be extracted" + ruleSet = ZallyRuleSet::class, + id = "H001", + severity = Severity.HINT, + title = "Base path can be extracted" ) class ExtractBasePathRule { - private val description = "All paths start with prefix '%s'. This prefix could be part of base path." - @Check(severity = Severity.HINT) fun validate(context: Context): List { val paths = context.api.paths?.keys.orEmpty() val prefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } return when { paths.size < 2 || prefix.isEmpty() -> emptyList() - else -> listOf(Violation(description.format(prefix), emptyList(), JsonPointer.compile("/paths"))) + context.isOpenAPI3() -> violations(prefix, "servers' urls") + else -> violations(prefix, "basePath") } } + fun violations(prefix: String, target: String) = + listOf( + Violation( + "All paths start with prefix '$prefix' which could be part of $target.", + JsonPointer.compile("/paths") + ) + ) + private fun findCommonPrefix(s1: String, s2: String): String { val parts1 = s1.split("/") val parts2 = s2.split("/") diff --git a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt index 469d01780..e2a19a9f4 100644 --- a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt @@ -1,5 +1,6 @@ package de.zalando.zally.rule.zally +import de.zalando.zally.getOpenApiContextFromContent import de.zalando.zally.getSwaggerContextFromContent import de.zalando.zally.rule.ZallyAssertions import org.intellij.lang.annotations.Language @@ -52,7 +53,7 @@ class ExtractBasePathRuleTest { ZallyAssertions .assertThat(rule.validate(context)) - .descriptionsEqualTo("All paths start with prefix '/shipment'. This prefix could be part of base path.") + .descriptionsEqualTo("All paths start with prefix '/shipment' which could be part of basePath.") .pointersEqualTo("/paths") } @@ -70,7 +71,7 @@ class ExtractBasePathRuleTest { ZallyAssertions .assertThat(rule.validate(context)) - .descriptionsEqualTo("All paths start with prefix '/queue/models'. This prefix could be part of base path.") + .descriptionsEqualTo("All paths start with prefix '/queue/models' which could be part of basePath.") .pointersEqualTo("/paths") } @@ -90,4 +91,21 @@ class ExtractBasePathRuleTest { .assertThat(rule.validate(context)) .isEmpty() } + + @Test + fun `validate openapi with common first segment returns violation`() { + @Language("YAML") + val context = getOpenApiContextFromContent(""" + openapi: 3.0.1 + paths: + /shipment/{shipment_id}: {} + /shipment/{shipment_id}/status: {} + /shipment/{shipment_id}/details: {} + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .descriptionsEqualTo("All paths start with prefix '/shipment' which could be part of servers' urls.") + .pointersEqualTo("/paths") + } } From 52976af58e7372155d1cd56d3bb1ee1073008524 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Thu, 27 Sep 2018 13:02:27 +0100 Subject: [PATCH 4/4] feat(server): ExtractBasePathRule handles specs with no paths #834 --- .../zally/rule/zally/ExtractBasePathRule.kt | 16 +++++++++------- .../zally/rule/zally/ExtractBasePathRuleTest.kt | 12 ++++++++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt index 72e0da5be..4a28fb2f3 100644 --- a/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt +++ b/server/src/main/java/de/zalando/zally/rule/zally/ExtractBasePathRule.kt @@ -18,21 +18,23 @@ class ExtractBasePathRule { @Check(severity = Severity.HINT) fun validate(context: Context): List { val paths = context.api.paths?.keys.orEmpty() + if (paths.size < 2) { + return emptyList() + } val prefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } return when { - paths.size < 2 || prefix.isEmpty() -> emptyList() + prefix.isEmpty() -> emptyList() context.isOpenAPI3() -> violations(prefix, "servers' urls") else -> violations(prefix, "basePath") } } - fun violations(prefix: String, target: String) = - listOf( - Violation( - "All paths start with prefix '$prefix' which could be part of $target.", - JsonPointer.compile("/paths") - ) + private fun violations(prefix: String, target: String) = listOf( + Violation( + "All paths start with prefix '$prefix' which could be part of $target.", + JsonPointer.compile("/paths") ) + ) private fun findCommonPrefix(s1: String, s2: String): String { val parts1 = s1.split("/") diff --git a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt index e2a19a9f4..69bd12d11 100644 --- a/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zally/ExtractBasePathRuleTest.kt @@ -10,6 +10,18 @@ class ExtractBasePathRuleTest { private val rule = ExtractBasePathRule() + @Test + fun `validate swagger with no paths returns no violations`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + """.trimIndent()) + + ZallyAssertions + .assertThat(rule.validate(context)) + .isEmpty() + } + @Test fun `validate swagger with no common first segments returns no violations`() { @Language("YAML")