Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule exceptions #747

Closed
xuorig opened this issue Nov 6, 2019 · 9 comments · Fixed by #939
Closed

Rule exceptions #747

xuorig opened this issue Nov 6, 2019 · 9 comments · Fixed by #939
Assignees
Labels
enhancement New feature or request p/high

Comments

@xuorig
Copy link

xuorig commented Nov 6, 2019

User story.
As a user, I can add exceptions to certain rules to ignore failures so that I can guard against new changes but run against legacy descriptions.

Is your feature request related to a problem?

For API descriptions that have a "pre-linting" era, but with linting rules that should be enforced for new changes, it would be nice to have some kind of mechanism similar to eslint's // eslint-disable-line no-use-before-define.

Describe the solution you'd like

I couldn't find any talks about this feature in issues / PRs, let me know if it exists already. I can see different ways of tackling this.

  • YAGNI: Users are expected to run spectral in a custom way, running only against new changes / diffs. Or maybe expected to write custom JS rules and do the filtering there.

  • Users are expected to handle this with given's JSON PATH entirely.

  • A new rule attribute:

  paths-kebab-case:
    description: Should paths be kebab-case.
    message: '{{property}} is not kebab-case: {{error}}'
    severity: error
    recommended: true
    given: $.paths[*]~
    then:
      function: pattern
      functionOptions:
        match: "^(\/[a-z0-9-{}]+)+$"
  except:
    - <json_path_matching-legacy-paths>
  • An eslint style convention directly on API specifications (Tricky since users would be required to strip this out before making public)
x-spectral-disable: "paths-kebab-case"
@philsturgeon
Copy link
Contributor

The CLI has —skip-rule Which probably doesn’t help, and the config files (rulesets) allow turning entire rules off, but we don’t support specific exceptions on a line or file basis just yet. I think x- extensions would be the way to go for now yeah.

@nulltoken
Copy link
Contributor

Interesting!

We had a similar requirement at $DAYJOB.

An analysis showed that when an exception to a rule was required, 98%+ of the time it was required for the whole file (as we organize OAS specs per resource and version) for stable/legacy versions that we can no longer change.

The way we implemented was as follows:

  • Teach to cli to accept an additional optional parameter (--exceptions || -e) that should be fed with a filepath
  • Post process spectral results to detect failures that match described exceptions and decorate them with a new status: skipped
  • Alter the report so that it also displays rules that would have caused the build to fail but were skipped because of being enlisted as exceptions.

Below a sample of an exceptions.yaml file:

# This file contains exceptions to the standard 'MY_RULESET.yaml' ruleset
#
# Note: Specified paths have to be defined relatively to the folder
# containing the 'exceptions.yaml' file

property-names-snake-case:
  - "./area1/v1/main_resource_1/main_resource1_spec.yaml"

path-no-sub-sub-resources:
  - "./area2/v2/main_resource_3/main_resource3_spec.yaml"

invalid-read_only-scope:
  - "./area1/v1/main_resource_1/main_resource1_spec.yaml"
  - "./area1/v1/main_resource_2/main_resource2_spec.yaml"
  - "./area2/v2/main_resource_3/main_resource3_spec.yaml"

invalid-scope-in-security-definition:
  - "./area2/v2/main_resource_3/main_resource3_spec.yaml"

[...] 

And a redacted view of a report:
image

@philsturgeon Would you think something like this might be of any value to Spectral, I'd be happy to contribute something along those lines.

@marbemac
Copy link
Contributor

marbemac commented Nov 19, 2019

Interesting use case! My two cents is to explore the "in ruleset" approach (described in the issue above as "A new rule attribute:") rather than "in openapi document". There are a variety of ways to accomplish this, here's another:

rules:
  paths-kebab-case:
    description: Should paths be kebab-case.
    message: '{{property}} is not kebab-case: {{error}}'
    severity: error
    recommended: true
    given: $.paths[*]~
    then:
      function: pattern
      functionOptions:
        match: "^(\/[a-z0-9-{}]+)+$"
except:
  <json_path_matching-legacy-paths>: ['paths-kebab-case', 'foo-bar'] # turn these two rules off for anything that matches this json path
  <json_path_matching-legacy-paths3>: ['*'] # all rules off if match this path

@philsturgeon
Copy link
Contributor

@nulltoken this looks great, and I like the syntax @marbemac!

@nulltoken
Copy link
Contributor

@marbemac I like the path based approach very much! A lot less brittle than a line/range based one and finer grained than a file one.

Would this proposal allow one to store rules and exceptions in different files as well?

@idosal
Copy link

idosal commented Dec 22, 2019

Interesting use case! My two cents is to explore the "in ruleset" approach (described in the issue above as "A new rule attribute:") rather than "in openapi document". There are a variety of ways to accomplish this, here's another:

rules:
  paths-kebab-case:
    description: Should paths be kebab-case.
    message: '{{property}} is not kebab-case: {{error}}'
    severity: error
    recommended: true
    given: $.paths[*]~
    then:
      function: pattern
      functionOptions:
        match: "^(\/[a-z0-9-{}]+)+$"
except:
  <json_path_matching-legacy-paths>: ['paths-kebab-case', 'foo-bar'] # turn these two rules off for anything that matches this json path
  <json_path_matching-legacy-paths3>: ['*'] # all rules off if match this path

This looks great. Is anyone working on it? Will such a PR be merged if it were implemented?

@philsturgeon
Copy link
Contributor

@idosal so far efforts have been going into v5.0 and cleaning out some of the old cruft which has been deprecated throughout v4.2. With that work nearing an end, we can look at new functionality like this.

@P0lip @marbemac I would love to see this coming for v5.1 so we can work on getting it into Studio too? A Right Click > "Ignore this instance" to accompany the Right Click > "Ignore this rule" option I've heard mentioned before. :)

@nulltoken
Copy link
Contributor

@philsturgeon started to work on this

@philsturgeon
Copy link
Contributor

This has made it into v1.12.0. Upgrade now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request p/high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants