diff --git a/tools/spectral/ipa/CONTRIBUTING.md b/tools/spectral/ipa/CONTRIBUTING.md index 3c9c2df596..c15645c5f6 100644 --- a/tools/spectral/ipa/CONTRIBUTING.md +++ b/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 ` 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 diff --git a/tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js b/tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js new file mode 100644 index 0000000000..3c966f347b --- /dev/null +++ b/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); +}); diff --git a/tools/spectral/ipa/metrics/collector.js b/tools/spectral/ipa/metrics/collector.js index 15ec5131d2..e41cb77c71 100644 --- a/tools/spectral/ipa/metrics/collector.js +++ b/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', () => { diff --git a/tools/spectral/ipa/scripts/ipa-snapshot-compare.js b/tools/spectral/ipa/scripts/ipa-snapshot-compare.js new file mode 100644 index 0000000000..1785747dd0 --- /dev/null +++ b/tools/spectral/ipa/scripts/ipa-snapshot-compare.js @@ -0,0 +1,163 @@ +// Compares the results of the IPA ruleset at two points and reports per-rule differences. +// +// Usage: node tools/spectral/ipa/scripts/ipa-snapshot-compare.js --spec --base [--head ] [--verbose] +// node tools/spectral/ipa/scripts/ipa-snapshot-compare.js [--verbose] +// +// With --base, each side is linted with the ruleset of that git ref, checked out into a temporary worktree and removed +// afterwards. Without --head, the head is this checkout as it is now, including uncommitted changes. This script and +// the spec always come from this checkout, so the base ref does not need to contain them. +// +// With two directories, compares snapshots already written by scripts/ipa-snapshot.js. +// +// Prints, for the findings and for each collector entry type, which rules gained (+) or lost (-) entries, and with +// --verbose the entries themselves. Exits 0 when the results are identical and 1 otherwise, so it can be used to +// prove that a refactor is behavior-neutral. + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { parseArgs } from 'node:util'; +import { DEFAULT_RULESET, checkoutWorktree, takeSnapshot } from './ipa-snapshot.js'; + +/** + * Computes the multiset difference between two lists of entries, grouped by rule. An entry that appears three times in + * base and twice in head counts as one removed. + * + * @param {Array} base entries, optionally carrying a 'count' multiplicity + * @param {Array} head entries, optionally carrying a 'count' multiplicity + * @param {(entry: Object) => string} ruleOf returns the rule name of an entry + * @returns {Map, removed: Array}>} differences per rule, only for rules that differ + */ +export function diffEntries(base, head, ruleOf) { + const counts = new Map(); + const tally = (list, sign) => { + for (const entry of list) { + const { count = 1, ...rest } = entry; + const key = JSON.stringify(rest); + const delta = sign * count; + counts.set(key, (counts.get(key) ?? 0) + delta); + } + }; + tally(base, -1); + tally(head, 1); + + const byRule = new Map(); + for (const [key, delta] of counts) { + if (delta === 0) { + continue; + } + const entry = JSON.parse(key); + const rule = ruleOf(entry); + if (!byRule.has(rule)) { + byRule.set(rule, { added: [], removed: [] }); + } + const bucket = delta > 0 ? byRule.get(rule).added : byRule.get(rule).removed; + bucket.push({ ...entry, count: Math.abs(delta) }); + } + return byRule; +} + +/** + * Compares two snapshot directories. + * + * @param {string} baseDir + * @param {string} headDir + * @returns {{findings: Map, collector: Object}} differences for findings and for each collector entry type + */ +export function compareSnapshots(baseDir, headDir) { + const read = (dir, file) => JSON.parse(fs.readFileSync(path.join(dir, file), 'utf-8')); + const findings = diffEntries(read(baseDir, 'findings.json'), read(headDir, 'findings.json'), (f) => f.code); + + const baseCollector = read(baseDir, 'collector.json'); + const headCollector = read(headDir, 'collector.json'); + const collector = {}; + for (const type of new Set([...Object.keys(baseCollector), ...Object.keys(headCollector)])) { + collector[type] = diffEntries(baseCollector[type] ?? [], headCollector[type] ?? [], (e) => e.ruleName); + } + return { findings, collector }; +} + +/** + * Lints a spec with the ruleset at two git refs and compares the results. The temporary worktrees and snapshots are + * removed afterwards. + * + * @param {Object} params + * @param {string} params.spec the OpenAPI file, linted unchanged on both sides + * @param {string} params.base git ref for the base ruleset + * @param {string} [params.head] git ref for the head ruleset; this checkout, as it is now, when omitted + * @returns {{findings: Map, collector: Object}} see compareSnapshots + */ +export function compareRefs({ spec, base, head }) { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'ipa-snapshot-compare-')); + const worktrees = []; + const snapshotOf = (name, ref) => { + let ruleset = DEFAULT_RULESET; + if (ref) { + const worktree = checkoutWorktree(ref); + worktrees.push(worktree); + ruleset = worktree.ruleset; + } + const out = path.join(tmp, name); + takeSnapshot({ spec, out, ruleset }); + return out; + }; + try { + const baseDir = snapshotOf('base', base); + const headDir = snapshotOf('head', head); + return compareSnapshots(baseDir, headDir); + } finally { + worktrees.forEach((worktree) => worktree.remove()); + fs.rmSync(tmp, { recursive: true, force: true }); + } +} + +function sum(list) { + return list.reduce((total, entry) => total + entry.count, 0); +} + +function printSection(title, diff, verbose, describe) { + if (diff.size === 0) { + console.log(`${title}: identical`); + return; + } + console.log(`${title}:`); + for (const rule of [...diff.keys()].sort()) { + const { added, removed } = diff.get(rule); + console.log(` ${rule}: +${sum(added)} -${sum(removed)}`); + if (verbose) { + removed.forEach((e) => console.log(` - ${describe(e)}${e.count > 1 ? ` (x${e.count})` : ''}`)); + added.forEach((e) => console.log(` + ${describe(e)}${e.count > 1 ? ` (x${e.count})` : ''}`)); + } + } +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const { values, positionals } = parseArgs({ + allowPositionals: true, + options: { + verbose: { type: 'boolean', short: 'v', default: false }, + spec: { type: 'string' }, + base: { type: 'string' }, + head: { type: 'string' }, + }, + }); + const byRef = Boolean(values.spec && values.base) && positionals.length === 0; + const byDir = positionals.length === 2 && !values.spec && !values.base && !values.head; + if (!byRef && !byDir) { + console.error( + 'Usage: node ipa-snapshot-compare.js --spec --base [--head ] [--verbose]' + ); + console.error(' node ipa-snapshot-compare.js [--verbose]'); + process.exit(2); + } + + const { findings, collector } = byRef ? compareRefs(values) : compareSnapshots(positionals[0], positionals[1]); + printSection('Findings', findings, values.verbose, (f) => `${f.path}: ${f.message}`); + for (const [type, diff] of Object.entries(collector)) { + printSection(`Collector ${type}`, diff, values.verbose, (e) => e.componentId); + } + + const identical = findings.size === 0 && Object.values(collector).every((diff) => diff.size === 0); + process.exit(identical ? 0 : 1); +} diff --git a/tools/spectral/ipa/scripts/ipa-snapshot.js b/tools/spectral/ipa/scripts/ipa-snapshot.js new file mode 100644 index 0000000000..35670b38bf --- /dev/null +++ b/tools/spectral/ipa/scripts/ipa-snapshot.js @@ -0,0 +1,148 @@ +// Runs the IPA ruleset against a spec and writes a normalized, diffable snapshot of the results. +// +// Use it to show that a change to the ruleset does not change its results: snapshot the same spec before and after, +// then compare the two with scripts/ipa-snapshot-compare.js. +// +// Usage: node tools/spectral/ipa/scripts/ipa-snapshot.js --spec --out [--ruleset ] +// +// Writes to : +// findings.json - what Spectral reports: [{ code, path, severity, message }], sorted. +// code is the rule name and path is the JSON path of the finding, e.g. "paths./groups.get". +// Source file paths are left out so snapshots from different checkouts compare equal. +// collector.json - what the metrics collector records, per entry type (violations, adoptions, exceptions): +// { violations: [{ componentId, ruleName, count }] }. An adoption is a component that passed a +// rule. An exception is a component that failed a rule but carries x-xgen-IPA-exception for it, so +// no finding is reported (the entry also has an exceptionReason). These feed the metrics and do +// not line up one to one with the findings: adoptions and exceptions never appear in them. + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { parseArgs } from 'node:util'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const IPA_DIR_IN_REPO = path.join('tools', 'spectral', 'ipa'); +const REPO_ROOT = path.join(__dirname, '..', '..', '..', '..'); +export const DEFAULT_RULESET = path.join(__dirname, '..', 'ipa-spectral.yaml'); +const SPECTRAL_BIN = path.join(REPO_ROOT, 'node_modules', '.bin', 'spectral'); +// Rulesets that predate IPA_COLLECTOR_FILE (e.g. an older ref checked out for comparison) write this file to the cwd +const DEFAULT_COLLECTOR_FILE_NAME = 'ipa-collector-results-combined.log'; + +// Spectral's raw output has no stable order and carries absolute source file paths, so keep only the fields that +// identify a finding and sort them. Two runs that find the same things then produce identical JSON. +export function normalizeFindings(results) { + return results + .map(({ code, path: jsonPath, severity, message }) => ({ code, path: jsonPath.join('.'), severity, message })) + .sort(compareByJson); +} + +// The collector appends entries in the order rules ran, and a component can be recorded more than once (for example a +// schema reached through several $refs). Group identical entries and count them, in a stable order, so the snapshot +// does not depend on run order but still notices when the number of entries changes. +export function normalizeCollectorEntries(entries) { + const normalized = {}; + for (const [type, list] of Object.entries(entries)) { + const counts = new Map(); + for (const entry of list) { + const key = JSON.stringify(sortKeys(entry)); + counts.set(key, (counts.get(key) ?? 0) + 1); + } + normalized[type] = [...counts.entries()] + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + .map(([key, count]) => ({ ...JSON.parse(key), count })); + } + return normalized; +} + +// So that entries with the same fields in a different order give the same JSON key above +function sortKeys(object) { + return Object.fromEntries(Object.entries(object).sort(([a], [b]) => (a < b ? -1 : 1))); +} + +function compareByJson(a, b) { + const x = JSON.stringify(a); + const y = JSON.stringify(b); + return x < y ? -1 : x > y ? 1 : 0; +} + +/** + * Checks out a git ref into a temporary worktree that shares this checkout's node_modules, so the ruleset of that ref + * can be run without touching the working tree. Nothing needs building. + * + * @param {string} ref any git ref or commit + * @returns {{dir: string, ruleset: string, remove: () => void}} the ruleset file inside the worktree + */ +export function checkoutWorktree(ref) { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'ipa-worktree-')); + execFileSync('git', ['worktree', 'add', '--detach', dir, ref], { cwd: REPO_ROOT, stdio: 'ignore' }); + fs.symlinkSync(path.join(REPO_ROOT, 'node_modules'), path.join(dir, 'node_modules')); + return { + dir, + ruleset: path.join(dir, IPA_DIR_IN_REPO, 'ipa-spectral.yaml'), + remove: () => execFileSync('git', ['worktree', 'remove', '--force', dir], { cwd: REPO_ROOT, stdio: 'ignore' }), + }; +} + +export function takeSnapshot({ spec, out, ruleset = DEFAULT_RULESET }) { + fs.mkdirSync(out, { recursive: true }); + const rawFindingsFile = path.resolve(out, 'spectral-output.json'); + const rawCollectorFile = path.resolve(out, DEFAULT_COLLECTOR_FILE_NAME); + fs.rmSync(rawCollectorFile, { force: true }); + + // Running in the out dir keeps older rulesets from overwriting the collector log in the caller's cwd + const result = spawnSync( + SPECTRAL_BIN, + ['lint', path.resolve(spec), '--ruleset', path.resolve(ruleset), '-f', 'json', '-o', rawFindingsFile], + { + cwd: out, + encoding: 'utf-8', + maxBuffer: 100 * 1024 * 1024, + env: { ...process.env, IPA_COLLECTOR_FILE: rawCollectorFile }, + } + ); + + if (result.error) { + throw result.error; + } + // Spectral exits 1 when it reports error-severity findings; anything else means the lint itself failed + if (result.status !== 0 && result.status !== 1) { + throw new Error(`Spectral exited with status ${result.status}:\n${result.stderr}`); + } + if (!fs.existsSync(rawFindingsFile)) { + throw new Error(`Spectral did not produce ${rawFindingsFile}:\n${result.stderr}`); + } + + const findings = normalizeFindings(JSON.parse(fs.readFileSync(rawFindingsFile, 'utf-8'))); + const collectorEntries = fs.existsSync(rawCollectorFile) + ? normalizeCollectorEntries(JSON.parse(fs.readFileSync(rawCollectorFile, 'utf-8'))) + : {}; + + fs.writeFileSync(path.join(out, 'findings.json'), JSON.stringify(findings, null, 2)); + fs.writeFileSync(path.join(out, 'collector.json'), JSON.stringify(collectorEntries, null, 2)); + fs.rmSync(rawFindingsFile, { force: true }); + fs.rmSync(rawCollectorFile, { force: true }); + + return { findings, collectorEntries }; +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const { values } = parseArgs({ + options: { + spec: { type: 'string' }, + out: { type: 'string' }, + ruleset: { type: 'string', default: DEFAULT_RULESET }, + }, + }); + if (!values.spec || !values.out) { + console.error('Usage: node ipa-snapshot.js --spec --out [--ruleset ]'); + process.exit(2); + } + + const { findings, collectorEntries } = takeSnapshot(values); + const counts = Object.entries(collectorEntries) + .map(([type, list]) => `${type}: ${list.reduce((sum, e) => sum + e.count, 0)}`) + .join(', '); + console.log(`Wrote snapshot to ${values.out} (findings: ${findings.length}; ${counts})`); +}