feat: teach rulesets to accept exceptions #939

nulltoken · 2020-01-23T07:52:28Z

Fixes #747.

Checklist

Tests added / updated
Docs added / updated
Add a STDIN test
Create an issue to explicit the potential benefit of not swallowing up the ignored results
Create an issue to keep track of STDIN as a potential source ([RFC] Allow except to cope with STDIN input #1001)
Create an issue to keep track of potential changes to the exceptions engine to make it laxer ([RFC] Teach overrides to honor JSON Pointers #1002)

Does this PR introduce a breaking change?

Yes
No

P0lip · 2020-01-27T10:57:03Z

src/rulesets/reader.ts

@@ -97,6 +101,7 @@ const createRulesetProcessor = (
        if (extendedRuleset !== null) {
          mergeRules(rules, extendedRuleset.rules, parentSeverity);
          Object.assign(functions, extendedRuleset.functions);
+          mergeExceptions(baseUri, exceptions, extendedRuleset.exceptions);


Food for thoughts, but I'm pretty sure we'll want to filter out certain exceptions.
For instance,

{ "file/#": ["foo-rule"], "file/#.bar": ["foo-rule"] }

This could be expressed in a single exception "file/#": ["foo-rule"].
We'll have fewer paths to query later if we narrow them down here.

@P0lip Is that a use case so common that it should be included in that PR or should we wait until we get some feedback that maintaining exceptions is too tedious?

m-mohr · 2020-01-27T15:18:06Z

I like this a lot. I guess it would make #923 obsolete.

nulltoken · 2020-02-15T04:47:55Z

@philsturgeon @P0lip Here's a first alpha version of the PR.

repro/one.yaml

openapi: 3.0.0
info:
  version: 1.0.0
  title: Stoplight
paths: {}

repro/ruleset.yaml

extends: spectral:oas

except:
  "one.yaml#":
    - oas3-api-servers
  "./one.yaml#/info":  # Beside relative ones, also supports rooted paths ;-)
    - info-contact

cli usage

$ yarn cli lint repro/one.yaml -r repro/ruleset.yaml
OpenAPI 3.x detected

f:/spectral/repro/one.yaml
 1:1  warning  openapi-tags      OpenAPI object should have non-empty `tags` array.
 2:6  warning  info-description  OpenAPI object info `description` must be present and non-empty string.

✖ 2 problems (0 errors, 2 warnings, 0 infos, 0 hints)

I'm going to start working on the doco.
Still wrestling a bit with a harness related issue, but should be ready for a first review pass.

/cc @m-mohr Would you agree to give it a try against your own use cases, I'd be delighted to get some feedback!

src/utils/sortResults.ts

src/runner.ts

nulltoken · 2020-02-15T04:58:31Z

src/runner.ts

 ): IRuleResult[] => {
  const results: IRuleResult[] = [];

+  // TODO:
+  // Ruleset
+  // - To be investigated: How to cope with STDIN as a source and exception relative locations?


@P0lip I haven't started to dig into that. Any idea?

@P0lip As I'm not sure how to build a ruleset that would be able to reference a document passed through STDIN, I'm wondering if issuing a warning wouldn't be the best short-term approach.

When the ruleset contains exceptions
And the document is passed as STDIN
A warning would be issued notifying that exceptions handling isn't supported in that mode and that the run will ignore them.

Thoughts?

If you STDIN a file, it may well still have $ref's, and those $ref's would still have exceptions working just the same right? It'd just be the initial "entry file" being passed in as STDIN, and that could have things ignored with {ENTRY FILE OR ROOT IDENTIFIER}#/components and not mentioning a filename? Or do we need some sort of wildcard for saying "this is the first/main/root/entry file?

@philsturgeon I'm not even sure how a $ref containing a relative file location would be resolved in that context and I can't find any test in the codebase about that use case. Does it consider the current working directory as the root to be used?

@P0lip Any idea?

IIRC, the document path would be equal to the output of process.cwd().
I wouldn't really bother too much though, as it's kind of expected you provide a dereferenced document as STDIN.

🎉 It took me some time (as Windows and tty don't play well together) but I managed to find a way to honor exceptions in a stdin context.

@P0lip @philsturgeon Could you please take a look at the proposal?

As a side note, that more or less compelled me to find a sustainable way to develop in a xplat way from a Windows computer. I'm going to open a series of tiny pull requests to make contributing from Windows a bit easier.

src/__tests__/linter.jest.test.ts

test-harness/index.ts

nulltoken · 2020-02-15T18:59:02Z

@P0lip As this builds upon #966, maybe reviewing first #966 (and maybe (with a little luck 🤞 ) getting it merged 😉 ) would reduce the diff noise in this one and make it slightly easier to review.

danielgtaylor · 2020-02-20T20:10:46Z

I like this approach, but what do you think of also supporting something in the OpenAPI itself, e.g. something like this:

openapi: 3.0.0
info:
  version: 1.0.0
  title: Stoplight
  x-linter-ignore:
    - info-contact
x-linter-ignore:
  - oas3-api-servers
paths: {}

Then the exceptions can be explicitly listed in the API description document rather than having to link two places via paths in separate documents, and it prevents the need for an almost empty .spectral.yaml when using a shared set of rules.

You'd have to decide if the rules apply to all children as well, e.g. if ignoring a camel-casing rule on a path would apply to all parameters for all operations under that path.

philsturgeon · 2020-03-02T16:26:30Z

@danielgtaylor I'd like to keep the scope of this PR to ruleset based ignore, but we could have another issue discussing the pros and cons of letting API description authors mark their own descriptions with ignores.

It could be seen as a bit cheeky, trying to squeak an API with "naughty things" past automated API governance, but... make a case for it in a new issue and we'll see! :D

src/rulesets/mergers/exceptions.ts

philsturgeon · 2020-03-03T21:12:35Z

docs/getting-started/rulesets.md

@@ -249,6 +249,51 @@ The example above will run all of the rules defined in the `spectral:oas` rulese

 - [Rules relevant to OpenAPI v2 and v3](../reference/openapi-rules.md)

+## Selectively silencing some results
+
+From time to time, you want to ignore some specific results without turning off 


Please can we try to pick a word and stick to it? Using both "silencing" and "ignoring" is ok, but then we talk about exceptions.

Can we either ignore, or except? I think personally think ignore is more clear, and in line with .eslintignore, .gitignore, etc.

@philsturgeon Fixed. Do you like it better?

Yeah this is great! I'll give it a lick of paint after so you can focus on writing amazing code instead of mucking around with minor tweaks.

philsturgeon · 2020-03-03T21:12:55Z

docs/getting-started/rulesets.md

+
+`except` describes a map of locations (expressed as Json paths) and rules that should be ignored.
+
+Locations be can either described as relatively to the ruleset or absolute paths.


Suggested change

Locations be can either described as relatively to the ruleset or absolute paths.

Locations be can either described as relative to the ruleset or absolute paths.

@philsturgeon Fixed

philsturgeon · 2020-03-03T21:22:58Z

docs/getting-started/rulesets.md

+extends: spectral:oas
+
+except:
+  "<STDIN>#":


I don't think we need to mess around with this. I think putting something in the ruleset which demands knowledge of exactly how it's being called is going to be more confusion than its worth.

I would expect to see people writing #/info, or models/awful.json to make it shut up about issues in info, or a particularly awful model. If that info ignore just so happens to quieten up two different infos im not sure people will be that upset.

Let's try and avoid overthinking things, and leave room for more improvements later if we get feedback that they're required. I don't think entrance files are the biggest concern for people, and if they are a more specific exception can be written, so this will possibly not come up for a long time - if ever. 🎲

@philsturgeon Thanks for the detailed feedback!

The current implementation was designed to help make one of the initial requirements happen (from #747 (comment))

As such, the current syntax expects a file and a non ambiguous path within it that should be ignored.

Of course, the model can be extended to support more "handwritten" scenarios:

models/awful.json: [ rule1, rule2]: Would ignore rule1 and rule2 results wheverever in the file

#/info": [rule1, rule2]: Would ignore rule1 and rule2 results in any processed document provided they originate from a path that's #/info or below (eg. #/info/sub/path/1). This would be in line with @P0lip's comment

Would that fit you better?

Ok if that’s the goal just ignore STDIN entirely and support files.

@philsturgeon Dropped. STDIN and laxer syntax will be tracked through separate issues to keep that PR on scope.

P0lip

Just one extra note on STDIN, but looking really good besides that.

P0lip · 2020-03-04T12:59:20Z

src/rulesets/mergers/exceptions.ts

+      set.add(r);
+    });
+
+    target[normalizedLocation] = [...set].sort();


Suggested change

target[normalizedLocation] = [...set].sort();

target[normalizedLocation] = [...set].sort((a, b) => a.localeCompare(b));

@P0lip Fixed

P0lip · 2020-03-04T12:59:32Z

src/rulesets/mergers/exceptions.ts

+const buildErrorMessagePrefix = ($ref: string, rulesetUri?: string): string => {
+  let prefix = '';
+
+  if (rulesetUri !== undefined) prefix += `in ruleset \`${rulesetUri}\`, `;


Suggested change

if (rulesetUri !== undefined) prefix += `in ruleset \`${rulesetUri}\`, `;

if (rulesetUri !== void 0) prefix += `in ruleset \`${rulesetUri}\`, `;

@P0lip Fixed

src/rulesets/mergers/exceptions.ts

P0lip · 2020-03-04T13:03:46Z

docs/getting-started/rulesets.md

+except:
+  "<STDIN>#":
+    - oas3-api-servers
+  "<STDIN>#/info":


Hm, not a fan of it tbh.
How about ./#info? This would be more universal IMHO, as you could also target the input document, i.e. it'd target foo.yaml in when the following command spectral lint foo.yaml is executed.

I've been recommending we just ditch STDIN support entirely it's not worth the hassle generally speaking we should plan features before we get people working on them :D

This is trying to solve the Studio use case, and if people ask for this we can bring it in, but until then its scope creep.

@P0lip @philsturgeon STDIN support has been dropped. A warning is now generated when the document is consumed from stdin and the ruleset contains exceptions.

This is trying to solve the Studio use case, and if people ask for this we can bring it in, but until then its scope creep.

Studio should not be affected too much, since it doesn't use STDIN.

@P0lip that's what im trying to say. We are trying to get this working for the studio use case, which doesnt need STDIN, so we don't need to worry about STDIN here.

P0lip

Looking good to me.

P0lip · 2020-03-05T09:09:07Z

src/document.ts

@@ -6,6 +6,8 @@ import { IParser } from './parsers/types';
 import { IRuleResult } from './types';
 import { startsWithProtocol } from './utils';

+export const StdIn: string = '<STDIN>';


Suggested change

export const StdIn: string = '<STDIN>';

export const STDIN: string = '<STDIN>';

@P0lip Fixed

nulltoken force-pushed the ntk/exceptions branch from cff8072 to 5288ca0 Compare January 25, 2020 15:42

P0lip reviewed Jan 27, 2020

View reviewed changes

nulltoken force-pushed the ntk/exceptions branch 4 times, most recently from 78b6c8f to 07e3314 Compare February 3, 2020 19:22

m-mohr mentioned this pull request Feb 6, 2020

fix #915: Ignore examples with circular schemas #923

Closed

4 tasks

nulltoken force-pushed the ntk/exceptions branch 8 times, most recently from 64fa0df to a748cbb Compare February 15, 2020 04:22

nulltoken requested review from philsturgeon and P0lip February 15, 2020 04:48

nulltoken self-assigned this Feb 15, 2020

nulltoken commented Feb 15, 2020

View reviewed changes

src/utils/sortResults.ts Outdated Show resolved Hide resolved

nulltoken commented Feb 15, 2020

View reviewed changes

nulltoken mentioned this pull request Feb 15, 2020

chore(harness): reduce tests failures on Windows #966

Merged

4 tasks

nulltoken force-pushed the ntk/exceptions branch from bb6fc90 to ea310bd Compare February 15, 2020 18:38

nulltoken mentioned this pull request Feb 15, 2020

chore: introduce comparePosition #967

Merged

4 tasks

nulltoken force-pushed the ntk/exceptions branch from ea310bd to 990224d Compare February 15, 2020 18:51

nulltoken commented Feb 15, 2020

View reviewed changes

test-harness/index.ts Show resolved Hide resolved

nulltoken force-pushed the ntk/exceptions branch from 990224d to 1b0752f Compare February 16, 2020 10:21

nulltoken force-pushed the ntk/exceptions branch 3 times, most recently from ca194cd to b0b778b Compare February 28, 2020 06:41

nulltoken force-pushed the ntk/exceptions branch 2 times, most recently from 052eec4 to 4fc6f28 Compare March 2, 2020 19:22

nulltoken commented Mar 3, 2020

View reviewed changes

src/rulesets/mergers/exceptions.ts Outdated Show resolved Hide resolved

philsturgeon reviewed Mar 3, 2020

View reviewed changes

nulltoken force-pushed the ntk/exceptions branch from d4bb66a to 08a1959 Compare March 3, 2020 21:17

philsturgeon reviewed Mar 3, 2020

View reviewed changes

nulltoken mentioned this pull request Mar 3, 2020

docs(contribution): Windows based contributors developer workflow #999

Merged

4 tasks

P0lip reviewed Mar 4, 2020

View reviewed changes

nulltoken added 2 commits March 5, 2020 08:21

chore: Minor refactoring

3831404

chore(harness): support replacements in contents

a33af1d

nulltoken force-pushed the ntk/exceptions branch 2 times, most recently from aa2e102 to a3d4f9b Compare March 5, 2020 07:32

nulltoken requested review from philsturgeon and P0lip March 5, 2020 07:45

This was referenced Mar 5, 2020

[RFC] Allow except to cope with STDIN input #1001

Closed

[RFC] Teach overrides to honor JSON Pointers #1002

Closed

P0lip previously approved these changes Mar 5, 2020

View reviewed changes

feat: teach rulesets to accept exceptions

0148062

nulltoken dismissed P0lip’s stale review via 0148062 March 5, 2020 19:03

nulltoken force-pushed the ntk/exceptions branch from a3d4f9b to 0148062 Compare March 5, 2020 19:03

nulltoken requested a review from P0lip March 10, 2020 12:57

P0lip merged commit ffa70f3 into develop Mar 10, 2020

P0lip deleted the ntk/exceptions branch March 10, 2020 13:23

nulltoken mentioned this pull request Mar 26, 2020

Can't show more than one message from a custom function, even though the response is an array #1030

Closed

roubles mentioned this pull request Mar 25, 2021

Add rule exceptions per operation or path #1558

Closed


		`except` describes a map of locations (expressed as Json paths) and rules that should be ignored.

		Locations be can either described as relatively to the ruleset or absolute paths.

	target[normalizedLocation] = [...set].sort();
	target[normalizedLocation] = [...set].sort((a, b) => a.localeCompare(b));

	if (rulesetUri !== undefined) prefix += `in ruleset \`${rulesetUri}\`, `;
	if (rulesetUri !== void 0) prefix += `in ruleset \`${rulesetUri}\`, `;

	export const StdIn: string = '<STDIN>';
	export const STDIN: string = '<STDIN>';

feat: teach rulesets to accept exceptions #939

feat: teach rulesets to accept exceptions #939

Conversation

nulltoken commented Jan 23, 2020 • edited Loading

P0lip Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-mohr commented Jan 27, 2020 • edited Loading

nulltoken commented Feb 15, 2020 • 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

nulltoken commented Feb 15, 2020 • edited Loading

danielgtaylor commented Feb 20, 2020

philsturgeon commented Mar 2, 2020

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

P0lip 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

P0lip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Jan 23, 2020 •

edited

Loading

P0lip Jan 27, 2020 •

edited

Loading

m-mohr commented Jan 27, 2020 •

edited

Loading

nulltoken commented Feb 15, 2020 •

edited

Loading

nulltoken commented Feb 15, 2020 •

edited

Loading