From e233d1ed99ce14b76bf648588c1c8ecff5b164ef Mon Sep 17 00:00:00 2001 From: David Dufour-Boivin Date: Thu, 5 Jul 2018 19:17:34 +0200 Subject: [PATCH] WIP First version of the check that does not work (#714) --- .../java/de/zalando/zally/rule/Context.kt | 51 +++++++++---- .../rule/zally/NoUnusedDefinitionsRule2.kt | 73 ++++++++++++------- .../zally/NoUnusedDefinitionsRule2Test.kt | 48 ++++++++++++ 3 files changed, 131 insertions(+), 41 deletions(-) create mode 100644 server/src/test/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2Test.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 42176225d..ff80490e4 100644 --- a/server/src/main/java/de/zalando/zally/rule/Context.kt +++ b/server/src/main/java/de/zalando/zally/rule/Context.kt @@ -12,18 +12,20 @@ import io.swagger.v3.oas.models.Operation import io.swagger.v3.oas.models.PathItem import io.swagger.v3.oas.models.PathItem.HttpMethod import io.swagger.v3.oas.models.media.Schema +import io.swagger.v3.oas.models.parameters.Parameter 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) { +class Context(private val 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 + val unrecordedApi = openApi /** * Convenience method for filtering and iterating over the paths in order to create Violations. @@ -74,6 +76,21 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) { .flatMap(action) .filterNotNull() + /** + * Convenience method for filtering and iterating over the parameters in order to create Violations. + * @param parameterFilter a filter selecting the parameters to validate + * @param action the action to perform on filtered items + * @return a list of Violations + */ + fun validateParameters( + parameterFilter: (Map.Entry) -> Boolean = { true }, + action: (Map.Entry) -> List + ): List = api.components.parameters + .orEmpty() + .filter(parameterFilter) + .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. @@ -113,7 +130,7 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) { Violation(description, pointer ?: recorder.pointer) /** - * Check whether a location should be ignored by a specific rule. + * Checks 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 @@ -121,7 +138,12 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) { fun isIgnored(pointer: JsonPointer, ruleId: String): Boolean = swaggerAst?.isIgnored(pointer, ruleId) ?: openApiAst.isIgnored(pointer, ruleId) - private fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { + /** + * Gets a [JsonPointer] reference to a given [value]. + * @param value the value for which we want a pointer + * @return a [JsonPointer] reference to the [value] or `null` if it was not found. + */ + fun pointerForValue(value: Any): JsonPointer? = if (swaggerAst != null) { val swaggerPointer = swaggerAst.getPointer(value) if (swaggerPointer != null) swaggerPointer @@ -150,18 +172,19 @@ class Context(openApi: OpenAPI, swagger: Swagger? = null) { } 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) + SwaggerParser().readWithInfo(content, true)?.let { + val swagger = it.swagger ?: return null + val conversion = SwaggerConverter().convert(it) + val openApi = conversion?.openAPI + + openApi?.let { + try { + ResolverFully(true).resolveFully(it) + } catch (e: NullPointerException) { + log.warn("Failed to fully resolve Swagger schema.", e) } + Context(it, swagger) } + } } } diff --git a/server/src/main/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2.kt b/server/src/main/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2.kt index f67ea397c..d6e76f587 100644 --- a/server/src/main/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2.kt +++ b/server/src/main/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2.kt @@ -14,58 +14,77 @@ import io.swagger.v3.oas.models.parameters.Parameter import io.swagger.v3.oas.models.responses.ApiResponse @Rule( - ruleSet = ZallyRuleSet::class, - id = "S005", - severity = Severity.SHOULD, - title = "Do not leave unused definitions" + ruleSet = ZallyRuleSet::class, + id = "S005", + severity = Severity.SHOULD, + title = "Do not leave unused definitions" ) class NoUnusedDefinitionsRule2 { @Check(severity = Severity.SHOULD) - fun validate(context: Context): List { + fun validate(context: Context): List = + findUnreferencedParameters(context) + findUnreferencedSchemas(context) - val paramsInPaths = context.api.paths.orEmpty().flatMap { (_, path) -> + private fun findUnreferencedParameters(context: Context): List { + val paramsInPaths = context.unrecordedApi.paths.orEmpty().flatMap { (_, path) -> path.readOperations().flatMap { operation -> operation.parameters.orEmpty() + .map { it.`$ref` } + .filter { !it.isNullOrBlank() } } } + return context.validateParameters { (_, parameter) -> + val pointer = context.pointerForValue(parameter) + if (pointer.toString() in paramsInPaths) { + emptyList() + } else { + context.violations("Unused parameter definition: $pointer", pointer) + } + } + } - val allRefs = findAllRefs(context.api).toSet() - - TODO() + private fun findUnreferencedSchemas(context: Context): List { + val allRefs: Set = findAllRefs(context.unrecordedApi).toSet() + return context.validateSchemas { (_, schema) -> + val pointer = context.pointerForValue(schema) + if (pointer.toString() in allRefs) { + emptyList() + } else { + context.violations("Unused schema definition: $pointer", pointer) + } + } } private fun findAllRefs(api: OpenAPI): List = - findAllRefs(api.paths) + findAllRefs(api.components) + findAllRefs(api.paths) + findAllRefs(api.components) private fun findAllRefs(paths: Paths?): List = - paths.orEmpty().flatMap { (_, path) -> - path.readOperations().flatMap { operation -> - val inParams = operation.parameters.orEmpty().flatMap(this::findAllRefs) - val inResponse = operation.responses.orEmpty().values.flatMap(this::findAllRefs) - inParams + inResponse + paths.orEmpty().flatMap { (_, path) -> + path.readOperations().flatMap { operation -> + val inParams = operation.parameters.orEmpty().flatMap(this::findAllRefs) + val inResponse = operation.responses.orEmpty().values.flatMap(this::findAllRefs) + inParams + inResponse + } } - } private fun findAllRefs(components: Components?): List = - components?.schemas?.values.orEmpty().flatMap(::findAllRefs) + components?.schemas?.values.orEmpty().flatMap(::findAllRefs) private fun findAllRefs(param: Parameter): List = - listOfNotNull(param.`$ref`) + findAllRefs(param.schema) + listOfNotNull(param.`$ref`.takeIf { !it.isNullOrBlank() }) + + findAllRefs(param.schema) private fun findAllRefs(schema: Schema<*>?): List = - if (schema === null) emptyList() - else { - listOfNotNull(schema.`$ref`) - } + if (schema === null) emptyList() + else listOfNotNull(schema.`$ref`.takeIf { !it.isNullOrBlank() }) private fun findAllRefs(apiResponse: ApiResponse): List = - listOfNotNull(apiResponse.`$ref`) + findAllRefs(apiResponse.content) + listOfNotNull(apiResponse.`$ref`.takeIf { !it.isNullOrBlank() }) + findAllRefs(apiResponse.content) private fun findAllRefs(content: Content?): List = content - .orEmpty() - .values - .map { it.schema } - .flatMap(::findAllRefs) + .orEmpty() + .values + .map { it.schema } + .flatMap(::findAllRefs) } diff --git a/server/src/test/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2Test.kt b/server/src/test/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2Test.kt new file mode 100644 index 000000000..ca0ae3714 --- /dev/null +++ b/server/src/test/java/de/zalando/zally/rule/zally/NoUnusedDefinitionsRule2Test.kt @@ -0,0 +1,48 @@ +package de.zalando.zally.rule.zally + +import com.fasterxml.jackson.core.JsonPointer +import de.zalando.zally.getContextFromFixture +import de.zalando.zally.getFixture +import de.zalando.zally.rule.api.Violation +import io.swagger.models.Swagger +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test + +class NoUnusedDefinitionsRule2Test { + + private val rule = NoUnusedDefinitionsRule2() + + @Test + fun positiveCase() { + val context = getContextFromFixture("unusedDefinitionsValid.json")!! + assertThat(rule.validate(context)) + .isEmpty() + } + + @Test + fun negativeCase() { + val results = rule.validate(getContextFromFixture("unusedDefinitionsInvalid.json")!!) + assertThat(results).hasSameElementsAs(listOf( + Violation("", JsonPointer.compile("/definitions/PetName")), + Violation("", JsonPointer.compile("/parameters/FlowId")) + )) + } + +// @Test +// fun emptySwaggerShouldPass() { +// val swagger = Swagger() +// 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() +// } +}