Fix test, NPE in rules and catch parsing errors in OpenApi parser

zalando · Jun 29, 2021 · 291f4bd · 291f4bd
1 parent a0551a4
commit 291f4bd
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 17 deletions.
diff --git a/server/zally-core/src/main/kotlin/org/zalando/zally/core/CaseChecker.kt b/server/zally-core/src/main/kotlin/org/zalando/zally/core/CaseChecker.kt
@@ -178,6 +178,7 @@ class CaseChecker(
         .paths
         ?.values
         .orEmpty()
+        .filterNotNull()
         .flatMap { path -> path.readOperations() }
         .flatMap { op ->
             op?.tags.orEmpty().flatMap { name ->

diff --git a/server/zally-core/src/main/kotlin/org/zalando/zally/core/DefaultContextFactory.kt b/server/zally-core/src/main/kotlin/org/zalando/zally/core/DefaultContextFactory.kt
@@ -1,7 +1,5 @@
 package org.zalando.zally.core
 
-import org.zalando.zally.rule.api.Context
-import org.zalando.zally.rule.api.Violation
 import io.swagger.parser.SwaggerParser
 import io.swagger.parser.util.SwaggerDeserializationResult
 import io.swagger.v3.parser.OpenAPIV3Parser
@@ -11,6 +9,8 @@ import io.swagger.v3.parser.core.models.ParseOptions
 import io.swagger.v3.parser.core.models.SwaggerParseResult
 import io.swagger.v3.parser.util.ResolverFully
 import org.slf4j.LoggerFactory
+import org.zalando.zally.rule.api.Context
+import org.zalando.zally.rule.api.Violation
 import java.util.regex.Pattern
 
 class DefaultContextFactory(
@@ -52,13 +52,18 @@ class DefaultContextFactory(
     }
 
     fun parseOpenApiContext(content: String, authorization: String? = null): ContentParseResult<Context> {
-        val parseResult = parseOpenApi(content, authorization)
-        if (parseResult !is ContentParseResult.ParsedSuccessfully) return parseResult.of()
+        try {
+            val parseResult = parseOpenApi(content, authorization)
+            if (parseResult !is ContentParseResult.ParsedSuccessfully) return parseResult.of()
 
-        val resolveResult = resolveOpenApi(parseResult.result)
-        if (resolveResult !is ContentParseResult.ParsedSuccessfully) return resolveResult.of()
+            val resolveResult = resolveOpenApi(parseResult.result)
+            if (resolveResult !is ContentParseResult.ParsedSuccessfully) return resolveResult.of()
 
-        return ContentParseResult.ParsedSuccessfully(DefaultContext(content, parseResult.result.openAPI))
+            return ContentParseResult.ParsedSuccessfully(DefaultContext(content, parseResult.result.openAPI))
+        } catch (e: Exception) {
+            log.error("Failed to parse OpenApi schema", e)
+            return ContentParseResult.ParsedWithErrors(listOf(errorToViolation(e.localizedMessage)))
+        }
     }
 
     fun parseSwaggerContext(content: String): ContentParseResult<Context> {
@@ -71,7 +76,13 @@ class DefaultContextFactory(
         val resolveResult = resolveOpenApi(convertResult.result)
         if (resolveResult !is ContentParseResult.ParsedSuccessfully) return resolveResult.of()
 
-        return ContentParseResult.ParsedSuccessfully(DefaultContext(content, convertResult.result.openAPI, parseResult.result.swagger))
+        return ContentParseResult.ParsedSuccessfully(
+            DefaultContext(
+                content,
+                convertResult.result.openAPI,
+                parseResult.result.swagger
+            )
+        )
     }
 
     private fun parseOpenApi(content: String, authorization: String?): ContentParseResult<SwaggerParseResult> {
@@ -103,7 +114,10 @@ class DefaultContextFactory(
         try {
             ResolverFully(true).resolveFully(parseResult.openAPI)
         } catch (e: NullPointerException) {
-            log.warn("Failed to fully resolve OpenAPI schema. Error not covered by pre-resolve checks.\n${parseResult.openAPI}", e)
+            log.warn(
+                "Failed to fully resolve OpenAPI schema. Error not covered by pre-resolve checks.\n${parseResult.openAPI}",
+                e
+            )
         }
         return ContentParseResult.ParsedSuccessfully(parseResult)
     }

diff --git a/...lly-ruleset-zally/src/main/kotlin/org/zalando/zally/ruleset/zally/TagAllOperationsRule.kt b/...lly-ruleset-zally/src/main/kotlin/org/zalando/zally/ruleset/zally/TagAllOperationsRule.kt
@@ -42,6 +42,7 @@ class TagAllOperationsRule {
     fun checkDefinedTagsAreUsed(context: Context): List<Violation> {
         val used = context.api.paths?.values
             .orEmpty()
+            .filterNotNull()
             .flatMap { it.readOperations() }
             .flatMap { it?.tags.orEmpty() }
             .toSet()

diff --git a/server/zally-server/src/test/java/org/zalando/zally/rule/NullPointerExceptionTest.kt b/server/zally-server/src/test/java/org/zalando/zally/rule/NullPointerExceptionTest.kt
@@ -234,7 +234,7 @@ class NullPointerExceptionTest(
                       content:
                         schema:
                           type: object
-                        example: 
+                        example: {}
                         encoding:
                           propName:
                             contentType: binary
@@ -254,7 +254,7 @@ class NullPointerExceptionTest(
                       content:
                         schema:
                           type: object
-                        example: 
+                        example: {}
                         encoding:
                           propName:
                             contentType: binary
@@ -366,9 +366,6 @@ class NullPointerExceptionTest(
                     trace: {}
                     delete: {}
                     options: {}
-                    servers:
-                    - url: 'https://example.com/'
-                      description: Example Server
                     parameters:
                     - name: Parameter name
 
@@ -428,7 +425,7 @@ class NullPointerExceptionTest(
                         }
                     }
                 }
-                else -> emptyList<JsonPointer>()
+                else -> emptyList()
             }
     }
 
@@ -444,8 +441,13 @@ class NullPointerExceptionTest(
 
     @Test
     fun `validate with spec does not throw NullPointerException`() {
-        assertRuleManagerUsingAllAnnotatedRules(rules)
+        try {
+            assertRuleManagerUsingAllAnnotatedRules(rules)
 
-        validator.validate(spec, RulesPolicy(emptyList()))
+            validator.validate(spec, RulesPolicy(emptyList()))
+        } catch (e: NullPointerException) {
+            println("Test $name failed with NPE")
+            throw e
+        }
     }
 }