fix: supported references for problem+json (#1274) #1277
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1277 +/- ##
============================================
- Coverage 76.07% 76.01% -0.07%
- Complexity 1009 1017 +8
============================================
Files 187 187
Lines 2922 2935 +13
Branches 501 508 +7
============================================
+ Hits 2223 2231 +8
Misses 435 435
- Partials 264 269 +5
Continue to review full report at Codecov.
|
| .filter { "default" in it.second.keys } | ||
| .flatMap { it.second.getValue("default").content.orEmpty().entries } | ||
| .filter { (contentType, _) -> contentType in validContentTypes } | ||
| .filterNot { it.value?.schema?.`$ref` in validRefs || isProblemJsonSchema(it.value?.schema) } | ||
| .map { context.violation("problem json has to be used as default response (${validRefs[0]})", it.value) } | ||
| .filterNot { it.value?.schema?.`$ref` in validRefs } |
There was a problem hiding this comment.
This filter is actually only active, when the references are not resolved, which we currently have no happy path test case for.
There was a problem hiding this comment.
Swagger parser in Zally always fully resolves references (applies both resolve and resolveFully options). I think we can remove completely the ref check from this rule and check only that the default response is a Problem.json object regardless of the reference, because refs will be removed from the schema.
There was a problem hiding this comment.
I think, keeping it is more robust. If we drop it, the server will fail, when the problem+json resource is unreachable - not sure how often this may happen. I would more like to add a test that enforces non-loading ...
Anyhow, unless you have an idea, I suggest to merge it now finding a fix for this later.
There was a problem hiding this comment.
Non-loading would never happen in reality unless we allow to do so :) If the resource is unreachable, then the response will be resolved to nullable response without properties in the schema. This is what the rule can check.
tkrop
force-pushed
the
1274-supported-references-for-problem
branch
2 times, most recently
from
0246c86
to
4cab783
Compare
tkrop
force-pushed
the
1274-supported-references-for-problem
branch
from
4cab783
to
345d406
Compare
…rences-for-problem
|
👍 |
1 similar comment
|
👍 |
fixes #1274
fixes #891
Tricky! As it actually turned out, not the missing reference broke the check, but the change in the problem specification the check was not adapted to. However, the reference check does play a role as soon as the reference cannot be resolved for any reason. It is just not tested while validating the check.
@vadeg if you know how to switch of reference resolution for one test, I would like to add this as an additional test case.
Anyhow, I added a separate check method as well as full test coverage for checking and notifying the media-type violations.