refactor(ipa): CLOUDP-453872 named rule conditions and defineRule

tools/spectral/ipa/CONTRIBUTING.md

@@ -44,6 +44,18 @@ npm run test -- singletonHasNoId
 ```
 ---
 
+### Checking that a change does not alter results
+
+For a refactor of the ruleset, `ipa-snapshot-compare.js` lints a spec with the ruleset at a git ref and with this checkout, and reports per rule which findings and collector entries (violations, adoptions, exceptions) were gained or lost. It exits 1 on any difference.
+
+```
+node tools/spectral/ipa/scripts/ipa-snapshot-compare.js --spec openapi/v2.json --base origin/main
+```
+
+- The ref is checked out into a temporary worktree that is removed afterwards, so your working tree is not touched. Nothing needs building.
+- Without `--head`, the head is this checkout as it is now, including uncommitted changes. Pass `--head <git-ref>` to compare two refs.
+- Add `--verbose` to list the entries. A full run on `openapi/v2.json` takes about 15 seconds.
+
 ## Code Style
 
 - Use [Prettier](https://prettier.io/) for code formatting
@@ -106,11 +118,35 @@ To learn more about Spectral custom functions, see the [Spectral Documentation](
 
 ---
 
+### Where a Rule Applies: Conditions and `defineRule`
+
+Concepts such as "singleton resource", "read-only resource" or "single resource path" are defined once, as named **conditions** in [`rulesets/functions/utils/conditions`](./rulesets/functions/utils/conditions). Each condition has a plain-English definition and examples (add a `note` to an example when what makes it match isn't visible in its path), and the glossary [`rulesets/CONDITIONS.md`](./rulesets/CONDITIONS.md) is generated from them.
+
+Build new rules with [`defineRule`](./rulesets/functions/utils/defineRule.js), declaring the conditions under which the rule applies instead of returning early:
+
+```js
+import { defineRule } from './utils/defineRule.js';
+import { not } from './utils/conditions/registry.js';
+import { isReadOnly, isSingleton } from './utils/conditions/resource.js';
+
+export default defineRule({
+  appliesTo: [isSingleton, not(isReadOnly)],
+  check: ({ input, path }) => (hasPutMethod(input) || hasPatchMethod(input) ? [] : [{ path, message: ERROR_MESSAGE }]),
+});
+```
+
+- `appliesTo` conditions are evaluated in order. If one does not hold, the rule does not apply and nothing is collected.
+- `check` returns the validation errors; adoptions, violations and exceptions are collected for you (see below).
+- The rule's "Applies to" section in [`rulesets/README.md`](./rulesets/README.md) is generated from `appliesTo`, so don't describe these conditions in the YAML description.
+- Reuse an existing condition where possible. If a rule needs a different definition, add a condition with `variantOf` and a `divergence` note rather than changing the shared one.
+
+To check that a refactor changes no result, run `tools/spectral/ipa/scripts/ipa-snapshot.js` on the same spec before and after, then compare the two outputs with `ipa-snapshot-compare.js` (see the header of each script for options).
+
 ### Resource & Singleton Evaluation
 
 In IPA Spectral validation, a **resource** is typically identified using a *resource collection path*, such as `/resource`.
 
-To develop rules that evaluate resource and singleton patterns, you can use the following utility functions:
+The conditions above wrap the following utility functions, which rules not yet built with `defineRule` use directly:
 
 ####  Retrieve Resource Path Items
 

tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js

@@ -0,0 +1,89 @@
+import fs from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { afterAll, beforeAll, describe, expect, it } from '@jest/globals';
+import { normalizeCollectorEntries, normalizeFindings } from '../../scripts/ipa-snapshot';
+import { compareRefs, diffEntries } from '../../scripts/ipa-snapshot-compare';
+
+describe('tools/spectral/ipa/scripts/ipa-snapshot.js', () => {
+  it('sorts findings and drops the source file', () => {
+    const finding = (code, jsonPath) => ({ code, path: jsonPath, severity: 0, message: 'm', source: '/tmp/a.yaml' });
+    expect(normalizeFindings([finding('b', ['paths', '/b']), finding('a', ['paths', '/a'])])).toEqual([
+      { code: 'a', path: 'paths./a', severity: 0, message: 'm' },
+      { code: 'b', path: 'paths./b', severity: 0, message: 'm' },
+    ]);
+  });
+
+  it('counts identical collector entries regardless of key order', () => {
+    const normalized = normalizeCollectorEntries({
+      violations: [
+        { ruleName: 'r', componentId: 'c' },
+        { componentId: 'c', ruleName: 'r' },
+        { ruleName: 'r', componentId: 'd' },
+      ],
+    });
+    expect(normalized.violations).toEqual([
+      { componentId: 'c', ruleName: 'r', count: 2 },
+      { componentId: 'd', ruleName: 'r', count: 1 },
+    ]);
+  });
+});
+
+describe('tools/spectral/ipa/scripts/ipa-snapshot-compare.js', () => {
+  const ruleOf = (entry) => entry.ruleName;
+
+  it('reports nothing for identical entries', () => {
+    const entries = [{ ruleName: 'r', componentId: 'c', count: 2 }];
+    expect(diffEntries(entries, entries, ruleOf).size).toBe(0);
+  });
+
+  it('reports added and removed entries per rule, using the count difference', () => {
+    const base = [
+      { ruleName: 'r', componentId: 'c', count: 2 },
+      { ruleName: 'r', componentId: 'gone' },
+    ];
+    const head = [
+      { ruleName: 'r', componentId: 'c', count: 3 },
+      { ruleName: 's', componentId: 'new' },
+    ];
+    const diff = diffEntries(base, head, ruleOf);
+    expect(diff.get('r')).toEqual({
+      added: [{ ruleName: 'r', componentId: 'c', count: 1 }],
+      removed: [{ ruleName: 'r', componentId: 'gone', count: 1 }],
+    });
+    expect(diff.get('s')).toEqual({ added: [{ ruleName: 's', componentId: 'new', count: 1 }], removed: [] });
+  });
+});
+
+describe('tools/spectral/ipa/scripts/ipa-snapshot-compare.js compareRefs', () => {
+  const spec = path.join(os.tmpdir(), `ipa-snapshot-test-spec-${process.pid}.yaml`);
+
+  beforeAll(() => {
+    fs.writeFileSync(
+      spec,
+      [
+        'openapi: 3.0.1',
+        'info: { title: t, version: "1" }',
+        'paths:',
+        '  /api/atlas/v2/groups/{groupId}:',
+        '    get:',
+        '      operationId: getGroup',
+        '      responses:',
+        "        '200': { description: OK }",
+        'components: {}',
+      ].join('\n')
+    );
+  });
+
+  afterAll(() => {
+    fs.rmSync(spec, { force: true });
+  });
+
+  it('finds no difference between a ref and the same checkout, and leaves no worktree behind', () => {
+    const { findings, collector } = compareRefs({ spec, base: 'HEAD' });
+    expect(findings.size).toBe(0);
+    expect(Object.values(collector).every((diff) => diff.size === 0)).toBe(true);
+    const leftovers = fs.readdirSync(os.tmpdir()).filter((name) => /^ipa-(worktree|snapshot-compare)-/.test(name));
+    expect(leftovers).toEqual([]);
+  }, 60000);
+});

tools/spectral/ipa/__tests__/utils/conditions.test.js

@@ -0,0 +1,94 @@
+import { describe, expect, it } from '@jest/globals';
+import {
+  allOf,
+  anyOf,
+  defineCondition,
+  evaluate,
+  listConditions,
+  not,
+} from '../../rulesets/functions/utils/conditions/registry';
+import { RuleContext } from '../../rulesets/functions/utils/ruleContext';
+import '../../rulesets/functions/utils/conditions/path';
+import '../../rulesets/functions/utils/conditions/resource';
+import '../../rulesets/functions/utils/conditions/operation';
+import '../../rulesets/functions/utils/conditions/response';
+
+function contextFrom({ input, options, path, paths = {} }) {
+  return new RuleContext({ input, options, path, ruleName: 'test-rule', oas: { paths } });
+}
+
+describe('tools/spectral/ipa/rulesets/functions/utils/conditions', () => {
+  describe.each(listConditions().map((condition) => [condition.id, condition]))('%s', (_id, condition) => {
+    it('has a definition and examples', () => {
+      expect(condition.definition).toBeTruthy();
+      expect(condition.examples.match.length).toBeGreaterThan(0);
+      expect(condition.examples.noMatch.length).toBeGreaterThan(0);
+    });
+
+    it('matches its match examples', () => {
+      condition.examples.match.forEach((example) => {
+        expect({ example, pass: evaluate(condition, contextFrom(example)).pass }).toEqual({ example, pass: true });
+      });
+    });
+
+    it('does not match its noMatch examples', () => {
+      condition.examples.noMatch.forEach((example) => {
+        expect({ example, pass: evaluate(condition, contextFrom(example)).pass }).toEqual({ example, pass: false });
+      });
+    });
+
+    it('has examples that can be told apart in the glossary', () => {
+      const shown = [...condition.examples.match, ...condition.examples.noMatch].map(
+        (example) => `${example.path.slice(1).join(' ')} ${example.note ?? ''}`
+      );
+      expect(new Set(shown).size).toBe(shown.length);
+    });
+
+    it('has an id that starts with its subject', () => {
+      expect(condition.id.split('.')[0]).toBe(condition.subject);
+    });
+
+    it('names a known condition as variantOf', () => {
+      if (condition.variantOf) {
+        expect(listConditions().map((c) => c.id)).toContain(condition.variantOf);
+        expect(condition.divergence).toBeTruthy();
+      }
+    });
+  });
+
+  describe('combinators', () => {
+    const yes = defineCondition({ id: 'test.yes', fn: () => true });
+    const no = defineCondition({ id: 'test.no', fn: () => false });
+    const named = defineCondition({ id: 'test.named', of: allOf(yes, no) });
+
+    it('reports the failing leaf of an anonymous allOf', () => {
+      expect(evaluate(allOf(yes, no), {})).toEqual({ pass: false, failedAt: ['test.no'] });
+    });
+
+    it('reports a named composite and the operand that failed', () => {
+      expect(evaluate(named, {})).toEqual({ pass: false, failedAt: ['test.named', 'test.no'] });
+    });
+
+    it('evaluates not and anyOf', () => {
+      expect(evaluate(not(no), {}).pass).toBe(true);
+      expect(evaluate(not(yes), {})).toEqual({ pass: false, failedAt: ['not(test.yes)'] });
+      expect(evaluate(anyOf(no, yes), {}).pass).toBe(true);
+      expect(evaluate(anyOf(no, no), {}).pass).toBe(false);
+    });
+
+    it('short-circuits like && and ||', () => {
+      const throws = defineCondition({
+        id: 'test.throws',
+        fn: () => {
+          throw new Error('evaluated');
+        },
+      });
+      expect(evaluate(allOf(no, throws), {}).pass).toBe(false);
+      expect(evaluate(anyOf(yes, throws), {}).pass).toBe(true);
+    });
+
+    it('rejects duplicate ids', () => {
+      expect(() => defineCondition({ id: 'test.yes', fn: () => true })).toThrow('already defined');
+    });
+  });
+});

tools/spectral/ipa/__tests__/utils/defineRule.test.js

@@ -0,0 +1,110 @@
+import { beforeEach, describe, expect, it } from '@jest/globals';
+import collector, { EntryType } from '../../metrics/collector';
+import { defineRule } from '../../rulesets/functions/utils/defineRule';
+import { defineCondition } from '../../rulesets/functions/utils/conditions/registry';
+
+const holds = defineCondition({ id: 'defineRuleTest.holds', fn: () => true });
+const fails = defineCondition({ id: 'defineRuleTest.fails', fn: () => false });
+
+const spectralContext = {
+  path: ['paths', '/resource', 'get'],
+  documentInventory: { resolved: { paths: { '/resource': { get: {} } } }, unresolved: { paths: {} } },
+  rule: { name: 'test-rule' },
+};
+
+function collected() {
+  return Object.fromEntries(Object.entries(collector.entries).map(([type, list]) => [type, list.length]));
+}
+
+describe('tools/spectral/ipa/rulesets/functions/utils/defineRule.js', () => {
+  beforeEach(() => {
+    collector.entries = { [EntryType.VIOLATION]: [], [EntryType.ADOPTION]: [], [EntryType.EXCEPTION]: [] };
+  });
+
+  it('attaches the conditions as meta', () => {
+    const rule = defineRule({ appliesTo: [holds], check: () => [] });
+    expect(rule.meta).toEqual({
+      appliesTo: [holds],
+      checkUses: [],
+      document: 'resolved',
+      onInternalError: 'violation',
+    });
+  });
+
+  it('does not run the check or collect when a condition fails', () => {
+    let checked = false;
+    const rule = defineRule({
+      appliesTo: [holds, fails],
+      check: () => {
+        checked = true;
+        return [];
+      },
+    });
+    expect(rule({}, null, spectralContext)).toBeUndefined();
+    expect(checked).toBe(false);
+    expect(collected()).toEqual({ violations: 0, adoptions: 0, exceptions: 0 });
+  });
+
+  it('collects an adoption when the check returns no errors', () => {
+    const rule = defineRule({ appliesTo: [holds], check: () => [] });
+    expect(rule({}, null, spectralContext)).toBeUndefined();
+    expect(collected()).toEqual({ violations: 0, adoptions: 1, exceptions: 0 });
+  });
+
+  it('collects an exception from the exception target', () => {
+    const rule = defineRule({
+      check: ({ path }) => [{ path, message: 'bad' }],
+      exceptionTarget: () => ({ 'x-xgen-IPA-exception': { 'test-rule': 'reason' } }),
+    });
+    expect(rule({}, null, spectralContext)).toBeUndefined();
+    expect(collected()).toEqual({ violations: 0, adoptions: 0, exceptions: 1 });
+  });
+
+  it('gives the check the context and options', () => {
+    const rule = defineRule({
+      check: ({ options, resourcePath, path }) => [{ path, message: `${options.a} ${resourcePath}` }],
+    });
+    expect(rule({}, { a: 'x' }, spectralContext)).toEqual([{ path: spectralContext.path, message: 'x /resource' }]);
+  });
+
+  it('runs beforeCheck only when the rule applies, and lets its exceptions propagate', () => {
+    const beforeCheck = () => {
+      throw new Error('before');
+    };
+    expect(() => defineRule({ appliesTo: [holds], beforeCheck, check: () => [] })({}, null, spectralContext)).toThrow(
+      'before'
+    );
+    expect(() =>
+      defineRule({ appliesTo: [fails], beforeCheck, check: () => [] })({}, null, spectralContext)
+    ).not.toThrow();
+  });
+
+  it('names the function after the rule, so a crash outside the check says which rule', () => {
+    const rule = defineRule({ check: () => [] });
+    rule({}, null, spectralContext);
+    expect(rule.name).toBe('test-rule');
+  });
+
+  describe('when the check throws', () => {
+    const throwing = () => {
+      throw new Error('boom');
+    };
+
+    it("reports and collects a violation in 'violation' mode", () => {
+      const rule = defineRule({ check: throwing });
+      expect(rule({}, null, spectralContext)[0].message).toContain('test-rule Internal Rule Error: Error: boom');
+      expect(collected()).toEqual({ violations: 1, adoptions: 0, exceptions: 0 });
+    });
+
+    it("reports without collecting in 'return' mode", () => {
+      const rule = defineRule({ check: throwing, onInternalError: 'return' });
+      expect(rule({}, null, spectralContext)[0].message).toContain('Internal Rule Error');
+      expect(collected()).toEqual({ violations: 0, adoptions: 0, exceptions: 0 });
+    });
+
+    it("propagates in 'throw' mode", () => {
+      const rule = defineRule({ check: throwing, onInternalError: 'throw' });
+      expect(() => rule({}, null, spectralContext)).toThrow('boom');
+    });
+  });
+});

tools/spectral/ipa/metrics/collector.js

@@ -27,7 +27,8 @@ class Collector {
       [EntryType.EXCEPTION]: [],
     };
 
-    this.fileName = 'ipa-collector-results-combined.log';
+    // IPA_COLLECTOR_FILE lets tooling (e.g. scripts/ipa-snapshot.js) direct the log to a private path
+    this.fileName = process.env.IPA_COLLECTOR_FILE || 'ipa-collector-results-combined.log';
 
     process.on('exit', () => this.flushToFile());
     process.on('SIGINT', () => {