Merge pull request #810 from zalando/714-migrate-extensible-enum-rule

feat(server): migrate extensible enum rule to context object to support openapi 3

server/src/main/java/de/zalando/zally/rule/zalando/ExtensibleEnumRule.kt

@@ -1,83 +1,25 @@
 package de.zalando.zally.rule.zalando
 
 import de.zalando.zally.rule.api.Check
+import de.zalando.zally.rule.api.Context
 import de.zalando.zally.rule.api.Severity
 import de.zalando.zally.rule.api.Violation
 import de.zalando.zally.rule.api.Rule
-import io.swagger.models.Operation
-import io.swagger.models.Swagger
-import io.swagger.models.parameters.Parameter
-import io.swagger.models.parameters.SerializableParameter
-import io.swagger.models.properties.BinaryProperty
-import io.swagger.models.properties.BooleanProperty
-import io.swagger.models.properties.DateProperty
-import io.swagger.models.properties.DateTimeProperty
-import io.swagger.models.properties.DoubleProperty
-import io.swagger.models.properties.EmailProperty
-import io.swagger.models.properties.FloatProperty
-import io.swagger.models.properties.IntegerProperty
-import io.swagger.models.properties.LongProperty
-import io.swagger.models.properties.PasswordProperty
-import io.swagger.models.properties.Property
-import io.swagger.models.properties.StringProperty
+import de.zalando.zally.util.getAllTransitiveSchemas
 
 @Rule(
-        ruleSet = ZalandoRuleSet::class,
-        id = "107",
-        severity = Severity.SHOULD,
-        title = "Prefer Compatible Extensions"
+    ruleSet = ZalandoRuleSet::class,
+    id = "107",
+    severity = Severity.SHOULD,
+    title = "Prefer Compatible Extensions"
 )
 class ExtensibleEnumRule {
 
-    @Check(severity = Severity.SHOULD)
-    fun validate(swagger: Swagger): Violation? {
-        val properties = enumProperties(swagger)
-        val parameters = enumParameters(swagger)
-
-        val enumNames = (properties.keys + parameters.keys).distinct()
-        val enumPaths = (properties.values + parameters.values).distinct()
-        return if (enumNames.isNotEmpty()) Violation(
-                "Properties/Parameters $enumNames are not extensible enums", enumPaths)
-        else null
-    }
-
-    private fun enumProperties(swagger: Swagger): Map<String, String> =
-        swagger.definitions.orEmpty().flatMap { (defName, model) ->
-            val enumProps = model.properties.orEmpty().filter { (_, prop) -> prop.isEnum() }
-            enumProps.map { (propName, _) -> propName to "/definitions/$defName/properties/$propName" }
-        }.toMap()
-
-    private fun enumParameters(swagger: Swagger): Map<String, String> {
-        val pathsOperationsAndEnums = swagger.paths.orEmpty().map { (pathName, path) ->
-            pathName to path.operationMap.orEmpty().map { (opName, op) -> opName to op.getEnumParameters() }.toMap()
-        }.toMap()
-
-        return pathsOperationsAndEnums
-            .filter { (_, opAndEnums) -> opAndEnums.isNotEmpty() }
-            .flatMap { (pathName, opAndEnums) -> opAndEnums.map { (op, enums) -> "/paths$pathName/$op" to enums } }
-            .flatMap { (operationPath, enums) -> enums.map { it to "$operationPath/parameters/$it" } }.toMap()
-    }
+    val description = "Property is not an extensible enum (use `x-extensible-enum` instead)"
 
-    private fun Property.isEnum(): Boolean = when (this) {
-        is StringProperty -> this.enum.hasValues()
-        is BinaryProperty -> this.enum.hasValues()
-        is DateProperty -> this.enum.hasValues()
-        is DateTimeProperty -> this.enum.hasValues()
-        is BooleanProperty -> this.enum.hasValues()
-        is DoubleProperty -> this.enum.hasValues()
-        is EmailProperty -> this.enum.hasValues()
-        is FloatProperty -> this.enum.hasValues()
-        is IntegerProperty -> this.enum.hasValues()
-        is LongProperty -> this.enum.hasValues()
-        is PasswordProperty -> this.enum.hasValues()
-        else -> false
-    }
-
-    private fun <T> List<T>?.hasValues(): Boolean {
-        return this.orEmpty().isNotEmpty()
-    }
-
-    private fun Operation?.getEnumParameters() = this?.parameters.orEmpty().filter { it.isEnum() }.map { it.name }
-
-    private fun Parameter?.isEnum() = (this as? SerializableParameter)?.enum?.orEmpty()?.isNotEmpty() ?: false
+    @Check(severity = Severity.SHOULD)
+    fun checkForEnums(context: Context): List<Violation> =
+        context.api.getAllTransitiveSchemas()
+            .filter { it.enum != null && it.enum.isNotEmpty() }
+            .map { context.violation(description, it) }
 }

server/src/main/java/de/zalando/zally/util/OpenApiUtil.kt

@@ -56,6 +56,29 @@ fun OpenAPI.getAllSchemas(): Collection<Schema<Any>> = this.components.schemas.o
         it.value.readOperations().flatMap { it.requestBody?.content.orEmpty().values.mapNotNull { it.schema } }
     }
 
+/**
+ * Traverses the schemas and returns all included schemas
+ * @return a collection of all transitive schemas
+ */
+fun OpenAPI.getAllTransitiveSchemas(): Set<Schema<Any>> {
+    fun isPrimitive(schema: Schema<Any>): Boolean = schema.properties.orEmpty().isEmpty()
+    val collector = mutableSetOf<Schema<Any>>()
+
+    tailrec fun traverseSchemas(schemasToScan: Collection<Schema<Any>>) {
+        if (schemasToScan.isEmpty()) return
+
+        val primitiveSchemas = schemasToScan.filter { isPrimitive(it) }
+        val nonPrimitiveSchemas = schemasToScan.filterNot { isPrimitive(it) }
+
+        collector.addAll(primitiveSchemas)
+        traverseSchemas(nonPrimitiveSchemas.flatMap { it.properties.values })
+    }
+
+    traverseSchemas(this.getAllSchemas())
+
+    return collector
+}
+
 /**
  * Returns all defined parameters of an API specification
  * @return a collection of parameters

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

@@ -1,8 +1,6 @@
 package de.zalando.zally.rule.zalando
 
-import de.zalando.zally.getFixture
-import de.zalando.zally.rule.api.Violation
-import io.swagger.models.Swagger
+import de.zalando.zally.getOpenApiContextFromContent
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.Test
 
@@ -11,41 +9,68 @@ class ExtensibleEnumRuleTest {
     val rule = ExtensibleEnumRule()
 
     @Test
-    fun returnsNoViolationIfEmptySwagger() {
-        assertThat(rule.validate(Swagger())).isNull()
-    }
-
-    @Test
-    fun returnsViolationIfAnEnumInModelProperty() {
-        val swagger = getFixture("enum_in_model_property.yaml")
-        val expectedViolation = Violation(
-                description = "Properties/Parameters [status] are not extensible enums",
-                paths = listOf("/definitions/CrawledAPIDefinition/properties/status"))
+    fun `checkForEnums should return violation if an enum is used in schema`() {
+        val content = """
+            openapi: 3.0.1
+            components:
+              schemas:
+                article:
+                  properties:
+                    color:
+                      type: string
+                      enum:
+                        - white
+                        - black
+        """.trimIndent()
+        val context = getOpenApiContextFromContent(content)
 
-        val violation = rule.validate(swagger)
+        val violations = rule.checkForEnums(context)
 
-        assertThat(violation).isNotNull()
-        assertThat(violation).isEqualTo(expectedViolation)
+        assertThat(violations).isNotEmpty
+        assertThat(violations[0].description).containsPattern("Property is not an extensible enum.*")
+        assertThat(violations[0].pointer.toString()).isEqualTo("/components/schemas/article/properties/color")
     }
 
     @Test
-    fun returnsViolationIfAnEnumInRequestParameter() {
-        val swagger = getFixture("enum_in_request_parameter.yaml")
-        val expectedViolation = Violation(
-                description = "Properties/Parameters [lifecycle_state, environment] are not extensible enums",
-                paths = listOf("/paths/apis/{api_id}/versions/GET/parameters/lifecycle_state",
-                        "/paths/apis/GET/parameters/environment"))
+    fun `checkForEnums should return violation if an enum is used as parameter`() {
+        val content = """
+            openapi: 3.0.1
+            paths:
+              /article:
+                get:
+                  parameters:
+                    - name: country
+                      in: query
+                      schema:
+                        type: string
+                        enum:
+                          - germany
+                          - sweden
+        """.trimIndent()
+        val context = getOpenApiContextFromContent(content)
 
-        val violation = rule.validate(swagger)
+        val violations = rule.checkForEnums(context)
 
-        assertThat(violation).isNotNull()
-        assertThat(violation).isEqualTo(expectedViolation)
+        assertThat(violations).isNotEmpty
+        assertThat(violations[0].description).containsPattern("Property is not an extensible enum.*")
+        assertThat(violations[0].pointer.toString()).isEqualTo("/paths/~1article/get/parameters/0/schema")
     }
 
     @Test
-    fun returnsNoViolationIfNoEnums() {
-        val swagger = getFixture("no_must_violations.yaml")
+    fun `checkForEnums should return violation if no enums are used`() {
+        val content = """
+            openapi: 3.0.1
+            components:
+              schemas:
+                article:
+                  properties:
+                    color:
+                      type: string
+        """.trimIndent()
+        val context = getOpenApiContextFromContent(content)
+
+        val violations = rule.checkForEnums(context)
 
-        assertThat(rule.validate(swagger)).isNull()
+        assertThat(violations).isEmpty()
     }
 }