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..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 @@ -1,33 +1,41 @@ 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, - 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(swagger: Swagger): Violation? { - val paths = swagger.paths.orEmpty().keys + fun validate(context: Context): List { + val paths = context.api.paths?.keys.orEmpty() if (paths.size < 2) { - return null + return emptyList() + } + val prefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } + return when { + prefix.isEmpty() -> emptyList() + context.isOpenAPI3() -> violations(prefix, "servers' urls") + else -> violations(prefix, "basePath") } - val commonPrefix = paths.reduce { s1, s2 -> findCommonPrefix(s1, s2) } - return if (commonPrefix.isNotEmpty()) - Violation(description.format(commonPrefix), emptyList()) - else null } + 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("/") 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 faf4053a1..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 @@ -1,81 +1,123 @@ 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 de.zalando.zally.getOpenApiContextFromContent +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 validateEmptyPath() { - assertThat(rule.validate(Swagger())).isNull() - } + fun `validate swagger with no paths returns no violations`() { + @Language("YAML") + val context = getSwaggerContextFromContent(""" + swagger: 2.0 + """.trimIndent()) - @Test - fun simplePositiveCase() { - val swagger = swaggerWithPaths("/orders/{order_id}", "/orders/{updates}", "/merchants") - assertThat(rule.validate(swagger)).isNull() + ZallyAssertions + .assertThat(rule.validate(context)) + .isEmpty() } @Test - fun singlePathShouldPass() { - val swagger = swaggerWithPaths("/orders/{order_id}") - 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 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 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 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 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' which could be part of basePath.") + .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 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' which could be part of basePath.") + .pointersEqualTo("/paths") } @Test - fun positiveCaseSpp() { - val swagger = getFixture("api_spp.json") - 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() } @Test - fun positiveCaseTinbox() { - val swagger = getFixture("api_tinbox.yaml") - assertThat(rule.validate(swagger)).isNull() + 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") } }