Merge branch 'feature/collection-rules-use-swagger-validate' into 'de…

…velop'

Feature/collection rules use swagger validate

Closes #2

See merge request pdds/zally!39

server/src/main/kotlin/com/corefiling/pdds/zally/extensions/SwaggerExtensions.kt

@@ -5,6 +5,7 @@ import de.zalando.zally.rule.api.Violation
 import io.swagger.models.HttpMethod
 import io.swagger.models.Operation
 import io.swagger.models.Path
+import io.swagger.models.Response
 import io.swagger.models.Swagger
 import io.swagger.models.parameters.Parameter
 
@@ -36,6 +37,14 @@ private fun validateParameter(description: String, swagger: Swagger, toMessages:
                     }
         }
 
+private fun validateResponse(description: String, swagger: Swagger, toMessages: (pattern: String, path: Path, method: HttpMethod, operation: Operation, status: String, response: Response) -> List<String>) =
+        validateOperation(description, swagger) { pattern, path, method, operation ->
+            operation.responses
+                    .flatMap { (status, response) ->
+                        toMessages(pattern, path, method, operation, status, response).map { "response $status $it" }
+                    }
+        }
+
 private fun String?.normalizeLocations() = this?.let { listOf(it) }.orEmpty()
 
 fun Swagger.validatePath(description: String, getViolationMessage: (pattern: String, path: Path) -> String?) =
@@ -52,3 +61,8 @@ fun Swagger.validateParameter(description: String, getViolationMessage: (pattern
         validateParameter(description, this) { pattern, path, method, operation, parameter ->
             getViolationMessage(pattern, path, method, operation, parameter).normalizeLocations()
         }
+
+fun Swagger.validateResponse(description: String, getViolationMessage: (pattern: String, path: Path, method: HttpMethod, operation: Operation, status: String, response: Response) -> String?) =
+        validateResponse(description, this) { pattern, path, method, operation, status, response ->
+            getViolationMessage(pattern, path, method, operation, status, response).normalizeLocations()
+        }

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/CollectionsReturnArrays.kt

@@ -1,11 +1,13 @@
 package com.corefiling.pdds.zally.rule.collections
 
+import com.corefiling.pdds.zally.extensions.validateResponse
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
 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.ArrayModel
+import io.swagger.models.HttpMethod
 import io.swagger.models.Model
 import io.swagger.models.Response
 import io.swagger.models.Swagger
@@ -24,16 +26,13 @@ class CollectionsReturnArrays {
 
     @Check(Severity.MUST)
     fun validate(swagger: Swagger): Violation? =
-            swagger.collections()
-                    .flatMap { (pattern, path) ->
-                        path.get?.responses.orEmpty()
-                                .filterKeys { Integer.parseInt(it) in 200..299 }
-                                .filterValues { !isArrayResponse(it, swagger) }
-                                .map { (code, response) ->
-                                    "paths $pattern GET responses $code schema type: expected array but found ${response?.schema?.type}"
-                                }
-                    }
-                    .ifNotEmptyLet { Violation(description, it) }
+            swagger.validateResponse(description) { pattern, path, method, _, status, response ->
+                "schema type: expected array but found ${response?.schema?.type}"
+                        .onlyIf(method == HttpMethod.GET
+                                && swagger.isCollection(pattern, path)
+                                && Integer.parseInt(status) in 200..299
+                                && !isArrayResponse(response, swagger))
+            }
 
     private fun isArrayResponse(response: Response, swagger: Swagger): Boolean {
         val schema = response.schema ?: return false

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/CollectionsReturnTotalItemsHeader.kt

@@ -1,10 +1,12 @@
 package com.corefiling.pdds.zally.rule.collections
 
+import com.corefiling.pdds.zally.extensions.validateResponse
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
 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.HttpMethod
 import io.swagger.models.Response
 import io.swagger.models.Swagger
 
@@ -20,16 +22,13 @@ class CollectionsReturnTotalItemsHeader {
 
     @Check(Severity.SHOULD)
     fun validate(swagger: Swagger): Violation? =
-            swagger.collections()
-                    .flatMap { (pattern, path) ->
-                        path.get?.responses.orEmpty()
-                                .filterKeys { Integer.parseInt(it) in 200..299 }
-                                .filterValues { !hasTotalItemsHeader(it) }
-                                .map { (code, _) ->
-                                    "paths $pattern GET responses $code headers: does not include an int32 format integer Total-Items header"
-                                }
-                    }
-                    .ifNotEmptyLet { Violation(description, it) }
+            swagger.validateResponse(description) { pattern, path, method, _, status, response ->
+                "headers: does not include an int32 format integer Total-Items header"
+                        .onlyIf(method == HttpMethod.GET
+                                && swagger.isCollection(pattern, path)
+                                && Integer.parseInt(status) in 200..299
+                                && !hasTotalItemsHeader(response))
+            }
 
     private fun hasTotalItemsHeader(response: Response?): Boolean {
         val header = response?.headers?.get("Total-Items") ?: return false

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/PaginatedCollectionsReturnTotalPagesHeader.kt

@@ -1,10 +1,12 @@
 package com.corefiling.pdds.zally.rule.collections
 
+import com.corefiling.pdds.zally.extensions.validateResponse
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
 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.HttpMethod
 import io.swagger.models.Response
 import io.swagger.models.Swagger
 
@@ -20,16 +22,13 @@ class PaginatedCollectionsReturnTotalPagesHeader {
 
     @Check(Severity.SHOULD)
     fun validate(swagger: Swagger): Violation? =
-            swagger.collections()
-                    .flatMap { (pattern, path) ->
-                        path.get?.responses.orEmpty()
-                                .filterKeys { Integer.parseInt(it) in 200..299 }
-                                .filterValues { !hasTotalPagesHeader(it) }
-                                .map { (code, _) ->
-                                    "paths $pattern GET responses $code headers: does not include an int32 format integer Total-Pages header"
-                                }
-                    }
-                    .ifNotEmptyLet { Violation(description, it) }
+            swagger.validateResponse(description) { pattern, path, method, _, status, response ->
+                "headers: does not include an int32 format integer Total-Pages header"
+                        .onlyIf(method == HttpMethod.GET
+                                && swagger.isCollection(pattern, path)
+                                && Integer.parseInt(status) in 200..299
+                                && !hasTotalPagesHeader(response))
+            }
 
     private fun hasTotalPagesHeader(response: Response?): Boolean {
         val header = response?.headers?.get("Total-Pages") ?: return false

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/PaginatedCollectionsSupportPageNumberQueryParameter.kt

@@ -1,10 +1,12 @@
 package com.corefiling.pdds.zally.rule.collections
 
+import com.corefiling.pdds.zally.extensions.validateOperation
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
 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.HttpMethod
 import io.swagger.models.Operation
 import io.swagger.models.Swagger
 import io.swagger.models.parameters.Parameter
@@ -23,14 +25,12 @@ class PaginatedCollectionsSupportPageNumberQueryParameter {
 
     @Check(Severity.SHOULD)
     fun validate(swagger: Swagger): Violation? =
-            swagger.collections()
-                    .map { (pattern, path) ->
-                        when {
-                            (hasPageNumberQueryParam(path.get)) -> null
-                            else -> "paths $pattern GET parameters: does not include a valid pageNumber query parameter"
-                        }
-                    }
-                    .ifNotEmptyLet { Violation(description, it) }
+            swagger.validateOperation(description) { pattern, path, method, _ ->
+                "parameters: does not include a valid pageNumber query parameter"
+                        .onlyIf(method == HttpMethod.GET
+                                && swagger.isCollection(pattern, path)
+                                && !hasPageNumberQueryParam(path.get))
+            }
 
     private fun hasPageNumberQueryParam(op: Operation?): Boolean =
             op?.parameters?.find { isPageNumberQueryParam(it) } != null
@@ -48,4 +48,4 @@ class PaginatedCollectionsSupportPageNumberQueryParameter {
             else -> true
         }
     }
-}
+}

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/PaginatedCollectionsSupportPageSizeQueryParameter.kt

@@ -1,10 +1,12 @@
 package com.corefiling.pdds.zally.rule.collections
 
+import com.corefiling.pdds.zally.extensions.validateOperation
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
 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.HttpMethod
 import io.swagger.models.Operation
 import io.swagger.models.Swagger
 import io.swagger.models.parameters.Parameter
@@ -23,14 +25,12 @@ class PaginatedCollectionsSupportPageSizeQueryParameter {
 
     @Check(Severity.SHOULD)
     fun validate(swagger: Swagger): Violation? =
-            swagger.collections()
-                    .map { (pattern, path) ->
-                        when {
-                            hasPageSizeQueryParam(path.get) -> null
-                            else -> "paths $pattern GET parameters: does not include a valid pageSize query parameter"
-                        }
-                    }
-                    .ifNotEmptyLet { Violation(description, it) }
+            swagger.validateOperation(description) { pattern, path, method, _ ->
+                "parameters: does not include a valid pageSize query parameter"
+                        .onlyIf(method == HttpMethod.GET
+                                && swagger.isCollection(pattern, path)
+                                && !hasPageSizeQueryParam(path.get))
+            }
 
     private fun hasPageSizeQueryParam(op: Operation?): Boolean =
             op?.parameters?.find { isPageSizeQueryParam(it) } != null
@@ -47,4 +47,4 @@ class PaginatedCollectionsSupportPageSizeQueryParameter {
             else -> true
         }
     }
-}
+}

server/src/main/kotlin/com/corefiling/pdds/zally/rule/collections/Utils.kt

@@ -18,14 +18,14 @@ fun Swagger.collections(): Map<String, Path> = collectionPaths(this)
 
 fun collectionPaths(swagger: Swagger?): Map<String, Path> {
     return swagger?.paths.orEmpty().filter {
-        entry: Entry<String, Path> -> detectCollection(swagger!!, entry.key, entry.value)
+        entry: Entry<String, Path> -> swagger!!.isCollection(entry.key, entry.value)
     }
 }
 
-fun detectCollection(swagger: Swagger, pattern: String, path: Path): Boolean {
-    return detectCollectionByGetResponseReturningArray(swagger, path) ||
-            detectCollectionByParameterizedSubresources(swagger, pattern) ||
-            detectCollectionByPaginationQueryParameters(swagger, path)
+fun Swagger.isCollection(pattern: String, path: Path): Boolean {
+    return detectCollectionByGetResponseReturningArray(this, path) ||
+            detectCollectionByParameterizedSubresources(this, pattern) ||
+            detectCollectionByPaginationQueryParameters(this, path)
 }
 
 fun detectCollectionByGetResponseReturningArray(swagger: Swagger, path: Path): Boolean {
@@ -78,3 +78,7 @@ inline fun <I : Any, O> Iterable<I?>.ifNotEmptyLet(block: (List<I>) -> O): O? {
     val nonNull = this.filterNotNull()
     return if (nonNull.isEmpty()) null else block(nonNull)
 }
+
+fun String.onlyIf(condition: Boolean): String? {
+    return if (condition) this else null
+}

server/src/main/kotlin/com/corefiling/pdds/zally/rule/operations/PostResponding200ConsideredSuspicious.kt

@@ -2,7 +2,7 @@ package com.corefiling.pdds.zally.rule.operations
 
 import com.corefiling.pdds.zally.extensions.validateOperation
 import com.corefiling.pdds.zally.rule.CoreFilingRuleSet
-import com.corefiling.pdds.zally.rule.collections.detectCollection
+import com.corefiling.pdds.zally.rule.collections.isCollection
 import de.zalando.zally.rule.api.Check
 import de.zalando.zally.rule.api.Rule
 import de.zalando.zally.rule.api.Severity
@@ -25,8 +25,8 @@ class PostResponding200ConsideredSuspicious {
                 when {
                     method != HttpMethod.POST -> null
                     op.responses["200"] == null -> null
-                    detectCollection(swagger, pattern, path) -> "response 200 OK probably should be a 201 Created"
+                    swagger.isCollection(pattern, path) -> "response 200 OK probably should be a 201 Created"
                     else -> "response 200 OK probably should be a 202 Accepted"
                 }
             }
-}
+}

server/src/test/java/de/zalando/zally/rule/CoreFilingAPITest.kt

@@ -114,6 +114,13 @@ class CoreFilingAPITest {
         assertEmptyResults(results)
     }
 
+    @Test
+    fun `organisation-service`() {
+        val results = validate("platform", "organisation-service", policy)
+
+        assertEmptyResults(results)
+    }
+
     @Test
     fun `table-rendering-service`() {
         val results = validate("platform", "table-rendering-service",
@@ -198,7 +205,11 @@ class CoreFilingAPITest {
         val results = validate("tms", "taxonomy-modelling-service",
                 // ignoring rules that historically failed for this service
                 policy.withMoreIgnores(listOf(
+                        "CollectionsReturnTotalItemsHeader",
                         "MatchingSummaryAndOperationIdNames",
+                        "PaginatedCollectionsReturnTotalPagesHeader",
+                        "PaginatedCollectionsSupportPageNumberQueryParameter",
+                        "PaginatedCollectionsSupportPageSizeQueryParameter",
                         "101", // UseOpenApiRule
                         "146", // LimitNumberOfResourcesRule
                         "150", // UseSpecificHttpStatusCodes

server/src/test/kotlin/com/corefiling/pdds/zally/rule/collections/CollectionsArePluralTest.kt

@@ -99,4 +99,4 @@ paths:
         Assertions.assertThat(cut.validate(SwaggerParser().parse(yaml))!!.paths)
                 .hasSameElementsAs(listOf("paths /path/to/taxonomy-package/: 'package' appears to be singular"))
     }
-}
+}

server/src/test/kotlin/com/corefiling/pdds/zally/rule/collections/CollectionsReturnArraysTest.kt

@@ -58,6 +58,6 @@ paths:
         required: true
 """
         assertThat(cut.validate(SwaggerParser().parse(yaml))!!.paths)
-                .hasSameElementsAs(listOf("paths /things GET responses 200 schema type: expected array but found object"))
+                .hasSameElementsAs(listOf("/things GET response 200 schema type: expected array but found object"))
     }
-}
+}

server/src/test/kotlin/com/corefiling/pdds/zally/rule/collections/CollectionsReturnTotalItemsHeaderTest.kt

@@ -59,6 +59,6 @@ paths:
               type: string
 """
         Assertions.assertThat(cut.validate(SwaggerParser().parse(yaml))!!.paths)
-                .hasSameElementsAs(listOf("paths /things GET responses 200 headers: does not include an int32 format integer Total-Items header"))
+                .hasSameElementsAs(listOf("/things GET response 200 headers: does not include an int32 format integer Total-Items header"))
     }
 }

server/src/test/kotlin/com/corefiling/pdds/zally/rule/collections/PaginatedCollectionsReturnTotalPagesHeaderTest.kt

@@ -59,6 +59,6 @@ paths:
               type: string
 """
         Assertions.assertThat(cut.validate(SwaggerParser().parse(yaml))!!.paths)
-                .hasSameElementsAs(listOf("paths /things GET responses 200 headers: does not include an int32 format integer Total-Pages header"))
+                .hasSameElementsAs(listOf("/things GET response 200 headers: does not include an int32 format integer Total-Pages header"))
     }
 }

server/src/test/kotlin/com/corefiling/pdds/zally/rule/collections/PaginatedCollectionsSupportPageNumberQueryParameterTest.kt

@@ -123,6 +123,61 @@ paths:
               type: string
 """
         Assertions.assertThat(cut.validate(SwaggerParser().parse(yaml))!!.paths)
-                .hasSameElementsAs(listOf("paths /things GET parameters: does not include a valid pageNumber query parameter"))
+                .hasSameElementsAs(listOf("/things GET parameters: does not include a valid pageNumber query parameter"))
     }
-}
+
+    @Test
+    fun withMissingPagingParametersIssueOnNonExistentOperation() {
+        val yaml = """
+            swagger: '2.0'
+            info:
+              title: An API
+              description: Description goes here.
+              version: 1.0.0
+            paths:
+              /bundles:
+                post:
+                  description: Creates a new bundle.
+                  parameters:
+                    - name: bundle
+                      in: body
+                      description: Bundle to create.
+                      required: true
+                  responses:
+                    201:
+                      description: Bundle was created
+              /bundles/{bundleId}:
+                parameters:
+                  - ${'$'}ref: '#/parameters/bundleId'
+                get:
+                  description: Get a single bundle.
+                  responses:
+                    200:
+                      description: Bundle.
+            parameters:
+              bundleId:
+                name: bundleId
+                type: string
+                format: uuid
+                in: path
+                description: ID of a bundle.
+                required: true
+              pageNumber:
+                name: pageNumber
+                in: query
+                type: integer
+                format: int32
+                minimum: 1
+                required: true
+                description: Specifies page of results to start from. The first page is numbered 1.
+              pageSize:
+                name: pageSize
+                in: query
+                type: integer
+                format: int32
+                minimum: 1
+                description: How many data-sets in one page of results.
+            """.trimIndent()
+        Assertions.assertThat(cut.validate(SwaggerParser().parse(yaml))).isNull()
+    }
+}