-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
714 query parameter collection format rule migration #805
Merged
maxim-tschumak
merged 3 commits into
master
from
714-query-parameter-collection-format-rule-migration
Aug 22, 2018
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
8e52ec6
refactor(server): extend OpenAPI class with getAllSchemas function (#…
maxim-tschumak 0052f90
feat(server): migrate collection format checks to use new context obj…
maxim-tschumak 35ca4da
refactor(server): get rid of multiple calls of .value to have cleaner…
maxim-tschumak File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 16 additions & 33 deletions
49
server/src/main/java/de/zalando/zally/rule/zalando/QueryParameterCollectionFormatRule.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,46 +1,29 @@ | ||
| 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.Rule | ||
| import de.zalando.zally.rule.api.Severity | ||
| import de.zalando.zally.rule.api.Violation | ||
| import io.swagger.models.Swagger | ||
| import io.swagger.models.parameters.Parameter | ||
| import io.swagger.models.parameters.QueryParameter | ||
| import de.zalando.zally.util.getAllParameters | ||
| import io.swagger.v3.oas.models.parameters.Parameter | ||
|
|
||
| @Rule( | ||
| ruleSet = ZalandoRuleSet::class, | ||
| id = "154", | ||
| severity = Severity.SHOULD, | ||
| title = "Explicitly define the Collection Format of Query Parameters" | ||
| ruleSet = ZalandoRuleSet::class, | ||
| id = "154", | ||
| severity = Severity.SHOULD, | ||
| title = "Use and Specify Explicitly the Form-Style Query Format for Collection Parameters" | ||
| ) | ||
| class QueryParameterCollectionFormatRule { | ||
| private val formatsAllowed = listOf("csv", "multi") | ||
| private val violationDescription = "CollectionFormat should be one of: $formatsAllowed" | ||
| private val allowedStyle = Parameter.StyleEnum.FORM | ||
| private val description = "Parameter style have to be `form`" | ||
|
|
||
| @Check(severity = Severity.SHOULD) | ||
| fun validate(swagger: Swagger): Violation? { | ||
| fun Collection<Parameter>?.extractInvalidQueryParam(path: String) = | ||
| orEmpty().filterIsInstance<QueryParameter>() | ||
| .filter { it.`type` == "array" && it.collectionFormat !in formatsAllowed } | ||
| .map { path to it.name } | ||
|
|
||
| val fromParams = swagger.parameters.orEmpty().values.extractInvalidQueryParam("parameters") | ||
| val fromPaths = swagger.paths.orEmpty().entries.flatMap { (name, path) -> | ||
| path.parameters.extractInvalidQueryParam(name) + path.operations.flatMap { operation -> | ||
| operation.parameters.extractInvalidQueryParam(name) | ||
| } | ||
| } | ||
|
|
||
| val allHeaders = fromParams + fromPaths | ||
| val paths = allHeaders | ||
| .map { "${it.first} ${it.second}" } | ||
| .distinct() | ||
|
|
||
| return if (paths.isNotEmpty()) createViolation(paths) else null | ||
| } | ||
|
|
||
| fun createViolation(paths: List<String>): Violation { | ||
| return Violation(violationDescription, paths) | ||
| } | ||
| fun checkParametersCollectionFormat(context: Context): List<Violation> = | ||
| if (context.isOpenAPI3()) | ||
| context.api.getAllParameters().values | ||
| .filter { "query" == it.`in` && "array" == it.schema.type } | ||
| .filter { it.style == null || allowedStyle != it.style } | ||
| .map { context.violation(description, it) } | ||
| else emptyList() | ||
| } |
25 changes: 0 additions & 25 deletions
25
server/src/main/java/de/zalando/zally/util/OpenAPIObjectUtils.kt
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
139 changes: 60 additions & 79 deletions
139
server/src/test/java/de/zalando/zally/rule/zalando/QueryParameterCollectionFormatRuleTest.kt
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,97 +1,78 @@ | ||
| package de.zalando.zally.rule.zalando | ||
|
|
||
| import io.swagger.models.Operation | ||
| import io.swagger.models.Path | ||
| import io.swagger.models.Swagger | ||
| import io.swagger.models.parameters.QueryParameter | ||
| import de.zalando.zally.getOpenApiContextFromContent | ||
| import de.zalando.zally.getSwaggerContextFromContent | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.intellij.lang.annotations.Language | ||
| import org.junit.Test | ||
|
|
||
| class QueryParameterCollectionFormatRuleTest { | ||
|
|
||
| private val rule = QueryParameterCollectionFormatRule() | ||
|
|
||
| @Test | ||
| fun negativeCaseCollectionFormatNotSupported() { | ||
| val swagger = Swagger().apply { | ||
| parameters = mapOf("test" to QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "notSupported" }) | ||
| } | ||
|
|
||
| val result = rule.validate(swagger)!! | ||
| assertThat(result.description).isEqualTo("CollectionFormat should be one of: [csv, multi]") | ||
| assertThat(result.paths).isEqualTo(listOf("parameters test")) | ||
| } | ||
|
|
||
| @Test | ||
| fun negativeCaseCollectionFormatNotSupportedFromPath() { | ||
| val paramList = listOf(QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "notSupported" }) | ||
| val swagger = Swagger().apply { | ||
| paths = mapOf("/apis" to Path().apply { get = Operation().apply { parameters = paramList } }) | ||
| } | ||
|
|
||
| val result = rule.validate(swagger)!! | ||
| assertThat(result.description).isEqualTo("CollectionFormat should be one of: [csv, multi]") | ||
| assertThat(result.paths).isEqualTo(listOf("/apis test")) | ||
| } | ||
|
|
||
| @Test | ||
| fun negativeCaseCollectionFormatNull() { | ||
| val swagger = Swagger().apply { | ||
| parameters = mapOf("test" to QueryParameter().apply { name = "test"; type = "array"; collectionFormat = null }) | ||
| } | ||
|
|
||
| val result = rule.validate(swagger)!! | ||
| assertThat(result.description).isEqualTo("CollectionFormat should be one of: [csv, multi]") | ||
| assertThat(result.paths).isEqualTo(listOf("parameters test")) | ||
| fun `checkParametersCollectionFormat should return violation if invalid collection format is set`() { | ||
| @Language("YAML") | ||
| val content = """ | ||
| openapi: 3.0.1 | ||
| components: | ||
| parameters: | ||
| filters: | ||
| in: query | ||
| style: spaceDelimited | ||
| schema: | ||
| type: array | ||
| """.trimIndent() | ||
| val context = getOpenApiContextFromContent(content) | ||
|
|
||
| val violations = rule.checkParametersCollectionFormat(context) | ||
|
|
||
| assertThat(violations).isNotEmpty | ||
| assertThat(violations.size).isEqualTo(1) | ||
| assertThat(violations[0].description).isEqualTo("Parameter style have to be `form`") | ||
| assertThat(violations[0].pointer.toString()).isEqualTo("/components/parameters/filters") | ||
| } | ||
|
|
||
| @Test | ||
| fun negativeCaseCollectionFormatNullFromPath() { | ||
| val paramList = listOf(QueryParameter().apply { name = "test"; type = "array"; collectionFormat = null }) | ||
| val swagger = Swagger().apply { | ||
| paths = mapOf("/apis" to Path().apply { get = Operation().apply { parameters = paramList } }) | ||
| } | ||
|
|
||
| val result = rule.validate(swagger)!! | ||
| assertThat(result.description).isEqualTo("CollectionFormat should be one of: [csv, multi]") | ||
| assertThat(result.paths).isEqualTo(listOf("/apis test")) | ||
| fun `checkParametersCollectionFormat should return no violation if valid collection format is set`() { | ||
| @Language("YAML") | ||
| val content = """ | ||
| openapi: 3.0.1 | ||
| components: | ||
| parameters: | ||
| filters: | ||
| in: query | ||
| style: form | ||
| schema: | ||
| type: array | ||
| """.trimIndent() | ||
| val context = getOpenApiContextFromContent(content) | ||
|
|
||
| val violations = rule.checkParametersCollectionFormat(context) | ||
|
|
||
| assertThat(violations).isEmpty() | ||
| } | ||
|
|
||
| @Test | ||
| fun positiveCaseCsv() { | ||
| val swagger = Swagger().apply { | ||
| parameters = mapOf("test" to QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "csv" }) | ||
| } | ||
|
|
||
| assertThat(rule.validate(swagger)).isNull() | ||
| } | ||
|
|
||
| @Test | ||
| fun positiveCaseCsvFromPath() { | ||
| val paramList = listOf(QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "csv" }) | ||
| val swagger = Swagger().apply { | ||
| paths = mapOf("/apis" to Path().apply { get = Operation().apply { parameters = paramList } }) | ||
| } | ||
|
|
||
| assertThat(rule.validate(swagger)).isNull() | ||
| } | ||
|
|
||
| @Test | ||
| fun positiveCaseMulti() { | ||
| val swagger = Swagger().apply { | ||
| parameters = mapOf("test" to QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "multi" }) | ||
| } | ||
|
|
||
| assertThat(rule.validate(swagger)).isNull() | ||
| } | ||
|
|
||
| @Test | ||
| fun positiveCaseMultiFromPath() { | ||
| val paramList = listOf(QueryParameter().apply { name = "test"; type = "array"; collectionFormat = "multi" }) | ||
| val swagger = Swagger().apply { | ||
| paths = mapOf("/apis" to Path().apply { get = Operation().apply { parameters = paramList } }) | ||
| } | ||
|
|
||
| assertThat(rule.validate(swagger)).isNull() | ||
| fun `checkParametersCollectionFormat should ignore OpenAPI 2 (Swagger) specifications`() { | ||
| @Language("YAML") | ||
| val content = """ | ||
| swagger: 2.0 | ||
| info: | ||
| title: Old API | ||
| version: 1.0.0 | ||
| parameters: | ||
| filters: | ||
| name: filters | ||
| in: query | ||
| type: array | ||
| items: | ||
| type: string | ||
| """.trimIndent() | ||
| val context = getSwaggerContextFromContent(content) | ||
|
|
||
| val violations = rule.checkParametersCollectionFormat(context) | ||
|
|
||
| assertThat(violations).isEmpty() | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxim-tschumak :
What happened to the long exhaustive version of that that I did some weeks ago...? Did we discarded it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I don't know. This is the function I've implemented last week and now I've just moved it to the right class (as you proposed).