From 4c9a13ec57f03994d43a9b34683d203556210cd8 Mon Sep 17 00:00:00 2001 From: Mitrofanos Ntatidis Date: Thu, 24 Sep 2026 12:07:26 +0100 Subject: [PATCH 1/4] feat(ipa): add snapshot and compare scripts for behavior-neutral refactors - collector: honor IPA_COLLECTOR_FILE for the output path - scripts/ipa-snapshot.js: lint a spec and write sorted findings.json plus collector.json multiset counts - scripts/ipa-snapshot-compare.js: per-rule added/removed between two snapshots, exit 1 on any difference --- .../ipa/__tests__/scripts/ipaSnapshot.test.js | 53 ++++++++ tools/spectral/ipa/metrics/collector.js | 3 +- .../ipa/scripts/ipa-snapshot-compare.js | 110 +++++++++++++++ tools/spectral/ipa/scripts/ipa-snapshot.js | 127 ++++++++++++++++++ 4 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js create mode 100644 tools/spectral/ipa/scripts/ipa-snapshot-compare.js create mode 100644 tools/spectral/ipa/scripts/ipa-snapshot.js 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..58a5e4ebe0 --- /dev/null +++ b/tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js @@ -0,0 +1,53 @@ +import { describe, expect, it } from '@jest/globals'; +import { normalizeCollectorEntries, normalizeFindings } from '../../scripts/ipa-snapshot'; +import { 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: [] }); + }); +}); 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..e741015283 --- /dev/null +++ b/tools/spectral/ipa/scripts/ipa-snapshot-compare.js @@ -0,0 +1,110 @@ +// Compares two snapshots written by scripts/ipa-snapshot.js and reports per-rule differences. +// +// Usage: node tools/spectral/ipa/scripts/ipa-snapshot-compare.js [--verbose] +// +// 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 snapshots are identical and 1 otherwise, so it can be used to +// prove that a refactor is behavior-neutral. + +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { parseArgs } from 'node:util'; + +/** + * 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 }; +} + +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 } }, + }); + if (positionals.length !== 2) { + console.error('Usage: node ipa-snapshot-compare.js [--verbose]'); + process.exit(2); + } + + const { findings, collector } = 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..ea6cce93c1 --- /dev/null +++ b/tools/spectral/ipa/scripts/ipa-snapshot.js @@ -0,0 +1,127 @@ +// 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 path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; +import { parseArgs } from 'node:util'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const DEFAULT_RULESET = path.join(__dirname, '..', 'ipa-spectral.yaml'); +const SPECTRAL_BIN = path.join(__dirname, '..', '..', '..', '..', '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; +} + +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})`); +} From ba120dfd8cebbd347f261c982f82ad598588dc6b Mon Sep 17 00:00:00 2001 From: Mitrofanos Ntatidis Date: Wed, 7 Oct 2026 17:06:37 +0100 Subject: [PATCH 2/4] feat(ipa): compare snapshots by git ref and document the usage ipa-snapshot-compare.js takes --spec and --base (and optionally --head) and does what had to be done by hand: it checks each ref out into a temporary worktree, lints the spec with that ref's ruleset, compares, and removes the worktree and snapshots. The scripts and the spec come from this checkout, so the base ref does not need to contain them. The two-directory form is unchanged. Adds the usage to the IPA CONTRIBUTING guide. --- tools/spectral/ipa/CONTRIBUTING.md | 12 ++++ .../ipa/__tests__/scripts/ipaSnapshot.test.js | 40 ++++++++++- .../ipa/scripts/ipa-snapshot-compare.js | 67 +++++++++++++++++-- tools/spectral/ipa/scripts/ipa-snapshot.js | 27 +++++++- 4 files changed, 134 insertions(+), 12 deletions(-) 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 index 58a5e4ebe0..3c966f347b 100644 --- a/tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js +++ b/tools/spectral/ipa/__tests__/scripts/ipaSnapshot.test.js @@ -1,6 +1,9 @@ -import { describe, expect, it } from '@jest/globals'; +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 { diffEntries } from '../../scripts/ipa-snapshot-compare'; +import { compareRefs, diffEntries } from '../../scripts/ipa-snapshot-compare'; describe('tools/spectral/ipa/scripts/ipa-snapshot.js', () => { it('sorts findings and drops the source file', () => { @@ -51,3 +54,36 @@ describe('tools/spectral/ipa/scripts/ipa-snapshot-compare.js', () => { 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/scripts/ipa-snapshot-compare.js b/tools/spectral/ipa/scripts/ipa-snapshot-compare.js index e741015283..1785747dd0 100644 --- a/tools/spectral/ipa/scripts/ipa-snapshot-compare.js +++ b/tools/spectral/ipa/scripts/ipa-snapshot-compare.js @@ -1,15 +1,24 @@ -// Compares two snapshots written by scripts/ipa-snapshot.js and reports per-rule differences. +// 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 [--verbose] +// 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 snapshots are identical and 1 otherwise, so it can be used to +// --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 @@ -69,6 +78,40 @@ export function compareSnapshots(baseDir, headDir) { 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); } @@ -92,14 +135,24 @@ function printSection(title, diff, verbose, describe) { if (process.argv[1] === fileURLToPath(import.meta.url)) { const { values, positionals } = parseArgs({ allowPositionals: true, - options: { verbose: { type: 'boolean', short: 'v', default: false } }, + options: { + verbose: { type: 'boolean', short: 'v', default: false }, + spec: { type: 'string' }, + base: { type: 'string' }, + head: { type: 'string' }, + }, }); - if (positionals.length !== 2) { - console.error('Usage: node ipa-snapshot-compare.js [--verbose]'); + 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 } = compareSnapshots(positionals[0], positionals[1]); + 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); diff --git a/tools/spectral/ipa/scripts/ipa-snapshot.js b/tools/spectral/ipa/scripts/ipa-snapshot.js index ea6cce93c1..35670b38bf 100644 --- a/tools/spectral/ipa/scripts/ipa-snapshot.js +++ b/tools/spectral/ipa/scripts/ipa-snapshot.js @@ -16,14 +16,17 @@ // 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 { spawnSync } from 'node:child_process'; +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 DEFAULT_RULESET = path.join(__dirname, '..', 'ipa-spectral.yaml'); -const SPECTRAL_BIN = path.join(__dirname, '..', '..', '..', '..', 'node_modules', '.bin', 'spectral'); +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'; @@ -64,6 +67,24 @@ function compareByJson(a, 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'); From 4770a55ca8325bf4bcead5d03a16c49a3a5baf30 Mon Sep 17 00:00:00 2001 From: Mitrofanos Ntatidis Date: Thu, 24 Sep 2026 12:51:11 +0100 Subject: [PATCH 3/4] refactor(ipa): condition registry and defineRule, migrate IPA-104 and IPA-113 Rules now declare the conditions under which they apply (appliesTo) instead of early returns. Conditions are named, defined once with a plain-English definition and examples, and the divergent variants (path.isLegacyGetTarget, resource.isReadOnly's exception clause) carry a divergence note. - utils/conditions/: registry (defineCondition, allOf/anyOf/not, evaluate) and path, resource, operation, response conditions wrapping the existing helpers - utils/ruleContext.js: per-call context with lazy, cached resourcePathItems/node/operation. Values a condition and the check both need (e.g. the base Get response of a custom method) are exposed as cached accessors next to the condition, so both read one definition - utils/operationIdRules.js: validOperationIdRule / operationIdLengthRule factories (the conditions they take live in utils/conditions/operationIdRules.js); the IPA-104 operation ID rules are built from them with the conditions that decide which operations are a Get - utils/defineRule.js: builds the Spectral function; onInternalError preserves each rule's existing try/catch behavior - resourceEvaluation.js: isReadOnlyResource split into anyPathHasException and getResponseIsAllReadOnly (same result) No behavior change: ipa-snapshot-compare is identical on openapi/.raw/v2.yaml and openapi/v2.json. --- .../ipa/__tests__/utils/conditions.test.js | 87 ++++++++ .../ipa/__tests__/utils/defineRule.test.js | 110 ++++++++++ .../IPA104EachResourceHasGetMethod.js | 62 ++---- .../IPA104GetMethodHasNoRequestBody.js | 40 +--- ...IPA104GetMethodResponseHasNoInputFields.js | 48 +---- ...4GetMethodReturnsResponseSuffixedObject.js | 43 ++-- .../IPA104GetMethodReturnsSingleResource.js | 57 ++--- .../IPA104GetResponseCodeShouldBe200OK.js | 32 +-- .../functions/IPA104OperationIdLength.js | 36 +--- .../functions/IPA104ValidOperationID.js | 34 +-- ...IPA113ResetMethodMustNotHaveRequestBody.js | 31 +-- .../IPA113ResetMethodMustReturn200OK.js | 61 ++---- .../functions/IPA113ResetMethodMustUsePost.js | 44 +--- ...IPA113ResetMethodNotOnReadonlySingleton.js | 48 ++--- ...A113ResetMethodOnlyOnSingletonResources.js | 50 ++--- ...3ResetMethodResponseIsGetMethodResponse.js | 67 ++---- .../IPA113ResetMethodValidOperationID.js | 35 ++- .../IPA113SingletonHasNoDeleteMethod.js | 39 +--- .../functions/IPA113SingletonHasNoId.js | 47 +---- .../IPA113SingletonHasUpdateMethod.js | 46 +--- .../functions/utils/conditions/operation.js | 67 ++++++ .../utils/conditions/operationIdRules.js | 25 +++ .../functions/utils/conditions/path.js | 68 ++++++ .../functions/utils/conditions/registry.js | 124 +++++++++++ .../functions/utils/conditions/resource.js | 199 ++++++++++++++++++ .../functions/utils/conditions/response.js | 43 ++++ .../rulesets/functions/utils/defineRule.js | 95 +++++++++ .../functions/utils/operationIdRules.js | 58 +++++ .../functions/utils/resourceEvaluation.js | 34 ++- .../rulesets/functions/utils/ruleContext.js | 128 +++++++++++ 30 files changed, 1230 insertions(+), 628 deletions(-) create mode 100644 tools/spectral/ipa/__tests__/utils/conditions.test.js create mode 100644 tools/spectral/ipa/__tests__/utils/defineRule.test.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/operationIdRules.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/path.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/conditions/response.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/defineRule.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/operationIdRules.js create mode 100644 tools/spectral/ipa/rulesets/functions/utils/ruleContext.js diff --git a/tools/spectral/ipa/__tests__/utils/conditions.test.js b/tools/spectral/ipa/__tests__/utils/conditions.test.js new file mode 100644 index 0000000000..91dfd8072c --- /dev/null +++ b/tools/spectral/ipa/__tests__/utils/conditions.test.js @@ -0,0 +1,87 @@ +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 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'); + }); + }); +}); diff --git a/tools/spectral/ipa/__tests__/utils/defineRule.test.js b/tools/spectral/ipa/__tests__/utils/defineRule.test.js new file mode 100644 index 0000000000..848a4d4e18 --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104EachResourceHasGetMethod.js b/tools/spectral/ipa/rulesets/functions/IPA104EachResourceHasGetMethod.js index 51f5673488..4a0fa67bb9 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104EachResourceHasGetMethod.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104EachResourceHasGetMethod.js @@ -1,47 +1,23 @@ -import { - getResourcePathItems, - hasGetMethod, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isCollection } from './utils/conditions/path.js'; +import { hasSingletonShape, instancePath } from './utils/conditions/resource.js'; +import { hasGetMethod } from './utils/resourceEvaluation.js'; const ERROR_MESSAGE = 'APIs must provide a get method for resources.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - if (!isResourceCollectionIdentifier(input)) { - return; - } - - const oas = documentInventory.resolved; - - const errors = checkViolationsAndReturnErrors(oas.paths, input, path, ruleName); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, oas.paths[input], path); -}; - -function checkViolationsAndReturnErrors(oasPaths, input, path, ruleName) { - try { - const resourcePathItems = getResourcePathItems(input, oasPaths); - const resourcePaths = Object.keys(resourcePathItems); - - if (isSingletonResource(resourcePathItems)) { - if (!hasGetMethod(resourcePathItems[resourcePaths[0]])) { - return [{ path, message: ERROR_MESSAGE }]; - } - } else { - if (resourcePaths.length === 1) { - return [{ path, message: ERROR_MESSAGE }]; - } - const singleResourcePath = resourcePaths.find((p) => isSingleResourceIdentifier(p)); - if (!singleResourcePath || !hasGetMethod(resourcePathItems[singleResourcePath])) { - return [{ path, message: ERROR_MESSAGE }]; - } +export default defineRule({ + appliesTo: [isCollection], + checkUses: [hasSingletonShape], + exceptionTarget: (ctx) => ctx.pathItem, + check: (ctx) => { + const error = [{ path: ctx.path, message: ERROR_MESSAGE }]; + // A singleton has its Get on the first of its paths (in spec order), other resources on the instance path + if (ctx.is(hasSingletonShape)) { + const [firstPath] = Object.keys(ctx.resourcePathItems); + return hasGetMethod(ctx.resourcePathItems[firstPath]) ? [] : error; } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} + const instance = instancePath(ctx); + return instance && hasGetMethod(ctx.oas.paths[instance]) ? [] : error; + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodHasNoRequestBody.js b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodHasNoRequestBody.js index b35635e0df..8f0a62032a 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodHasNoRequestBody.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodHasNoRequestBody.js @@ -1,35 +1,11 @@ -import { evaluateAndCollectAdoptionStatus } from './utils/collectionUtils.js'; -import { - getResourcePathItems, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isGetTarget } from './utils/conditions/resource.js'; const ERROR_MESSAGE = 'The Get method must not include a request body.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const oas = documentInventory.resolved; - - if ( - !isSingleResourceIdentifier(resourcePath) && - !( - isResourceCollectionIdentifier(resourcePath) && isSingletonResource(getResourcePathItems(resourcePath, oas.paths)) - ) - ) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(getOperationObject, path) { - if (getOperationObject.requestBody) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; -} +export default defineRule({ + appliesTo: [isGetTarget], + onInternalError: 'throw', + check: ({ input, path }) => (input.requestBody ? [{ path, message: ERROR_MESSAGE }] : []), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodResponseHasNoInputFields.js b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodResponseHasNoInputFields.js index 02f85faf3b..d55149f341 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodResponseHasNoInputFields.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodResponseHasNoInputFields.js @@ -1,41 +1,15 @@ -import { evaluateAndCollectAdoptionStatus } from './utils/collectionUtils.js'; -import { - getResourcePathItems, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { resolveObject } from './utils/componentUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isGetTarget } from './utils/conditions/resource.js'; +import { hasSchema, is2xx, isJson } from './utils/conditions/response.js'; import { checkForbiddenPropertyAttributesAndReturnErrors } from './utils/validations/checkForbiddenPropertyAttributesAndReturnErrors.js'; const ERROR_MESSAGE = 'The get method response object must not include output fields (writeOnly properties).'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const responseCode = path[4]; - const oas = documentInventory.resolved; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - const contentPerMediaType = resolveObject(oas, path); - - if ( - !responseCode.startsWith('2') || - !contentPerMediaType || - !contentPerMediaType.schema || - !input.endsWith('json') || - (!isSingleResourceIdentifier(resourcePath) && - !(isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePaths))) - ) { - return; - } - - const errors = checkForbiddenPropertyAttributesAndReturnErrors( - contentPerMediaType.schema, - 'writeOnly', - path, - [], - ERROR_MESSAGE - ); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, contentPerMediaType, path); -}; +export default defineRule({ + appliesTo: [is2xx, hasSchema, isJson, isGetTarget], + onInternalError: 'throw', + exceptionTarget: (ctx) => ctx.node, + check: ({ node, path }) => + checkForbiddenPropertyAttributesAndReturnErrors(node.schema, 'writeOnly', path, [], ERROR_MESSAGE), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsResponseSuffixedObject.js b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsResponseSuffixedObject.js index 8303011dfd..d48f2a71a8 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsResponseSuffixedObject.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsResponseSuffixedObject.js @@ -1,33 +1,14 @@ -import { - getResourcePathItems, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { resolveObject } from './utils/componentUtils.js'; -import { evaluateAndCollectAdoptionStatus } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isGetTarget } from './utils/conditions/resource.js'; +import { hasSchema, is2xx, isJson } from './utils/conditions/response.js'; import { checkSchemaRefSuffixAndReturnErrors } from './utils/validations/checkSchemaRefSuffixAndReturnErrors.js'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const responseCode = path[4]; - const oas = documentInventory.unresolved; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - const contentPerMediaType = resolveObject(oas, path); - - if ( - !responseCode.startsWith('2') || - !contentPerMediaType || - !contentPerMediaType.schema || - !input.endsWith('json') || - (!isSingleResourceIdentifier(resourcePath) && - !(isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePaths))) - ) { - return; - } - - const errors = checkSchemaRefSuffixAndReturnErrors(path, contentPerMediaType, 'Response', ruleName); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, contentPerMediaType, path); -}; +export default defineRule({ + // Reads the unresolved document so the schema $ref name can be checked + document: 'unresolved', + appliesTo: [is2xx, hasSchema, isJson, isGetTarget], + onInternalError: 'throw', + exceptionTarget: (ctx) => ctx.node, + check: ({ node, path, ruleName }) => checkSchemaRefSuffixAndReturnErrors(path, node, 'Response', ruleName), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsSingleResource.js b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsSingleResource.js index ef092decb3..43a2f30eb8 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsSingleResource.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104GetMethodReturnsSingleResource.js @@ -1,57 +1,28 @@ -import { - getResourcePathItems, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isGetTarget, isSingleton } from './utils/conditions/resource.js'; +import { hasSchema, is2xx, isJson } from './utils/conditions/response.js'; import { schemaIsArray, schemaIsPaginated } from './utils/schemaUtils.js'; -import { resolveObject } from './utils/componentUtils.js'; const ERROR_MESSAGE_STANDARD_RESOURCE = 'Get methods should return data for a single resource. This method returns an array or a paginated response.'; const ERROR_MESSAGE_SINGLETON_RESOURCE = 'Get methods for singleton resource should return data for a single resource. This method returns an array or a paginated response. If this is not a singleton resource, please implement all standard methods.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - const responseCode = path[4]; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - const contentPerMediaType = resolveObject(oas, path); - - const isSingleResource = isSingleResourceIdentifier(resourcePath); - const isSingleton = isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePaths); - - if ( - !responseCode.startsWith('2') || - !contentPerMediaType || - !contentPerMediaType.schema || - !input.endsWith('json') || - (!isSingleResource && !isSingleton) - ) { - return; - } - - const errors = checkViolationsAndReturnErrors(contentPerMediaType, path, isSingleton, ruleName); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, contentPerMediaType, path); -}; - -function checkViolationsAndReturnErrors(contentPerMediaType, path, isSingleton, ruleName) { - try { - const schema = contentPerMediaType.schema; +export default defineRule({ + appliesTo: [is2xx, hasSchema, isJson, isGetTarget], + checkUses: [isSingleton], + exceptionTarget: (ctx) => ctx.node, + check: (ctx) => { + const schema = ctx.node.schema; if (schemaIsPaginated(schema) || schemaIsArray(schema)) { return [ { - path, - message: isSingleton ? ERROR_MESSAGE_SINGLETON_RESOURCE : ERROR_MESSAGE_STANDARD_RESOURCE, + path: ctx.path, + message: ctx.is(isSingleton) ? ERROR_MESSAGE_SINGLETON_RESOURCE : ERROR_MESSAGE_STANDARD_RESOURCE, }, ]; } return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104GetResponseCodeShouldBe200OK.js b/tools/spectral/ipa/rulesets/functions/IPA104GetResponseCodeShouldBe200OK.js index a4a94231bd..4d526218a9 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104GetResponseCodeShouldBe200OK.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104GetResponseCodeShouldBe200OK.js @@ -1,29 +1,13 @@ -import { evaluateAndCollectAdoptionStatus } from './utils/collectionUtils.js'; -import { - getResourcePathItems, - isResourceCollectionIdentifier, - isSingleResourceIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isGetTarget } from './utils/conditions/resource.js'; import { checkResponseCodeAndReturnErrors } from './utils/validations/checkResponseCodeAndReturnErrors.js'; const ERROR_MESSAGE = 'The Get method must return a 200 OK response. This method either lacks a 200 OK response or defines a different 2xx status code.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const oas = documentInventory.resolved; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - - if ( - !isSingleResourceIdentifier(resourcePath) && - !(isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePaths)) - ) { - return; - } - - const errors = checkResponseCodeAndReturnErrors(input, '200', path, ruleName, ERROR_MESSAGE); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; +export default defineRule({ + appliesTo: [isGetTarget], + onInternalError: 'throw', + check: ({ input, path, ruleName }) => checkResponseCodeAndReturnErrors(input, '200', path, ruleName, ERROR_MESSAGE), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104OperationIdLength.js b/tools/spectral/ipa/rulesets/functions/IPA104OperationIdLength.js index 3c380691df..bf36b04052 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104OperationIdLength.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104OperationIdLength.js @@ -1,33 +1,5 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { getResourcePathItems } from './utils/resourceEvaluation.js'; -import { hasCustomMethodOverride, hasMethodVerbOverride, VERB_OVERRIDE_EXTENSION } from './utils/extensions.js'; -import { isInvalidGetMethod } from './utils/methodLogic.js'; -import { validateOperationIdLengthAndReturnErrors } from './utils/validations/validateOperationIdAndReturnErrors.js'; -import { generateOperationID } from './utils/operationIdGeneration.js'; +// @ts-check +import { getOperationIdRuleConditions } from './utils/conditions/operationIdRules.js'; +import { operationIdLengthRule } from './utils/operationIdRules.js'; -export default (input, { methodName, ignoreSingularizationList, maxLength }, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const oas = documentInventory.resolved; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - - if ( - hasCustomMethodOverride(input) || - hasMethodVerbOverride(input, 'list') || - (isInvalidGetMethod(resourcePath, resourcePaths) && !hasMethodVerbOverride(input, methodName)) - ) { - return; - } - - try { - if (hasMethodVerbOverride(input, methodName)) { - methodName = input[VERB_OVERRIDE_EXTENSION].verb; - } - const expectedOperationId = generateOperationID(methodName, resourcePath, ignoreSingularizationList); - const errors = validateOperationIdLengthAndReturnErrors(input, path, expectedOperationId, maxLength); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); - } catch (e) { - return handleInternalError(ruleName, path, e); - } -}; +export default operationIdLengthRule(getOperationIdRuleConditions()); diff --git a/tools/spectral/ipa/rulesets/functions/IPA104ValidOperationID.js b/tools/spectral/ipa/rulesets/functions/IPA104ValidOperationID.js index 08dda68753..c23d067afe 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA104ValidOperationID.js +++ b/tools/spectral/ipa/rulesets/functions/IPA104ValidOperationID.js @@ -1,31 +1,5 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { getResourcePathItems } from './utils/resourceEvaluation.js'; -import { hasCustomMethodOverride, hasMethodVerbOverride, VERB_OVERRIDE_EXTENSION } from './utils/extensions.js'; -import { isInvalidGetMethod } from './utils/methodLogic.js'; -import { validateOperationIdAndReturnErrors } from './utils/validations/validateOperationIdAndReturnErrors.js'; +// @ts-check +import { getOperationIdRuleConditions } from './utils/conditions/operationIdRules.js'; +import { validOperationIdRule } from './utils/operationIdRules.js'; -export default (input, { methodName, ignoreSingularizationList }, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - const oas = documentInventory.resolved; - const resourcePaths = getResourcePathItems(resourcePath, oas.paths); - - if ( - hasCustomMethodOverride(input) || - hasMethodVerbOverride(input, 'list') || - (isInvalidGetMethod(resourcePath, resourcePaths) && !hasMethodVerbOverride(input, methodName)) - ) { - return; - } - - try { - if (hasMethodVerbOverride(input, methodName)) { - methodName = input[VERB_OVERRIDE_EXTENSION].verb; - } - const errors = validateOperationIdAndReturnErrors(methodName, resourcePath, input, path, ignoreSingularizationList); - - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); - } catch (e) { - return handleInternalError(ruleName, path, e); - } -}; +export default validOperationIdRule(getOperationIdRuleConditions()); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustNotHaveRequestBody.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustNotHaveRequestBody.js index 57e03c66a2..ad3632d321 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustNotHaveRequestBody.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustNotHaveRequestBody.js @@ -1,27 +1,10 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { isResetMethod } from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; const ERROR_MESSAGE = 'The :reset custom method must not have a request body.'; -export default (input, _, { path, rule }) => { - const ruleName = rule.name; - const pathString = path[1]; // e.g., ['paths', '/resource/{id}/singleton:reset', 'post'] - - if (!isResetMethod(pathString)) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { - if (input.requestBody) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} +export default defineRule({ + appliesTo: [isReset], + check: ({ input, path }) => (input.requestBody ? [{ path, message: ERROR_MESSAGE }] : []), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustReturn200OK.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustReturn200OK.js index 4cac9b7c03..49b91ea1cd 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustReturn200OK.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustReturn200OK.js @@ -1,61 +1,30 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { isResetMethod } from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; import { checkResponseCodeAndReturnErrors } from './utils/validations/checkResponseCodeAndReturnErrors.js'; const ERROR_MESSAGE = 'The :reset custom method must return a 200 OK response with the reset resource in the response body. This method either lacks a 200 OK response or defines a different 2xx status code.'; +const MISSING_BODY_ERROR_MESSAGE = + 'The :reset custom method must return a 200 OK response with the reset resource in the response body.'; -export default (input, _, { path, rule }) => { - const ruleName = rule.name; - const pathString = path[1]; - - if (!isResetMethod(pathString)) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { +export default defineRule({ + appliesTo: [isReset], + check: ({ input, path, ruleName }) => { // First check if 200 OK response exists const responseCodeErrors = checkResponseCodeAndReturnErrors(input, '200', path, ruleName, ERROR_MESSAGE); if (responseCodeErrors.length > 0) { return responseCodeErrors; } - // Check if the 200 response has a response body with schema + // Check that the 200 response has a body, and at least one content type has a schema const response200 = input.responses['200']; - if (!response200 || !response200.content) { - return [ - { - path, - message: - 'The :reset custom method must return a 200 OK response with the reset resource in the response body.', - }, - ]; - } - - // Verify that at least one content type has a schema - const contentTypes = Object.keys(response200.content); - const hasSchema = contentTypes.some((contentType) => { - const mediaTypeObj = response200.content[contentType]; - return mediaTypeObj && mediaTypeObj.schema; - }); - + const hasSchema = + response200?.content && + Object.values(response200.content).some((mediaTypeObject) => mediaTypeObject && mediaTypeObject.schema); if (!hasSchema) { - return [ - { - path, - message: - 'The :reset custom method must return a 200 OK response with the reset resource in the response body.', - }, - ]; + return [{ path, message: MISSING_BODY_ERROR_MESSAGE }]; } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustUsePost.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustUsePost.js index 1169a9425d..26f54dd88c 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustUsePost.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodMustUsePost.js @@ -1,41 +1,19 @@ -import { isResetMethod } from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; const ERROR_MESSAGE = 'The :reset custom method must use the POST HTTP method.'; const VALID_METHOD = 'post'; const HTTP_METHODS = ['get', 'put', 'post', 'delete', 'options', 'head', 'patch', 'trace']; -export default (input, _, { path, rule }) => { - const ruleName = rule.name; - // Extract the path key (e.g., '/resource/{id}/singleton:reset') from the JSONPath. - let pathKey = path[1]; - - if (!isResetMethod(pathKey)) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { - // Extract the keys which are equivalent of the http methods - let keys = Object.keys(input); - const httpMethods = keys.filter((key) => HTTP_METHODS.includes(key)); - - // Check if POST is not used or if other methods are used - if (!httpMethods.includes(VALID_METHOD)) { +export default defineRule({ + appliesTo: [isReset], + check: ({ input, path }) => { + // POST must be the only HTTP method defined on the path + const httpMethods = Object.keys(input).filter((key) => HTTP_METHODS.includes(key)); + if (!httpMethods.includes(VALID_METHOD) || httpMethods.length > 1) { return [{ path, message: ERROR_MESSAGE }]; } - - // Check for multiple methods (only POST should be defined) - if (httpMethods.length > 1) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodNotOnReadonlySingleton.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodNotOnReadonlySingleton.js index 7b1a9b00af..652dcf14a7 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodNotOnReadonlySingleton.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodNotOnReadonlySingleton.js @@ -1,38 +1,18 @@ -import { - getResourcePathItems, - isReadOnlyResource, - isResetMethod, - stripCustomMethodName, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; +import { isReadOnly } from './utils/conditions/resource.js'; +import { stripCustomMethodName } from './utils/resourceEvaluation.js'; const ERROR_MESSAGE = 'Read-only singleton resources must not define a :reset custom method. Read-only resources cannot be modified, so reset is not applicable.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - - if (!isResetMethod(resourcePath)) { - return; - } - - const errors = checkViolationsAndReturnErrors(resourcePath, oas, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(resourcePath, oas, path, ruleName) { - try { - const baseResourcePath = stripCustomMethodName(resourcePath); - const resourcePathItems = getResourcePathItems(baseResourcePath, oas.paths); - - if (isReadOnlyResource(resourcePathItems)) { - return [{ path, message: ERROR_MESSAGE }]; - } - - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} +// Despite the rule name, the base resource is not required to be a singleton: any read-only resource with :reset violates +export default defineRule({ + appliesTo: [isReset], + checkUses: [isReadOnly], + check: (ctx) => { + const baseResource = ctx.atPath(stripCustomMethodName(ctx.resourcePath)); + return baseResource.is(isReadOnly) ? [{ path: ctx.path, message: ERROR_MESSAGE }] : []; + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodOnlyOnSingletonResources.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodOnlyOnSingletonResources.js index dce0a08dd7..814fb5aa04 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodOnlyOnSingletonResources.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodOnlyOnSingletonResources.js @@ -1,40 +1,16 @@ -import { - getResourcePathItems, - isResetMethod, - isResourceCollectionIdentifier, - isSingletonResource, - stripCustomMethodName, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; +import { isSingleton } from './utils/conditions/resource.js'; +import { stripCustomMethodName } from './utils/resourceEvaluation.js'; const ERROR_MESSAGE = 'The :reset custom method must only be defined on singleton resources.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - - if (!isResetMethod(resourcePath)) { - return; - } - - const errors = checkViolationsAndReturnErrors(resourcePath, oas, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(resourcePath, oas, path, ruleName) { - try { - // Get the base resource path (without :reset) - const baseResourcePath = stripCustomMethodName(resourcePath); - const resourcePathItems = getResourcePathItems(baseResourcePath, oas.paths); - - // Check if the base resource is a singleton - if (!(isResourceCollectionIdentifier(baseResourcePath) && isSingletonResource(resourcePathItems))) { - return [{ path, message: ERROR_MESSAGE }]; - } - - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} +export default defineRule({ + appliesTo: [isReset], + checkUses: [isSingleton], + check: (ctx) => { + const baseResource = ctx.atPath(stripCustomMethodName(ctx.resourcePath)); + return baseResource.is(isSingleton) ? [] : [{ path: ctx.path, message: ERROR_MESSAGE }]; + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodResponseIsGetMethodResponse.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodResponseIsGetMethodResponse.js index f2243a5526..c0f85b2dbf 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodResponseIsGetMethodResponse.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodResponseIsGetMethodResponse.js @@ -1,54 +1,21 @@ -import { getResourcePathItems, isResetMethod, stripCustomMethodName } from './utils/resourceEvaluation.js'; -import { resolveObject } from './utils/componentUtils.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { getGETMethodResponseSchemaFromPathItem, getSchemaRef } from './utils/methodUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; +import { baseGetHasMediaType, baseGetResponse } from './utils/conditions/resource.js'; +import { hasSchema, isJson } from './utils/conditions/response.js'; +import { getSchemaRef } from './utils/methodUtils.js'; const ERROR_MESSAGE = 'The schema in the :reset method response must be the same schema as the response of the Get method.'; -export default (input, _, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.unresolved; - const resourcePath = path[1]; - const mediaType = input; +export default defineRule({ + document: 'unresolved', + appliesTo: [isJson, isReset, hasSchema, baseGetHasMediaType], + exceptionTarget: (ctx) => ctx.node, + check: (ctx) => { + const path = ctx.path; + const getMethodResponseContent = baseGetResponse(ctx); - if (!mediaType.endsWith('json') || !isResetMethod(resourcePath)) { - return; - } - - // Ignore if the Reset method does not have a response schema - const resetMethodResponse = resolveObject(oas, path); - - if (!resetMethodResponse || !resetMethodResponse.schema) { - return; - } - - // Get the base resource path (without :reset) - const baseResourcePath = stripCustomMethodName(resourcePath); - const resourcePathItems = getResourcePathItems(baseResourcePath, oas.paths); - - // Ignore if there is no matching Get method on the base resource - const basePathItem = resourcePathItems[baseResourcePath]; - if (!basePathItem) { - return; - } - - const getMethodResponseContentPerMediaType = getGETMethodResponseSchemaFromPathItem(basePathItem, mediaType); - if (!getMethodResponseContentPerMediaType) { - return; - } - - const errors = checkViolationsAndReturnErrors( - path, - resetMethodResponse, - getMethodResponseContentPerMediaType, - ruleName - ); - return evaluateAndCollectAdoptionStatus(errors, ruleName, resetMethodResponse, path); -}; - -function checkViolationsAndReturnErrors(path, resetMethodResponseContent, getMethodResponseContent, ruleName) { - try { // Error if the Get method does not have a schema if (!getMethodResponseContent.schema) { return [ @@ -59,7 +26,7 @@ function checkViolationsAndReturnErrors(path, resetMethodResponseContent, getMet ]; } - const resetMethodSchemaRef = getSchemaRef(resetMethodResponseContent.schema); + const resetMethodSchemaRef = getSchemaRef(ctx.node.schema); const getMethodSchemaRef = getSchemaRef(getMethodResponseContent.schema); // Error if the Get method does not have a schema ref @@ -77,7 +44,5 @@ function checkViolationsAndReturnErrors(path, resetMethodResponseContent, getMet return [{ path, message: ERROR_MESSAGE }]; } return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodValidOperationID.js b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodValidOperationID.js index 26c3e2e39e..567635eacd 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodValidOperationID.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113ResetMethodValidOperationID.js @@ -1,21 +1,18 @@ -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; -import { isResetMethod, stripCustomMethodName } from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isReset } from './utils/conditions/path.js'; +import { stripCustomMethodName } from './utils/resourceEvaluation.js'; import { validateOperationIdAndReturnErrors } from './utils/validations/validateOperationIdAndReturnErrors.js'; -export default (input, { ignoreSingularizationList }, { path, rule }) => { - const ruleName = rule.name; - const resourcePath = path[1]; - - if (!isResetMethod(resourcePath)) { - return; - } - - try { - const methodName = 'reset'; - const endpointUrl = stripCustomMethodName(resourcePath); - const errors = validateOperationIdAndReturnErrors(methodName, endpointUrl, input, path, ignoreSingularizationList); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); - } catch (e) { - return handleInternalError(ruleName, path, e); - } -}; +export default defineRule({ + appliesTo: [isReset], + onInternalError: 'return', + check: ({ input, options, path, resourcePath }) => + validateOperationIdAndReturnErrors( + 'reset', + stripCustomMethodName(resourcePath), + input, + path, + options.ignoreSingularizationList + ), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoDeleteMethod.js b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoDeleteMethod.js index e0e30dc7ef..460db969a6 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoDeleteMethod.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoDeleteMethod.js @@ -1,35 +1,12 @@ -import { - getResourcePathItems, - hasDeleteMethod, - isResourceCollectionIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isSingleton } from './utils/conditions/resource.js'; +import { hasDeleteMethod } from './utils/resourceEvaluation.js'; const ERROR_MESSAGE = 'Singleton resources must not define the Delete standard method. If this is not a singleton resource, please implement all CRUDL methods.'; -export default (input, opts, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - const resourcePathItems = getResourcePathItems(resourcePath, oas.paths); - - if (!(isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePathItems))) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { - if (hasDeleteMethod(input)) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} +export default defineRule({ + appliesTo: [isSingleton], + check: ({ input, path }) => (hasDeleteMethod(input) ? [{ path, message: ERROR_MESSAGE }] : []), +}); diff --git a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoId.js b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoId.js index 91b902f35b..7577a946ee 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoId.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasNoId.js @@ -1,31 +1,18 @@ -import { - getResourcePathItems, - hasGetMethod, - isResourceCollectionIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { isSingleton } from './utils/conditions/resource.js'; +import { hasGet } from './utils/conditions/operation.js'; import { getAllSuccessfulResponseSchemas } from './utils/methodUtils.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; const ERROR_MESSAGE = 'Singleton resources must not have a user-provided or system-generated ID.'; -export default (input, opts, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - const resourcePathItems = getResourcePathItems(resourcePath, oas.paths); - - if (!(isResourceCollectionIdentifier(resourcePath) && isSingletonResource(resourcePathItems))) { - return; - } - - if (!hasGetMethod(input)) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; +export default defineRule({ + appliesTo: [isSingleton, hasGet], + check: ({ input, path }) => { + const resourceSchemas = getAllSuccessfulResponseSchemas(input['get']); + return resourceSchemas.some(({ schema }) => schemaHasIdProperty(schema)) ? [{ path, message: ERROR_MESSAGE }] : []; + }, +}); function schemaHasIdProperty(schema) { if (Object.keys(schema).includes('properties')) { @@ -34,15 +21,3 @@ function schemaHasIdProperty(schema) { } return false; } - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { - const resourceSchemas = getAllSuccessfulResponseSchemas(input['get']); - if (resourceSchemas.some(({ schema }) => schemaHasIdProperty(schema))) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} diff --git a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasUpdateMethod.js b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasUpdateMethod.js index 889a29cb42..ef023a71c4 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasUpdateMethod.js +++ b/tools/spectral/ipa/rulesets/functions/IPA113SingletonHasUpdateMethod.js @@ -1,41 +1,13 @@ -import { - getResourcePathItems, - hasPatchMethod, - hasPutMethod, - isReadOnlyResource, - isResourceCollectionIdentifier, - isSingletonResource, -} from './utils/resourceEvaluation.js'; -import { evaluateAndCollectAdoptionStatus, handleInternalError } from './utils/collectionUtils.js'; +// @ts-check +import { defineRule } from './utils/defineRule.js'; +import { not } from './utils/conditions/registry.js'; +import { isReadOnly, isSingleton } from './utils/conditions/resource.js'; +import { hasPatchMethod, hasPutMethod } from './utils/resourceEvaluation.js'; const ERROR_MESSAGE = 'Singleton resources should define the Update method. If this is not a singleton resource, please implement all CRUDL methods.'; -export default (input, opts, { path, documentInventory, rule }) => { - const ruleName = rule.name; - const oas = documentInventory.resolved; - const resourcePath = path[1]; - const resourcePathItems = getResourcePathItems(resourcePath, oas.paths); - - if (!isResourceCollectionIdentifier(resourcePath) || !isSingletonResource(resourcePathItems)) { - return; - } - - if (isReadOnlyResource(resourcePathItems)) { - return; - } - - const errors = checkViolationsAndReturnErrors(input, path, ruleName); - return evaluateAndCollectAdoptionStatus(errors, ruleName, input, path); -}; - -function checkViolationsAndReturnErrors(input, path, ruleName) { - try { - if (!(hasPutMethod(input) || hasPatchMethod(input))) { - return [{ path, message: ERROR_MESSAGE }]; - } - return []; - } catch (e) { - return handleInternalError(ruleName, path, e); - } -} +export default defineRule({ + appliesTo: [isSingleton, not(isReadOnly)], + check: ({ input, path }) => (hasPutMethod(input) || hasPatchMethod(input) ? [] : [{ path, message: ERROR_MESSAGE }]), +}); diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js new file mode 100644 index 0000000000..545ec0c15a --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js @@ -0,0 +1,67 @@ +// @ts-check +// Conditions on a path item (ctx.pathItem) or an operation (ctx.operation) and its extensions. + +import { defineCondition } from './registry.js'; +import { hasGetMethod } from '../resourceEvaluation.js'; +import { hasCustomMethodOverride as hasCustomMethodOverrideExtension, hasMethodVerbOverride } from '../extensions.js'; + +const withOperation = (operation, options = {}) => ({ + path: ['paths', '/groups/{groupId}', 'get'], + paths: { '/groups/{groupId}': { get: operation } }, + options, +}); + +export const hasGet = defineCondition({ + id: 'pathItem.hasGet', + subject: 'pathItem', + definition: 'The path item defines a `get` operation.', + fn: (ctx) => hasGetMethod(ctx.pathItem), + examples: { + match: [{ path: ['paths', '/groups/{groupId}/settings'], paths: { '/groups/{groupId}/settings': { get: {} } } }], + noMatch: [ + { path: ['paths', '/groups/{groupId}/settings'], paths: { '/groups/{groupId}/settings': { patch: {} } } }, + ], + }, +}); + +export const hasCustomMethodOverride = defineCondition({ + id: 'operation.hasCustomMethodOverride', + subject: 'operation', + definition: + 'The operation has an `x-xgen-method-verb-override` extension with `customMethod: true`, declaring a legacy custom method that has no `:` in its path.', + fn: (ctx) => hasCustomMethodOverrideExtension(ctx.operation), + examples: { + match: [withOperation({ 'x-xgen-method-verb-override': { verb: 'search', customMethod: true } })], + noMatch: [ + withOperation({}), + withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }), + ], + }, +}); + +export const hasListVerbOverride = defineCondition({ + id: 'operation.hasListVerbOverride', + subject: 'operation', + definition: + 'The operation has an `x-xgen-method-verb-override` extension whose verb starts with `list`, declaring it a List method.', + fn: (ctx) => hasMethodVerbOverride(ctx.operation, 'list'), + examples: { + match: [withOperation({ 'x-xgen-method-verb-override': { verb: 'listByGroup', customMethod: false } })], + noMatch: [withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } })], + }, +}); + +export const hasRuleMethodVerbOverride = defineCondition({ + id: 'operation.hasRuleMethodVerbOverride', + subject: 'operation', + definition: + "The operation has an `x-xgen-method-verb-override` extension whose verb starts with the rule's `methodName` option (for IPA-104, `get`), declaring it that standard method.", + readsRuleOptions: true, + fn: (ctx) => hasMethodVerbOverride(ctx.operation, ctx.options.methodName), + examples: { + match: [ + withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }, { methodName: 'get' }), + ], + noMatch: [withOperation({}, { methodName: 'get' })], + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/operationIdRules.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/operationIdRules.js new file mode 100644 index 0000000000..fea13793ec --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/operationIdRules.js @@ -0,0 +1,25 @@ +// @ts-check +// Conditions that decide which operations the operation ID rules of the standard methods apply to (see +// utils/operationIdRules.js for the rules). + +import { anyOf, not } from './registry.js'; +import { hasCustomMethodOverride, hasListVerbOverride, hasRuleMethodVerbOverride } from './operation.js'; +import { isLegacyGetTarget } from './resource.js'; +import { hasMethodVerbOverride, VERB_OVERRIDE_EXTENSION } from '../extensions.js'; + +/** + * @returns {Array} the operation is a Get: not a custom method or List by verb + * override, and either on a Get path or declared a Get by verb override + */ +export function getOperationIdRuleConditions() { + return [not(hasCustomMethodOverride), not(hasListVerbOverride), anyOf(isLegacyGetTarget, hasRuleMethodVerbOverride)]; +} + +/** + * @param {Object} operation + * @param {string} methodName the rule's methodName option + * @returns {string} the verb from the operation's verb override if it overrides methodName, otherwise methodName + */ +export function getMethodName(operation, methodName) { + return hasMethodVerbOverride(operation, methodName) ? operation[VERB_OVERRIDE_EXTENSION].verb : methodName; +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js new file mode 100644 index 0000000000..c2ae854375 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js @@ -0,0 +1,68 @@ +// @ts-check +// Conditions on the path string alone (ctx.resourcePath, e.g. '/api/atlas/v2/groups/{groupId}/clusters'). +// The /api/atlas/v2 and /api/atlas/v2/unauth prefixes are ignored. + +import { defineCondition } from './registry.js'; +import { + isCustomMethodIdentifier, + isResetMethod, + isResourceCollectionIdentifier, + isSingleResourceIdentifier, +} from '../resourceEvaluation.js'; + +const at = (resourcePath) => ({ path: ['paths', resourcePath] }); + +export const isCollection = defineCondition({ + id: 'path.isCollection', + subject: 'path', + definition: + 'The path ends in a resource name: either a single top-level segment (`/groups`) or a segment following a path parameter (`/groups/{groupId}/clusters`). This is the path of a resource collection or of a singleton resource.', + fn: (ctx) => isResourceCollectionIdentifier(ctx.resourcePath), + examples: { + match: [at('/api/atlas/v2/groups'), at('/api/atlas/v2/groups/{groupId}/clusters')], + noMatch: [ + at('/api/atlas/v2/groups/{groupId}'), + at('/api/atlas/v2/groups/clusters'), + at('/api/atlas/v2/groups:search'), + ], + }, +}); + +export const isInstance = defineCondition({ + id: 'path.isInstance', + subject: 'path', + definition: + 'The path is a collection path (see `path.isCollection`) followed by a single path parameter, e.g. `/groups/{groupId}`. This is the path of one resource in a collection.', + fn: (ctx) => isSingleResourceIdentifier(ctx.resourcePath), + examples: { + match: [at('/api/atlas/v2/groups/{groupId}'), at('/api/atlas/v2/groups/{groupId}/clusters/{clusterName}')], + noMatch: [ + at('/api/atlas/v2/groups'), + at('/api/atlas/v2/groups/{groupId}/clusters'), + at('/api/atlas/v2/groups/settings/{name}'), + ], + }, +}); + +export const isCustomMethod = defineCondition({ + id: 'path.isCustomMethod', + subject: 'path', + definition: + 'The path contains `:`, e.g. `/groups/{groupId}:migrate`. Custom methods declared by the `x-xgen-method-verb-override` extension are not detected (see `operation.hasCustomMethodOverride`).', + fn: (ctx) => isCustomMethodIdentifier(ctx.resourcePath), + examples: { + match: [at('/api/atlas/v2/groups/{groupId}:migrate'), at('/api/atlas/v2/groups:search')], + noMatch: [at('/api/atlas/v2/groups/{groupId}')], + }, +}); + +export const isReset = defineCondition({ + id: 'path.isResetMethod', + subject: 'path', + definition: 'The path is a custom method named `reset`, e.g. `/groups/{groupId}/settings:reset`.', + fn: (ctx) => isResetMethod(ctx.resourcePath), + examples: { + match: [at('/api/atlas/v2/groups/{groupId}/settings:reset')], + noMatch: [at('/api/atlas/v2/groups/{groupId}/settings'), at('/api/atlas/v2/groups/{groupId}/settings:restart')], + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js new file mode 100644 index 0000000000..57606a79b9 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js @@ -0,0 +1,124 @@ +// @ts-check +// Named conditions that decide whether a rule applies to a component. +// +// A rule declares its conditions (see defineRule.js) instead of hiding them in early returns, so the same +// definition is shared across rules, documented once (rulesets/CONDITIONS.md), and can be counted and traced. + +/** + * @typedef {import('../ruleContext.js').RuleContext} RuleContext + */ + +/** + * @typedef {Object} Condition + * @property {string} id unique; the part before the dot is the subject, e.g. 'resource.isSingleton' (checked by the tests) + * @property {'leaf'|'allOf'|'anyOf'|'not'} op + * @property {(ctx: RuleContext) => unknown} [fn] predicate for leaf conditions; reads the rule context + * @property {Array} [children] operands for allOf/anyOf/not + * @property {string} [subject] what the condition is evaluated on, e.g. 'path', 'resource', 'operation' + * @property {string} [definition] plain-English definition, rendered in the glossary + * @property {{match: Array, noMatch: Array}} [examples] rule context fixtures, run by the tests + * @property {string} [variantOf] id of the condition this one is a divergent variant of + * @property {string} [divergence] how this variant differs from variantOf, and where it is used + * @property {boolean} [readsRuleOptions] the result depends on the rule's functionOptions, so it has no rule-independent count + */ + +/** + * @typedef {Object} EvaluationResult + * @property {boolean} pass + * @property {Array} failedAt condition ids from the evaluated condition down to the one that failed; empty on pass + */ + +/** @type {Map} */ +const registry = new Map(); + +/** + * Defines and registers a named condition. Provide either fn (a leaf) or of (a named combination of conditions). + * + * @param {Omit & {of?: Condition}} spec + * @returns {Condition} + */ +export function defineCondition({ of, ...spec }) { + if (registry.has(spec.id)) { + throw new Error(`Condition ${spec.id} is already defined`); + } + /** @type {Condition} */ + const condition = of ? { ...spec, op: of.op, children: of.children } : { ...spec, op: 'leaf' }; + registry.set(condition.id, condition); + return condition; +} + +/** + * @param {Condition} condition + * @returns {Condition} holds when condition does not + */ +export function not(condition) { + return { id: `not(${condition.id})`, op: 'not', children: [condition] }; +} + +/** + * @param {...Condition} conditions evaluated in order, stopping at the first that fails + * @returns {Condition} holds when all conditions hold + */ +export function allOf(...conditions) { + return { id: `allOf(${conditions.map((c) => c.id).join(', ')})`, op: 'allOf', children: conditions }; +} + +/** + * @param {...Condition} conditions evaluated in order, stopping at the first that holds + * @returns {Condition} holds when at least one condition holds + */ +export function anyOf(...conditions) { + return { id: `anyOf(${conditions.map((c) => c.id).join(', ')})`, op: 'anyOf', children: conditions }; +} + +/** + * Evaluates a condition, short-circuiting like && and ||. + * + * @param {Condition} condition + * @param {RuleContext} ctx + * @param {(condition: Condition) => EvaluationResult} [evaluateChild] evaluates operands, e.g. with a cache; defaults + * to evaluating them directly + * @returns {EvaluationResult} + */ +export function evaluate(condition, ctx, evaluateChild = (child) => evaluate(child, ctx, evaluateChild)) { + const children = condition.children ?? []; + switch (condition.op) { + case 'leaf': + return condition.fn?.(ctx) ? passed() : failed(condition); + case 'not': + return evaluateChild(children[0]).pass ? failed(condition) : passed(); + case 'allOf': + for (const child of children) { + const result = evaluateChild(child); + if (!result.pass) { + // Report the named composite as well as the operand that failed, e.g. ['resource.isSingleton', 'path.isCollection'] + return { pass: false, failedAt: isNamed(condition) ? [condition.id, ...result.failedAt] : result.failedAt }; + } + } + return passed(); + case 'anyOf': + return children.some((child) => evaluateChild(child).pass) ? passed() : failed(condition); + default: + throw new Error(`Unknown condition op: ${condition.op}`); + } +} + +/** + * @returns {Array} all named conditions, in definition order + */ +export function listConditions() { + return [...registry.values()]; +} + +// By id rather than identity, so conditions from another copy of this module (e.g. Spectral's bundle) are recognized +function isNamed(condition) { + return registry.has(condition.id); +} + +function passed() { + return { pass: true, failedAt: [] }; +} + +function failed(condition) { + return { pass: false, failedAt: [condition.id] }; +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js new file mode 100644 index 0000000000..4c9325d32e --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js @@ -0,0 +1,199 @@ +// @ts-check +// Conditions on a resource: the path items that belong to it (ctx.resourcePathItems, see getResourcePathItems), +// e.g. '/groups/{groupId}/clusters', '/groups/{groupId}/clusters/{clusterName}' and their custom methods. + +import { allOf, anyOf, defineCondition, not } from './registry.js'; +import { isCollection, isInstance } from './path.js'; +import { + anyPathHasException, + getResponseIsAllReadOnly, + isSingleResourceIdentifier, + isSingletonResource, + stripCustomMethodName, +} from '../resourceEvaluation.js'; +import { isInvalidGetMethod } from '../methodLogic.js'; +import { getGETMethodResponseSchemaFromPathItem } from '../methodUtils.js'; + +const readOnlyGet = { get: { responses: { 200: { content: { 'application/json': { schema: readOnlySchema() } } } } } }; +const writableGet = { + get: { + responses: { 200: { content: { 'application/json': { schema: { properties: { name: { type: 'string' } } } } } } }, + }, +}; + +function readOnlySchema() { + return { properties: { id: { type: 'string', readOnly: true } } }; +} + +const example = (resourcePath, paths) => ({ path: ['paths', resourcePath], paths }); + +export const hasSingletonShape = defineCondition({ + id: 'resource.hasSingletonShape', + subject: 'resource', + definition: + "The resource's path items contain exactly one collection path, that path belongs to a parent resource (`/parent/{id}/child`), it has no Create (POST) method, and every other path is a custom method. The current path is not required to be the collection path; see `resource.isSingleton`.", + fn: (ctx) => isSingletonResource(ctx.resourcePathItems), + examples: { + match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { get: {}, patch: {} } })], + noMatch: [ + example('/groups/{groupId}/clusters', { + '/groups/{groupId}/clusters': { get: {}, post: {} }, + '/groups/{groupId}/clusters/{clusterName}': { get: {} }, + }), + example('/settings', { '/settings': { get: {} } }), + ], + }, +}); + +export const isSingleton = defineCondition({ + id: 'resource.isSingleton', + subject: 'resource', + definition: 'The path is a collection path and the resource it names has the singleton shape.', + of: allOf(isCollection, hasSingletonShape), + examples: { + match: [ + example('/groups/{groupId}/settings', { + '/groups/{groupId}/settings': { get: {} }, + '/groups/{groupId}/settings:reset': { post: {} }, + }), + ], + noMatch: [ + example('/groups/{groupId}/settings:reset', { + '/groups/{groupId}/settings': { get: {} }, + '/groups/{groupId}/settings:reset': { post: {} }, + }), + ], + }, +}); + +export const hasResourceHasGetException = defineCondition({ + id: 'resource.hasResourceHasGetException', + subject: 'resource', + definition: 'Some path item of the resource carries an `x-xgen-IPA-exception` for `xgen-IPA-104-resource-has-GET`.', + fn: (ctx) => anyPathHasException(ctx.resourcePathItems, 'xgen-IPA-104-resource-has-GET'), + examples: { + match: [ + example('/groups/{groupId}/settings', { + '/groups/{groupId}/settings': { 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' } }, + }), + ], + noMatch: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], + }, +}); + +export const getResponseAllReadOnly = defineCondition({ + id: 'resource.getResponseIsAllReadOnly', + subject: 'resource', + definition: + "Every property of the 2xx response schema of the resource's Get method is `readOnly: true`. The Get on the instance path is used if there is one; otherwise the first path with a Get, which may be a List, in which case `results.items` is inspected.", + fn: (ctx) => getResponseIsAllReadOnly(ctx.resourcePathItems), + examples: { + match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], + noMatch: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...writableGet } })], + }, +}); + +export const isReadOnly = defineCondition({ + id: 'resource.isReadOnly', + subject: 'resource', + definition: + 'The resource has a Get method whose response is entirely read-only, and no path of the resource has an exception for `xgen-IPA-104-resource-has-GET`.', + divergence: + 'The exception clause is not part of the IPA definition: a resource with an IPA-104 resource-has-GET exception is never read-only, whatever its Get response looks like.', + of: allOf(not(hasResourceHasGetException), getResponseAllReadOnly), + examples: { + match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], + noMatch: [ + example('/groups/{groupId}/settings', { + '/groups/{groupId}/settings': { + ...readOnlyGet, + 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' }, + }, + }), + ], + }, +}); + +export const isGetTarget = defineCondition({ + id: 'path.isGetTarget', + subject: 'path', + definition: 'The path is where a standard Get method belongs: an instance path, or the path of a singleton resource.', + of: anyOf(isInstance, isSingleton), + examples: { + match: [ + example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }), + example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { get: {} } }), + ], + noMatch: [ + example('/groups', { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } }), + example('/groups/settings/{name}', { '/groups/settings/{name}': { get: {} } }), + ], + }, +}); + +export const isLegacyGetTarget = defineCondition({ + id: 'path.isLegacyGetTarget', + subject: 'path', + variantOf: 'path.isGetTarget', + definition: + 'The path ends in a path parameter (its last character is `}`), or is the path of a singleton resource. Implemented by `methodLogic.isInvalidGetMethod`.', + divergence: + 'Accepts any path ending in `}`, including ones that are not instance paths such as `/groups/settings/{name}`. Used by the IPA-104 operation ID rules; the other IPA-104 rules use `path.isGetTarget`.', + fn: (ctx) => !isInvalidGetMethod(ctx.resourcePath, ctx.resourcePathItems), + examples: { + match: [ + example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }), + example('/groups/settings/{name}', { '/groups/settings/{name}': { get: {} } }), + ], + noMatch: [example('/groups', { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } })], + }, +}); + +/** + * The 200 response of the Get method of the resource a custom method belongs to, for the current media type, in the + * document the rule reads. Shared by the condition below and the rule's check. + * + * @param {import('../ruleContext.js').RuleContext} ctx a context whose path ends in a media type + * @returns {{schema?: Object}|null|undefined} + */ +export function baseGetResponse(ctx) { + return ctx.memo('baseGetResponse', () => { + const basePathItem = ctx.oas.paths[stripCustomMethodName(ctx.resourcePath)]; + return basePathItem && getGETMethodResponseSchemaFromPathItem(basePathItem, ctx.path.at(-1)); + }); +} + +export const baseGetHasMediaType = defineCondition({ + id: 'mediaType.baseGetHasMediaType', + subject: 'mediaType', + definition: + 'The resource the custom method belongs to (its path without `:`) has a Get method with a 200 response for the same media type as the current response.', + fn: (ctx) => Boolean(baseGetResponse(ctx)), + examples: { + match: [ + { + path: ['paths', '/groups/{groupId}/settings:reset', 'post', 'responses', '200', 'content', 'application/json'], + paths: { + '/groups/{groupId}/settings': { get: { responses: { 200: { content: { 'application/json': {} } } } } }, + }, + }, + ], + noMatch: [ + { + path: ['paths', '/groups/{groupId}/settings:reset', 'post', 'responses', '200', 'content', 'application/json'], + paths: { '/groups/{groupId}/settings': { patch: {} } }, + }, + ], + }, +}); + +/** + * The instance path of the resource at the current path, e.g. '/groups/{groupId}' for '/groups'. Undefined when the + * resource has no instance path. + * + * @param {import('../ruleContext.js').RuleContext} ctx + * @returns {string|undefined} + */ +export function instancePath(ctx) { + return ctx.memo('instancePath', () => Object.keys(ctx.resourcePathItems).find((p) => isSingleResourceIdentifier(p))); +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js new file mode 100644 index 0000000000..be4f18cdd7 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js @@ -0,0 +1,43 @@ +// @ts-check +// Conditions on a response or media type object, for rules whose path is +// ['paths', , , 'responses', , 'content', ]. + +import { defineCondition } from './registry.js'; + +const content = (code, mediaType, mediaTypeObject) => ({ + path: ['paths', '/groups/{groupId}', 'get', 'responses', code, 'content', mediaType], + paths: { '/groups/{groupId}': { get: { responses: { [code]: { content: { [mediaType]: mediaTypeObject } } } } } }, +}); + +export const is2xx = defineCondition({ + id: 'response.is2xx', + subject: 'response', + definition: 'The response status code starts with `2`.', + fn: (ctx) => ctx.path[4].startsWith('2'), + examples: { + match: [content('200', 'application/json', {}), content('202', 'application/json', {})], + noMatch: [content('404', 'application/json', {})], + }, +}); + +export const hasSchema = defineCondition({ + id: 'mediaType.hasSchema', + subject: 'mediaType', + definition: 'The media type object exists and defines a `schema`.', + fn: (ctx) => Boolean(ctx.node?.schema), + examples: { + match: [content('200', 'application/json', { schema: { type: 'object' } })], + noMatch: [content('200', 'application/json', { example: {} })], + }, +}); + +export const isJson = defineCondition({ + id: 'mediaType.isJson', + subject: 'mediaType', + definition: 'The media type ends in `json`, e.g. `application/vnd.atlas.2023-01-01+json`.', + fn: (ctx) => ctx.path.at(-1).endsWith('json'), + examples: { + match: [content('200', 'application/vnd.atlas.2023-01-01+json', {})], + noMatch: [content('200', 'application/vnd.atlas.2023-01-01+csv', {})], + }, +}); diff --git a/tools/spectral/ipa/rulesets/functions/utils/defineRule.js b/tools/spectral/ipa/rulesets/functions/utils/defineRule.js new file mode 100644 index 0000000000..3f8f569c73 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/defineRule.js @@ -0,0 +1,95 @@ +// @ts-check +import { evaluateAndCollectAdoptionStatus, handleInternalError } from './collectionUtils.js'; +import { createRuleContext } from './ruleContext.js'; + +/** + * @typedef {import('./conditions/registry.js').Condition} Condition + * @typedef {import('./ruleContext.js').RuleContext} RuleContext + * @typedef {Array<{path?: Array, message: string}>} RuleErrors + */ + +/** + * How a rule reports an exception thrown by its check. The modes preserve how rules behaved before defineRule; they + * are not a recommendation. Conditions are never covered: an exception there always fails the lint run. + * - 'violation': the internal error is reported, and collected as a violation (or exception, if the component has one) + * - 'return': the internal error is reported, and nothing is collected + * - 'throw': the exception propagates and Spectral fails the lint run + * @typedef {'violation'|'return'|'throw'} InternalErrorMode + */ + +/** + * Builds a Spectral rule function from the conditions under which the rule applies and the check it performs. + * + * A call goes through these steps: + * 1. The rule's `given` in the YAML selects the input. + * 2. `appliesTo` conditions are evaluated in order. If one fails, the rule does not apply and nothing is collected. + * 3. `check` returns the validation errors, an empty array meaning the component adopts the rule. What happens when + * it throws depends on `onInternalError`. + * 4. The result is collected as an adoption, violation or exception, reading `x-xgen-IPA-exception` from + * `exceptionTarget`. + * + * The conditions are attached to the function as `meta`, for generated docs and reports: `appliesTo`, and `checkUses` for + * conditions the check evaluates itself (e.g. to pick an error message), which have no effect on whether the rule applies. + * + * @param {Object} spec + * @param {Array} [spec.appliesTo] conditions that must all hold for the rule to apply + * @param {(ctx: RuleContext) => RuleErrors} spec.check + * @param {(ctx: RuleContext) => void} [spec.beforeCheck] runs once the rule applies, outside any try like the conditions, so + * an exception fails the lint run instead of being reported as an internal error + * @param {Array} [spec.checkUses] conditions evaluated inside check, listed for docs and reports only + * @param {(ctx: RuleContext) => Object} [spec.exceptionTarget] the object that carries the exception, defaults to the input + * @param {import('./ruleContext.js').DocumentKind} [spec.document] the document the context reads, defaults to 'resolved' + * @param {InternalErrorMode} [spec.onInternalError] what happens when check throws, defaults to 'violation' + * @returns {Function & {meta: {appliesTo: Array, checkUses: Array, document: string, onInternalError: InternalErrorMode}}} + */ +export function defineRule({ + appliesTo = [], + check, + beforeCheck, + checkUses = [], + exceptionTarget = (ctx) => ctx.input, + document = 'resolved', + onInternalError = 'violation', +}) { + const collect = (ctx, errors) => + evaluateAndCollectAdoptionStatus(errors, ctx.ruleName, exceptionTarget(ctx), ctx.path); + + const ruleFunction = (input, options, spectralContext) => { + const ctx = createRuleContext(input, options, spectralContext, document); + // Spectral names the function in the error when it throws, and every rule here would otherwise be 'ruleFunction' + Object.defineProperty(ruleFunction, 'name', { value: ctx.ruleName }); + + // Conditions are evaluated outside any try, so a condition that throws fails the lint run + for (const condition of appliesTo) { + if (!ctx.is(condition)) { + return; + } + } + + beforeCheck?.(ctx); + + switch (onInternalError) { + case 'violation': { + let errors; + try { + errors = check(ctx); + } catch (e) { + errors = handleInternalError(ctx.ruleName, ctx.path, e); + } + return collect(ctx, errors); + } + case 'return': + try { + return collect(ctx, check(ctx)); + } catch (e) { + return handleInternalError(ctx.ruleName, ctx.path, e); + } + case 'throw': + return collect(ctx, check(ctx)); + default: + throw new Error(`Unknown onInternalError mode: ${onInternalError}`); + } + }; + + return Object.assign(ruleFunction, { meta: { appliesTo, checkUses, document, onInternalError } }); +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/operationIdRules.js b/tools/spectral/ipa/rulesets/functions/utils/operationIdRules.js new file mode 100644 index 0000000000..96f392bdc9 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/operationIdRules.js @@ -0,0 +1,58 @@ +// @ts-check +// Builds the valid-operation-id and operation-id-length rules of the standard methods. Kept out of utils/conditions, +// which tooling loads outside Spectral: operationIdGeneration.js only loads through Spectral's bundler. + +import { defineRule } from './defineRule.js'; +import { getMethodName } from './conditions/operationIdRules.js'; +import { generateOperationID } from './operationIdGeneration.js'; +import { + validateOperationIdAndReturnErrors, + validateOperationIdLengthAndReturnErrors, +} from './validations/validateOperationIdAndReturnErrors.js'; + +// A verb override without a `verb` fails the lint run, as it did before these rules were built with defineRule +const verbOverrideIsReadable = ({ input, options }) => getMethodName(input, options.methodName); + +/** + * Builds a rule checking that the operation ID is the one generated from the method and the path. The verb is the rule's + * `methodName` option, or the operation's verb override. + * + * @param {Array} appliesTo which operations are the rule's method + */ +export function validOperationIdRule(appliesTo) { + return defineRule({ + appliesTo, + onInternalError: 'return', + beforeCheck: verbOverrideIsReadable, + check: ({ input, options, path, resourcePath }) => + validateOperationIdAndReturnErrors( + getMethodName(input, options.methodName), + resourcePath, + input, + path, + options.ignoreSingularizationList + ), + }); +} + +/** + * Builds a rule checking that the operation ID, or its `x-xgen-operation-id-override`, is at most `maxLength` words. + * + * @param {Array} appliesTo which operations are the rule's method + */ +export function operationIdLengthRule(appliesTo) { + return defineRule({ + appliesTo, + onInternalError: 'return', + beforeCheck: verbOverrideIsReadable, + check: ({ input, options, path, resourcePath }) => { + // The generated ID is only used to suggest a shorter override in the message + const expectedOperationId = generateOperationID( + getMethodName(input, options.methodName), + resourcePath, + options.ignoreSingularizationList + ); + return validateOperationIdLengthAndReturnErrors(input, path, expectedOperationId, options.maxLength); + }, + }); +} diff --git a/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js b/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js index 607c76bf91..1b42cfae5a 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js +++ b/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js @@ -292,15 +292,33 @@ export function allPropertiesAreReadOnly(schema) { * @returns {boolean} true if the resource is read-only, false otherwise */ export function isReadOnlyResource(resourcePathItems) { - const resourcePaths = Object.keys(resourcePathItems); + // If any path has an exception for xgen-IPA-104-resource-has-GET, we cannot determine if the resource is read-only + return ( + !anyPathHasException(resourcePathItems, 'xgen-IPA-104-resource-has-GET') && + getResponseIsAllReadOnly(resourcePathItems) + ); +} - // Check if any path has an exception for xgen-IPA-104-resource-has-GET - // If so, we cannot determine if the resource is read-only - for (const path of resourcePaths) { - if (hasException(resourcePathItems[path], 'xgen-IPA-104-resource-has-GET')) { - return false; - } - } +/** + * Checks if any path item of a resource has an exception for the given rule. + * + * @param {Object} resourcePathItems - All path items for the resource to be evaluated + * @param {string} ruleName - The name of the rule to look for + * @returns {boolean} + */ +export function anyPathHasException(resourcePathItems, ruleName) { + return Object.values(resourcePathItems).some((pathItem) => hasException(pathItem, ruleName)); +} + +/** + * Checks if all properties in the response schema of a resource's Get method are readOnly: true. The standard Get method + * (on the single resource path) is preferred; otherwise the first path with a Get method is used, which may be a List. + * + * @param {Object} resourcePathItems - All path items for the resource to be evaluated + * @returns {boolean} + */ +export function getResponseIsAllReadOnly(resourcePathItems) { + const resourcePaths = Object.keys(resourcePathItems); // First, look for a standard Get method let getPathItem = null; diff --git a/tools/spectral/ipa/rulesets/functions/utils/ruleContext.js b/tools/spectral/ipa/rulesets/functions/utils/ruleContext.js new file mode 100644 index 0000000000..d30e7f7f03 --- /dev/null +++ b/tools/spectral/ipa/rulesets/functions/utils/ruleContext.js @@ -0,0 +1,128 @@ +// @ts-check +import { getResourcePathItems } from './resourceEvaluation.js'; +import { resolveObject } from './componentUtils.js'; +import { evaluate } from './conditions/registry.js'; + +/** + * @typedef {'resolved'|'unresolved'} DocumentKind + * @typedef {import('./conditions/registry.js').Condition} Condition + * @typedef {import('./conditions/registry.js').EvaluationResult} EvaluationResult + */ + +/** + * Per-call context for a rule function. Derived values are computed on first use and cached, so conditions and + * the rule's check share them. + */ +export class RuleContext { + /** + * @param {Object} params + * @param {*} params.input the value Spectral passes to the rule function + * @param {Object} params.options the rule's functionOptions + * @param {Array} params.path the JSON path of the input, e.g. ['paths', '/groups/{groupId}', 'get'] + * @param {string} params.ruleName + * @param {{paths: Object}} params.oas the document the rule reads, see {@link DocumentKind} + */ + constructor({ input, options, path, ruleName, oas }) { + this.input = input; + this.options = options; + this.path = path; + this.ruleName = ruleName; + this.oas = oas; + /** @type {Map} */ + this.evaluations = new Map(); + /** @type {Map} */ + this.cache = new Map(); + } + + /** @returns {string} the path key, e.g. '/api/atlas/v2/groups/{groupId}' */ + get resourcePath() { + return this.path[1]; + } + + /** @returns {Object} the path items of the resource at resourcePath, see getResourcePathItems */ + get resourcePathItems() { + return this.memo('resourcePathItems', () => getResourcePathItems(this.resourcePath, this.oas.paths)); + } + + /** @returns {Object|undefined} the path item at resourcePath */ + get pathItem() { + return this.oas.paths[this.resourcePath]; + } + + /** @returns {Object|undefined} the operation at path[2], if the path points into one */ + get operation() { + return this.pathItem?.[this.path[2]]; + } + + /** @returns {*} the value at path in the document the rule reads, or undefined if missing or falsy */ + get node() { + return this.memo('node', () => resolveObject(this.oas, this.path)); + } + + /** + * @param {Condition} condition + * @returns {EvaluationResult} + */ + evaluate(condition) { + if (!this.evaluations.has(condition.id)) { + // Operands go through this cache too, so a condition shared by appliesTo and the check is evaluated once + this.evaluations.set( + condition.id, + evaluate(condition, this, (child) => this.evaluate(child)) + ); + } + return /** @type {EvaluationResult} */ (this.evaluations.get(condition.id)); + } + + /** + * @param {Condition} condition + * @returns {boolean} + */ + is(condition) { + return this.evaluate(condition).pass; + } + + /** + * @param {string} resourcePath another path key in the same document, e.g. the base resource of a custom method + * @returns {RuleContext} a context for that path, without an input + */ + atPath(resourcePath) { + return new RuleContext({ + input: undefined, + options: this.options, + path: ['paths', resourcePath], + ruleName: this.ruleName, + oas: this.oas, + }); + } + + /** + * @param {string} key + * @param {() => *} compute + */ + memo(key, compute) { + if (!this.cache.has(key)) { + this.cache.set(key, compute()); + } + return this.cache.get(key); + } +} + +/** + * Creates the context for one call of a rule function. + * + * @param {*} input + * @param {Object} options + * @param {{path: Array, documentInventory: {resolved: Object, unresolved: Object}, rule: {name: string}}} spectralContext + * @param {DocumentKind} document which document the rule reads; independent of the rule's YAML `resolved` flag + * @returns {RuleContext} + */ +export function createRuleContext(input, options, { path, documentInventory, rule }, document) { + return new RuleContext({ + input, + options, + path, + ruleName: rule.name, + oas: documentInventory[document], + }); +} From c9b0edfdac0b51829c9a6318513390c0e5968b4f Mon Sep 17 00:00:00 2001 From: Mitrofanos Ntatidis Date: Thu, 24 Sep 2026 14:56:48 +0100 Subject: [PATCH 4/4] docs(ipa): generate Applies to sections and a conditions glossary - generateRulesetReadme.js renders an "Applies to" section for rules built with defineRule, from their given and appliesTo conditions, and writes rulesets/CONDITIONS.md: each named condition's definition, examples, variant/divergence notes and the rules using it. - Conditions get a short label for these docs. - Hand-written applicability bullets removed from the 18 IPA-104/113 descriptions, since they are now generated and can't drift. Two descriptions corrected: reset-method-not-on-readonly-singleton does not check for a singleton, and both reset rules evaluate the base resource. - CONTRIBUTING.md: new section on conditions, defineRule and the report scripts. The code-health docs check covers CONDITIONS.md, as it is written by gen-ipa-docs. No behavior change: snapshots identical on both specs. --- tools/spectral/ipa/CONTRIBUTING.md | 26 +- .../ipa/__tests__/utils/conditions.test.js | 7 + tools/spectral/ipa/rulesets/CONDITIONS.md | 280 ++++++++++++++++++ tools/spectral/ipa/rulesets/IPA-104.yaml | 8 - tools/spectral/ipa/rulesets/IPA-113.yaml | 21 +- tools/spectral/ipa/rulesets/README.md | 125 ++++++-- .../functions/utils/conditions/operation.js | 54 +++- .../functions/utils/conditions/path.js | 4 + .../functions/utils/conditions/registry.js | 4 +- .../functions/utils/conditions/resource.js | 130 +++++--- .../functions/utils/conditions/response.js | 10 +- .../ipa/scripts/generateRulesetReadme.js | 121 ++++++++ 12 files changed, 689 insertions(+), 101 deletions(-) create mode 100644 tools/spectral/ipa/rulesets/CONDITIONS.md diff --git a/tools/spectral/ipa/CONTRIBUTING.md b/tools/spectral/ipa/CONTRIBUTING.md index c15645c5f6..97af074452 100644 --- a/tools/spectral/ipa/CONTRIBUTING.md +++ b/tools/spectral/ipa/CONTRIBUTING.md @@ -118,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 diff --git a/tools/spectral/ipa/__tests__/utils/conditions.test.js b/tools/spectral/ipa/__tests__/utils/conditions.test.js index 91dfd8072c..023c62d23b 100644 --- a/tools/spectral/ipa/__tests__/utils/conditions.test.js +++ b/tools/spectral/ipa/__tests__/utils/conditions.test.js @@ -37,6 +37,13 @@ describe('tools/spectral/ipa/rulesets/functions/utils/conditions', () => { }); }); + 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); }); diff --git a/tools/spectral/ipa/rulesets/CONDITIONS.md b/tools/spectral/ipa/rulesets/CONDITIONS.md new file mode 100644 index 0000000000..2f04d57a73 --- /dev/null +++ b/tools/spectral/ipa/rulesets/CONDITIONS.md @@ -0,0 +1,280 @@ + + +# IPA Validation Conditions + +Named conditions that decide where IPA rules apply. They are defined in [rulesets/functions/utils/conditions](https://github.com/mongodb/openapi/tree/main/tools/spectral/ipa/rulesets/functions/utils/conditions) and used by rules built with `defineRule`. Only conditions used by at least one rule are listed. + +### mediaType.baseGetHasMediaType + +Evaluated on: mediaType + +The resource the custom method belongs to (its path without `:`) has a Get method with a 200 response for the same media type as the current response. + +Matches: +- `/groups/{groupId}/settings:reset post responses 200 content application/json` (the Get on /groups/{groupId}/settings has an application/json 200 response) + +Does not match: +- `/groups/{groupId}/settings:reset post responses 200 content application/json` (the base resource has no Get) + +Used by: [xgen-IPA-113-reset-method-response-is-get-method-response](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-response-is-get-method-response) + +### mediaType.hasSchema + +Evaluated on: mediaType + +The media type object exists and defines a `schema`. + +Matches: +- `/groups/{groupId} get responses 200 content application/json` (has a schema) + +Does not match: +- `/groups/{groupId} get responses 200 content application/json` (an example but no schema) + +Used by: [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource), [xgen-IPA-113-reset-method-response-is-get-method-response](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-response-is-get-method-response) + +### mediaType.isJson + +Evaluated on: mediaType + +The media type ends in `json`, e.g. `application/vnd.atlas.2023-01-01+json`. + +Matches: +- `/groups/{groupId} get responses 200 content application/vnd.atlas.2023-01-01+json` + +Does not match: +- `/groups/{groupId} get responses 200 content application/vnd.atlas.2023-01-01+csv` + +Used by: [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource), [xgen-IPA-113-reset-method-response-is-get-method-response](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-response-is-get-method-response) + +### operation.hasCustomMethodOverride + +Evaluated on: operation + +The operation has an `x-xgen-method-verb-override` extension with `customMethod: true`, declaring a legacy custom method that has no `:` in its path. + +Matches: +- `/groups/{groupId} get` (verb override `search` with customMethod: true) + +Does not match: +- `/groups/{groupId} get` (no verb override) +- `/groups/{groupId} get` (verb override `get` with customMethod: false) + +Used by: [xgen-IPA-104-operation-id-length](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-operation-id-length), [xgen-IPA-104-valid-operation-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-valid-operation-id) + +### operation.hasListVerbOverride + +Evaluated on: operation + +The operation has an `x-xgen-method-verb-override` extension whose verb starts with `list`, declaring it a List method. + +Matches: +- `/groups/{groupId} get` (verb override `listByGroup`) + +Does not match: +- `/groups/{groupId} get` (verb override `get`) + +Used by: [xgen-IPA-104-operation-id-length](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-operation-id-length), [xgen-IPA-104-valid-operation-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-valid-operation-id) + +### operation.hasRuleMethodVerbOverride + +Evaluated on: operation + +The operation has an `x-xgen-method-verb-override` extension whose verb starts with the rule's `methodName` option (for IPA-104, `get`), declaring it that standard method. + +Matches: +- `/groups/{groupId} get` (verb override `get`, rule methodName `get`) + +Does not match: +- `/groups/{groupId} get` (no verb override, rule methodName `get`) + +Used by: [xgen-IPA-104-operation-id-length](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-operation-id-length), [xgen-IPA-104-valid-operation-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-valid-operation-id) + +### path.isCollection + +Evaluated on: path + +The path ends in a resource name: either a single top-level segment (`/groups`) or a segment following a path parameter (`/groups/{groupId}/clusters`). This is the path of a resource collection or of a singleton resource. + +Matches: +- `/api/atlas/v2/groups` +- `/api/atlas/v2/groups/{groupId}/clusters` + +Does not match: +- `/api/atlas/v2/groups/{groupId}` +- `/api/atlas/v2/groups/clusters` +- `/api/atlas/v2/groups:search` + +Used by: [xgen-IPA-104-get-method-no-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body), [xgen-IPA-104-get-method-response-code-is-200](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200), [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource), [xgen-IPA-104-resource-has-GET](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-resource-has-GET), [xgen-IPA-113-reset-method-only-on-singleton-resources](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-only-on-singleton-resources), [xgen-IPA-113-singleton-must-not-have-delete-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-delete-method), [xgen-IPA-113-singleton-must-not-have-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-id), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### path.isGetTarget + +Evaluated on: path + +The path is where a standard Get method belongs: an instance path, or the path of a singleton resource. + +Defined as: any of: the path is a single resource (instance) path ([`path.isInstance`](#pathisinstance)); the path is a singleton resource ([`resource.isSingleton`](#resourceissingleton)) + +Matches: +- `/groups/{groupId}` (instance path) +- `/groups/{groupId}/settings` (singleton) + +Does not match: +- `/groups` (collection of a standard resource) +- `/groups/settings/{name}` (not an instance path) + +Used by: [xgen-IPA-104-get-method-no-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body), [xgen-IPA-104-get-method-response-code-is-200](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200), [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource) + +### path.isInstance + +Evaluated on: path + +The path is a collection path (see `path.isCollection`) followed by a single path parameter, e.g. `/groups/{groupId}`. This is the path of one resource in a collection. + +Matches: +- `/api/atlas/v2/groups/{groupId}` +- `/api/atlas/v2/groups/{groupId}/clusters/{clusterName}` + +Does not match: +- `/api/atlas/v2/groups` +- `/api/atlas/v2/groups/{groupId}/clusters` +- `/api/atlas/v2/groups/settings/{name}` + +Used by: [xgen-IPA-104-get-method-no-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body), [xgen-IPA-104-get-method-response-code-is-200](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200), [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource) + +### path.isLegacyGetTarget + +Evaluated on: path + +The path ends in a path parameter (its last character is `}`), or is the path of a singleton resource. Implemented by `methodLogic.isInvalidGetMethod`. + +> **Variant of [`path.isGetTarget`](#pathisgettarget).** Accepts any path ending in `}`, including ones that are not instance paths such as `/groups/settings/{name}`. Used by the IPA-104 operation ID rules; the other IPA-104 rules use `path.isGetTarget`. + +Matches: +- `/groups/{groupId}` (instance path) +- `/groups/settings/{name}` (ends in a path parameter, though not an instance path) + +Does not match: +- `/groups` (collection of a standard resource) + +Used by: [xgen-IPA-104-operation-id-length](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-operation-id-length), [xgen-IPA-104-valid-operation-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-valid-operation-id) + +### path.isResetMethod + +Evaluated on: path + +The path is a custom method named `reset`, e.g. `/groups/{groupId}/settings:reset`. + +Matches: +- `/api/atlas/v2/groups/{groupId}/settings:reset` + +Does not match: +- `/api/atlas/v2/groups/{groupId}/settings` +- `/api/atlas/v2/groups/{groupId}/settings:restart` + +Used by: [xgen-IPA-113-reset-method-must-not-have-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-must-not-have-request-body), [xgen-IPA-113-reset-method-must-return-200-OK](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-must-return-200-OK), [xgen-IPA-113-reset-method-must-use-POST](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-must-use-POST), [xgen-IPA-113-reset-method-not-on-readonly-singleton](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton), [xgen-IPA-113-reset-method-only-on-singleton-resources](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-only-on-singleton-resources), [xgen-IPA-113-reset-method-response-is-get-method-response](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-response-is-get-method-response), [xgen-IPA-113-reset-method-valid-operation-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-valid-operation-id) + +### pathItem.hasGet + +Evaluated on: pathItem + +The path item defines a `get` operation. + +Matches: +- `/groups/{groupId}/settings` (defines get) + +Does not match: +- `/groups/{groupId}/settings` (patch only) + +Used by: [xgen-IPA-113-singleton-must-not-have-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-id) + +### resource.getResponseIsAllReadOnly + +Evaluated on: resource + +Every property of the 2xx response schema of the resource's Get method is `readOnly: true`. The Get on the instance path is used if there is one; otherwise the first path with a Get, which may be a List, in which case `results.items` is inspected. + +Matches: +- `/groups/{groupId}/settings` (every Get response property is readOnly) + +Does not match: +- `/groups/{groupId}/settings` (the Get response has a writable property) + +Used by: [xgen-IPA-113-reset-method-not-on-readonly-singleton](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### resource.hasResourceHasGetException + +Evaluated on: resource + +Some path item of the resource carries an `x-xgen-IPA-exception` for `xgen-IPA-104-resource-has-GET`. + +Matches: +- `/groups/{groupId}/settings` (the path has the exception) + +Does not match: +- `/groups/{groupId}/settings` (no exception) + +Used by: [xgen-IPA-113-reset-method-not-on-readonly-singleton](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### resource.hasSingletonShape + +Evaluated on: resource + +The resource's path items contain exactly one collection path, that path belongs to a parent resource (`/parent/{id}/child`), it has no Create (POST) method, and every other path is a custom method. The current path is not required to be the collection path; see `resource.isSingleton`. + +Matches: +- `/groups/{groupId}/settings` (Get and Patch only) + +Does not match: +- `/groups/{groupId}/clusters` (has Create and an instance path) +- `/settings` (no parent resource) + +Used by: [xgen-IPA-104-get-method-no-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body), [xgen-IPA-104-get-method-response-code-is-200](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200), [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource), [xgen-IPA-104-resource-has-GET](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-resource-has-GET), [xgen-IPA-113-reset-method-only-on-singleton-resources](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-only-on-singleton-resources), [xgen-IPA-113-singleton-must-not-have-delete-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-delete-method), [xgen-IPA-113-singleton-must-not-have-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-id), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### resource.isReadOnly + +Evaluated on: resource + +The resource has a Get method whose response is entirely read-only, and no path of the resource has an exception for `xgen-IPA-104-resource-has-GET`. + +Defined as: all of: not: some path of the resource has an exception for `xgen-IPA-104-resource-has-GET` ([`resource.hasResourceHasGetException`](#resourcehasresourcehasgetexception)); all properties of the resource's Get response are read-only ([`resource.getResponseIsAllReadOnly`](#resourcegetresponseisallreadonly)) + +> **Divergence.** The exception clause is not part of the IPA definition: a resource with an IPA-104 resource-has-GET exception is never read-only, whatever its Get response looks like. + +Matches: +- `/groups/{groupId}/settings` (every Get response property is readOnly) + +Does not match: +- `/groups/{groupId}/settings` (the same read-only Get response, but the path has an xgen-IPA-104-resource-has-GET exception) + +Used by: [xgen-IPA-113-reset-method-not-on-readonly-singleton](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### resource.isSingleton + +Evaluated on: resource + +The path is a collection path and the resource it names has the singleton shape. + +Defined as: all of: the path is a resource collection path ([`path.isCollection`](#pathiscollection)); the resource has the singleton shape ([`resource.hasSingletonShape`](#resourcehassingletonshape)) + +Matches: +- `/groups/{groupId}/settings` (singleton with a custom method) + +Does not match: +- `/groups/{groupId}/settings:reset` (the custom method path of a singleton, not its collection path) + +Used by: [xgen-IPA-104-get-method-no-request-body](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body), [xgen-IPA-104-get-method-response-code-is-200](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200), [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource), [xgen-IPA-113-reset-method-only-on-singleton-resources](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-only-on-singleton-resources), [xgen-IPA-113-singleton-must-not-have-delete-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-delete-method), [xgen-IPA-113-singleton-must-not-have-id](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-id), [xgen-IPA-113-singleton-should-have-update-method](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method) + +### response.is2xx + +Evaluated on: response + +The response status code starts with `2`. + +Matches: +- `/groups/{groupId} get responses 200 content application/json` +- `/groups/{groupId} get responses 202 content application/json` + +Does not match: +- `/groups/{groupId} get responses 404 content application/json` + +Used by: [xgen-IPA-104-get-method-response-has-no-input-fields](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields), [xgen-IPA-104-get-method-returns-response-suffixed-object](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object), [xgen-IPA-104-get-method-returns-single-resource](https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource) diff --git a/tools/spectral/ipa/rulesets/IPA-104.yaml b/tools/spectral/ipa/rulesets/IPA-104.yaml index f13f83baff..47fd4cf125 100644 --- a/tools/spectral/ipa/rulesets/IPA-104.yaml +++ b/tools/spectral/ipa/rulesets/IPA-104.yaml @@ -22,7 +22,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Only applies to resource collection identifiers - For singleton resources, verifies the resource has a GET method - For regular resources, verifies there is a single resource path with a GET method message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-resource-has-GET' @@ -37,7 +36,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Verifies the response is not an array or paginated result - Different error messages are provided for standard vs singleton resources message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-single-resource' @@ -51,7 +49,6 @@ rules: The Get method must return a 200 OK response. ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies the 200 OK response code is present - Fails if the method lacks a 200 OK response or defines a different 2xx status code message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-code-is-200' @@ -64,7 +61,6 @@ rules: The Get method of a resource should return a "Response" suffixed object. ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Verifies the schema references a predefined schema (not inline) - Confirms the referenced schema name ends with "Response" suffix message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-returns-response-suffixed-object' @@ -78,7 +74,6 @@ rules: The Get method response object must not include writeOnly properties (fields that should be used only on creation or update, ie output fields). ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Searches through the schema to find any properties marked with writeOnly attribute - Fails if any writeOnly properties are found in the response schema message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-response-has-no-input-fields' @@ -92,7 +87,6 @@ rules: The Get method request must not include a body. ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies that the operation object does not contain a requestBody property message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-104-get-method-no-request-body' severity: error @@ -108,7 +102,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Confirms that the existing OperationId is compliant with generated IPA Compliant OperationId ##### Configuration @@ -133,7 +126,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies that the operation ID is not longer than 4 words - If the operation ID is longer than 4 words, requires an 'x-xgen-operation-id-override' extension - Can be exempted using 'x-xgen-IPA-exception' for APIs where long operation IDs are intentional diff --git a/tools/spectral/ipa/rulesets/IPA-113.yaml b/tools/spectral/ipa/rulesets/IPA-113.yaml index eaa966aaa7..e27bd066e3 100644 --- a/tools/spectral/ipa/rulesets/IPA-113.yaml +++ b/tools/spectral/ipa/rulesets/IPA-113.yaml @@ -20,8 +20,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources that are identified as resource collection identifiers - - Checks that the resource has a GET method defined - Examines all 2xx response schemas from the GET method - Verifies that no schema contains 'id' or '_id' properties in their object definitions - Fails if any response schema contains these identifier properties @@ -36,7 +34,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources - Checks that the resource does not have a DELETE method defined message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-must-not-have-delete-method' severity: error @@ -49,8 +46,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources - - Excludes read-only singleton resources (where all properties in the GET response schema are marked as readOnly; for List responses, all properties in the items schema must be readOnly) - Checks that the resource has the PUT and/or PATCH methods defined message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method' severity: error @@ -63,7 +58,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - Verifies that only POST method is defined - Fails if GET or any other HTTP method is used - Fails if multiple HTTP methods are defined for the same :reset endpoint @@ -78,7 +72,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Verifies that the operation object does not contain a requestBody property - Fails if any request body is defined message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-must-not-have-request-body' @@ -92,7 +85,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Verifies that a 200 OK response code is present - Fails if the method lacks a 200 OK response or defines a different 2xx status code - Verifies that the 200 response has a response body with schema @@ -107,8 +99,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - - Applies only to JSON response content types - Verifies that both :reset and GET methods have schema references - Confirms that the :reset method 200 response schema reference matches the GET method response schema reference - Ensures the reset resource returned is the same type as the singleton resource @@ -124,9 +114,7 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - - Verifies that the parent path (without :reset) is a singleton resource - - Uses existing isSingletonResource() helper function + - Verifies that the base resource (the path without :reset) is a singleton resource (see [`resource.isSingleton`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceissingleton)) - Fails if :reset is defined on a non-singleton resource message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-only-on-singleton-resources' severity: error @@ -139,10 +127,8 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - - Verifies that the parent singleton resource is not read-only - - Uses existing isReadOnlyResource() helper function - - Fails if the singleton resource has all properties marked as readOnly: true + - Verifies that the base resource (the path without :reset) is not read-only (see [`resource.isReadOnly`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceisreadonly)) + - Does not check that the base resource is a singleton; that is covered by xgen-IPA-113-reset-method-only-on-singleton-resources message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton' severity: error given: '$.paths[*]' @@ -154,7 +140,6 @@ rules: ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Confirms that the operation ID follows the pattern: reset{ResourceName} - Uses existing operation ID validation infrastructure diff --git a/tools/spectral/ipa/rulesets/README.md b/tools/spectral/ipa/rulesets/README.md index 0816e17fb6..4f8f94a42c 100644 --- a/tools/spectral/ipa/rulesets/README.md +++ b/tools/spectral/ipa/rulesets/README.md @@ -92,10 +92,13 @@ APIs must provide a Get method for resources. ##### Implementation details Rule checks for the following conditions: - - Only applies to resource collection identifiers - For singleton resources, verifies the resource has a GET method - For regular resources, verifies there is a single resource path with a GET method +##### Applies to +Each path, when: + - the path is a resource collection path ([`path.isCollection`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathiscollection)) + #### xgen-IPA-104-get-method-returns-single-resource ![error](https://img.shields.io/badge/error-red) @@ -103,49 +106,73 @@ The purpose of the Get method is to return data from a single resource. ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Verifies the response is not an array or paginated result - Different error messages are provided for standard vs singleton resources +##### Applies to +Each media type of each Get response, when: + - the response is a 2xx response ([`response.is2xx`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#responseis2xx)) + - the media type defines a schema ([`mediaType.hasSchema`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypehasschema)) + - the media type is JSON ([`mediaType.isJson`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypeisjson)) + - the path is a single resource path or a singleton resource ([`path.isGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisgettarget)) + #### xgen-IPA-104-get-method-response-code-is-200 ![error](https://img.shields.io/badge/error-red) The Get method must return a 200 OK response. ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies the 200 OK response code is present - Fails if the method lacks a 200 OK response or defines a different 2xx status code +##### Applies to +Each Get operation, when: + - the path is a single resource path or a singleton resource ([`path.isGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisgettarget)) + #### xgen-IPA-104-get-method-returns-response-suffixed-object ![error](https://img.shields.io/badge/error-red) The Get method of a resource should return a "Response" suffixed object. ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Verifies the schema references a predefined schema (not inline) - Confirms the referenced schema name ends with "Response" suffix +##### Applies to +Each media type of each Get response, when: + - the response is a 2xx response ([`response.is2xx`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#responseis2xx)) + - the media type defines a schema ([`mediaType.hasSchema`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypehasschema)) + - the media type is JSON ([`mediaType.isJson`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypeisjson)) + - the path is a single resource path or a singleton resource ([`path.isGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisgettarget)) + #### xgen-IPA-104-get-method-response-has-no-input-fields ![error](https://img.shields.io/badge/error-red) The Get method response object must not include writeOnly properties (fields that should be used only on creation or update, ie output fields). ##### Implementation details Rule checks for the following conditions: - - Applies only to 2xx responses of GET methods on single resources or singleton resources - Searches through the schema to find any properties marked with writeOnly attribute - Fails if any writeOnly properties are found in the response schema +##### Applies to +Each media type of each Get response, when: + - the response is a 2xx response ([`response.is2xx`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#responseis2xx)) + - the media type defines a schema ([`mediaType.hasSchema`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypehasschema)) + - the media type is JSON ([`mediaType.isJson`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypeisjson)) + - the path is a single resource path or a singleton resource ([`path.isGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisgettarget)) + #### xgen-IPA-104-get-method-no-request-body ![error](https://img.shields.io/badge/error-red) The Get method request must not include a body. ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies that the operation object does not contain a requestBody property +##### Applies to +Each Get operation, when: + - the path is a single resource path or a singleton resource ([`path.isGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisgettarget)) + #### xgen-IPA-104-valid-operation-id ![error](https://img.shields.io/badge/error-red) @@ -156,7 +183,6 @@ For a collection-scoped Operations resource, such as '/resources/operations/{ope ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Confirms that the existing OperationId is compliant with generated IPA Compliant OperationId ##### Configuration @@ -165,6 +191,14 @@ This rule includes three configuration options: - `ignoreSingularizationList`: Words that are allowed to maintain their assumed plurality (e.g., "Fts") - `maxLength`: The maximum number of words allowed in the operation ID override (default: 4) +##### Applies to +Each Get operation, when: + - any of: the path ends in a path parameter or is a singleton resource ([`path.isLegacyGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathislegacygettarget)); the operation is declared the rule's method by `x-xgen-method-verb-override` ([`operation.hasRuleMethodVerbOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhasrulemethodverboverride)) + +Except when: + - the operation is declared a custom method by `x-xgen-method-verb-override` ([`operation.hasCustomMethodOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhascustommethodoverride)) + - the operation is declared a List method by `x-xgen-method-verb-override` ([`operation.hasListVerbOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhaslistverboverride)) + #### xgen-IPA-104-operation-id-length ![error](https://img.shields.io/badge/error-red) @@ -173,7 +207,6 @@ add an 'x-xgen-operation-id-override' extension with a shorter operation ID. ##### Implementation details Rule checks for the following conditions: - - Applies only to GET methods on single resources or singleton resources - Verifies that the operation ID is not longer than 4 words - If the operation ID is longer than 4 words, requires an 'x-xgen-operation-id-override' extension - Can be exempted using 'x-xgen-IPA-exception' for APIs where long operation IDs are intentional @@ -184,6 +217,14 @@ This rule includes three configuration options: - `ignoreSingularizationList`: Words that are allowed to maintain their assumed plurality (e.g., "Fts") - `maxLength`: The maximum number of words allowed in the operation ID (default: 4) +##### Applies to +Each Get operation, when: + - any of: the path ends in a path parameter or is a singleton resource ([`path.isLegacyGetTarget`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathislegacygettarget)); the operation is declared the rule's method by `x-xgen-method-verb-override` ([`operation.hasRuleMethodVerbOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhasrulemethodverboverride)) + +Except when: + - the operation is declared a custom method by `x-xgen-method-verb-override` ([`operation.hasCustomMethodOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhascustommethodoverride)) + - the operation is declared a List method by `x-xgen-method-verb-override` ([`operation.hasListVerbOverride`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#operationhaslistverboverride)) + ### IPA-105 @@ -877,12 +918,15 @@ Singleton resources must not have a user-provided or system-generated ID. ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources that are identified as resource collection identifiers - - Checks that the resource has a GET method defined - Examines all 2xx response schemas from the GET method - Verifies that no schema contains 'id' or '_id' properties in their object definitions - Fails if any response schema contains these identifier properties +##### Applies to +Each path item, when: + - the path is a singleton resource ([`resource.isSingleton`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceissingleton)) + - the path defines a Get method ([`pathItem.hasGet`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathitemhasget)) + #### xgen-IPA-113-singleton-must-not-have-delete-method ![error](https://img.shields.io/badge/error-red) @@ -890,9 +934,12 @@ Singleton resources must not define the Delete standard method. ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources - Checks that the resource does not have a DELETE method defined +##### Applies to +Each path item, when: + - the path is a singleton resource ([`resource.isSingleton`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceissingleton)) + #### xgen-IPA-113-singleton-should-have-update-method ![error](https://img.shields.io/badge/error-red) @@ -900,10 +947,15 @@ Singleton resources should define the Update method. Validation for the presence ##### Implementation details Rule checks for the following conditions: - - Applies only to singleton resources - - Excludes read-only singleton resources (where all properties in the GET response schema are marked as readOnly; for List responses, all properties in the items schema must be readOnly) - Checks that the resource has the PUT and/or PATCH methods defined +##### Applies to +Each path item, when: + - the path is a singleton resource ([`resource.isSingleton`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceissingleton)) + +Except when: + - the resource is read-only ([`resource.isReadOnly`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceisreadonly)) + #### xgen-IPA-113-reset-method-must-use-POST ![error](https://img.shields.io/badge/error-red) @@ -911,11 +963,14 @@ The :reset custom method must use the POST HTTP method. ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - Verifies that only POST method is defined - Fails if GET or any other HTTP method is used - Fails if multiple HTTP methods are defined for the same :reset endpoint +##### Applies to +Each path item, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + #### xgen-IPA-113-reset-method-must-not-have-request-body ![error](https://img.shields.io/badge/error-red) @@ -923,10 +978,13 @@ The :reset custom method must not have a request body. ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Verifies that the operation object does not contain a requestBody property - Fails if any request body is defined +##### Applies to +Each POST operation, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + #### xgen-IPA-113-reset-method-must-return-200-OK ![error](https://img.shields.io/badge/error-red) @@ -934,11 +992,14 @@ The :reset custom method must return a 200 OK response with the reset resource i ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Verifies that a 200 OK response code is present - Fails if the method lacks a 200 OK response or defines a different 2xx status code - Verifies that the 200 response has a response body with schema +##### Applies to +Each POST operation, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + #### xgen-IPA-113-reset-method-response-is-get-method-response ![error](https://img.shields.io/badge/error-red) @@ -946,12 +1007,17 @@ The :reset custom method response must match the GET method response schema. ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - - Applies only to JSON response content types - Verifies that both :reset and GET methods have schema references - Confirms that the :reset method 200 response schema reference matches the GET method response schema reference - Ensures the reset resource returned is the same type as the singleton resource +##### Applies to +Each media type of each POST 200 response, when: + - the media type is JSON ([`mediaType.isJson`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypeisjson)) + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + - the media type defines a schema ([`mediaType.hasSchema`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypehasschema)) + - the custom method's resource has a Get response for the same media type ([`mediaType.baseGetHasMediaType`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#mediatypebasegethasmediatype)) + #### xgen-IPA-113-reset-method-only-on-singleton-resources ![error](https://img.shields.io/badge/error-red) @@ -959,11 +1025,13 @@ The :reset custom method must only be defined on singleton resources. ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - - Verifies that the parent path (without :reset) is a singleton resource - - Uses existing isSingletonResource() helper function + - Verifies that the base resource (the path without :reset) is a singleton resource (see [`resource.isSingleton`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceissingleton)) - Fails if :reset is defined on a non-singleton resource +##### Applies to +Each path item, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + #### xgen-IPA-113-reset-method-not-on-readonly-singleton ![error](https://img.shields.io/badge/error-red) @@ -971,10 +1039,12 @@ Read-only singleton resources must not define a :reset custom method. ##### Implementation details Rule checks for the following conditions: - - Applies only to paths ending with :reset - - Verifies that the parent singleton resource is not read-only - - Uses existing isReadOnlyResource() helper function - - Fails if the singleton resource has all properties marked as readOnly: true + - Verifies that the base resource (the path without :reset) is not read-only (see [`resource.isReadOnly`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#resourceisreadonly)) + - Does not check that the base resource is a singleton; that is covered by xgen-IPA-113-reset-method-only-on-singleton-resources + +##### Applies to +Each path item, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) #### xgen-IPA-113-reset-method-valid-operation-id @@ -983,7 +1053,6 @@ The :reset custom method must have a valid operation ID. ##### Implementation details Rule checks for the following conditions: - - Applies only to POST methods on paths ending with :reset - Confirms that the operation ID follows the pattern: reset{ResourceName} - Uses existing operation ID validation infrastructure @@ -991,6 +1060,10 @@ Rule checks for the following conditions: This rule includes a configuration option: - `ignoreSingularizationList`: Words that are allowed to maintain their assumed plurality (e.g., "Fts") +##### Applies to +Each POST operation, when: + - the path is a `:reset` custom method ([`path.isResetMethod`](https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md#pathisresetmethod)) + ### IPA-114 diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js index 545ec0c15a..df75af50f6 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/operation.js @@ -5,54 +5,84 @@ import { defineCondition } from './registry.js'; import { hasGetMethod } from '../resourceEvaluation.js'; import { hasCustomMethodOverride as hasCustomMethodOverrideExtension, hasMethodVerbOverride } from '../extensions.js'; -const withOperation = (operation, options = {}) => ({ +const withOperation = (operation, note, options = {}) => ({ path: ['paths', '/groups/{groupId}', 'get'], paths: { '/groups/{groupId}': { get: operation } }, options, + note, }); export const hasGet = defineCondition({ id: 'pathItem.hasGet', + label: 'the path defines a Get method', subject: 'pathItem', definition: 'The path item defines a `get` operation.', fn: (ctx) => hasGetMethod(ctx.pathItem), examples: { - match: [{ path: ['paths', '/groups/{groupId}/settings'], paths: { '/groups/{groupId}/settings': { get: {} } } }], + match: [ + { + path: ['paths', '/groups/{groupId}/settings'], + paths: { '/groups/{groupId}/settings': { get: {} } }, + note: 'defines get', + }, + ], noMatch: [ - { path: ['paths', '/groups/{groupId}/settings'], paths: { '/groups/{groupId}/settings': { patch: {} } } }, + { + path: ['paths', '/groups/{groupId}/settings'], + paths: { '/groups/{groupId}/settings': { patch: {} } }, + note: 'patch only', + }, ], }, }); export const hasCustomMethodOverride = defineCondition({ id: 'operation.hasCustomMethodOverride', + label: 'the operation is declared a custom method by `x-xgen-method-verb-override`', subject: 'operation', definition: 'The operation has an `x-xgen-method-verb-override` extension with `customMethod: true`, declaring a legacy custom method that has no `:` in its path.', fn: (ctx) => hasCustomMethodOverrideExtension(ctx.operation), examples: { - match: [withOperation({ 'x-xgen-method-verb-override': { verb: 'search', customMethod: true } })], + match: [ + withOperation( + { 'x-xgen-method-verb-override': { verb: 'search', customMethod: true } }, + 'verb override `search` with customMethod: true' + ), + ], noMatch: [ - withOperation({}), - withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }), + withOperation({}, 'no verb override'), + withOperation( + { 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }, + 'verb override `get` with customMethod: false' + ), ], }, }); export const hasListVerbOverride = defineCondition({ id: 'operation.hasListVerbOverride', + label: 'the operation is declared a List method by `x-xgen-method-verb-override`', subject: 'operation', definition: 'The operation has an `x-xgen-method-verb-override` extension whose verb starts with `list`, declaring it a List method.', fn: (ctx) => hasMethodVerbOverride(ctx.operation, 'list'), examples: { - match: [withOperation({ 'x-xgen-method-verb-override': { verb: 'listByGroup', customMethod: false } })], - noMatch: [withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } })], + match: [ + withOperation( + { 'x-xgen-method-verb-override': { verb: 'listByGroup', customMethod: false } }, + 'verb override `listByGroup`' + ), + ], + noMatch: [ + withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }, 'verb override `get`'), + ], }, }); export const hasRuleMethodVerbOverride = defineCondition({ id: 'operation.hasRuleMethodVerbOverride', + label: "the operation is declared the rule's method by `x-xgen-method-verb-override`", subject: 'operation', definition: "The operation has an `x-xgen-method-verb-override` extension whose verb starts with the rule's `methodName` option (for IPA-104, `get`), declaring it that standard method.", @@ -60,8 +90,12 @@ export const hasRuleMethodVerbOverride = defineCondition({ fn: (ctx) => hasMethodVerbOverride(ctx.operation, ctx.options.methodName), examples: { match: [ - withOperation({ 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }, { methodName: 'get' }), + withOperation( + { 'x-xgen-method-verb-override': { verb: 'get', customMethod: false } }, + 'verb override `get`, rule methodName `get`', + { methodName: 'get' } + ), ], - noMatch: [withOperation({}, { methodName: 'get' })], + noMatch: [withOperation({}, 'no verb override, rule methodName `get`', { methodName: 'get' })], }, }); diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js index c2ae854375..eb4ee25ca5 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/path.js @@ -14,6 +14,7 @@ const at = (resourcePath) => ({ path: ['paths', resourcePath] }); export const isCollection = defineCondition({ id: 'path.isCollection', + label: 'the path is a resource collection path', subject: 'path', definition: 'The path ends in a resource name: either a single top-level segment (`/groups`) or a segment following a path parameter (`/groups/{groupId}/clusters`). This is the path of a resource collection or of a singleton resource.', @@ -30,6 +31,7 @@ export const isCollection = defineCondition({ export const isInstance = defineCondition({ id: 'path.isInstance', + label: 'the path is a single resource (instance) path', subject: 'path', definition: 'The path is a collection path (see `path.isCollection`) followed by a single path parameter, e.g. `/groups/{groupId}`. This is the path of one resource in a collection.', @@ -46,6 +48,7 @@ export const isInstance = defineCondition({ export const isCustomMethod = defineCondition({ id: 'path.isCustomMethod', + label: 'the path is a custom method', subject: 'path', definition: 'The path contains `:`, e.g. `/groups/{groupId}:migrate`. Custom methods declared by the `x-xgen-method-verb-override` extension are not detected (see `operation.hasCustomMethodOverride`).', @@ -58,6 +61,7 @@ export const isCustomMethod = defineCondition({ export const isReset = defineCondition({ id: 'path.isResetMethod', + label: 'the path is a `:reset` custom method', subject: 'path', definition: 'The path is a custom method named `reset`, e.g. `/groups/{groupId}/settings:reset`.', fn: (ctx) => isResetMethod(ctx.resourcePath), diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js index 57606a79b9..71976aa5e5 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/registry.js @@ -14,9 +14,11 @@ * @property {'leaf'|'allOf'|'anyOf'|'not'} op * @property {(ctx: RuleContext) => unknown} [fn] predicate for leaf conditions; reads the rule context * @property {Array} [children] operands for allOf/anyOf/not + * @property {string} [label] short phrase completing "Applies when ...", rendered in the rule docs * @property {string} [subject] what the condition is evaluated on, e.g. 'path', 'resource', 'operation' * @property {string} [definition] plain-English definition, rendered in the glossary - * @property {{match: Array, noMatch: Array}} [examples] rule context fixtures, run by the tests + * @property {{match: Array, noMatch: Array}} [examples] rule context fixtures, run by the tests; each may + * have a `note` saying what makes it (not) match, shown in the glossary when that is not visible in its path * @property {string} [variantOf] id of the condition this one is a divergent variant of * @property {string} [divergence] how this variant differs from variantOf, and where it is used * @property {boolean} [readsRuleOptions] the result depends on the rule's functionOptions, so it has no rule-independent count diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js index 4c9325d32e..1898663867 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/resource.js @@ -25,76 +25,109 @@ function readOnlySchema() { return { properties: { id: { type: 'string', readOnly: true } } }; } -const example = (resourcePath, paths) => ({ path: ['paths', resourcePath], paths }); +const example = (resourcePath, paths, note) => ({ path: ['paths', resourcePath], paths, ...(note && { note }) }); export const hasSingletonShape = defineCondition({ id: 'resource.hasSingletonShape', + label: 'the resource has the singleton shape', subject: 'resource', definition: "The resource's path items contain exactly one collection path, that path belongs to a parent resource (`/parent/{id}/child`), it has no Create (POST) method, and every other path is a custom method. The current path is not required to be the collection path; see `resource.isSingleton`.", fn: (ctx) => isSingletonResource(ctx.resourcePathItems), examples: { - match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { get: {}, patch: {} } })], + match: [ + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { get: {}, patch: {} } }, + 'Get and Patch only' + ), + ], noMatch: [ - example('/groups/{groupId}/clusters', { - '/groups/{groupId}/clusters': { get: {}, post: {} }, - '/groups/{groupId}/clusters/{clusterName}': { get: {} }, - }), - example('/settings', { '/settings': { get: {} } }), + example( + '/groups/{groupId}/clusters', + { + '/groups/{groupId}/clusters': { get: {}, post: {} }, + '/groups/{groupId}/clusters/{clusterName}': { get: {} }, + }, + 'has Create and an instance path' + ), + example('/settings', { '/settings': { get: {} } }, 'no parent resource'), ], }, }); export const isSingleton = defineCondition({ id: 'resource.isSingleton', + label: 'the path is a singleton resource', subject: 'resource', definition: 'The path is a collection path and the resource it names has the singleton shape.', of: allOf(isCollection, hasSingletonShape), examples: { match: [ - example('/groups/{groupId}/settings', { - '/groups/{groupId}/settings': { get: {} }, - '/groups/{groupId}/settings:reset': { post: {} }, - }), + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { get: {} }, '/groups/{groupId}/settings:reset': { post: {} } }, + 'singleton with a custom method' + ), ], noMatch: [ - example('/groups/{groupId}/settings:reset', { - '/groups/{groupId}/settings': { get: {} }, - '/groups/{groupId}/settings:reset': { post: {} }, - }), + example( + '/groups/{groupId}/settings:reset', + { '/groups/{groupId}/settings': { get: {} }, '/groups/{groupId}/settings:reset': { post: {} } }, + 'the custom method path of a singleton, not its collection path' + ), ], }, }); export const hasResourceHasGetException = defineCondition({ id: 'resource.hasResourceHasGetException', + label: 'some path of the resource has an exception for `xgen-IPA-104-resource-has-GET`', subject: 'resource', definition: 'Some path item of the resource carries an `x-xgen-IPA-exception` for `xgen-IPA-104-resource-has-GET`.', fn: (ctx) => anyPathHasException(ctx.resourcePathItems, 'xgen-IPA-104-resource-has-GET'), examples: { match: [ - example('/groups/{groupId}/settings', { - '/groups/{groupId}/settings': { 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' } }, - }), + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' } } }, + 'the path has the exception' + ), + ], + noMatch: [ + example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } }, 'no exception'), ], - noMatch: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], }, }); export const getResponseAllReadOnly = defineCondition({ id: 'resource.getResponseIsAllReadOnly', + label: "all properties of the resource's Get response are read-only", subject: 'resource', definition: "Every property of the 2xx response schema of the resource's Get method is `readOnly: true`. The Get on the instance path is used if there is one; otherwise the first path with a Get, which may be a List, in which case `results.items` is inspected.", fn: (ctx) => getResponseIsAllReadOnly(ctx.resourcePathItems), examples: { - match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], - noMatch: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...writableGet } })], + match: [ + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { ...readOnlyGet } }, + 'every Get response property is readOnly' + ), + ], + noMatch: [ + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { ...writableGet } }, + 'the Get response has a writable property' + ), + ], }, }); export const isReadOnly = defineCondition({ id: 'resource.isReadOnly', + label: 'the resource is read-only', subject: 'resource', definition: 'The resource has a Get method whose response is entirely read-only, and no path of the resource has an exception for `xgen-IPA-104-resource-has-GET`.', @@ -102,37 +135,53 @@ export const isReadOnly = defineCondition({ 'The exception clause is not part of the IPA definition: a resource with an IPA-104 resource-has-GET exception is never read-only, whatever its Get response looks like.', of: allOf(not(hasResourceHasGetException), getResponseAllReadOnly), examples: { - match: [example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { ...readOnlyGet } })], + match: [ + example( + '/groups/{groupId}/settings', + { '/groups/{groupId}/settings': { ...readOnlyGet } }, + 'every Get response property is readOnly' + ), + ], noMatch: [ - example('/groups/{groupId}/settings', { - '/groups/{groupId}/settings': { - ...readOnlyGet, - 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' }, + example( + '/groups/{groupId}/settings', + { + '/groups/{groupId}/settings': { + ...readOnlyGet, + 'x-xgen-IPA-exception': { 'xgen-IPA-104-resource-has-GET': 'reason' }, + }, }, - }), + 'the same read-only Get response, but the path has an xgen-IPA-104-resource-has-GET exception' + ), ], }, }); export const isGetTarget = defineCondition({ id: 'path.isGetTarget', + label: 'the path is a single resource path or a singleton resource', subject: 'path', definition: 'The path is where a standard Get method belongs: an instance path, or the path of a singleton resource.', of: anyOf(isInstance, isSingleton), examples: { match: [ - example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }), - example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { get: {} } }), + example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }, 'instance path'), + example('/groups/{groupId}/settings', { '/groups/{groupId}/settings': { get: {} } }, 'singleton'), ], noMatch: [ - example('/groups', { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } }), - example('/groups/settings/{name}', { '/groups/settings/{name}': { get: {} } }), + example( + '/groups', + { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } }, + 'collection of a standard resource' + ), + example('/groups/settings/{name}', { '/groups/settings/{name}': { get: {} } }, 'not an instance path'), ], }, }); export const isLegacyGetTarget = defineCondition({ id: 'path.isLegacyGetTarget', + label: 'the path ends in a path parameter or is a singleton resource', subject: 'path', variantOf: 'path.isGetTarget', definition: @@ -142,10 +191,20 @@ export const isLegacyGetTarget = defineCondition({ fn: (ctx) => !isInvalidGetMethod(ctx.resourcePath, ctx.resourcePathItems), examples: { match: [ - example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }), - example('/groups/settings/{name}', { '/groups/settings/{name}': { get: {} } }), + example('/groups/{groupId}', { '/groups/{groupId}': { get: {} } }, 'instance path'), + example( + '/groups/settings/{name}', + { '/groups/settings/{name}': { get: {} } }, + 'ends in a path parameter, though not an instance path' + ), + ], + noMatch: [ + example( + '/groups', + { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } }, + 'collection of a standard resource' + ), ], - noMatch: [example('/groups', { '/groups': { get: {}, post: {} }, '/groups/{groupId}': { get: {} } })], }, }); @@ -165,6 +224,7 @@ export function baseGetResponse(ctx) { export const baseGetHasMediaType = defineCondition({ id: 'mediaType.baseGetHasMediaType', + label: "the custom method's resource has a Get response for the same media type", subject: 'mediaType', definition: 'The resource the custom method belongs to (its path without `:`) has a Get method with a 200 response for the same media type as the current response.', @@ -176,12 +236,14 @@ export const baseGetHasMediaType = defineCondition({ paths: { '/groups/{groupId}/settings': { get: { responses: { 200: { content: { 'application/json': {} } } } } }, }, + note: 'the Get on /groups/{groupId}/settings has an application/json 200 response', }, ], noMatch: [ { path: ['paths', '/groups/{groupId}/settings:reset', 'post', 'responses', '200', 'content', 'application/json'], paths: { '/groups/{groupId}/settings': { patch: {} } }, + note: 'the base resource has no Get', }, ], }, diff --git a/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js b/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js index be4f18cdd7..f608f4d269 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js +++ b/tools/spectral/ipa/rulesets/functions/utils/conditions/response.js @@ -4,13 +4,15 @@ import { defineCondition } from './registry.js'; -const content = (code, mediaType, mediaTypeObject) => ({ +const content = (code, mediaType, mediaTypeObject, note) => ({ path: ['paths', '/groups/{groupId}', 'get', 'responses', code, 'content', mediaType], paths: { '/groups/{groupId}': { get: { responses: { [code]: { content: { [mediaType]: mediaTypeObject } } } } } }, + ...(note && { note }), }); export const is2xx = defineCondition({ id: 'response.is2xx', + label: 'the response is a 2xx response', subject: 'response', definition: 'The response status code starts with `2`.', fn: (ctx) => ctx.path[4].startsWith('2'), @@ -22,17 +24,19 @@ export const is2xx = defineCondition({ export const hasSchema = defineCondition({ id: 'mediaType.hasSchema', + label: 'the media type defines a schema', subject: 'mediaType', definition: 'The media type object exists and defines a `schema`.', fn: (ctx) => Boolean(ctx.node?.schema), examples: { - match: [content('200', 'application/json', { schema: { type: 'object' } })], - noMatch: [content('200', 'application/json', { example: {} })], + match: [content('200', 'application/json', { schema: { type: 'object' } }, 'has a schema')], + noMatch: [content('200', 'application/json', { example: {} }, 'an example but no schema')], }, }); export const isJson = defineCondition({ id: 'mediaType.isJson', + label: 'the media type is JSON', subject: 'mediaType', definition: 'The media type ends in `json`, e.g. `application/vnd.atlas.2023-01-01+json`.', fn: (ctx) => ctx.path.at(-1).endsWith('json'), diff --git a/tools/spectral/ipa/scripts/generateRulesetReadme.js b/tools/spectral/ipa/scripts/generateRulesetReadme.js index 560f5a8917..c8abb3bb5f 100644 --- a/tools/spectral/ipa/scripts/generateRulesetReadme.js +++ b/tools/spectral/ipa/scripts/generateRulesetReadme.js @@ -6,6 +6,18 @@ import { loadRuleset } from '../utils.js'; const dirname = path.dirname(fileURLToPath(import.meta.url)); const readmeFilePath = path.join(dirname, '../rulesets', 'README.md'); +const conditionsFilePath = path.join(dirname, '../rulesets', 'CONDITIONS.md'); +const CONDITIONS_URL = 'https://github.com/mongodb/openapi/blob/main/tools/spectral/ipa/rulesets/CONDITIONS.md'; + +// Plain-English names for the `given` of rules built with defineRule, for their Applies to section +const GIVEN_DESCRIPTIONS = { + '$.paths': 'each path', + '$.paths[*]': 'each path item', + '$.paths[*].post': 'each POST operation', + '#GetOperationObject': 'each Get operation', + '#GetOperationObject.responses[*].content': 'each media type of each Get response', + '$.paths[*].post.responses.200.content': 'each media type of each POST 200 response', +}; const rulesetsSection = await getRulesetsSection(); @@ -23,6 +35,13 @@ fs.writeFile(readmeFilePath, fileContent, (error) => { } }); +fs.writeFile(conditionsFilePath, await getConditionsFileContent(), (error) => { + if (error) { + console.error('Error while generating the IPA rulesets CONDITIONS.md:', error); + process.exit(1); + } +}); + async function getRulesetsSection() { let content = '## Rulesets\n\n' + 'Below is a list of all available rules, their descriptions and severity levels.\n\n'; @@ -48,6 +67,8 @@ function generateRulesetSections(rules) { name: ruleName, description: rules[ruleName].description, severity: rules[ruleName].definition.severity, + given: rules[ruleName].definition.given, + meta: rules[ruleName].then[0]?.function?.meta, })) .sort((a, b) => { if (a.severity < b.severity) { @@ -61,6 +82,9 @@ function generateRulesetSections(rules) { sortedRuleEntries.forEach((rule) => { const severityFormatted = formatSeverity(rule.severity); sections += `#### ${rule.name}\n\n ${severityFormatted} \n${rule.description}\n`; + if (rule.meta) { + sections += getAppliesToSection(rule); + } }); return sections; @@ -115,3 +139,100 @@ function filterRulesByIpaNumber(ipaNumber, rules) { }; }, {}); } + +// ---------- Rules built with defineRule: generated from the rule's conditions ---------- + +function getAppliesToSection(rule) { + const given = [rule.given].flat().map((g) => GIVEN_DESCRIPTIONS[g] ?? `\`${g}\``); + const subject = given.join(', ').replace(/^./, (c) => c.toUpperCase()); + // not(...) conditions read better as a separate list than as "when: unless ..." + const required = rule.meta.appliesTo.filter((c) => c.op !== 'not' || c.label); + const excluded = rule.meta.appliesTo.filter((c) => c.op === 'not' && !c.label).map((c) => c.children[0]); + let section = `##### Applies to\n${subject}${required.length ? ', when:' : ''}\n`; + required.forEach((condition) => { + section += ` - ${describeCondition(condition)}\n`; + }); + if (excluded.length) { + section += `${required.length ? '\nExcept' : 'Except'} when:\n`; + excluded.forEach((condition) => { + section += ` - ${describeCondition(condition)}\n`; + }); + } + return section + '\n'; +} + +// Named conditions link to the glossary (in-page when rendering the glossary itself); combinations are spelled out +function describeCondition(condition, glossaryUrl = CONDITIONS_URL) { + const children = condition.children ?? []; + if (condition.label) { + return `${condition.label} ([\`${condition.id}\`](${glossaryUrl}#${anchor(condition.id)}))`; + } + switch (condition.op) { + case 'not': + return `not: ${describeCondition(children[0], glossaryUrl)}`; + case 'anyOf': + return `any of: ${children.map((c) => describeCondition(c, glossaryUrl)).join('; ')}`; + case 'allOf': + return `all of: ${children.map((c) => describeCondition(c, glossaryUrl)).join('; ')}`; + default: + return `\`${condition.id}\``; + } +} + +function anchor(conditionId) { + return conditionId.toLowerCase().replace(/[^a-z0-9-]/g, ''); +} + +async function getConditionsFileContent() { + const rules = await getAllRules(); + const named = new Map(); + const usedBy = new Map(); + const collect = (condition, ruleName) => { + if (condition.label) { + named.set(condition.id, condition); + if (!usedBy.has(condition.id)) { + usedBy.set(condition.id, new Set()); + } + usedBy.get(condition.id).add(ruleName); + } + (condition.children ?? []).forEach((child) => collect(child, ruleName)); + }; + Object.entries(rules).forEach(([ruleName, rule]) => { + const meta = rule.then[0]?.function?.meta; + [...(meta?.appliesTo ?? []), ...(meta?.checkUses ?? [])].forEach((condition) => collect(condition, ruleName)); + }); + + let content = + '\n\n' + + '# IPA Validation Conditions\n\n' + + 'Named conditions that decide where IPA rules apply. They are defined in ' + + '[rulesets/functions/utils/conditions](https://github.com/mongodb/openapi/tree/main/tools/spectral/ipa/rulesets/functions/utils/conditions) ' + + 'and used by rules built with `defineRule`. Only conditions used by at least one rule are listed.\n\n'; + + const sorted = [...named.values()].sort((a, b) => a.id.localeCompare(b.id)); + sorted.forEach((condition) => { + content += `### ${condition.id}\n\n`; + content += `Evaluated on: ${condition.subject ?? 'n/a'} + +${condition.definition}\n\n`; + if (condition.children?.length) { + content += `Defined as: ${describeCondition({ ...condition, label: undefined }, '')}\n\n`; + } + if (condition.variantOf) { + content += `> **Variant of [\`${condition.variantOf}\`](#${anchor(condition.variantOf)}).** ${condition.divergence}\n\n`; + } else if (condition.divergence) { + content += `> **Divergence.** ${condition.divergence}\n\n`; + } + // An example's note says what makes it (not) match when that is not visible in the path + const examples = condition.examples; + if (examples) { + const describeExample = (example) => + `\`${example.path.slice(1).join(' ')}\`${example.note ? ` (${example.note})` : ''}`; + content += `Matches:\n${examples.match.map((e) => `- ${describeExample(e)}`).join('\n')}\n\n`; + content += `Does not match:\n${examples.noMatch.map((e) => `- ${describeExample(e)}`).join('\n')}\n\n`; + } + const ruleNames = [...usedBy.get(condition.id)].sort(); + content += `Used by: ${ruleNames.map((name) => `[${name}](https://mdb.link/mongodb-atlas-openapi-validation#${name})`).join(', ')}\n\n`; + }); + return content.trimEnd() + '\n'; +}