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

Error disabling an extended ruleset in an extended ruleset #1352

Closed
adanilev opened this issue Sep 24, 2020 · 7 comments · Fixed by #1465
Closed

Error disabling an extended ruleset in an extended ruleset #1352

adanilev opened this issue Sep 24, 2020 · 7 comments · Fixed by #1465
Assignees
Labels
t/bug Something isn't working
Milestone

Comments

@adanilev
Copy link
Contributor

Describe the bug
When I structure my rules like so:

index.json

{
  "extends": [
    "rules-one.json",
    "rules-two.json",
  ]
}

rules-one.json

{
  "extends": ["spectral:oas", "off"],
  "rules": {
    "path-params": true
  }
}

...and then load them like so:

const spectral = new Spectral();
spectral.registerFormat("oas3", isOpenApiv3);
await spectral.loadRuleset(path.resolve("***/index.json"));

I get the following error:

    Error: Provided ruleset is not an object
        at Object.assertValidRuleset (/***/node_modules/@stoplight/spectral/dist/rulesets/validation.js:23:15)
        at processRuleset (/***/node_modules/@stoplight/spectral/dist/rulesets/reader.js:62:38)
        at process._tickCallback (internal/process/next_tick.js:68:7)

This does not occur if:

  • I don't disable the spectral:oas rules
  • I extend and disable them in index.json instead of rules-one.json

I also tried to extend/disable the spectral rules in index.json and then enable specific ones in rules-one.json but that didn't work (they don't get enabled).

I printed the ruleset that fails to validate and it's an IIFE that starts with:
(function (root, factory) { if (typeof define === \"function\" && define.amd) {

To Reproduce

  1. Set up and load rules as described above
  2. Observe error

Expected behavior
The rules load successfully and allow me to disable an extended ruleset, in an extended ruleset, and then selectively enable them.

Environment (remove any that are not applicable):

  • Library version: 5.5.0
  • OS: macOS 10.15.6

Additional context
I organised my rules to be aligned with the OAS schema objects (e.g. operation-object.json) to keep things manageable. I want to enable specific spectral:oas rules alongside related, custom ones.

Workaround is to extend/disable in index.json and then enable specific rules there too.

@adanilev adanilev added the t/bug Something isn't working label Sep 24, 2020
@P0lip
Copy link
Contributor

P0lip commented Sep 24, 2020

@adanilev
Hey!
Does "extends": [["spectral:oas", "off"]] (note, it's a multi-dimensional array) work? This is the correct way of extending a ruleset with all rules disabled.

@adanilev
Copy link
Contributor Author

Hi @P0lip, ugh, I'm an idiot but there's still a problem.

No error is thrown but the rules are not getting disabled when I use "extends": [["spectral:oas", "off"]] in rules-one.json.

@P0lip
Copy link
Contributor

P0lip commented Sep 25, 2020

Hey @adanilev.
It seems to work correctly for me.
I've got the following ruleset

// .spectral.json
{
  "extends": [["spectral:oas", "off"]],
  "rules": {
    "path-params": true
  }
}

Do you load such a ruleset directly or do you load something as follows?

{
  "extends": "./foo.json"
}

where foo.json has exactly the same contents as .spectral.json above.

The latter example (ruleset extending foo.json) is indeed somewhat broken - it doesn't seem like we disable rules.
Is this what you are referring to?

@adanilev
Copy link
Contributor Author

@P0lip, yep, that's the issue I'm referring to.

@mkistler
Copy link
Contributor

I ran into this same problem in the last few days and I think I understand what's going on here.

I think that the way "extends" works -- it enables or disables rules in the "parent" ruleset based on the second value in the array -- "on", "off", or "recommended" -- not whether the rule is actually enabled in the parent ruleset. So for this case:

foo.yaml

extends: [[spectral:oas, off]]
rules:
  operation-operationId-unique: true

All rules are off except operation-operationId-unique, but the full set of rules are defined and retain their "recommended" setting from spectral:oas.

So then if we have:

bar.yaml

extends: foo.yaml

it will enable all the "recommended" rules in the foo.yaml, which is identical to the set of "recommended" rules in spectral.oas.

So this behavior may not be intuitive, but I think it does match the documentation.

@mkistler
Copy link
Contributor

Based on the above, I thought I saw a path to working-around this issue -- just set all the spectral:oas rules that I wanted disabled to "recommended: no". Unfortunately, you can't just modify "recommended" when redefining a rule -- you have to repeat the entire rule. Ugh.

So I would like to propose a change to the way extends works. The most straightforward change would be to change the way extends: [[spectral:oas, off]] works to set both the severity and "recommended" values in the derived ruleset. This would be a change from the current behavior but I think more natural/intuitive.

If there is concern about this kind of behavioral change, another possibility is to support a third element in the array to specify how to set "recommended". So extends: [[spectral:oas, off, off]] would disable all rules and set "recommended" to "false" for all rules.

For this to fully work, we'd also need the "enabling" syntax to update "recommended", again either transparently or with a second option.

rules:
  operation-operationId-unique: [true, true]

Would enable the rule and set "recommended" to true.

@mkistler
Copy link
Contributor

Well ... curiously ... things work a bit differently if you don't disable all the rules with extends: [[spectral:oas, off]] and instead disable each one individually. With that approach, you can extend the ruleset without disabled (but "recommended") rules becoming enabled again.

So it does seem like there is an inconsistency here. If extends: [[spectral:oas, off]] could be made to work like disabling all the rules individually, I think that would resolve the inconsistency in a way that would match most people's expectations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants