Landing remaining rules #99
Conversation
Add rules in
|
|
| require("./rulesets/specification").rules, | ||
| ); | ||
|
|
||
| snykRulesService.useSpectralRuleset({ |
There was a problem hiding this comment.
How spectral rules can be added
| @@ -212,7 +214,7 @@ export class SnykApiCheckDsl implements ApiCheckDsl { | |||
| method: "", | |||
| }, | |||
| }, | |||
| }; | |||
| } as any; | |||
| @@ -366,7 +419,7 @@ export class SnykApiCheckDsl implements ApiCheckDsl { | |||
| method: "", | |||
| }, | |||
| kind: "ContextRule", | |||
| }, | |||
| } as any, | |||
There was a problem hiding this comment.
These are custom rules that don't really come out of OpenAPI (in this example its rules about your Context)
We added typing in the library last week to make the OpenAPI traverser more reliable / type safe, but these changes did not fit in. We need to think about custom rule types and how we want them to interop going forward. Definitely temporary
src/rulesets/api-lifeycle.ts
Outdated
| const { expect } = require("chai"); | ||
| import { expect } from 'chai'; |
src/rulesets/properties.ts
Outdated
| propertyFormat: ({ bodyProperties }: SnykApiCheckDsl) => { | ||
| bodyProperties.requirement.should( | ||
| "have a format when a string", | ||
| ({ flatSchema }, context) => { | ||
| if (flatSchema.type !== "string") return; | ||
| if (!withinAttributes(context)) return; | ||
| expect(flatSchema.format).to.exist; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
We found requiring format: to be problematic; some fields need to contain string content not expressed in the default OAS3 formats, and while we might invent custom formats for these, it'd be difficult to coordinate processing these custom formats consistently across multiple systems in different languages. So we removed the format: requirement on string properties in #89.
We feel that requiring example: and description: should be good enough for a great documentation experience.
There was a problem hiding this comment.
Makes sense, you're right this stuff is hard to enforce across languages.
Do you want us to comment this out, remove it?
| [200, 201].includes(response.statusCode) | ||
| ) { | ||
| const responseSchema = | ||
| specItem.content?.["application/vnd.api+json"]?.schema; |
There was a problem hiding this comment.
If the developer mis-types the content type, uses something else like application/json, is the rule error message meaningful enough to the developer, to understand what needs fixing?
There was a problem hiding this comment.
It made sense in our testing, but it's hard to say what the average developer will know about the guidelines. Do you have an idea what you'd want it to say?
Also we should go through and add a docsLink for each rule that we can so developers can click into sweater comb docs
There was a problem hiding this comment.
It made sense in our testing, but it's hard to say what the average developer will know about the guidelines. Do you have an idea what you'd want it to say?
As I'm writing this, I realize this might be better expressed as a different rule (all 2xx responses except 204 need an application/vnd.api+json response), but seeing it referenced here made me wonder how it'd be handled.
Also we should go through and add a docsLink for each rule that we can so developers can click into sweater comb docs
Yes please, that would be great!
There was a problem hiding this comment.
Looks like the 2xx rule I'm thinking of is already here: https://github.com/snyk/sweater-comb/pull/99/files#diff-efdbbec0a6118d1d560fdec0ba5b89c1cf08991e76c3f6d8c5143f4229c6c22dR97.
Sorry about that. All our internal repos have single quote on, so when we copy code in and out it gets messed up. I realized we should be running |
|
I'd be OK with single quote (less typing!) -- as long as it's all consistently formatted. Though, I'd rather do that change in a separate PR, to keep the diffs readable. |
|
Just one more thing, can we update json-schema and jsonpointer to address these vulns?
|
|
These appear to be in |
|
Thanks for checking! We'll have to ignore them for now and revisit later. For a CI tool, the risk is low, but a service that uses this may need more careful consideration down the road. |
This PR has been a 2 week effort from @smizell to:
Let us know how these look, specifically if the API Next and Json API rules feel more readable and maintainable (one of the goals).
Also noted and commented, the spectral compatibility feels like a big win here. If there are other rulesets you want in the future from the spectral community, they should work OOTB.