fix: collect all nested properties of a schema to check (#747)

zalando · Jul 18, 2018 · 792faee · 792faee
1 parent 6a2f77b
commit 792faee
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 38 deletions.
diff --git a/server/src/main/java/de/zalando/zally/rule/zalando/CommonFieldTypesRule.kt b/server/src/main/java/de/zalando/zally/rule/zalando/CommonFieldTypesRule.kt
@@ -22,39 +22,57 @@ class CommonFieldTypesRule(@Autowired rulesConfig: Config) {
     private val commonFields = rulesConfig.getConfig("${javaClass.simpleName}.common_types").entrySet()
             .map { (key, config) -> key to config.unwrapped() as List<String?> }.toMap()
 
-    fun checkField(name: String, property: Schema<Any>): String? =
-            commonFields[name.toLowerCase()]?.let { (type, format) ->
+    @Check(severity = Severity.MUST)
+    fun checkTypesOfCommonFields(context: Context): List<Violation> =
+            allSchemas(context.api).flatMap {
+                checkAllPropertiesOf(it, check = { name, schema ->
+                    val violationDesc = checkField(name, schema)
+                    if (violationDesc != null) {
+                        context.violations(violationDesc, schema)
+                    } else {
+                        emptyList()
+                    }
+                })
+            }
+
+    private fun checkAllPropertiesOf(
+        objectSchema: Schema<Any>,
+        check: (name: String, schema: Schema<Any>) -> Collection<Violation>
+    ): Collection<Violation> {
+
+        fun traverse(oSchema: Schema<Any>): List<Violation?> =
+                oSchema.properties.orEmpty().flatMap { (name, schema) ->
+                    if (schema.properties == null || schema.properties.isEmpty()) {
+                        check(name, schema)
+                    } else {
+                        traverse(schema)
+                    }
+                }
+
+        return traverse(objectSchema).filterNotNull()
+    }
+
+    internal fun checkField(name: String, property: Schema<Any>): String? =
+            commonFields[name]?.let { (type, format) ->
                 if (property.type != type)
                     "field '$name' has type '${property.type}' (expected type '$type')"
                 else if (property.format != format && format != null)
                     "field '$name' has type '${property.type}' with format '${property.format}' (expected format '$format')"
                 else null
             }
 
-    private fun allSchemas(api: OpenAPI): List<Map.Entry<String, Schema<Any>>> {
-        val objectSchemas = (api.components.schemas.orEmpty().values +
-                api.components.responses.values.flatMap { it.content.values.map { it.schema } } +
-                api.components.requestBodies.values.flatMap { it.content.values.map { it.schema } } +
-                api.paths.orEmpty().flatMap {
-                    it.value.readOperations().flatMap {
-                        it.parameters.orEmpty()
-                                .map { it.schema }
-                    }
-                } +
-                api.paths.orEmpty().flatMap {
-                    it.value.readOperations().flatMap {
-                        it.responses.orEmpty()
-                                .flatMap { it.value.content.orEmpty().values.map { it.schema } }
-                    }
-                })
-        return objectSchemas.flatMap { it.properties.orEmpty().entries }
-    }
-
-    @Check(severity = Severity.MUST)
-    fun checkTypesOfCommonFields(context: Context): List<Violation> {
-        return allSchemas(context.api)
-                .map { (name, schema) -> Pair(checkField(name, schema).orEmpty(), schema) }
-                .filterNot { it.first.isEmpty() }
-                .map { context.violation(it.first, it.second) }
-    }
+    private fun allSchemas(api: OpenAPI): Collection<Schema<Any>> = api.components.schemas.orEmpty().values +
+            api.components.responses.values.flatMap { it.content.values.map { it.schema } } +
+            api.components.requestBodies.values.flatMap { it.content.values.map { it.schema } } +
+            api.paths.orEmpty().flatMap {
+                it.value.readOperations().flatMap { it.parameters.orEmpty().map { it.schema } }
+            } +
+            api.paths.orEmpty().flatMap {
+                it.value.readOperations().flatMap {
+                    it.responses.orEmpty().flatMap { it.value.content.orEmpty().values.map { it.schema } }
+                }
+            } +
+            api.paths.orEmpty().flatMap {
+                it.value.readOperations().flatMap { it.requestBody?.content.orEmpty().values.map { it.schema } }
+            }
 }
diff --git a/server/src/test/java/de/zalando/zally/rule/zalando/CommonFieldTypesRuleTest.kt b/server/src/test/java/de/zalando/zally/rule/zalando/CommonFieldTypesRuleTest.kt
@@ -32,16 +32,6 @@ class CommonFieldTypesRuleTest {
         assertThat(rule.checkField("type", createSchema(STRING_TYPE, null))).isNull()
     }
 
-    @Test
-    fun `checkField matching should be case insensitive`() {
-        assertThat(rule.checkField("iD", createSchema(INTEGER_TYPE, null))).isNotNull()
-        assertThat(rule.checkField("CREATED", createSchema(INTEGER_TYPE, null))).isNotNull()
-        assertThat(rule.checkField("tYpE", createSchema(null, null))).isNotNull()
-        assertThat(rule.checkField("CREated", createSchema(STRING_TYPE, "time"))).isNotNull()
-        assertThat(rule.checkField("ID", createSchema(STRING_TYPE, UUID_FORMAT))).isNull()
-        assertThat(rule.checkField("Id", createSchema(STRING_TYPE, null))).isNull()
-    }
-
     @Test
     fun `checkField should return violation description for invalid type`() {
         assertThat(rule.checkField("id", createSchema(INTEGER_TYPE, null))).isNotNull()
@@ -174,6 +164,28 @@ class CommonFieldTypesRuleTest {
         assertThat(violations[0].pointer.toString()).isEqualTo("/paths/~1pets/get/responses/200/content/application~1json/schema/properties/id")
     }
 
+    @Test
+    fun `checkTypesOfCommonFields should return a violation for invalid common field in nested objects`() {
+        val context = Context.createOpenApiContext("""
+            openapi: 3.0.1
+            components:
+              schemas:
+                Pet:
+                  properties:
+                    address:
+                      properties:
+                        modified:
+                          type: number
+        """.trimIndent())!!
+
+        val violations = rule.checkTypesOfCommonFields(context)
+
+        assertThat(violations).isNotEmpty
+        assertThat(violations).hasSize(1)
+        assertThat(violations[0].description).containsPattern(".*expected type 'string'.*")
+        assertThat(violations[0].pointer.toString()).isEqualTo("/components/schemas/Pet/properties/address/properties/modified")
+    }
+
     @Test
     fun `checkTypesOfCommonFields should also test references`() {
         val context = Context.createOpenApiContext("""