From 11f63d84539313f93a10bb7c1f36757616b3cff2 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Thu, 14 Jun 2018 22:11:53 +0100 Subject: [PATCH 1/3] AvoidTrailingSlashesRule and test use new convenience utilities #725 #714 --- .../java/de/zalando/zally/rule/Context.kt | 225 ++++++++++-------- .../rule/zalando/AvoidTrailingSlashesRule.kt | 50 ++-- .../de/zalando/zally/rule/ViolationAssert.kt | 32 +++ .../de/zalando/zally/rule/ViolationsAssert.kt | 40 ++++ .../de/zalando/zally/rule/ZallyAssertions.kt | 10 + .../zally/rule/zalando/ApiAudienceRuleTest.kt | 17 +- .../rule/zalando/ApiIdentifierRuleTest.kt | 17 +- .../zalando/AvoidTrailingSlashesRuleTest.kt | 42 +++- 8 files changed, 293 insertions(+), 140 deletions(-) create mode 100644 server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt create mode 100644 server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt create mode 100644 server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt diff --git a/server/src/main/java/de/zalando/zally/rule/Context.kt b/server/src/main/java/de/zalando/zally/rule/Context.kt index 09a4faab3..e7e42c89d 100644 --- a/server/src/main/java/de/zalando/zally/rule/Context.kt +++ b/server/src/main/java/de/zalando/zally/rule/Context.kt @@ -1,95 +1,130 @@ -package de.zalando.zally.rule - -import com.fasterxml.jackson.core.JsonPointer -import de.zalando.zally.rule.api.Violation -import de.zalando.zally.util.ast.JsonPointers -import de.zalando.zally.util.ast.MethodCallRecorder -import de.zalando.zally.util.ast.ReverseAst -import io.swagger.models.Swagger -import io.swagger.parser.SwaggerParser -import io.swagger.v3.oas.models.OpenAPI -import io.swagger.v3.parser.OpenAPIV3Parser -import io.swagger.v3.parser.converter.SwaggerConverter -import io.swagger.v3.parser.core.models.ParseOptions -import io.swagger.v3.parser.util.ResolverFully -import org.slf4j.LoggerFactory - -class Context(openApi: OpenAPI, swagger: Swagger? = null) { - private val recorder = MethodCallRecorder(openApi).skipMethods(*extensionNames) - private val openApiAst = ReverseAst.fromObject(openApi).withExtensionMethodNames(*extensionNames).build() - private val swaggerAst = swagger?.let { ReverseAst.fromObject(it).withExtensionMethodNames(*extensionNames).build() } - - val api = recorder.proxy - - /** - * Creates a Violation with a pointer to the OpenAPI or Swagger model node specified, - * defaulting to the last recorded location. - * @param description the description of the Violation - * @param value the OpenAPI or Swagger model node - * @return the new Violation - */ - fun violation(description: String, value: Any): Violation = - violation(description, pointerForValue(value)) - - /** - * Creates a Violation with the specified pointer, defaulting to the last recorded location. - * @param description the description of the Violation - * @param pointer an existing pointer or null - * @return the new Violation - */ - fun violation(description: String, pointer: JsonPointer? = null): Violation = - Violation(description, pointer ?: recorder.pointer) - - /** - * Check whether a location should be ignored by a specific rule. - * @param pointer the location to check - * @param ruleId the rule id to check - * @return true if the location should be ignored for this rule - */ - fun isIgnored(pointer: JsonPointer, ruleId: String): Boolean = - swaggerAst?.isIgnored(pointer, ruleId) ?: openApiAst.isIgnored(pointer, ruleId) - - private fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { - val swaggerPointer = swaggerAst.getPointer(value) - if (swaggerPointer != null) - swaggerPointer - else { - // Attempt to convert an OpenAPI pointer to a Swagger pointer. - val openApiPointer = openApiAst.getPointer(value) - JsonPointers.convertPointer(openApiPointer) ?: openApiPointer - } - } else { - openApiAst.getPointer(value) - } - - companion object { - private val log = LoggerFactory.getLogger(Context::class.java) - val extensionNames = arrayOf("getVendorExtensions", "getExtensions") - - fun createOpenApiContext(content: String): Context? { - val parseOptions = ParseOptions() - parseOptions.isResolve = true - // parseOptions.isResolveFully = true // https://github.com/swagger-api/swagger-parser/issues/682 - - return OpenAPIV3Parser().readContents(content, null, parseOptions)?.openAPI?.let { - ResolverFully(true).resolveFully(it) // workaround for NPE bug in swagger-parser - Context(it) - } - } - - fun createSwaggerContext(content: String): Context? = - SwaggerParser().readWithInfo(content, true)?.let { - val swagger = it.swagger ?: return null - val openApi = SwaggerConverter().convert(it)?.openAPI - - openApi?.let { - try { - ResolverFully(true).resolveFully(it) - } catch (e: NullPointerException) { - log.warn("Failed to fully resolve Swagger schema.", e) - } - Context(openApi, swagger) - } - } - } -} +package de.zalando.zally.rule + +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.rule.api.Violation +import de.zalando.zally.util.ast.JsonPointers +import de.zalando.zally.util.ast.MethodCallRecorder +import de.zalando.zally.util.ast.ReverseAst +import io.swagger.models.Swagger +import io.swagger.parser.SwaggerParser +import io.swagger.v3.oas.models.OpenAPI +import io.swagger.v3.oas.models.PathItem +import io.swagger.v3.parser.OpenAPIV3Parser +import io.swagger.v3.parser.converter.SwaggerConverter +import io.swagger.v3.parser.core.models.ParseOptions +import io.swagger.v3.parser.util.ResolverFully +import org.slf4j.LoggerFactory + +class Context(openApi: OpenAPI, swagger: Swagger? = null) { + private val recorder = MethodCallRecorder(openApi).skipMethods(*extensionNames) + private val openApiAst = ReverseAst.fromObject(openApi).withExtensionMethodNames(*extensionNames).build() + private val swaggerAst = swagger?.let { ReverseAst.fromObject(it).withExtensionMethodNames(*extensionNames).build() } + + val api = recorder.proxy + + /** + * Convenience method for filtering and iterating over the paths in order to create Violations. + * @param pathFilter a filter selecting the paths to validate + * @param action the action to perform on filtered items + * @return a list of Violations and/or nulls where no violations are necessary + */ + fun validatePaths( + pathFilter: (Map.Entry) -> Boolean = { true }, + action: (Map.Entry) -> List + ): List = api.paths + .orEmpty() + .filter(pathFilter) + .flatMap(action) + .filterNotNull() + + /** + * Creates a List of one Violation with a pointer to the OpenAPI or Swagger model node specified, + * defaulting to the last recorded location. + * @param description the description of the Violation + * @param value the OpenAPI or Swagger model node + * @return the new Violation + */ + fun violations(description: String, value: Any): List = + listOf(violation(description, value)) + + /** + * Creates a List of one Violation with the specified pointer, defaulting to the last recorded location. + * @param description the description of the Violation + * @param pointer an existing pointer or null + * @return the new Violation + */ + fun violations(description: String, pointer: JsonPointer? = null): List = + listOf(Violation(description, pointer as JsonPointer)) + + /** + * Creates a Violation with a pointer to the OpenAPI or Swagger model node specified, + * defaulting to the last recorded location. + * @param description the description of the Violation + * @param value the OpenAPI or Swagger model node + * @return the new Violation + */ + fun violation(description: String, value: Any): Violation = + violation(description, pointerForValue(value)) + + /** + * Creates a Violation with the specified pointer, defaulting to the last recorded location. + * @param description the description of the Violation + * @param pointer an existing pointer or null + * @return the new Violation + */ + fun violation(description: String, pointer: JsonPointer? = null): Violation = + Violation(description, pointer ?: recorder.pointer) + + /** + * Check whether a location should be ignored by a specific rule. + * @param pointer the location to check + * @param ruleId the rule id to check + * @return true if the location should be ignored for this rule + */ + fun isIgnored(pointer: JsonPointer, ruleId: String): Boolean = + swaggerAst?.isIgnored(pointer, ruleId) ?: openApiAst.isIgnored(pointer, ruleId) + + private fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { + val swaggerPointer = swaggerAst.getPointer(value) + if (swaggerPointer != null) + swaggerPointer + else { + // Attempt to convert an OpenAPI pointer to a Swagger pointer. + val openApiPointer = openApiAst.getPointer(value) + JsonPointers.convertPointer(openApiPointer) ?: openApiPointer + } + } else { + openApiAst.getPointer(value) + } + + companion object { + private val log = LoggerFactory.getLogger(Context::class.java) + val extensionNames = arrayOf("getVendorExtensions", "getExtensions") + + fun createOpenApiContext(content: String): Context? { + val parseOptions = ParseOptions() + parseOptions.isResolve = true + // parseOptions.isResolveFully = true // https://github.com/swagger-api/swagger-parser/issues/682 + + return OpenAPIV3Parser().readContents(content, null, parseOptions)?.openAPI?.let { + ResolverFully(true).resolveFully(it) // workaround for NPE bug in swagger-parser + Context(it) + } + } + + fun createSwaggerContext(content: String): Context? = + SwaggerParser().readWithInfo(content, true)?.let { + val swagger = it.swagger ?: return null + val openApi = SwaggerConverter().convert(it)?.openAPI + + openApi?.let { + try { + ResolverFully(true).resolveFully(it) + } catch (e: NullPointerException) { + log.warn("Failed to fully resolve Swagger schema.", e) + } + Context(openApi, swagger) + } + } + } +} diff --git a/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt b/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt index acbc7a0e2..7367c98fe 100644 --- a/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt +++ b/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt @@ -1,24 +1,26 @@ -package de.zalando.zally.rule.zalando - -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 de.zalando.zally.util.PatternUtil -import io.swagger.models.Swagger - -@Rule( - ruleSet = ZalandoRuleSet::class, - id = "136", - severity = Severity.MUST, - title = "Avoid Trailing Slashes" -) -class AvoidTrailingSlashesRule { - private val description = "Rule avoid trailing slashes is not followed" - - @Check(severity = Severity.MUST) - fun validate(swagger: Swagger): Violation? { - val paths = swagger.paths.orEmpty().keys.filter { it != null && PatternUtil.hasTrailingSlash(it) } - return if (!paths.isEmpty()) Violation(description, paths) else null - } -} +package de.zalando.zally.rule.zalando + +import de.zalando.zally.rule.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 de.zalando.zally.util.PatternUtil + +@Rule( + ruleSet = ZalandoRuleSet::class, + id = "136", + severity = Severity.MUST, + title = "Avoid Trailing Slashes" +) +class AvoidTrailingSlashesRule { + private val description = "Rule avoid trailing slashes is not followed" + + @Check(severity = Severity.MUST) + fun validate(context: Context): List = + context.validatePaths( + pathFilter = { PatternUtil.hasTrailingSlash(it.key) } + ) { + context.violations(description, it.value) + } +} diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt new file mode 100644 index 000000000..b03f6c612 --- /dev/null +++ b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt @@ -0,0 +1,32 @@ +package de.zalando.zally.rule + +import de.zalando.zally.rule.api.Violation +import org.assertj.core.api.AbstractAssert +import org.assertj.core.api.StringAssert + +class ViolationAssert(actual: Violation?) : AbstractAssert(actual, ViolationAssert::class.java) { + + fun descriptionEqualTo(description: String): ViolationAssert { + description().isEqualTo(description) + return this + } + + fun descriptionMatches(description: String): ViolationAssert { + description().matches(description) + return this + } + + private fun description() = StringAssert(actual?.description).`as`("description") + + fun pointerEqualTo(pointer: String): ViolationAssert { + pointer().isEqualTo(pointer) + return this + } + + fun pointerMatches(pointer: String): ViolationAssert { + pointer().matches(pointer) + return this + } + + private fun pointer() = StringAssert(actual?.pointer?.toString()).`as`("pointer") +} diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt new file mode 100644 index 000000000..b7d4454b9 --- /dev/null +++ b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt @@ -0,0 +1,40 @@ +package de.zalando.zally.rule + +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.rule.api.Violation +import org.assertj.core.api.AbstractListAssert +import org.assertj.core.api.ListAssert +import org.assertj.core.api.ObjectAssert + +class ViolationsAssert(violations: List?) : AbstractListAssert, Violation, ObjectAssert>(violations, ViolationsAssert::class.java) { + + override fun toAssert(value: Violation?, description: String?): ObjectAssert { + return ObjectAssert(value).`as`(description) + } + + fun descriptionsAllEqualTo(description: String): ViolationsAssert { + descriptions().containsOnly(description) + return this + } + + fun descriptionsEqualTo(vararg descriptions: String): ViolationsAssert { + descriptions().containsExactly(*descriptions) + return this + } + + fun descriptions(): ListAssert { + isNotNull + return ListAssert(actual.map { it.description }).`as`("descriptions") + } + + fun pointers(): ListAssert { + isNotNull + return ListAssert(actual.map { it.pointer }).`as`("pointers") + } + + fun pointersEqualTo(vararg pointers: String): ViolationsAssert { + isNotNull + ListAssert(actual.map { it.pointer!!.toString() }).`as`("pointers").containsExactly(*pointers) + return this + } +} diff --git a/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt new file mode 100644 index 000000000..74a0b8c98 --- /dev/null +++ b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt @@ -0,0 +1,10 @@ +package de.zalando.zally.rule + +import de.zalando.zally.rule.api.Violation + +class ZallyAssertions { + companion object { + fun assertThat(actual: Violation?): ViolationAssert = ViolationAssert(actual) + fun assertThat(actual: List?): ViolationsAssert = ViolationsAssert(actual) + } +} diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/ApiAudienceRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/ApiAudienceRuleTest.kt index dafa10937..e4e27ebed 100644 --- a/server/src/test/java/de/zalando/zally/rule/zalando/ApiAudienceRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zalando/ApiAudienceRuleTest.kt @@ -1,8 +1,8 @@ package de.zalando.zally.rule.zalando import de.zalando.zally.rule.Context +import de.zalando.zally.rule.ZallyAssertions.Companion.assertThat import de.zalando.zally.testConfig -import org.assertj.core.api.Assertions.assertThat import org.junit.Test class ApiAudienceRuleTest { @@ -13,7 +13,10 @@ class ApiAudienceRuleTest { fun correctApiAudienceIsSet() { val context = withAudience("company-internal") - assertThat(rule.validate(context)).isNull() + val violation = rule.validate(context) + + assertThat(violation) + .isNull() } @Test @@ -21,8 +24,9 @@ class ApiAudienceRuleTest { val context = withAudience("not-existing-audience") val violation = rule.validate(context) - assertThat(violation?.pointer).hasToString("/info/x-audience") - assertThat(violation?.description).matches(".*doesn't match.*") + assertThat(violation) + .pointerEqualTo("/info/x-audience") + .descriptionMatches(".*doesn't match.*") } @Test @@ -30,8 +34,9 @@ class ApiAudienceRuleTest { val context = withAudience("null") val violation = rule.validate(context) - assertThat(violation?.pointer).hasToString("/info/x-audience") - assertThat(violation?.description).matches(".*Audience must be provided.*") + assertThat(violation) + .pointerEqualTo("/info/x-audience") + .descriptionMatches(".*Audience must be provided.*") } private fun withAudience(apiAudience: String): Context { diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/ApiIdentifierRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/ApiIdentifierRuleTest.kt index 721c7d275..e3d3fa50b 100644 --- a/server/src/test/java/de/zalando/zally/rule/zalando/ApiIdentifierRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zalando/ApiIdentifierRuleTest.kt @@ -1,7 +1,7 @@ package de.zalando.zally.rule.zalando import de.zalando.zally.rule.Context -import org.assertj.core.api.Assertions.assertThat +import de.zalando.zally.rule.ZallyAssertions.Companion.assertThat import org.junit.Test class ApiIdentifierRuleTest { @@ -12,7 +12,10 @@ class ApiIdentifierRuleTest { fun correctApiIdIsSet() { val context = withApiId("zally-api") - assertThat(rule.validate(context)).isNull() + val violation = rule.validate(context) + + assertThat(violation) + .isNull() } @Test @@ -20,8 +23,9 @@ class ApiIdentifierRuleTest { val context = withApiId("This?iS//some|Incorrect+&ApI)(id!!!") val violation = rule.validate(context)!! - assertThat(violation.pointer).hasToString("/info/x-api-id") - assertThat(violation.description).matches(".*doesn't match.*") + assertThat(violation) + .pointerEqualTo("/info/x-api-id") + .descriptionMatches(".*doesn't match.*") } @Test @@ -29,8 +33,9 @@ class ApiIdentifierRuleTest { val context = withApiId("null") val violation = rule.validate(context)!! - assertThat(violation.pointer).hasToString("/info/x-api-id") - assertThat(violation.description).matches(".*should be provided.*") + assertThat(violation) + .pointerEqualTo("/info/x-api-id") + .descriptionMatches(".*should be provided.*") } private fun withApiId(apiId: String): Context { diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt index 88d7dc90b..c418a6a1e 100644 --- a/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt @@ -1,8 +1,7 @@ package de.zalando.zally.rule.zalando -import de.zalando.zally.swaggerWithPaths -import io.swagger.models.Swagger -import org.assertj.core.api.Assertions.assertThat +import de.zalando.zally.rule.Context +import de.zalando.zally.rule.ZallyAssertions.Companion.assertThat import org.junit.Test class AvoidTrailingSlashesRuleTest { @@ -11,19 +10,44 @@ class AvoidTrailingSlashesRuleTest { @Test fun emptySwagger() { - assertThat(rule.validate(Swagger())).isNull() + val context = Context.createOpenApiContext(""" + openapi: '3.0.0' + """.trimIndent())!! + + val violations = rule.validate(context) + + assertThat(violations).isEmpty() } @Test fun positiveCase() { - val testAPI = swaggerWithPaths("/api/test-api") - assertThat(rule.validate(testAPI)).isNull() + val context = Context.createOpenApiContext(""" + openapi: '3.0.0' + paths: + /api/test-api: {} + """.trimIndent())!! + + val violations = rule.validate(context) + + assertThat(violations).isEmpty() } @Test fun negativeCase() { - val testAPI = swaggerWithPaths("/api/test-api/", "/api/test", "/some/other/path", "/long/bad/path/with/slash/") - assertThat(rule.validate(testAPI)!!.paths).hasSameElementsAs( - listOf("/api/test-api/", "/long/bad/path/with/slash/")) + val context = Context.createOpenApiContext(""" + openapi: '3.0.0' + paths: + /api/test-api/: {} + /api/test: {} + /some/other/path: {} + /long/bad/path/with/slash/: {} + """.trimIndent())!! + + val violations = rule.validate(context) + + assertThat(violations) + .descriptionsAllEqualTo("Rule avoid trailing slashes is not followed") + .pointersEqualTo("/paths/~1api~1test-api~1", "/paths/~1long~1bad~1path~1with~1slash~1") + .hasSize(2) } } From 5ae003733e74d133eab80f15ed3e677e7f0309f0 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Fri, 15 Jun 2018 00:04:13 +0100 Subject: [PATCH 2/3] Added .gitattributes to enforce consistent line endings #725 --- .gitattributes | 40 +++ cli/zally/integration_test.go | 4 +- .../java/de/zalando/zally/rule/Context.kt | 260 +++++++++--------- .../rule/zalando/AvoidTrailingSlashesRule.kt | 52 ++-- .../de/zalando/zally/rule/ViolationAssert.kt | 64 ++--- .../de/zalando/zally/rule/ViolationsAssert.kt | 80 +++--- .../de/zalando/zally/rule/ZallyAssertions.kt | 20 +- 7 files changed, 280 insertions(+), 240 deletions(-) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..54468d96f --- /dev/null +++ b/.gitattributes @@ -0,0 +1,40 @@ +# Set the default behavior, in case people don't have core.autocrlf set. +* text=auto + +# Explicitly declare text files you want to always be normalized and converted +# to native line endings on checkout. + +Dockerfile text + +*.conf text +*.go text +*.gradle text +*.html text +*.java text +*.js text +*.json text +*.jsx text +*.kt text +*.lock text +*.md text +*.properties text +*.pug text +*.scss text +*.sql text +*.toml text +*.txt text +*.yaml text +*.yml text + +# Declare files that will always have CRLF line endings on checkout. +*.bat crlf eol=crlf + +# Declare files that will always have LF line endings on checkout. +gradlew text eol=lf +*.sh text eol=lf + +# Denote all files that are truly binary and should not be modified. +*.ico binary +*.jar binary +*.jpg binary +*.png binary diff --git a/cli/zally/integration_test.go b/cli/zally/integration_test.go index 362e8d471..9fcc7fcc2 100644 --- a/cli/zally/integration_test.go +++ b/cli/zally/integration_test.go @@ -135,7 +135,7 @@ func TestIntegrationWithZallyApiDefinition(t *testing.T) { t.Run("integrationWithZallyApiDefinition", func(t *testing.T) { out, e := RunAppAndCaptureOutput([]string{"", "lint", "../../server/src/main/resources/api/zally-api.yaml"}) - assert.Contains(t, out, "HINT Not Specify Standard Error Codes") + assert.Contains(t, out, "Not Specify Standard Error Codes") assert.Contains(t, out, "MUST violations: 0") assert.Contains(t, out, "SHOULD violations: 0") @@ -150,7 +150,7 @@ func TestIntegrationDisplayRulesList(t *testing.T) { t.Run("integrationDisplayRulesList", func(t *testing.T) { out, e := RunAppAndCaptureOutput([]string{"", "rules"}) - assert.Contains(t, out, "166 MUST: Avoid Link in Header Rule") + assert.Contains(t, out, "Avoid Link in Header Rule") assert.Contains(t, out, "https://zalando.github.io/restful-api-guidelines/#166") assert.Nil(t, e) diff --git a/server/src/main/java/de/zalando/zally/rule/Context.kt b/server/src/main/java/de/zalando/zally/rule/Context.kt index e7e42c89d..38c21cacf 100644 --- a/server/src/main/java/de/zalando/zally/rule/Context.kt +++ b/server/src/main/java/de/zalando/zally/rule/Context.kt @@ -1,130 +1,130 @@ -package de.zalando.zally.rule - -import com.fasterxml.jackson.core.JsonPointer -import de.zalando.zally.rule.api.Violation -import de.zalando.zally.util.ast.JsonPointers -import de.zalando.zally.util.ast.MethodCallRecorder -import de.zalando.zally.util.ast.ReverseAst -import io.swagger.models.Swagger -import io.swagger.parser.SwaggerParser -import io.swagger.v3.oas.models.OpenAPI -import io.swagger.v3.oas.models.PathItem -import io.swagger.v3.parser.OpenAPIV3Parser -import io.swagger.v3.parser.converter.SwaggerConverter -import io.swagger.v3.parser.core.models.ParseOptions -import io.swagger.v3.parser.util.ResolverFully -import org.slf4j.LoggerFactory - -class Context(openApi: OpenAPI, swagger: Swagger? = null) { - private val recorder = MethodCallRecorder(openApi).skipMethods(*extensionNames) - private val openApiAst = ReverseAst.fromObject(openApi).withExtensionMethodNames(*extensionNames).build() - private val swaggerAst = swagger?.let { ReverseAst.fromObject(it).withExtensionMethodNames(*extensionNames).build() } - - val api = recorder.proxy - - /** - * Convenience method for filtering and iterating over the paths in order to create Violations. - * @param pathFilter a filter selecting the paths to validate - * @param action the action to perform on filtered items - * @return a list of Violations and/or nulls where no violations are necessary - */ - fun validatePaths( - pathFilter: (Map.Entry) -> Boolean = { true }, - action: (Map.Entry) -> List - ): List = api.paths - .orEmpty() - .filter(pathFilter) - .flatMap(action) - .filterNotNull() - - /** - * Creates a List of one Violation with a pointer to the OpenAPI or Swagger model node specified, - * defaulting to the last recorded location. - * @param description the description of the Violation - * @param value the OpenAPI or Swagger model node - * @return the new Violation - */ - fun violations(description: String, value: Any): List = - listOf(violation(description, value)) - - /** - * Creates a List of one Violation with the specified pointer, defaulting to the last recorded location. - * @param description the description of the Violation - * @param pointer an existing pointer or null - * @return the new Violation - */ - fun violations(description: String, pointer: JsonPointer? = null): List = - listOf(Violation(description, pointer as JsonPointer)) - - /** - * Creates a Violation with a pointer to the OpenAPI or Swagger model node specified, - * defaulting to the last recorded location. - * @param description the description of the Violation - * @param value the OpenAPI or Swagger model node - * @return the new Violation - */ - fun violation(description: String, value: Any): Violation = - violation(description, pointerForValue(value)) - - /** - * Creates a Violation with the specified pointer, defaulting to the last recorded location. - * @param description the description of the Violation - * @param pointer an existing pointer or null - * @return the new Violation - */ - fun violation(description: String, pointer: JsonPointer? = null): Violation = - Violation(description, pointer ?: recorder.pointer) - - /** - * Check whether a location should be ignored by a specific rule. - * @param pointer the location to check - * @param ruleId the rule id to check - * @return true if the location should be ignored for this rule - */ - fun isIgnored(pointer: JsonPointer, ruleId: String): Boolean = - swaggerAst?.isIgnored(pointer, ruleId) ?: openApiAst.isIgnored(pointer, ruleId) - - private fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { - val swaggerPointer = swaggerAst.getPointer(value) - if (swaggerPointer != null) - swaggerPointer - else { - // Attempt to convert an OpenAPI pointer to a Swagger pointer. - val openApiPointer = openApiAst.getPointer(value) - JsonPointers.convertPointer(openApiPointer) ?: openApiPointer - } - } else { - openApiAst.getPointer(value) - } - - companion object { - private val log = LoggerFactory.getLogger(Context::class.java) - val extensionNames = arrayOf("getVendorExtensions", "getExtensions") - - fun createOpenApiContext(content: String): Context? { - val parseOptions = ParseOptions() - parseOptions.isResolve = true - // parseOptions.isResolveFully = true // https://github.com/swagger-api/swagger-parser/issues/682 - - return OpenAPIV3Parser().readContents(content, null, parseOptions)?.openAPI?.let { - ResolverFully(true).resolveFully(it) // workaround for NPE bug in swagger-parser - Context(it) - } - } - - fun createSwaggerContext(content: String): Context? = - SwaggerParser().readWithInfo(content, true)?.let { - val swagger = it.swagger ?: return null - val openApi = SwaggerConverter().convert(it)?.openAPI - - openApi?.let { - try { - ResolverFully(true).resolveFully(it) - } catch (e: NullPointerException) { - log.warn("Failed to fully resolve Swagger schema.", e) - } - Context(openApi, swagger) - } - } - } -} +package de.zalando.zally.rule + +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.rule.api.Violation +import de.zalando.zally.util.ast.JsonPointers +import de.zalando.zally.util.ast.MethodCallRecorder +import de.zalando.zally.util.ast.ReverseAst +import io.swagger.models.Swagger +import io.swagger.parser.SwaggerParser +import io.swagger.v3.oas.models.OpenAPI +import io.swagger.v3.oas.models.PathItem +import io.swagger.v3.parser.OpenAPIV3Parser +import io.swagger.v3.parser.converter.SwaggerConverter +import io.swagger.v3.parser.core.models.ParseOptions +import io.swagger.v3.parser.util.ResolverFully +import org.slf4j.LoggerFactory + +class Context(openApi: OpenAPI, swagger: Swagger? = null) { + private val recorder = MethodCallRecorder(openApi).skipMethods(*extensionNames) + private val openApiAst = ReverseAst.fromObject(openApi).withExtensionMethodNames(*extensionNames).build() + private val swaggerAst = swagger?.let { ReverseAst.fromObject(it).withExtensionMethodNames(*extensionNames).build() } + + val api = recorder.proxy + + /** + * Convenience method for filtering and iterating over the paths in order to create Violations. + * @param pathFilter a filter selecting the paths to validate + * @param action the action to perform on filtered items + * @return a list of Violations and/or nulls where no violations are necessary + */ + fun validatePaths( + pathFilter: (Map.Entry) -> Boolean = { true }, + action: (Map.Entry) -> List + ): List = api.paths + .orEmpty() + .filter(pathFilter) + .flatMap(action) + .filterNotNull() + + /** + * Creates a List of one Violation with a pointer to the OpenAPI or Swagger model node specified, + * defaulting to the last recorded location. + * @param description the description of the Violation + * @param value the OpenAPI or Swagger model node + * @return the new Violation + */ + fun violations(description: String, value: Any): List = + listOf(violation(description, value)) + + /** + * Creates a List of one Violation with the specified pointer, defaulting to the last recorded location. + * @param description the description of the Violation + * @param pointer an existing pointer or null + * @return the new Violation + */ + fun violations(description: String, pointer: JsonPointer? = null): List = + listOf(Violation(description, pointer as JsonPointer)) + + /** + * Creates a Violation with a pointer to the OpenAPI or Swagger model node specified, + * defaulting to the last recorded location. + * @param description the description of the Violation + * @param value the OpenAPI or Swagger model node + * @return the new Violation + */ + fun violation(description: String, value: Any): Violation = + violation(description, pointerForValue(value)) + + /** + * Creates a Violation with the specified pointer, defaulting to the last recorded location. + * @param description the description of the Violation + * @param pointer an existing pointer or null + * @return the new Violation + */ + fun violation(description: String, pointer: JsonPointer? = null): Violation = + Violation(description, pointer ?: recorder.pointer) + + /** + * Check whether a location should be ignored by a specific rule. + * @param pointer the location to check + * @param ruleId the rule id to check + * @return true if the location should be ignored for this rule + */ + fun isIgnored(pointer: JsonPointer, ruleId: String): Boolean = + swaggerAst?.isIgnored(pointer, ruleId) ?: openApiAst.isIgnored(pointer, ruleId) + + private fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { + val swaggerPointer = swaggerAst.getPointer(value) + if (swaggerPointer != null) + swaggerPointer + else { + // Attempt to convert an OpenAPI pointer to a Swagger pointer. + val openApiPointer = openApiAst.getPointer(value) + JsonPointers.convertPointer(openApiPointer) ?: openApiPointer + } + } else { + openApiAst.getPointer(value) + } + + companion object { + private val log = LoggerFactory.getLogger(Context::class.java) + val extensionNames = arrayOf("getVendorExtensions", "getExtensions") + + fun createOpenApiContext(content: String): Context? { + val parseOptions = ParseOptions() + parseOptions.isResolve = true + // parseOptions.isResolveFully = true // https://github.com/swagger-api/swagger-parser/issues/682 + + return OpenAPIV3Parser().readContents(content, null, parseOptions)?.openAPI?.let { + ResolverFully(true).resolveFully(it) // workaround for NPE bug in swagger-parser + Context(it) + } + } + + fun createSwaggerContext(content: String): Context? = + SwaggerParser().readWithInfo(content, true)?.let { + val swagger = it.swagger ?: return null + val openApi = SwaggerConverter().convert(it)?.openAPI + + openApi?.let { + try { + ResolverFully(true).resolveFully(it) + } catch (e: NullPointerException) { + log.warn("Failed to fully resolve Swagger schema.", e) + } + Context(openApi, swagger) + } + } + } +} diff --git a/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt b/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt index 7367c98fe..859605eaf 100644 --- a/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt +++ b/server/src/main/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRule.kt @@ -1,26 +1,26 @@ -package de.zalando.zally.rule.zalando - -import de.zalando.zally.rule.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 de.zalando.zally.util.PatternUtil - -@Rule( - ruleSet = ZalandoRuleSet::class, - id = "136", - severity = Severity.MUST, - title = "Avoid Trailing Slashes" -) -class AvoidTrailingSlashesRule { - private val description = "Rule avoid trailing slashes is not followed" - - @Check(severity = Severity.MUST) - fun validate(context: Context): List = - context.validatePaths( - pathFilter = { PatternUtil.hasTrailingSlash(it.key) } - ) { - context.violations(description, it.value) - } -} +package de.zalando.zally.rule.zalando + +import de.zalando.zally.rule.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 de.zalando.zally.util.PatternUtil + +@Rule( + ruleSet = ZalandoRuleSet::class, + id = "136", + severity = Severity.MUST, + title = "Avoid Trailing Slashes" +) +class AvoidTrailingSlashesRule { + private val description = "Rule avoid trailing slashes is not followed" + + @Check(severity = Severity.MUST) + fun validate(context: Context): List = + context.validatePaths( + pathFilter = { PatternUtil.hasTrailingSlash(it.key) } + ) { + context.violations(description, it.value) + } +} diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt index b03f6c612..0a38e1e9e 100644 --- a/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt +++ b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt @@ -1,32 +1,32 @@ -package de.zalando.zally.rule - -import de.zalando.zally.rule.api.Violation -import org.assertj.core.api.AbstractAssert -import org.assertj.core.api.StringAssert - -class ViolationAssert(actual: Violation?) : AbstractAssert(actual, ViolationAssert::class.java) { - - fun descriptionEqualTo(description: String): ViolationAssert { - description().isEqualTo(description) - return this - } - - fun descriptionMatches(description: String): ViolationAssert { - description().matches(description) - return this - } - - private fun description() = StringAssert(actual?.description).`as`("description") - - fun pointerEqualTo(pointer: String): ViolationAssert { - pointer().isEqualTo(pointer) - return this - } - - fun pointerMatches(pointer: String): ViolationAssert { - pointer().matches(pointer) - return this - } - - private fun pointer() = StringAssert(actual?.pointer?.toString()).`as`("pointer") -} +package de.zalando.zally.rule + +import de.zalando.zally.rule.api.Violation +import org.assertj.core.api.AbstractAssert +import org.assertj.core.api.StringAssert + +class ViolationAssert(actual: Violation?) : AbstractAssert(actual, ViolationAssert::class.java) { + + fun descriptionEqualTo(description: String): ViolationAssert { + description().isEqualTo(description) + return this + } + + fun descriptionMatches(description: String): ViolationAssert { + description().matches(description) + return this + } + + private fun description() = StringAssert(actual?.description).`as`("description") + + fun pointerEqualTo(pointer: String): ViolationAssert { + pointer().isEqualTo(pointer) + return this + } + + fun pointerMatches(pointer: String): ViolationAssert { + pointer().matches(pointer) + return this + } + + private fun pointer() = StringAssert(actual?.pointer?.toString()).`as`("pointer") +} diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt index b7d4454b9..32b22f43c 100644 --- a/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt +++ b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt @@ -1,40 +1,40 @@ -package de.zalando.zally.rule - -import com.fasterxml.jackson.core.JsonPointer -import de.zalando.zally.rule.api.Violation -import org.assertj.core.api.AbstractListAssert -import org.assertj.core.api.ListAssert -import org.assertj.core.api.ObjectAssert - -class ViolationsAssert(violations: List?) : AbstractListAssert, Violation, ObjectAssert>(violations, ViolationsAssert::class.java) { - - override fun toAssert(value: Violation?, description: String?): ObjectAssert { - return ObjectAssert(value).`as`(description) - } - - fun descriptionsAllEqualTo(description: String): ViolationsAssert { - descriptions().containsOnly(description) - return this - } - - fun descriptionsEqualTo(vararg descriptions: String): ViolationsAssert { - descriptions().containsExactly(*descriptions) - return this - } - - fun descriptions(): ListAssert { - isNotNull - return ListAssert(actual.map { it.description }).`as`("descriptions") - } - - fun pointers(): ListAssert { - isNotNull - return ListAssert(actual.map { it.pointer }).`as`("pointers") - } - - fun pointersEqualTo(vararg pointers: String): ViolationsAssert { - isNotNull - ListAssert(actual.map { it.pointer!!.toString() }).`as`("pointers").containsExactly(*pointers) - return this - } -} +package de.zalando.zally.rule + +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.rule.api.Violation +import org.assertj.core.api.AbstractListAssert +import org.assertj.core.api.ListAssert +import org.assertj.core.api.ObjectAssert + +class ViolationsAssert(violations: List?) : AbstractListAssert, Violation, ObjectAssert>(violations, ViolationsAssert::class.java) { + + override fun toAssert(value: Violation?, description: String?): ObjectAssert { + return ObjectAssert(value).`as`(description) + } + + fun descriptionsAllEqualTo(description: String): ViolationsAssert { + descriptions().containsOnly(description) + return this + } + + fun descriptionsEqualTo(vararg descriptions: String): ViolationsAssert { + descriptions().containsExactly(*descriptions) + return this + } + + fun descriptions(): ListAssert { + isNotNull + return ListAssert(actual.map { it.description }).`as`("descriptions") + } + + fun pointers(): ListAssert { + isNotNull + return ListAssert(actual.map { it.pointer }).`as`("pointers") + } + + fun pointersEqualTo(vararg pointers: String): ViolationsAssert { + isNotNull + ListAssert(actual.map { it.pointer!!.toString() }).`as`("pointers").containsExactly(*pointers) + return this + } +} diff --git a/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt index 74a0b8c98..304e6a6f2 100644 --- a/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt +++ b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt @@ -1,10 +1,10 @@ -package de.zalando.zally.rule - -import de.zalando.zally.rule.api.Violation - -class ZallyAssertions { - companion object { - fun assertThat(actual: Violation?): ViolationAssert = ViolationAssert(actual) - fun assertThat(actual: List?): ViolationsAssert = ViolationsAssert(actual) - } -} +package de.zalando.zally.rule + +import de.zalando.zally.rule.api.Violation + +class ZallyAssertions { + companion object { + fun assertThat(actual: Violation?): ViolationAssert = ViolationAssert(actual) + fun assertThat(actual: List?): ViolationsAssert = ViolationsAssert(actual) + } +} From 62d4c3beaeb89ea8d247a6b1c258c8da00542d72 Mon Sep 17 00:00:00 2001 From: Rob Oxspring Date: Fri, 15 Jun 2018 00:35:09 +0100 Subject: [PATCH 3/3] Addressed review issues #725 --- server/src/main/java/de/zalando/zally/rule/Context.kt | 4 ++-- .../java/de/zalando/zally/rule/ViolationAssert.kt | 1 + .../java/de/zalando/zally/rule/ViolationsAssert.kt | 11 +++-------- .../java/de/zalando/zally/rule/ZallyAssertions.kt | 3 +++ .../rule/zalando/AvoidTrailingSlashesRuleTest.kt | 1 + 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/de/zalando/zally/rule/Context.kt b/server/src/main/java/de/zalando/zally/rule/Context.kt index 38c21cacf..0d0becb47 100644 --- a/server/src/main/java/de/zalando/zally/rule/Context.kt +++ b/server/src/main/java/de/zalando/zally/rule/Context.kt @@ -53,8 +53,8 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) { * @param pointer an existing pointer or null * @return the new Violation */ - fun violations(description: String, pointer: JsonPointer? = null): List = - listOf(Violation(description, pointer as JsonPointer)) + fun violations(description: String, pointer: JsonPointer?): List = + listOf(violation(description, pointer)) /** * Creates a Violation with a pointer to the OpenAPI or Swagger model node specified, diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt index 0a38e1e9e..186e919a2 100644 --- a/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt +++ b/server/src/test/java/de/zalando/zally/rule/ViolationAssert.kt @@ -4,6 +4,7 @@ import de.zalando.zally.rule.api.Violation import org.assertj.core.api.AbstractAssert import org.assertj.core.api.StringAssert +@Suppress("UndocumentedPublicClass") class ViolationAssert(actual: Violation?) : AbstractAssert(actual, ViolationAssert::class.java) { fun descriptionEqualTo(description: String): ViolationAssert { diff --git a/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt index 32b22f43c..dea3bd294 100644 --- a/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt +++ b/server/src/test/java/de/zalando/zally/rule/ViolationsAssert.kt @@ -1,11 +1,11 @@ package de.zalando.zally.rule -import com.fasterxml.jackson.core.JsonPointer import de.zalando.zally.rule.api.Violation import org.assertj.core.api.AbstractListAssert import org.assertj.core.api.ListAssert import org.assertj.core.api.ObjectAssert +@Suppress("UndocumentedPublicClass", "SpreadOperator") class ViolationsAssert(violations: List?) : AbstractListAssert, Violation, ObjectAssert>(violations, ViolationsAssert::class.java) { override fun toAssert(value: Violation?, description: String?): ObjectAssert { @@ -22,19 +22,14 @@ class ViolationsAssert(violations: List?) : AbstractListAssert { + private fun descriptions(): ListAssert { isNotNull return ListAssert(actual.map { it.description }).`as`("descriptions") } - fun pointers(): ListAssert { - isNotNull - return ListAssert(actual.map { it.pointer }).`as`("pointers") - } - fun pointersEqualTo(vararg pointers: String): ViolationsAssert { isNotNull - ListAssert(actual.map { it.pointer!!.toString() }).`as`("pointers").containsExactly(*pointers) + ListAssert(actual.map { it.pointer?.toString() }).`as`("pointers").containsExactly(*pointers) return this } } diff --git a/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt index 304e6a6f2..f0ca50c45 100644 --- a/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt +++ b/server/src/test/java/de/zalando/zally/rule/ZallyAssertions.kt @@ -2,9 +2,12 @@ package de.zalando.zally.rule import de.zalando.zally.rule.api.Violation +@Suppress("UndocumentedPublicClass") class ZallyAssertions { companion object { + fun assertThat(actual: Violation?): ViolationAssert = ViolationAssert(actual) + fun assertThat(actual: List?): ViolationsAssert = ViolationsAssert(actual) } } diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt index c418a6a1e..5f20898e0 100644 --- a/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt +++ b/server/src/test/java/de/zalando/zally/rule/zalando/AvoidTrailingSlashesRuleTest.kt @@ -4,6 +4,7 @@ import de.zalando.zally.rule.Context import de.zalando.zally.rule.ZallyAssertions.Companion.assertThat import org.junit.Test +@Suppress("StringLiteralDuplication", "UndocumentedPublicClass", "UnsafeCallOnNullableType") class AvoidTrailingSlashesRuleTest { private val rule = AvoidTrailingSlashesRule()