Do not validate description in parameters for links #515

lag-of-death · 2019-08-30T20:17:07Z

Fixes #272

Checklist

Tests added / updated
Docs added / updated

Does this PR introduce a breaking change?

Yes
No

lag-of-death · 2019-09-02T16:18:48Z

src/rulesets/oas/index.json

-        "field": "@key",
-        "pattern": "^(?!.*(\\$ref))"
-      },
+      "given": "$..[?(@path === \"$['parameters']\" || (@.parameters && @parentProperty !== 'links'))]..[?(@.in)]",


@P0lip, I'm kinda pulling my hair here. I can't find a rule that would satisfy OAS2 and OAS3. I was trying to create a rule that would eliminate parameters under links from the equation. I also wanted to make sure that even if we have:

links: abc: parameters: param: in: body

then we don't require .description. I don't know, but I feel like we might need more rules or a custom function.
WDYT?

Ideally, Spectral would support an array of paths, so that you can supply a few json path expressions.
We could implement it as a part of 4.2 or 5.0.
For now, we could simply have 2 separate rules, one for oas2, and the other for oas3.
Once an array of paths can be provided, we can merge the rules.
Thoughts?

We can support "string or array" and keep it as non breaking. The next release is likely to be 5.0 but supporting both is just a good idea either way for the sake of existing users. No BC-breaks-for-the-sake-of-it here. :)

@lag-of-death OAS2 doesn't have links :p

thanks @P0lip, I'll try with more than one rule for this now. @philsturgeon, "OAS2 doesn't have links", that's correct. As I was saying in: https://github.com/stoplightio/spectral/pull/515/files/1a6bac8bc7ee4a3063b1e4d61f8c2b163993bb6a#r320012847 "I can't find a rule that would satisfy OAS2 and OAS3". So this was an attempt to have just one rule. Didn't work out

…ers' into fix/description-in-links-parameters

chris-miaskowski · 2019-09-05T15:06:59Z

src/rulesets/oas3/index.json

@@ -36,7 +36,7 @@
    },
    "oas3-operation-security-defined": {
      "description": "Operation `security` values must match a scheme defined in the `components.securitySchemes` object.",
-      "recommended": true,
+      "recommended": false,


Suggested change

"recommended": false,

"recommended": true,

chris-miaskowski · 2019-09-05T15:07:29Z

src/rulesets/oas2/index.json

+      "description": "Parameter objects should have a `description`.",
+      "recommended": false,
+      "type": "style",
+      "given": "$..[?(@parentProperty === 'parameters' && @.in)]",


Over a call we tried to simplify it and use some sort of "union" but weren't able to find the right syntax.

chris-miaskowski · 2019-09-05T15:12:07Z

src/rulesets/oas3/index.json

+      "description": "Parameter objects should have a `description`.",
+      "recommended": true,
+      "type": "style",
+      "given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",


Suggested change

"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",

"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters'].[?(@.in)]",

Maybe check if single dot is enough - I think it should be.

chris-miaskowski · 2019-09-05T15:14:23Z

test-harness/scenarios/parameter-description-parameters.oas2.scenario

@@ -0,0 +1,34 @@
+====test====


Consider adding a positive test checking whether we don't return warnings if descriptions are provided.

chris-miaskowski

To me this looks good, we had a video review. I left some minor comments. Pre-approving. Please address the remaining small feedback and feel free to merge.

P0lip · 2019-09-05T15:58:34Z

src/rulesets/oas2/index.json

+      "description": "Parameter objects should have a `description`.",
+      "recommended": false,
+      "type": "style",
+      "given": "$..[?(@parentProperty === 'parameters' && @.in)]",


Suggested change

"given": "$..[?(@parentProperty === 'parameters' && @.in)]",

"given": "$.paths..[?(@parentProperty === 'parameters' && @.in)]",

@P0lip, what about $.parameters?

P0lip · 2019-09-05T15:59:19Z

src/rulesets/oas3/__tests__/oas3-parameter-description.ts

+          code: 'oas3-parameter-description',
+          message: 'Parameter objects should have a `description`.',
+          path: ['components', 'parameters', 'address'],
+          severity: 1,


Suggested change

severity: 1,

severity: DiagnosticSeverity.Warning,

P0lip · 2019-09-05T15:59:49Z

src/rulesets/oas3/index.json

+      "description": "Parameter objects should have a `description`.",
+      "recommended": true,
+      "type": "style",
+      "given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",


Suggested change

"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",

"given": "$.paths..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",

what about $.components.parameters ?

Urgh, forgot about that. Yeah, you are right, let's leave it as is.

lag-of-death · 2019-09-06T08:09:43Z

@P0lip, can you approve it?

lag-of-death added 2 commits August 30, 2019 22:09

fix: do not required links parameters to have description

65fdf4a

refactor: do not required links parameters to have description

d549d58

lag-of-death added t/bug Something isn't working WIP labels Aug 30, 2019

lag-of-death self-assigned this Aug 30, 2019

lag-of-death added 3 commits September 2, 2019 14:50

refactor: do not require links parameters to have description

0430dfd

feat: consider links parameters that have in property

00f4ff5

test: adjust tests

1a6bac8

lag-of-death commented Sep 2, 2019

View reviewed changes

lag-of-death added 6 commits September 4, 2019 12:02

feat: handle description in parameters

0eb3a7a

test: temporarily disable tests

a9c3f91

feat: have 2 rules for description in parameters

d192d35

feat: unrecommend parameters-description

b59ceda

test: add tests

5667936

test: fix tests

7422943

lag-of-death force-pushed the fix/description-in-links-parameters branch from 92df9aa to 7422943 Compare September 4, 2019 12:24

lag-of-death added 6 commits September 4, 2019 14:33

Merge branch 'develop' into fix/description-in-links-parameters

5f9404a

Merge branch 'develop' into fix/description-in-links-parameters

6e79809

Merge branch 'develop' into fix/description-in-links-parameters

6fb2ea5

docs: update parameter-description info

6b0f94f

Merge remote-tracking branch 'origin/fix/description-in-links-paramet…

91c1a04

…ers' into fix/description-in-links-parameters

docs: fix docs

3e392a5

lag-of-death added this to the Aug '19 Hardening milestone Sep 4, 2019

lag-of-death changed the title ~~[WIP] Fix/description in links parameters~~ Do not validate description in parameters for links Sep 4, 2019

lag-of-death requested review from philsturgeon and P0lip September 4, 2019 13:14

lag-of-death removed the WIP label Sep 5, 2019

lag-of-death modified the milestones: Aug '19 Hardening, Sept '19 Sep 5, 2019

chris-miaskowski reviewed Sep 5, 2019

View reviewed changes

chris-miaskowski previously approved these changes Sep 5, 2019

View reviewed changes

refactor: address feedback

240b1a7

lag-of-death dismissed chris-miaskowski’s stale review via 240b1a7 September 5, 2019 16:07

P0lip reviewed Sep 5, 2019

View reviewed changes

lag-of-death added 3 commits September 5, 2019 18:11

test: address feedback

8ea23a4

refactor: use DiagnosticSeverity.Warning

1746184

Merge branch 'develop' into fix/description-in-links-parameters

85e6a82

P0lip approved these changes Sep 6, 2019

View reviewed changes

lag-of-death merged commit f035c05 into develop Sep 6, 2019

lag-of-death deleted the fix/description-in-links-parameters branch September 6, 2019 08:35

P0lip removed this from the Sept '19 milestone May 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not validate description in parameters for links #515

Do not validate description in parameters for links #515

lag-of-death commented Aug 30, 2019 •

edited

Loading

lag-of-death Sep 2, 2019

P0lip Sep 2, 2019

philsturgeon Sep 2, 2019

philsturgeon Sep 2, 2019

lag-of-death Sep 4, 2019

chris-miaskowski Sep 5, 2019

chris-miaskowski Sep 5, 2019

chris-miaskowski Sep 5, 2019

chris-miaskowski Sep 5, 2019

chris-miaskowski left a comment

P0lip Sep 5, 2019

lag-of-death Sep 5, 2019

P0lip Sep 5, 2019

P0lip Sep 5, 2019

lag-of-death Sep 5, 2019

P0lip Sep 5, 2019

lag-of-death commented Sep 6, 2019

	"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",
	"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters'].[?(@.in)]",

	"given": "$..[?(@parentProperty === 'parameters' && @.in)]",
	"given": "$.paths..[?(@parentProperty === 'parameters' && @.in)]",

	"given": "$..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",
	"given": "$.paths..[?(@parentProperty !== 'links' && @.parameters)]['parameters']..[?(@.in)]",

Do not validate description in parameters for links #515

Do not validate description in parameters for links #515

Conversation

lag-of-death commented Aug 30, 2019 • edited Loading

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

chris-miaskowski left a 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

lag-of-death commented Sep 6, 2019

lag-of-death commented Aug 30, 2019 •

edited

Loading