714-rules-to-aos--AvoidLinkHeadersRule #763

ghost · 2018-07-24T13:07:57Z

#714 Convert rule AvoidLinkHeadersRule to the next Context object (OpenAPI 3).

…ashed commit) Squashed commits: [0c9fcc7] Convert rule AvoidLinkHeaderRule to the Context object (#714)

codecov-io · 2018-07-24T13:18:17Z

Codecov Report

Merging #763 into master will decrease coverage by 0.36%.
The diff coverage is 56.86%.

@@             Coverage Diff              @@
##             master     #763      +/-   ##
============================================
- Coverage     86.08%   85.71%   -0.37%     
- Complexity     1020     1024       +4     
============================================
  Files           170      171       +1     
  Lines          2667     2703      +36     
  Branches        374      385      +11     
============================================
+ Hits           2296     2317      +21     
- Misses          186      194       +8     
- Partials        185      192       +7

Impacted Files	Coverage Δ	Complexity Δ
...ver/src/main/java/de/zalando/zally/rule/Context.kt	`66.12% <33.33%> (-13.04%)`	`17 <0> (+1)`
...src/main/java/de/zalando/zally/util/OpenApiUtil.kt	`75% <75%> (ø)`	`0 <0> (?)`
...zalando/zally/rule/zalando/AvoidLinkHeadersRule.kt	`85.71% <85.71%> (-14.29%)`	`6 <6> (+2)`
...n/java/de/zalando/zally/util/ast/JsonPointers.java	`89.74% <0%> (+5.12%)`	`13% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update add1ab6...10f787b. Read the comment docs.

maxim-tschumak · 2018-07-24T13:31:10Z

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

@@ -11,21 +13,30 @@ class AvoidLinkHeadersRuleTest {

    @Test
    fun positiveCaseSpp() {
-        val swagger = getFixture("api_spp.json")
-        assertThat(rule.validate(swagger)).isNull()
+        val context = getContextFromFixture("api_spp.json")


We want to use comprehensible minimal specification fixtures in tests instead of full-size API specifications.

For the negative cases, I do agree; but for the positive ones too?

Nevermind that. We said we wanted to get rid of all of most of them because of internal reasons.

maxim-tschumak · 2018-07-24T13:32:53Z

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

@@ -120,7 +120,7 @@ class MediaTypesRuleTest {

    @Test
    fun `the SPP API generates violations`() {


I would replace tests based on a complete API specification with test cases describing the behavior of the check functions.

This has not changed since the PR for this rule. Only the !! have been removed because of a change in getContextFromFixture.

maxim-tschumak · 2018-07-24T13:40:49Z

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

+import io.swagger.v3.oas.models.responses.ApiResponse
+
+// todo: When all rules deriving from `HttpHeadersRule` are converted to the `Context` object, replace `HttpHeadersRule` with this class (#714).
+abstract class HttpHeadersRuleWithContext(rulesConfig: Config) {


Just a comment: In general, I would avoid having a class hierarchy for rules. I would rather move these util functions to some Utility class or to the context's companion object.

I did not want to change what was in place more than necessary. This is simply a copy of the original base class.

I am also OK in changing that. This header-gathering function can be moved to either the context or simply some util object.

I did not want to change what was in place more than necessary. This is simply a copy of the original base class.

I know - that's why it's only a comment :)

I am also OK in changing that. This header-gathering function can be moved to either the context or simply some util object.

We don't have to do this in this PR.

maxim-tschumak · 2018-07-25T07:55:49Z

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

+    fun `a Swagger API with no header called Link produces no violation`() {
+        @Language("YAML")
+        val context = Context.createSwaggerContext("""
+            swagger: 2.0


You can have an even smaller example, like:

swagger: 2.0 paths: /foo: get: parameters: FlowId: name: X-Flow-Id in: header type: string

BTW, the parameters are wrongly intended in your example (should be under get:).

Those parameters are for all of the API, not just for that path/verb.

I wanted the example to include:

a header in a path/verb

a header in the global parameters

another parameter that is not a header

to make sure the test does not return false negatives.

I can remove one of the header parameter for shortening it.

I can also add one that is called Link but is not a header, again to shield against false negatives in the test.

maxim-tschumak · 2018-07-25T08:53:28Z

👍

ghost · 2018-07-26T09:12:33Z

👍

David Dufour-Boivin added 2 commits July 24, 2018 14:52

Convert rule AvoidLinkHeaderRule to the Context object (#714) (+1 squ…

0144fa6

…ashed commit) Squashed commits: [0c9fcc7] Convert rule AvoidLinkHeaderRule to the Context object (#714)

Merge branch 'master' into 714-rules-to-aos--AvoidLinkHeadersRule

5018e15

ghost requested review from maxim-tschumak and tkrop July 24, 2018 13:07

ghost requested a review from netme as a code owner July 24, 2018 13:07

maxim-tschumak mentioned this pull request Jul 24, 2018

Refactor Checks to Operate on the Context Object #714

Closed

25 tasks

maxim-tschumak reviewed Jul 24, 2018

View reviewed changes

Litteral YAML in tests + Less class inheritance for rules (#714)

70ad4ab

ghost force-pushed the 714-rules-to-aos--AvoidLinkHeadersRule branch from 1a144a9 to 70ad4ab Compare July 24, 2018 15:14

maxim-tschumak reviewed Jul 25, 2018

View reviewed changes

David Dufour-Boivin added 2 commits July 25, 2018 10:38

Better test for AvoidLinkHeaderRule (#714)

14fc1f8

Merge branch 'master' into 714-rules-to-aos--AvoidLinkHeadersRule

10f787b

ghost merged commit 2702979 into master Jul 26, 2018

ghost deleted the 714-rules-to-aos--AvoidLinkHeadersRule branch July 26, 2018 09:14

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

714-rules-to-aos--AvoidLinkHeadersRule #763

714-rules-to-aos--AvoidLinkHeadersRule #763

ghost commented Jul 24, 2018

codecov-io commented Jul 24, 2018 •

edited

maxim-tschumak Jul 24, 2018

ghost Jul 24, 2018

ghost Jul 24, 2018

maxim-tschumak Jul 24, 2018

ghost Jul 24, 2018

maxim-tschumak Jul 24, 2018

ghost Jul 24, 2018

maxim-tschumak Jul 24, 2018

maxim-tschumak Jul 25, 2018

ghost Jul 25, 2018

ghost Jul 25, 2018

maxim-tschumak commented Jul 25, 2018

ghost commented Jul 26, 2018

		@@ -120,7 +120,7 @@ class MediaTypesRuleTest {

		@Test
		fun `the SPP API generates violations`() {

714-rules-to-aos--AvoidLinkHeadersRule #763

714-rules-to-aos--AvoidLinkHeadersRule #763

Conversation

ghost commented Jul 24, 2018

codecov-io commented Jul 24, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxim-tschumak commented Jul 25, 2018

ghost commented Jul 26, 2018

codecov-io commented Jul 24, 2018 •

edited