fix: nested extends broken in rulesets (#1465)

* fix(ruleset): nested extends not working correctly * Update src/rule.ts Co-authored-by: Domagoj Kriskovic <dom@stoplight.io> Co-authored-by: Domagoj Kriskovic <dom@stoplight.io>
stoplightio · Jan 14, 2021 · e8c1846 · e8c1846
1 parent ba9470e
commit e8c1846
Show file tree

Hide file tree

Showing 11 changed files with 108 additions and 248 deletions.
diff --git a/src/rule.ts b/src/rule.ts
@@ -3,26 +3,26 @@ import { JSONPathExpression } from 'nimma';
 
 import { IDocument } from './document';
 import { DEFAULT_SEVERITY_LEVEL, getDiagnosticSeverity } from './ruleset/severity';
-import { IGivenNode, IRule, IThen, SpectralDiagnosticSeverity } from './types';
+import { IGivenNode, IProcessedRule, IThen } from './types';
 import { hasIntersectingElement } from './utils';
+import { DiagnosticSeverity } from '@stoplight/types';
 
 export class Rule {
   public readonly name: string;
   public readonly description: string | null;
   public readonly message: string | null;
-  public readonly severity: SpectralDiagnosticSeverity;
+  public readonly severity: DiagnosticSeverity;
   public readonly resolved: boolean;
   public readonly formats: Optional<string[]>;
 
   public readonly then: IThen[];
   public readonly given: string[];
 
-  public get enabled(): boolean {
-    return this.severity !== -1;
-  }
+  public readonly enabled: boolean;
 
-  constructor(name: string, rule: IRule) {
+  constructor(name: string, rule: IProcessedRule) {
     this.name = name;
+    this.enabled = rule.enabled ?? true;
     this.description = rule.description ?? null;
     this.message = rule.message ?? null;
     this.severity = rule.severity === void 0 ? DEFAULT_SEVERITY_LEVEL : getDiagnosticSeverity(rule.severity);
@@ -49,7 +49,7 @@ function stub(): void {
 export class OptimizedRule extends Rule {
   public readonly expressions: JSONPathExpression[];
 
-  constructor(name: string, rule: IRule) {
+  constructor(name: string, rule: IProcessedRule) {
     super(name, rule);
     this.expressions = this.given.map(given => {
       const expr = new JSONPathExpression(given, stub, stub);

diff --git a/src/ruleset/__tests__/__fixtures__/severity/off-proxy.json b/src/ruleset/__tests__/__fixtures__/severity/off-proxy.json
@@ -0,0 +1,3 @@
+{
+  "extends": "./off.json"
+}
diff --git a/src/ruleset/__tests__/reader.jest.test.ts b/src/ruleset/__tests__/reader.jest.test.ts
@@ -37,6 +37,7 @@ describe('Rulesets reader', () => {
           'valid-rule': {
             given: '$.info',
             severity: DiagnosticSeverity.Warning,
+            enabled: true,
             recommended: true,
             then: expect.any(Object),
           },
@@ -53,6 +54,7 @@ describe('Rulesets reader', () => {
         rules: {
           'valid-rule': {
             given: '$.info',
+            enabled: true,
             recommended: true,
             severity: DiagnosticSeverity.Warning,
             then: {
@@ -61,6 +63,7 @@ describe('Rulesets reader', () => {
           },
           'valid-rule-2': {
             given: '$.info',
+            enabled: true,
             recommended: true,
             severity: DiagnosticSeverity.Warning,
             then: {
@@ -74,7 +77,7 @@ describe('Rulesets reader', () => {
 
   describe('severity', () => {
     function getEnabledRules(rules: RuleCollection) {
-      return Object.keys(rules).filter(name => rules[name].severity !== -1);
+      return Object.keys(rules).filter(name => rules[name].enabled);
     }
 
     it('given ruleset with extends set to recommended, should enable recommended rules', async () => {
@@ -122,6 +125,18 @@ describe('Rulesets reader', () => {
 
       expect(getEnabledRules(rules)).toEqual(['overridable-rule']);
     });
+
+    it('given nested extends with severity set to off', async () => {
+      const { rules } = await readRuleset(getFixturePath('severity/off-proxy.json'));
+      expect(Object.keys(rules)).toEqual([
+        'description-matches-stoplight',
+        'title-matches-stoplight',
+        'contact-name-matches-stoplight',
+        'overridable-rule',
+      ]);
+
+      expect(getEnabledRules(rules)).toEqual(['overridable-rule']);
+    });
   });
 
   it('should limit the scope of formats to a ruleset', async () => {
@@ -175,6 +190,7 @@ describe('Rulesets reader', () => {
           'valid-foo-value': {
             given: '$',
             severity: DiagnosticSeverity.Warning,
+            enabled: true,
             recommended: true,
             then: {
               field: 'foo',
@@ -234,6 +250,7 @@ describe('Rulesets reader', () => {
           'valid-foo-value': {
             given: '$',
             severity: DiagnosticSeverity.Warning,
+            enabled: true,
             recommended: true,
             then: {
               field: 'foo',
@@ -378,6 +395,7 @@ describe('Rulesets reader', () => {
         'bar-rule': {
           given: '$.bar',
           message: 'Bar is truthy',
+          enabled: true,
           recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: {
@@ -387,6 +405,7 @@ describe('Rulesets reader', () => {
         'foo-rule': {
           given: '$.foo',
           message: 'Foo is falsy',
+          enabled: true,
           recommended: true,
           severity: DiagnosticSeverity.Warning,
           then: {
@@ -406,6 +425,7 @@ describe('Rulesets reader', () => {
           given: '$',
           message: 'Foo',
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
           recommended: true,
           then: {
             function: 'falsy',
@@ -480,6 +500,7 @@ describe('Rulesets reader', () => {
         description: "All 'HTTP' headers SHOULD NOT include 'X-' headers (https://tools.ietf.org/html/rfc6648).",
         given: ["$..parameters[?(@.in == 'header')].name"],
         message: "HTTP header '{{value}}' SHOULD NOT include 'X-' prefix in {{path}}",
+        enabled: true,
         recommended: true,
         severity: 1,
         then: {
@@ -494,6 +515,7 @@ describe('Rulesets reader', () => {
         description: "All 'HTTP' headers SHOULD NOT include 'X-' headers (https://tools.ietf.org/html/rfc6648).",
         given: ['$.[responses][*].headers.*~'],
         message: "HTTP header '{{value}}' SHOULD NOT include 'X-' prefix in {{path}}",
+        enabled: true,
         recommended: true,
         severity: 1,
         then: {
@@ -515,6 +537,7 @@ describe('Rulesets reader', () => {
         documentationUrl:
           'https://stoplight.io/p/docs/gh/stoplightio/spectral/docs/reference/openapi-rules.md#foo-rule',
         given: '',
+        enabled: true,
         recommended: true,
         severity: DiagnosticSeverity.Warning,
         then: {
@@ -524,6 +547,7 @@ describe('Rulesets reader', () => {
       'bar-rule': {
         documentationUrl: 'https://stoplight.io/p/docs/gh/stoplightio/spectral/docs/reference/bar-rule.md',
         given: '',
+        enabled: true,
         recommended: true,
         severity: DiagnosticSeverity.Warning,
         then: {

diff --git a/src/ruleset/__tests__/severity.test.ts b/src/ruleset/__tests__/severity.test.ts
diff --git a/src/ruleset/mergers/__tests__/rules.test.ts b/src/ruleset/mergers/__tests__/rules.test.ts
@@ -51,7 +51,7 @@ describe('Ruleset rules merging', () => {
       test: false,
     });
 
-    expect(rules).toHaveProperty('test.severity', -1);
+    expect(rules).toHaveProperty('test.enabled', false);
   });
 
   it('supports nested severity', () => {
@@ -144,7 +144,7 @@ describe('Ruleset rules merging', () => {
       test: ['off'],
     });
 
-    expect(rules).toHaveProperty('test.severity', -1);
+    expect(rules).toHaveProperty('test.enabled', false);
   });
 
   it('does not set functionOptions if rule does not implement it', () => {
@@ -185,8 +185,8 @@ describe('Ruleset rules merging', () => {
       'off',
     );
 
-    expect(rules).toHaveProperty('test.severity', -1);
-    expect(rules).toHaveProperty('test2.severity', -1);
+    expect(rules).toHaveProperty('test.enabled', false);
+    expect(rules).toHaveProperty('test2.enabled', false);
   });
 
   it('picks up recommended rules', () => {
@@ -212,9 +212,9 @@ describe('Ruleset rules merging', () => {
     expect(rules.test2.recommended).toBe(false);
     expect(rules.test3.recommended).toBe(true);
 
-    expect(rules).toHaveProperty('test.severity', DiagnosticSeverity.Warning);
-    expect(rules).toHaveProperty('test2.severity', -1);
-    expect(rules).toHaveProperty('test3.severity', DiagnosticSeverity.Warning);
+    expect(rules).toHaveProperty('test.enabled', true);
+    expect(rules).toHaveProperty('test2.enabled', false);
+    expect(rules).toHaveProperty('test3.enabled', true);
   });
 
   it('rules with no severity and no recommended set are treated as warnings', () => {
@@ -249,7 +249,7 @@ describe('Ruleset rules merging', () => {
       'recommended',
     );
 
-    expect(rules).toHaveProperty('rule.severity', -1);
+    expect(rules).toHaveProperty('rule.enabled', false);
   });
 
   it('sets warning as default severity level if a rule has no severity specified', () => {
@@ -388,45 +388,54 @@ describe('Ruleset rules merging', () => {
     });
 
     it('respects ruleset severity', () => {
-      expect(mergeRules({}, rules, 'all')).toEqual({
+      expect(mergeRules({}, JSON.parse(JSON.stringify(rules)), 'all')).toEqual({
         rule: expect.objectContaining({
           recommended: true,
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
         }),
         'rule-with-no-recommended': expect.objectContaining({
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
         }),
         'optional-rule': expect.objectContaining({
           recommended: false,
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
         }),
       });
 
-      expect(mergeRules({}, rules, 'recommended')).toEqual({
+      expect(mergeRules({}, JSON.parse(JSON.stringify(rules)), 'recommended')).toEqual({
         rule: expect.objectContaining({
           recommended: true,
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
         }),
         'rule-with-no-recommended': expect.objectContaining({
           severity: DiagnosticSeverity.Warning,
+          enabled: true,
         }),
         'optional-rule': expect.objectContaining({
           recommended: false,
-          severity: -1,
+          severity: DiagnosticSeverity.Warning,
+          enabled: false,
         }),
       });
 
-      expect(mergeRules({}, rules, 'off')).toEqual({
+      expect(mergeRules({}, JSON.parse(JSON.stringify(rules)), 'off')).toEqual({
         rule: expect.objectContaining({
           recommended: true,
-          severity: -1,
+          severity: DiagnosticSeverity.Warning,
+          enabled: false,
         }),
         'rule-with-no-recommended': expect.objectContaining({
-          severity: -1,
+          severity: DiagnosticSeverity.Warning,
+          enabled: false,
         }),
         'optional-rule': expect.objectContaining({
           recommended: false,
-          severity: -1,
+          severity: DiagnosticSeverity.Warning,
+          enabled: false,
         }),
       });
     });