WIP First version of the check that does not work (#714)

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<String, Parameter>) -> Boolean = { true },
+        action: (Map.Entry<String, Parameter>) -> List<Violation?>
+    ): List<Violation> = 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,15 +130,20 @@ 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
      */
     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)
                 }
+            }
     }
 }

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<Violation> {
+    fun validate(context: Context): List<Violation> =
+            findUnreferencedParameters(context) + findUnreferencedSchemas(context)
 
-        val paramsInPaths = context.api.paths.orEmpty().flatMap { (_, path) ->
+    private fun findUnreferencedParameters(context: Context): List<Violation> {
+        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<Violation> {
+        val allRefs: Set<String> = 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<String> =
-        findAllRefs(api.paths) + findAllRefs(api.components)
+            findAllRefs(api.paths) + findAllRefs(api.components)
 
     private fun findAllRefs(paths: Paths?): List<String> =
-        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<String> =
-        components?.schemas?.values.orEmpty().flatMap(::findAllRefs)
+            components?.schemas?.values.orEmpty().flatMap(::findAllRefs)
 
     private fun findAllRefs(param: Parameter): List<String> =
-        listOfNotNull(param.`$ref`) + findAllRefs(param.schema)
+            listOfNotNull(param.`$ref`.takeIf { !it.isNullOrBlank() }) +
+                    findAllRefs(param.schema)
 
     private fun findAllRefs(schema: Schema<*>?): List<String> =
-        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<String> =
-        listOfNotNull(apiResponse.`$ref`) + findAllRefs(apiResponse.content)
+            listOfNotNull(apiResponse.`$ref`.takeIf { !it.isNullOrBlank() }) + findAllRefs(apiResponse.content)
 
     private fun findAllRefs(content: Content?): List<String> = content
-        .orEmpty()
-        .values
-        .map { it.schema }
-        .flatMap(::findAllRefs)
+            .orEmpty()
+            .values
+            .map { it.schema }
+            .flatMap(::findAllRefs)
 
 }

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()
+//    }
+}