From 6bf201d66c08fdc1dffcca8e1ffffdd5804b4c8b Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Tue, 8 Sep 2026 17:12:13 +0000 Subject: [PATCH 1/8] fix(shared): resolve prefixItems alternatives in pattern validation A `prefixItems` entry is either a node or a `oneOf`/`anyOf` holding alternatives. Each rule handled that ambiguity differently, and four handled it wrongly or not at all. `declaration-paths.ts` now answers where a pattern declares a node, so the rules resolve declarations rather than entries. Four gaps close. Duplicate ids across alternatives are reported. Interfaces on an alternative are checked, and are no longer pooled across the alternatives of one entry. An unreferenced alternative is reported. An entry declaring both `oneOf` and `anyOf` is rejected: JSON Schema requires an element to satisfy both, so some alternatives can never be selected. Closes #3058 Closes #3062 Closes #3068 Closes #3069 --- PATTERN-DECISIONS.md | 63 ++++++ cli/test_fixtures/validate_output_junit.xml | 5 +- .../functions/pattern/declaration-paths.ts | 23 ++ .../functions/pattern/ids-are-unique.spec.ts | 210 ++++++++++++++++++ .../functions/pattern/ids-are-unique.ts | 14 +- .../interface-id-exists-on-node.spec.ts | 39 ++++ .../pattern/interface-id-exists-on-node.ts | 13 +- .../functions/pattern/node-id-exists.spec.ts | 50 +++++ .../functions/pattern/node-id-exists.ts | 10 +- .../prefix-items-declares-one-keyword.spec.ts | 50 +++++ .../prefix-items-declares-one-keyword.ts | 38 ++++ shared/src/spectral/rules-pattern.spec.ts | 141 ++++++++++++ shared/src/spectral/rules-pattern.ts | 13 +- 13 files changed, 647 insertions(+), 22 deletions(-) create mode 100644 PATTERN-DECISIONS.md create mode 100644 shared/src/spectral/functions/pattern/declaration-paths.ts create mode 100644 shared/src/spectral/functions/pattern/node-id-exists.spec.ts create mode 100644 shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts create mode 100644 shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts create mode 100644 shared/src/spectral/rules-pattern.spec.ts diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md new file mode 100644 index 0000000000..dbf8136b4e --- /dev/null +++ b/PATTERN-DECISIONS.md @@ -0,0 +1,63 @@ +# Pattern decisions + +A CALM pattern can offer a choice. This document records what each tool guarantees about +that choice. It describes behaviour only. It does not describe how a tool is built. + +Each section names the tests that hold its guarantees. A guarantee below with no test is a +gap. + +## Terms + +| Term | Meaning | +|---|---| +| alternative | One entry in a `oneOf` or an `anyOf` array. | +| decision | A relationship that carries `relationship-type.properties.options`. A decision asks which alternatives to include. | + +A pattern declares a node at three kinds of site: + +| Site | Meaning | +|---|---| +| a `prefixItems` entry | one node, at that position | +| `prefixItems[i].oneOf` | alternatives for that position | +| `prefixItems[i].anyOf` | alternatives for that position | + +A pattern declares a relationship at the same three sites. + +## Rules that hold across all tools + +A decision names a node or a relationship by its `unique-id`. Two declarations must +therefore never share a `unique-id`. If they did, no answer could select one and not the +other. + +Declare one keyword, not both. An element must satisfy every keyword declared beside it, so +declaring both `oneOf` and `anyOf` makes some alternatives impossible to select. + +Neither keyword controls how many alternatives an architecture includes. A `prefixItems` +entry is one position, so it takes one alternative. `minItems` and `maxItems` on the array +set the bounds. + +`oneOf` and `anyOf` do not differ for CALM alternatives. Each alternative pins a distinct +`unique-id`, so an element matches at most one of them, and "exactly one" and "at least one" +become the same test. The visualiser prints the keyword as the label on the decision box, so +the choice is visible to a reader. It changes no validation. + +## What validation guarantees + +Tests: [`shared/src/spectral/rules-pattern.spec.ts`](shared/src/spectral/rules-pattern.spec.ts) +and the rule tests beside it in `shared/src/spectral/functions/pattern/`. + +`calm validate` reads every node and every relationship a pattern declares. It reads all +three declaration sites listed above. + +`calm validate` reports these faults: + +| Fault | Severity | +|---|---| +| Two declarations share a `unique-id` | error | +| A relationship refers to a node that the pattern does not declare | error | +| A connects relationship refers to an interface that the named node does not declare | error | +| A `prefixItems` entry declares both `oneOf` and `anyOf` | error | +| No relationship and no decision refers to a declared node | warning | + +A pattern that declares alternatives inside an `allOf` branch is not supported. Two `allOf` +branches that declare the same property discard one of the two declarations. diff --git a/cli/test_fixtures/validate_output_junit.xml b/cli/test_fixtures/validate_output_junit.xml index da3078e739..a482fbe737 100644 --- a/cli/test_fixtures/validate_output_junit.xml +++ b/cli/test_fixtures/validate_output_junit.xml @@ -1,10 +1,10 @@ - + - @@ -41,6 +41,7 @@ + \ No newline at end of file diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts new file mode 100644 index 0000000000..4a7aac3d68 --- /dev/null +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -0,0 +1,23 @@ +import { JSONPath } from 'jsonpath-plus'; + +export type CalmType = 'nodes' | 'relationships'; + +/** + * Shared so that the rules resolving declarations cannot disagree about where they are. + */ +export function declarationPaths(calmType: CalmType): string[] { + const entry = `$.properties.${calmType}.prefixItems[*]`; + return [entry, `${entry}.oneOf[*]`, `${entry}.anyOf[*]`]; +} + +export function declaredIdPaths(calmType: CalmType): string[] { + return declarationPaths(calmType).map(path => `${path}.properties.unique-id.const`); +} + +export function declaredInterfaceIdPaths(): string[] { + return declarationPaths('nodes').map(path => `${path}.properties.interfaces.prefixItems[*].properties.unique-id.const`); +} + +export function declaredId(declaration: object): string | undefined { + return JSONPath({ path: '$.properties.unique-id.const', json: declaration })[0]; +} diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts index 91f27e3b64..3680211643 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts @@ -147,4 +147,214 @@ describe('idsAreUnique', () => { expect(result.length).toBeGreaterThan(0); expect(result[0].message).toContain('Duplicate unique-id detected. ID: node1, path: /properties/relationships/prefixItems/0/properties/unique-id/const'); }); + + it('should return an empty array when alternatives in a prefixItems entry have distinct IDs', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [ + { 'properties': { 'unique-id': { 'const': 'node1' } } }, + { 'properties': { 'unique-id': { 'const': 'node2' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result).toEqual([]); + }); + + it('should return messages for duplicate IDs across oneOf alternatives of a nodes prefixItems entry', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [ + { 'properties': { 'unique-id': { 'const': 'node1' } } }, + { 'properties': { 'unique-id': { 'const': 'node1' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: node1, path: /properties/nodes/prefixItems/0/oneOf/1/properties/unique-id/const'); + }); + + it('should return messages for duplicate IDs across anyOf alternatives of a nodes prefixItems entry', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'anyOf': [ + { 'properties': { 'unique-id': { 'const': 'node1' } } }, + { 'properties': { 'unique-id': { 'const': 'node1' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: node1, path: /properties/nodes/prefixItems/0/anyOf/1/properties/unique-id/const'); + }); + + it('should return messages when an alternative reuses the ID of a mandatory node', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'properties': { 'unique-id': { 'const': 'node1' } } }, + { 'oneOf': [ + { 'properties': { 'unique-id': { 'const': 'node1' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: node1, path: /properties/nodes/prefixItems/1/oneOf/0/properties/unique-id/const'); + }); + + it('should return messages for duplicate IDs across oneOf alternatives of a relationships prefixItems entry', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + relationships: { + prefixItems: [ + { 'oneOf': [ + { 'properties': { 'unique-id': { 'const': 'rel1' } } }, + { 'properties': { 'unique-id': { 'const': 'rel1' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: rel1, path: /properties/relationships/prefixItems/0/oneOf/1/properties/unique-id/const'); + }); + + it('should return messages for duplicate IDs across anyOf alternatives of a relationships prefixItems entry', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + relationships: { + prefixItems: [ + { 'anyOf': [ + { 'properties': { 'unique-id': { 'const': 'rel1' } } }, + { 'properties': { 'unique-id': { 'const': 'rel1' } } } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: rel1, path: /properties/relationships/prefixItems/0/anyOf/1/properties/unique-id/const'); + }); + + it('should return messages for duplicate interface IDs on oneOf alternatives', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [ + { 'properties': { + 'unique-id': { 'const': 'node1' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + }, + { 'properties': { + 'unique-id': { 'const': 'node2' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/oneOf/1/properties/interfaces/prefixItems/0/properties/unique-id/const'); + }); + + it('should return messages for duplicate interface IDs on anyOf alternatives', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'anyOf': [ + { 'properties': { + 'unique-id': { 'const': 'node1' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + }, + { 'properties': { + 'unique-id': { 'const': 'node2' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + } + ] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/anyOf/1/properties/interfaces/prefixItems/0/properties/unique-id/const'); + }); + + + + + + + }); \ No newline at end of file diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.ts b/shared/src/spectral/functions/pattern/ids-are-unique.ts index 7a04b2400f..84706b4cd5 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -1,17 +1,21 @@ import { JSONPath } from 'jsonpath-plus'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; import { detectDuplicates } from '../helper-functions'; +import { declaredIdPaths, declaredInterfaceIdPaths } from './declaration-paths'; + /** - * Checks that the input value exists as a node with a matching unique ID. + * Reports any unique-id a pattern declares more than once. */ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IFunctionResult[] => { if (!input) { return []; } - // get uniqueIds of all nodes - const nodeIdMatches = JSONPath({ path: '$.properties.nodes.prefixItems[*].properties.unique-id.const', json: context.document.data as object, resultType: 'all' }); - const relationshipIdMatches = JSONPath({ path: '$.properties.relationships.prefixItems[*].properties.unique-id.const', json: context.document.data as object, resultType: 'all' }); - const interfaceIdMatches = JSONPath({ path: '$.properties.nodes.prefixItems[*].properties.interfaces.prefixItems[*].properties.unique-id.const', json: context.document.data as object, resultType: 'all' }); + const collect = (paths: string[]) => paths.flatMap(path => + JSONPath({ path, json: context.document.data as object, resultType: 'all' })); + + const nodeIdMatches = collect(declaredIdPaths('nodes')); + const relationshipIdMatches = collect(declaredIdPaths('relationships')); + const interfaceIdMatches = collect(declaredInterfaceIdPaths()); const seenIds = new Set(); diff --git a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts index 44218b4f38..3aa06e7ed3 100644 --- a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts +++ b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts @@ -159,4 +159,43 @@ describe('interfaceIdExistsOnNode', () => { expect(result[0].message).toBe(`Referenced interface with ID '${input.interfaces[1]}' was not defined on the node with ID '${input.node}'.`); expect(result[0].path).toEqual(['/relationships/0/connects/destination']); }); + + + + + it('should check an alternative that is not the first in its prefixItems entry', () => { + const input = { node: 'queue', interfaces: ['missing-port'] }; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { + oneOf: [ + { properties: { 'unique-id': { const: 'cache' } } }, + { + properties: { + 'unique-id': { const: 'queue' }, + 'interfaces': { + prefixItems: [ + { properties: { 'unique-id': { const: 'queue-port' } } } + ] + } + } + } + ] + } + ] + } + } + } + }, + path: ['/relationships/0/connects/destination'] + }; + + const result = interfaceIdExistsOnNode(input, null, asContext(context)); + expect(result.length).toBe(1); + expect(result[0].message).toBe('Referenced interface with ID \'missing-port\' was not defined on the node with ID \'queue\'.'); + }); }); \ No newline at end of file diff --git a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts index 19c765bdfc..cbc6a7110d 100644 --- a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts +++ b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts @@ -1,6 +1,7 @@ import { JSONPath } from 'jsonpath-plus'; import { difference } from 'lodash'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; +import { declarationPaths, declaredId } from './declaration-paths'; interface ConnectsRelationship { node?: string; @@ -23,13 +24,9 @@ export function interfaceIdExistsOnNode(input: ConnectsRelationship | null | und } const nodeId = input.node; - const nodes: object[] = JSONPath({ path: '$.properties.nodes.prefixItems[*]', json: context.document.data as object }); - const node = nodes.find((node) => { - const uniqueId: string[] = JSONPath({ path: '$.properties.unique-id.const', json: node }); - uniqueId.push(...JSONPath({ path: '$.oneOf[*].properties.unique-id.const', json: node })); - uniqueId.push(...JSONPath({ path: '$.anyOf[*].properties.unique-id.const', json: node })); - return uniqueId && uniqueId[0] === nodeId; - }); + const nodes: object[] = declarationPaths('nodes').flatMap(path => + JSONPath({ path, json: context.document.data as object })); + const node = nodes.find(declaration => declaredId(declaration) === nodeId); if (!node) { // other rule will report undefined node return []; @@ -39,8 +36,6 @@ export function interfaceIdExistsOnNode(input: ConnectsRelationship | null | und const desiredInterfaces = input.interfaces; const nodeInterfaces = JSONPath({ path: '$.properties.interfaces.prefixItems[*].properties.unique-id.const', json: node }); - nodeInterfaces.push(...JSONPath({ path: '$.oneOf[*].properties.interfaces.prefixItems[*].properties.unique-id.const', json: node })); - nodeInterfaces.push(...JSONPath({ path: '$.anyOf[*].properties.interfaces.prefixItems[*].properties.unique-id.const', json: node })); if (!nodeInterfaces || nodeInterfaces.length === 0) { return [ { message: `Node with unique-id ${nodeId} has no interfaces defined, expected interfaces [${desiredInterfaces}]` } diff --git a/shared/src/spectral/functions/pattern/node-id-exists.spec.ts b/shared/src/spectral/functions/pattern/node-id-exists.spec.ts new file mode 100644 index 0000000000..852381a64b --- /dev/null +++ b/shared/src/spectral/functions/pattern/node-id-exists.spec.ts @@ -0,0 +1,50 @@ +import { asContext } from '../spectral-test-helpers'; +import nodeIdExists from './node-id-exists'; + +function contextFor(nodes: object) { + return { + document: { data: { properties: { nodes } } }, + path: ['properties', 'relationships'] + }; +} + +const node = (id: string) => ({ 'properties': { 'unique-id': { 'const': id } } }); + +describe('nodeIdExists', () => { + it('should return an empty array when there is no input', () => { + const result = nodeIdExists(null, null, asContext(contextFor({}))); + expect(result).toEqual([]); + }); + + it('should return an empty array when the input is not a string', () => { + const result = nodeIdExists({}, null, asContext(contextFor({}))); + expect(result).toEqual([]); + }); + + it('should accept a node declared as a bare prefixItems entry', () => { + const result = nodeIdExists('webapp', null, asContext(contextFor({ prefixItems: [node('webapp')] }))); + expect(result).toEqual([]); + }); + + it('should accept a node declared as a oneOf alternative', () => { + const nodes = { prefixItems: [{ 'oneOf': [node('cache'), node('queue')] }] }; + const result = nodeIdExists('queue', null, asContext(contextFor(nodes))); + expect(result).toEqual([]); + }); + + it('should accept a node declared as an anyOf alternative', () => { + const nodes = { prefixItems: [{ 'anyOf': [node('cache'), node('queue')] }] }; + const result = nodeIdExists('queue', null, asContext(contextFor(nodes))); + expect(result).toEqual([]); + }); + + + + + it('should return a message when the node does not exist', () => { + const nodes = { prefixItems: [node('webapp'), { 'oneOf': [node('cache')] }] }; + const result = nodeIdExists('typo', null, asContext(contextFor(nodes))); + expect(result.length).toBe(1); + expect(result[0].message).toEqual('\'typo\' does not refer to the unique-id of an existing node.'); + }); +}); diff --git a/shared/src/spectral/functions/pattern/node-id-exists.ts b/shared/src/spectral/functions/pattern/node-id-exists.ts index b34820b48c..d955cef888 100644 --- a/shared/src/spectral/functions/pattern/node-id-exists.ts +++ b/shared/src/spectral/functions/pattern/node-id-exists.ts @@ -1,5 +1,7 @@ import { JSONPath } from 'jsonpath-plus'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; +import { declaredIdPaths } from './declaration-paths'; + /** * Checks that the input value exists as a node with a matching unique ID. */ @@ -8,14 +10,12 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF return []; } - const names = JSONPath({ path: '$.properties.nodes.prefixItems[*].properties.unique-id.const', json: context.document.data as object }); - const oneofs = JSONPath({ path: '$.properties.nodes.prefixItems[*].oneOf[*].properties.unique-id.const', json: context.document.data as object }); - const anyofs = JSONPath({ path: '$.properties.nodes.prefixItems[*].anyOf[*].properties.unique-id.const', json: context.document.data as object }); + const declaredIds = declaredIdPaths('nodes').flatMap(path => + JSONPath({ path, json: context.document.data as object })); - // get uniqueIds of all nodes const results: IFunctionResult[] = []; - if (!names.includes(input) && !oneofs.includes(input) && !anyofs.includes(input)) { + if (!declaredIds.includes(input)) { results.push({ message: `'${input}' does not refer to the unique-id of an existing node.`, path: [...context.path], diff --git a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts new file mode 100644 index 0000000000..1f5f84a0b9 --- /dev/null +++ b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts @@ -0,0 +1,50 @@ +import { asContext } from '../spectral-test-helpers'; +import { prefixItemsDeclaresOneKeyword } from './prefix-items-declares-one-keyword'; + +const node = (id: string) => ({ properties: { 'unique-id': { const: id } } }); + +function contextFor(nodes: object) { + return { + document: { data: { properties: { nodes } } }, + path: [] + }; +} + +describe('prefixItemsDeclaresOneKeyword', () => { + it('should return an empty array when there is no input', () => { + const context = contextFor({ prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }); + + const result = prefixItemsDeclaresOneKeyword(null, null, asContext(context)); + expect(result).toEqual([]); + }); + + it('should report an entry whose keywords name different alternatives', () => { + const context = contextFor({ prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }); + + const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); + expect(result.length).toBe(1); + expect(result[0].message).toContain('A prefixItems entry declares both \'oneOf\' and \'anyOf\''); + expect(result[0].path).toEqual(['properties', 'nodes', 'prefixItems', '0']); + }); + + it('should report an entry whose keywords overlap', () => { + const context = contextFor({ prefixItems: [{ oneOf: [node('cache'), node('queue')], anyOf: [node('cache')] }] }); + + const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); + expect(result.length).toBe(1); + }); + + it('should return an empty array when an entry declares one keyword', () => { + const context = contextFor({ prefixItems: [{ oneOf: [node('cache'), node('queue')] }] }); + + const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); + expect(result).toEqual([]); + }); + + it('should return an empty array for a bare node entry', () => { + const context = contextFor({ prefixItems: [node('webapp')] }); + + const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); + expect(result).toEqual([]); + }); +}); diff --git a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts new file mode 100644 index 0000000000..4b25da1c01 --- /dev/null +++ b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts @@ -0,0 +1,38 @@ +import { JSONPath } from 'jsonpath-plus'; +import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; + +const ENTRIES = [ + '$.properties.nodes.prefixItems[*]', + '$.properties.relationships.prefixItems[*]', +]; + +interface Match { + value: Record; + pointer: string; +} + +/** + * Spectral needs a segment array. A pointer string reports at the document root. + */ +function pointerToPath(pointer: string): string[] { + return pointer.split('/').slice(1); +} + +/** + * Reports each `prefixItems` entry that declares both `oneOf` and `anyOf`. + */ +export function prefixItemsDeclaresOneKeyword(input: unknown, _: unknown, context: RulesetFunctionContext): IFunctionResult[] { + if (!input) { + return []; + } + + const matches: Match[] = ENTRIES.flatMap(path => + JSONPath({ path, json: context.document.data as object, resultType: 'all' })); + + return matches + .filter(match => Array.isArray(match.value?.['oneOf']) && Array.isArray(match.value?.['anyOf'])) + .map(match => ({ + message: 'A prefixItems entry declares both \'oneOf\' and \'anyOf\'. An element must satisfy both, so some alternatives can never be selected. Declare one keyword.', + path: pointerToPath(match.pointer), + })); +} diff --git a/shared/src/spectral/rules-pattern.spec.ts b/shared/src/spectral/rules-pattern.spec.ts new file mode 100644 index 0000000000..98533acde1 --- /dev/null +++ b/shared/src/spectral/rules-pattern.spec.ts @@ -0,0 +1,141 @@ +import patternRules from './rules-pattern'; +import { runSpectralValidations } from '../commands/validate/validation-helpers'; + +async function issuesFor(pattern: object) { + const result = await runSpectralValidations(JSON.stringify(pattern), patternRules, 'test'); + return result.spectralIssues; +} + +async function codesFor(pattern: object): Promise { + return (await issuesFor(pattern)).map(issue => String(issue.code)); +} + +async function pathFor(pattern: object, code: string): Promise { + return (await issuesFor(pattern)).find(issue => String(issue.code) === code)?.path; +} + +const node = (id: string) => ({ properties: { 'unique-id': { const: id } } }); + +const connects = (id: string, source: string, destination: string) => ({ + properties: { + 'unique-id': { const: id }, + 'relationship-type': { const: { connects: { source: { node: source }, destination: { node: destination } } } } + } +}); + +const decision = (id: string, nodeIds: string[]) => ({ + properties: { + 'unique-id': { const: id }, + description: { const: 'Pick an alternative' }, + 'relationship-type': { + properties: { + options: { + prefixItems: [{ + anyOf: nodeIds.map(nodeId => ({ + properties: { + description: { const: `Use ${nodeId}` }, + nodes: { const: [nodeId] }, + relationships: { const: [] } + } + })) + }] + } + } + } + } +}); + +describe('pattern ruleset', () => { + it('runs the ruleset at all', async () => { + const codes = await codesFor({ properties: {} }); + expect(codes).toContain('pattern-has-nodes-relationships'); + }); + + describe('pattern-nodes-must-be-referenced', () => { + it('does not warn about an alternative that a decision references', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), node('database'), { oneOf: [node('redis'), node('memcached')] }] }, + relationships: { + prefixItems: [connects('w-d', 'webapp', 'database'), decision('pick', ['redis', 'memcached'])] + } + } + }); + expect(codes).not.toContain('pattern-nodes-must-be-referenced'); + }); + + it('warns about an alternative that nothing references', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), node('database'), { oneOf: [node('orphan')] }] }, + relationships: { prefixItems: [connects('w-d', 'webapp', 'database')] } + } + }); + expect(codes).toContain('pattern-nodes-must-be-referenced'); + }); + }); + + describe('pattern-prefix-items-must-declare-one-keyword', () => { + it('accepts an entry that declares one keyword', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [{ oneOf: [node('cache'), node('queue')] }] }, + relationships: { prefixItems: [] } + } + }); + expect(codes).not.toContain('pattern-prefix-items-must-declare-one-keyword'); + }); + + it('rejects an entry that declares both keywords', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }, + relationships: { prefixItems: [] } + } + }); + expect(codes).toContain('pattern-prefix-items-must-declare-one-keyword'); + }); + + it('rejects a relationships entry that declares both keywords', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')] }, + relationships: { prefixItems: [{ oneOf: [node('a')], anyOf: [node('b')] }] } + } + }); + expect(codes).toContain('pattern-prefix-items-must-declare-one-keyword'); + }); + + it('reports the location of the offending entry', async () => { + const path = await pathFor({ + properties: { + nodes: { prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }, + relationships: { prefixItems: [] } + } + }, 'pattern-prefix-items-must-declare-one-keyword'); + expect(path).toBe('/properties/nodes/prefixItems/0'); + }); + }); + + describe('rules resolve alternatives, not prefixItems entries', () => { + it('does not report a connects relationship whose source is an alternative', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), { oneOf: [node('cache')] }] }, + relationships: { prefixItems: [connects('c-w', 'cache', 'webapp')] } + } + }); + expect(codes).not.toContain('connects-relationship-references-existing-nodes-in-pattern'); + }); + + it('reports a connects relationship whose source is an unknown node', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), { oneOf: [node('cache')] }] }, + relationships: { prefixItems: [connects('t-w', 'typo', 'webapp')] } + } + }); + expect(codes).toContain('connects-relationship-references-existing-nodes-in-pattern'); + }); + }); +}); diff --git a/shared/src/spectral/rules-pattern.ts b/shared/src/spectral/rules-pattern.ts index 8bec5c21db..3aba7cab08 100644 --- a/shared/src/spectral/rules-pattern.ts +++ b/shared/src/spectral/rules-pattern.ts @@ -7,6 +7,8 @@ import nodeHasRelationship from './functions/pattern/node-has-relationship'; import { interfaceIdExists } from './functions/pattern/interface-id-exists'; import { interfaceIdExistsOnNode } from './functions/pattern/interface-id-exists-on-node'; import { isDefinedInOneOfOrAnyOf } from './functions/pattern/is-defined-in-oneof-or-anyof'; +import { prefixItemsDeclaresOneKeyword } from './functions/pattern/prefix-items-declares-one-keyword'; +import { declaredIdPaths } from './functions/pattern/declaration-paths'; const patternRules: RulesetDefinition = { @@ -134,7 +136,7 @@ const patternRules: RulesetDefinition = { description: 'Nodes must be referenced by at least one relationship', severity: 'warn', message: '{{error}}', - given: '$.properties.nodes.prefixItems[*].properties.unique-id.const', + given: declaredIdPaths('nodes'), then: { function: nodeHasRelationship, }, @@ -187,6 +189,15 @@ const patternRules: RulesetDefinition = { }, }, }, + 'pattern-prefix-items-must-declare-one-keyword': { + description: 'A prefixItems entry must declare either oneOf or anyOf, not both', + severity: 'error', + message: '{{error}}', + given: '$', + then: { + function: prefixItemsDeclaresOneKeyword, + }, + }, 'pattern-option-relationship-must-have-max-one-item': { description: 'Options relationships must have max one item', severity: 'error', From 95da2710a06ec3cbe8517485948b6d9aace907a1 Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Thu, 10 Sep 2026 19:07:15 +0000 Subject: [PATCH 2/8] fix(shared): scope id uniqueness to declarations that can coexist Interface ids are compared only across node declarations that can appear in one architecture. Alternatives of one prefixItems entry never appear together, so they may repeat an interface id. An entry's own properties is not an alternative of its own oneOf, so those two are still compared. The two-keyword check becomes a filtered given with the built-in falsy, which deletes the custom function and its hand-written pointer conversion. Spectral's xor cannot express it, because an entry may legitimately declare neither keyword. is-defined-in-oneof-or-anyof now takes its queries from declaration-paths, so no rule keeps its own copies of the declaration sites. PATTERN-DECISIONS.md records the id contract and the one-level depth limit on alternatives. BREAKING CHANGE: calm validate rejects patterns it previously accepted. Main never read inside oneOf or anyOf, so four checks now apply where they did not. Two alternatives of one entry that share a node or relationship unique-id are rejected. Generate previously emitted both alternatives, producing an architecture with a duplicate id. A relationship that names an interface belonging to a sibling alternative is rejected. Interfaces were pooled across an entry's alternatives, so the chosen node need not have declared the interface. An entry that declares both oneOf and anyOf is rejected. An element must satisfy both, so the entry is unsatisfiable when the id sets are disjoint and declares unselectable alternatives when they overlap. An alternative that no relationship and no decision references is warned about. This fails a build only under --strict. One further rejection is a deliberate contract rather than a correction. A unique-id names one kind of thing, so a node id may not equal an interface id even across alternatives that never appear together. Both architectures such a pattern can produce are valid. --- PATTERN-DECISIONS.md | 21 ++- .../functions/pattern/ids-are-unique.spec.ts | 164 ++++++++++++++++-- .../functions/pattern/ids-are-unique.ts | 48 ++++- .../pattern/is-defined-in-oneof-or-anyof.ts | 12 +- .../prefix-items-declares-one-keyword.spec.ts | 50 ------ .../prefix-items-declares-one-keyword.ts | 38 ---- shared/src/spectral/rules-pattern.ts | 12 +- 7 files changed, 228 insertions(+), 117 deletions(-) delete mode 100644 shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts delete mode 100644 shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md index dbf8136b4e..dcd49c4024 100644 --- a/PATTERN-DECISIONS.md +++ b/PATTERN-DECISIONS.md @@ -25,9 +25,16 @@ A pattern declares a relationship at the same three sites. ## Rules that hold across all tools -A decision names a node or a relationship by its `unique-id`. Two declarations must -therefore never share a `unique-id`. If they did, no answer could select one and not the -other. +An id names one kind of thing. A name used for a node is never also used for a relationship +or an interface, anywhere in the pattern. + +A decision names a node or a relationship by its `unique-id`. Two alternatives of one entry +must therefore have different node ids and different relationship ids. If they did not, no +answer could select one and not the other. + +A decision never names an interface on its own, because a relationship names an interface +beside its node. Two alternatives may expose the same interface id, because only one of +them is ever built. Declare one keyword, not both. An element must satisfy every keyword declared beside it, so declaring both `oneOf` and `anyOf` makes some alternatives impossible to select. @@ -53,11 +60,17 @@ three declaration sites listed above. | Fault | Severity | |---|---| -| Two declarations share a `unique-id` | error | +| Two declarations that can appear together share a `unique-id` | error | +| Two alternatives of one entry share a node or relationship `unique-id` | error | +| One name is used for more than one kind of thing | error | | A relationship refers to a node that the pattern does not declare | error | | A connects relationship refers to an interface that the named node does not declare | error | | A `prefixItems` entry declares both `oneOf` and `anyOf` | error | | No relationship and no decision refers to a declared node | warning | +`calm validate` reads one level of alternatives. It does not read alternatives declared +inside another alternative. The keyword check reads node and relationship entries, not +interface entries. + A pattern that declares alternatives inside an `allOf` branch is not supported. Two `allOf` branches that declare the same property discard one of the two declarations. diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts index 3680211643..f798c6490a 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts @@ -291,7 +291,34 @@ describe('idsAreUnique', () => { expect(result[0].message).toContain('Duplicate unique-id detected. ID: rel1, path: /properties/relationships/prefixItems/0/anyOf/1/properties/unique-id/const'); }); - it('should return messages for duplicate interface IDs on oneOf alternatives', () => { + it('should return messages for duplicate IDs within one node\'s interfaces', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'properties': { + 'unique-id': { 'const': 'node1' }, + 'interfaces': { prefixItems: [ + { 'properties': { 'unique-id': { 'const': 'intf1' } } }, + { 'properties': { 'unique-id': { 'const': 'intf1' } } } + ] } } + } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/properties/interfaces/prefixItems/1/properties/unique-id/const'); + }); + + it('should return an empty array when oneOf alternatives of one entry share an interface ID', () => { const input = {}; const context = { document: { @@ -301,12 +328,12 @@ describe('idsAreUnique', () => { prefixItems: [ { 'oneOf': [ { 'properties': { - 'unique-id': { 'const': 'node1' }, - 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + 'unique-id': { 'const': 'postgres' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'db-port' } } }] } } }, { 'properties': { - 'unique-id': { 'const': 'node2' }, - 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + 'unique-id': { 'const': 'mysql' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'db-port' } } }] } } } ] } ] @@ -316,12 +343,10 @@ describe('idsAreUnique', () => { } }; - const result = idsAreUnique(input, null, asContext(context)); - expect(result.length).toBeGreaterThan(0); - expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/oneOf/1/properties/interfaces/prefixItems/0/properties/unique-id/const'); + expect(idsAreUnique(input, null, asContext(context))).toEqual([]); }); - it('should return messages for duplicate interface IDs on anyOf alternatives', () => { + it('should return an empty array when anyOf alternatives of one entry share an interface ID', () => { const input = {}; const context = { document: { @@ -330,10 +355,40 @@ describe('idsAreUnique', () => { nodes: { prefixItems: [ { 'anyOf': [ + { 'properties': { + 'unique-id': { 'const': 'postgres' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'db-port' } } }] } } + }, + { 'properties': { + 'unique-id': { 'const': 'mysql' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'db-port' } } }] } } + } + ] } + ] + } + } + } + } + }; + + expect(idsAreUnique(input, null, asContext(context))).toEqual([]); + }); + + it('should return messages for duplicate interface IDs on alternatives of different entries', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [ { 'properties': { 'unique-id': { 'const': 'node1' }, 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } - }, + } + ] }, + { 'oneOf': [ { 'properties': { 'unique-id': { 'const': 'node2' }, 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } @@ -348,13 +403,96 @@ describe('idsAreUnique', () => { const result = idsAreUnique(input, null, asContext(context)); expect(result.length).toBeGreaterThan(0); - expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/anyOf/1/properties/interfaces/prefixItems/0/properties/unique-id/const'); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/1/oneOf/0/properties/interfaces/prefixItems/0/properties/unique-id/const'); }); + it('should return messages when an entry and its own alternative share an interface ID', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { + 'properties': { + 'unique-id': { 'const': 'base' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } }, + 'oneOf': [ + { 'properties': { + 'unique-id': { 'const': 'alt' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + } + ] + } + ] + } + } + } + } + }; + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/0/oneOf/0/properties/interfaces/prefixItems/0/properties/unique-id/const'); + }); + it('should return an empty array when an entry and its own alternative have distinct interface IDs', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { + 'properties': { + 'unique-id': { 'const': 'base' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } }, + 'oneOf': [ + { 'properties': { + 'unique-id': { 'const': 'alt' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf2' } } }] } } + } + ] + } + ] + } + } + } + } + }; + expect(idsAreUnique(input, null, asContext(context))).toEqual([]); + }); + it('should return messages when an alternative reuses the interface ID of a mandatory node', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'properties': { + 'unique-id': { 'const': 'webapp' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + }, + { 'oneOf': [ + { 'properties': { + 'unique-id': { 'const': 'cache' }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'intf1' } } }] } } + } + ] } + ] + } + } + } + } + }; - -}); \ No newline at end of file + const result = idsAreUnique(input, null, asContext(context)); + expect(result.length).toBeGreaterThan(0); + expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/1/oneOf/0/properties/interfaces/prefixItems/0/properties/unique-id/const'); + }); +}); diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.ts b/shared/src/spectral/functions/pattern/ids-are-unique.ts index 84706b4cd5..c2a176c953 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -3,6 +3,50 @@ import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-cor import { detectDuplicates } from '../helper-functions'; import { declaredIdPaths, declaredInterfaceIdPaths } from './declaration-paths'; +interface Match { + value: unknown; + pointer: string; +} + +function declarationKey(pointer: string): string { + return pointer.split('/properties/interfaces/')[0]; +} + +function entryKey(pointer: string): string { + return declarationKey(pointer).split(/\/(?:oneOf|anyOf)\/\d+$/)[0]; +} + +function isAlternative(pointer: string): boolean { + return declarationKey(pointer) !== entryKey(pointer); +} + +function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] { + const groups = new Map(); + for (const match of matches) { + const groupKey = key(match.pointer); + const group = groups.get(groupKey) ?? []; + group.push(match); + groups.set(groupKey, group); + } + return [...groups.values()]; +} + +/** + * A relationship names an interface beside its node, so an interface id only has to be + * unique among the nodes that can appear in one architecture. At most one alternative of a + * prefixItems entry is ever chosen, so alternatives may repeat an interface id. + */ +function detectDuplicateInterfaceIds(matches: Match[], seenIds: Set, messages: IFunctionResult[]) { + for (const entry of groupBy(matches, entryKey)) { + const always = entry.filter(match => !isAlternative(match.pointer)); + const choices = groupBy(entry.filter(match => isAlternative(match.pointer)), declarationKey); + + detectDuplicates(always, seenIds, messages); + choices.forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); + choices.flat().forEach(match => seenIds.add(match.value)); + } +} + /** * Reports any unique-id a pattern declares more than once. */ @@ -10,7 +54,7 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF if (!input) { return []; } - const collect = (paths: string[]) => paths.flatMap(path => + const collect = (paths: string[]): Match[] => paths.flatMap(path => JSONPath({ path, json: context.document.data as object, resultType: 'all' })); const nodeIdMatches = collect(declaredIdPaths('nodes')); @@ -23,7 +67,7 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF detectDuplicates(nodeIdMatches, seenIds, messages); detectDuplicates(relationshipIdMatches, seenIds, messages); - detectDuplicates(interfaceIdMatches, seenIds, messages); + detectDuplicateInterfaceIds(interfaceIdMatches, seenIds, messages); return messages; }; \ No newline at end of file diff --git a/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts index 41121acd91..f6468e64dd 100644 --- a/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts +++ b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts @@ -1,20 +1,22 @@ import { JSONPath } from 'jsonpath-plus'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; +import { CalmType, declaredIdPaths } from './declaration-paths'; /** * Checks that the input value should be defined in a oneOf or anyOf block. */ -export function isDefinedInOneOfOrAnyOf(input: unknown, { calmType }: { calmType: 'nodes' | 'relationships'}, context: RulesetFunctionContext): IFunctionResult[] { +export function isDefinedInOneOfOrAnyOf(input: unknown, { calmType }: { calmType: CalmType }, context: RulesetFunctionContext): IFunctionResult[] { if (!input || typeof input !== 'string') { return []; } - const names = JSONPath({ path: `$.properties.${calmType}.prefixItems[*].properties.unique-id.const`, json: context.document.data as object }); - const oneofs = JSONPath({ path: `$.properties.${calmType}.prefixItems[*].oneOf[*].properties.unique-id.const`, json: context.document.data as object }); - const anyofs = JSONPath({ path: `$.properties.${calmType}.prefixItems[*].anyOf[*].properties.unique-id.const`, json: context.document.data as object }); + const [declaredPath, ...alternativePaths] = declaredIdPaths(calmType); + const declared = JSONPath({ path: declaredPath, json: context.document.data as object }); + const inAlternatives = alternativePaths.flatMap(path => + JSONPath({ path, json: context.document.data as object })); const results: IFunctionResult[] = []; - if (names.includes(input) && !oneofs.includes(input) && !anyofs.includes(input)) { + if (declared.includes(input) && !inAlternatives.includes(input)) { results.push({ message: `'${input}' is part of a pattern option and must be defined in a oneOf or anyOf block.`, path: [...context.path], diff --git a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts deleted file mode 100644 index 1f5f84a0b9..0000000000 --- a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.spec.ts +++ /dev/null @@ -1,50 +0,0 @@ -import { asContext } from '../spectral-test-helpers'; -import { prefixItemsDeclaresOneKeyword } from './prefix-items-declares-one-keyword'; - -const node = (id: string) => ({ properties: { 'unique-id': { const: id } } }); - -function contextFor(nodes: object) { - return { - document: { data: { properties: { nodes } } }, - path: [] - }; -} - -describe('prefixItemsDeclaresOneKeyword', () => { - it('should return an empty array when there is no input', () => { - const context = contextFor({ prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }); - - const result = prefixItemsDeclaresOneKeyword(null, null, asContext(context)); - expect(result).toEqual([]); - }); - - it('should report an entry whose keywords name different alternatives', () => { - const context = contextFor({ prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }); - - const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); - expect(result.length).toBe(1); - expect(result[0].message).toContain('A prefixItems entry declares both \'oneOf\' and \'anyOf\''); - expect(result[0].path).toEqual(['properties', 'nodes', 'prefixItems', '0']); - }); - - it('should report an entry whose keywords overlap', () => { - const context = contextFor({ prefixItems: [{ oneOf: [node('cache'), node('queue')], anyOf: [node('cache')] }] }); - - const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); - expect(result.length).toBe(1); - }); - - it('should return an empty array when an entry declares one keyword', () => { - const context = contextFor({ prefixItems: [{ oneOf: [node('cache'), node('queue')] }] }); - - const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); - expect(result).toEqual([]); - }); - - it('should return an empty array for a bare node entry', () => { - const context = contextFor({ prefixItems: [node('webapp')] }); - - const result = prefixItemsDeclaresOneKeyword({}, null, asContext(context)); - expect(result).toEqual([]); - }); -}); diff --git a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts b/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts deleted file mode 100644 index 4b25da1c01..0000000000 --- a/shared/src/spectral/functions/pattern/prefix-items-declares-one-keyword.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { JSONPath } from 'jsonpath-plus'; -import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; - -const ENTRIES = [ - '$.properties.nodes.prefixItems[*]', - '$.properties.relationships.prefixItems[*]', -]; - -interface Match { - value: Record; - pointer: string; -} - -/** - * Spectral needs a segment array. A pointer string reports at the document root. - */ -function pointerToPath(pointer: string): string[] { - return pointer.split('/').slice(1); -} - -/** - * Reports each `prefixItems` entry that declares both `oneOf` and `anyOf`. - */ -export function prefixItemsDeclaresOneKeyword(input: unknown, _: unknown, context: RulesetFunctionContext): IFunctionResult[] { - if (!input) { - return []; - } - - const matches: Match[] = ENTRIES.flatMap(path => - JSONPath({ path, json: context.document.data as object, resultType: 'all' })); - - return matches - .filter(match => Array.isArray(match.value?.['oneOf']) && Array.isArray(match.value?.['anyOf'])) - .map(match => ({ - message: 'A prefixItems entry declares both \'oneOf\' and \'anyOf\'. An element must satisfy both, so some alternatives can never be selected. Declare one keyword.', - path: pointerToPath(match.pointer), - })); -} diff --git a/shared/src/spectral/rules-pattern.ts b/shared/src/spectral/rules-pattern.ts index 3aba7cab08..b2cdbb48ad 100644 --- a/shared/src/spectral/rules-pattern.ts +++ b/shared/src/spectral/rules-pattern.ts @@ -1,5 +1,5 @@ import { RulesetDefinition } from '@stoplight/spectral-core'; -import { pattern, truthy, length, xor } from '@stoplight/spectral-functions'; +import { pattern, truthy, length, xor, falsy } from '@stoplight/spectral-functions'; import { numericalPlaceHolder } from './functions/helper-functions'; import nodeIdExists from './functions/pattern/node-id-exists'; import idsAreUnique from './functions/pattern/ids-are-unique'; @@ -7,7 +7,6 @@ import nodeHasRelationship from './functions/pattern/node-has-relationship'; import { interfaceIdExists } from './functions/pattern/interface-id-exists'; import { interfaceIdExistsOnNode } from './functions/pattern/interface-id-exists-on-node'; import { isDefinedInOneOfOrAnyOf } from './functions/pattern/is-defined-in-oneof-or-anyof'; -import { prefixItemsDeclaresOneKeyword } from './functions/pattern/prefix-items-declares-one-keyword'; import { declaredIdPaths } from './functions/pattern/declaration-paths'; @@ -192,10 +191,13 @@ const patternRules: RulesetDefinition = { 'pattern-prefix-items-must-declare-one-keyword': { description: 'A prefixItems entry must declare either oneOf or anyOf, not both', severity: 'error', - message: '{{error}}', - given: '$', + message: 'A prefixItems entry declares both \'oneOf\' and \'anyOf\'. An element must satisfy both, so some alternatives can never be selected. Declare one keyword.', + given: [ + '$.properties.nodes.prefixItems[?(@.oneOf && @.anyOf)]', + '$.properties.relationships.prefixItems[?(@.oneOf && @.anyOf)]', + ], then: { - function: prefixItemsDeclaresOneKeyword, + function: falsy, }, }, 'pattern-option-relationship-must-have-max-one-item': { From df55ebca785f9b0f741bd02d8623de4ab20d5654 Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Thu, 10 Sep 2026 19:24:01 +0000 Subject: [PATCH 3/8] refactor(shared): name the declaration paths instead of ordering them is-defined-in-oneof-or-anyof read the fixed entry path by destructuring declaredIdPaths, so reordering that array inverted the rule: it accepted an id declared only as a fixed entry and rejected one declared inside a choice. No test covered the function, and the only tests that turned red under a reorder were unrelated pointer assertions in ids-are-unique. fixedIdPath and alternativeIdPaths now name what a caller wants, and declaredIdPaths is composed from them, so the order lives in one place and nobody reads it by position. Adds the spec the function never had. --- .../functions/pattern/declaration-paths.ts | 25 +++++++-- .../is-defined-in-oneof-or-anyof.spec.ts | 54 +++++++++++++++++++ .../pattern/is-defined-in-oneof-or-anyof.ts | 9 ++-- 3 files changed, 79 insertions(+), 9 deletions(-) create mode 100644 shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.spec.ts diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts index 4a7aac3d68..11a9704449 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -2,20 +2,37 @@ import { JSONPath } from 'jsonpath-plus'; export type CalmType = 'nodes' | 'relationships'; +const ID = 'properties.unique-id.const'; + +function entryPath(calmType: CalmType): string { + return `$.properties.${calmType}.prefixItems[*]`; +} + +function alternativePaths(calmType: CalmType): string[] { + return ['oneOf', 'anyOf'].map(keyword => `${entryPath(calmType)}.${keyword}[*]`); +} + /** * Shared so that the rules resolving declarations cannot disagree about where they are. */ export function declarationPaths(calmType: CalmType): string[] { - const entry = `$.properties.${calmType}.prefixItems[*]`; - return [entry, `${entry}.oneOf[*]`, `${entry}.anyOf[*]`]; + return [entryPath(calmType), ...alternativePaths(calmType)]; +} + +export function fixedIdPath(calmType: CalmType): string { + return `${entryPath(calmType)}.${ID}`; +} + +export function alternativeIdPaths(calmType: CalmType): string[] { + return alternativePaths(calmType).map(path => `${path}.${ID}`); } export function declaredIdPaths(calmType: CalmType): string[] { - return declarationPaths(calmType).map(path => `${path}.properties.unique-id.const`); + return [fixedIdPath(calmType), ...alternativeIdPaths(calmType)]; } export function declaredInterfaceIdPaths(): string[] { - return declarationPaths('nodes').map(path => `${path}.properties.interfaces.prefixItems[*].properties.unique-id.const`); + return declarationPaths('nodes').map(path => `${path}.properties.interfaces.prefixItems[*].${ID}`); } export function declaredId(declaration: object): string | undefined { diff --git a/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.spec.ts b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.spec.ts new file mode 100644 index 0000000000..a78e1be413 --- /dev/null +++ b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.spec.ts @@ -0,0 +1,54 @@ +import { RulesetFunctionContext } from '@stoplight/spectral-core'; +import { isDefinedInOneOfOrAnyOf } from './is-defined-in-oneof-or-anyof'; + +const asContext = (data: object) => ({ document: { data }, path: ['relationships', 0] } as unknown as RulesetFunctionContext); + +const declaration = (id: string) => ({ properties: { 'unique-id': { const: id } } }); + +const pattern = (calmType: 'nodes' | 'relationships', entries: object[]) => ({ + properties: { [calmType]: { prefixItems: entries } } +}); + +const messagesFor = (data: object, input: unknown, calmType: 'nodes' | 'relationships' = 'nodes') => + isDefinedInOneOfOrAnyOf(input, { calmType }, asContext(data)).map(result => result.message); + +describe('isDefinedInOneOfOrAnyOf', () => { + it('reports an id declared only as a fixed entry', () => { + const data = pattern('nodes', [declaration('redis')]); + expect(messagesFor(data, 'redis')).toEqual([ + '\'redis\' is part of a pattern option and must be defined in a oneOf or anyOf block.' + ]); + }); + + it('reports a relationship id declared only as a fixed entry', () => { + const data = pattern('relationships', [declaration('cache-link')]); + expect(messagesFor(data, 'cache-link', 'relationships')).toHaveLength(1); + }); + + it('accepts an id declared inside oneOf', () => { + const data = pattern('nodes', [{ oneOf: [declaration('redis'), declaration('memcached')] }]); + expect(messagesFor(data, 'redis')).toEqual([]); + }); + + it('accepts an id declared inside anyOf', () => { + const data = pattern('nodes', [{ anyOf: [declaration('redis')] }]); + expect(messagesFor(data, 'redis')).toEqual([]); + }); + + it('accepts an id the pattern does not declare', () => { + const data = pattern('nodes', [declaration('webapp')]); + expect(messagesFor(data, 'redis')).toEqual([]); + }); + + it('reports at the path of the reference', () => { + const data = pattern('nodes', [declaration('redis')]); + const results = isDefinedInOneOfOrAnyOf('redis', { calmType: 'nodes' }, asContext(data)); + expect(results[0].path).toEqual(['relationships', 0]); + }); + + it.each([['', 'empty string'], [null, 'null'], [undefined, 'undefined'], [42, 'a number']])( + 'ignores %s input (%s)', input => { + const data = pattern('nodes', [declaration('redis')]); + expect(messagesFor(data, input)).toEqual([]); + }); +}); diff --git a/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts index f6468e64dd..265e22408c 100644 --- a/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts +++ b/shared/src/spectral/functions/pattern/is-defined-in-oneof-or-anyof.ts @@ -1,6 +1,6 @@ import { JSONPath } from 'jsonpath-plus'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; -import { CalmType, declaredIdPaths } from './declaration-paths'; +import { alternativeIdPaths, CalmType, fixedIdPath } from './declaration-paths'; /** * Checks that the input value should be defined in a oneOf or anyOf block. */ @@ -9,14 +9,13 @@ export function isDefinedInOneOfOrAnyOf(input: unknown, { calmType }: { calmType return []; } - const [declaredPath, ...alternativePaths] = declaredIdPaths(calmType); - const declared = JSONPath({ path: declaredPath, json: context.document.data as object }); - const inAlternatives = alternativePaths.flatMap(path => + const fixed = JSONPath({ path: fixedIdPath(calmType), json: context.document.data as object }); + const inAlternatives = alternativeIdPaths(calmType).flatMap(path => JSONPath({ path, json: context.document.data as object })); const results: IFunctionResult[] = []; - if (declared.includes(input) && !inAlternatives.includes(input)) { + if (fixed.includes(input) && !inAlternatives.includes(input)) { results.push({ message: `'${input}' is part of a pattern option and must be defined in a oneOf or anyOf block.`, path: [...context.path], From 960fa4fca94a6c21cada04c0d55e47a9380be951 Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Thu, 10 Sep 2026 23:40:30 +0000 Subject: [PATCH 4/8] fix(shared): report duplicate ids at the later declaration Three queries are collected per id kind, so matches arrived grouped by query rather than by document position. The rule blames the second declaration it sees, so it could name the earlier of two duplicates. Matches are now sorted before detection, padding the indexes so prefixItems/2 comes before prefixItems/10. Adds the test for the interface pooling fix in #3068, which had none. Reintroducing pooling passed all 116 spectral tests. declaration-paths.ts now owns both directions. One keyword list drives the queries that find declarations and the containingDeclaration and containingEntry helpers that read the resulting pointers back, so the two cannot drift. Adding a keyword to that list is picked up by both, verified. The helpers were pointer-string splits local to ids-are-unique that only worked for interface pointers. PATTERN-DECISIONS.md no longer claims an entry's own properties lands in the architecture alongside its alternatives. calm generate keeps the selected alternative and discards the entry's own properties, so what it declares is lost. The duplicate-id error catches the case where the two halves share an id without naming that fault. --- PATTERN-DECISIONS.md | 5 ++ .../pattern/declaration-paths.spec.ts | 50 ++++++++++++++++++ .../functions/pattern/declaration-paths.ts | 31 +++++++++-- .../functions/pattern/ids-are-unique.spec.ts | 51 +++++++++++++++++++ .../functions/pattern/ids-are-unique.ts | 37 +++++++------- .../interface-id-exists-on-node.spec.ts | 46 ++++++++++++++++- 6 files changed, 196 insertions(+), 24 deletions(-) create mode 100644 shared/src/spectral/functions/pattern/declaration-paths.spec.ts diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md index dcd49c4024..dc41b99b04 100644 --- a/PATTERN-DECISIONS.md +++ b/PATTERN-DECISIONS.md @@ -72,5 +72,10 @@ three declaration sites listed above. inside another alternative. The keyword check reads node and relationship entries, not interface entries. +Do not give a `prefixItems` entry its own `properties` as well as alternatives. `calm +generate` keeps the selected alternative and discards the entry's own `properties`, so +whatever the entry declares is lost. `calm validate` reports a duplicate `unique-id` when +the two halves share one. That catches the common case. It does not name the fault. + A pattern that declares alternatives inside an `allOf` branch is not supported. Two `allOf` branches that declare the same property discard one of the two declarations. diff --git a/shared/src/spectral/functions/pattern/declaration-paths.spec.ts b/shared/src/spectral/functions/pattern/declaration-paths.spec.ts new file mode 100644 index 0000000000..4413d9532c --- /dev/null +++ b/shared/src/spectral/functions/pattern/declaration-paths.spec.ts @@ -0,0 +1,50 @@ +import { alternativeIdPaths, containingDeclaration, containingEntry, declarationPaths, declaredId, declaredIdPaths, declaredInterfaceIdPaths, fixedIdPath, isAlternative } from './declaration-paths'; + +const ENTRY = '/properties/nodes/prefixItems/0'; +const ALTERNATIVE = `${ENTRY}/oneOf/1`; +const ID = '/properties/unique-id/const'; +const INTERFACE_ID = '/properties/interfaces/prefixItems/0/properties/unique-id/const'; + +describe('declaration paths', () => { + it('covers the fixed entry and both alternative keywords', () => { + expect(declarationPaths('relationships')).toEqual([ + '$.properties.relationships.prefixItems[*]', + '$.properties.relationships.prefixItems[*].oneOf[*]', + '$.properties.relationships.prefixItems[*].anyOf[*]' + ]); + }); + + it('builds id paths from the fixed entry followed by the alternatives', () => { + expect(declaredIdPaths('nodes')).toEqual([fixedIdPath('nodes'), ...alternativeIdPaths('nodes')]); + }); + + it('reaches interfaces on every node declaration site', () => { + expect(declaredInterfaceIdPaths()).toHaveLength(declarationPaths('nodes').length); + expect(declaredInterfaceIdPaths()[0]).toBe('$.properties.nodes.prefixItems[*].properties.interfaces.prefixItems[*].properties.unique-id.const'); + }); + + it('reads the id off a declaration', () => { + expect(declaredId({ properties: { 'unique-id': { const: 'webapp' } } })).toBe('webapp'); + expect(declaredId({ oneOf: [] })).toBeUndefined(); + }); +}); + +describe('declaration pointers', () => { + it.each([ + ['a fixed node id', `${ENTRY}${ID}`, ENTRY, ENTRY, false], + ['an alternative node id', `${ALTERNATIVE}${ID}`, ALTERNATIVE, ENTRY, true], + ['a fixed interface id', `${ENTRY}${INTERFACE_ID}`, ENTRY, ENTRY, false], + ['an alternative interface id', `${ALTERNATIVE}${INTERFACE_ID}`, ALTERNATIVE, ENTRY, true], + ['a relationship alternative', '/properties/relationships/prefixItems/2/anyOf/0' + ID, + '/properties/relationships/prefixItems/2/anyOf/0', '/properties/relationships/prefixItems/2', true], + ])('resolves %s', (_name, pointer, declaration, entry, alternative) => { + expect(containingDeclaration(pointer)).toBe(declaration); + expect(containingEntry(pointer)).toBe(entry); + expect(isAlternative(pointer)).toBe(alternative); + }); + + it('leaves a pointer from outside these paths alone', () => { + expect(containingDeclaration('/properties/metadata/0')).toBe('/properties/metadata/0'); + expect(isAlternative('/properties/metadata/0')).toBe(false); + }); +}); diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts index 11a9704449..fc8f5a2c29 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -2,18 +2,28 @@ import { JSONPath } from 'jsonpath-plus'; export type CalmType = 'nodes' | 'relationships'; +const ALTERNATIVE_KEYWORDS = ['oneOf', 'anyOf']; const ID = 'properties.unique-id.const'; +const INTERFACES = 'properties.interfaces.prefixItems[*]'; + +const ALTERNATIVES = `(?:${ALTERNATIVE_KEYWORDS.join('|')})`; +const DECLARATION_POINTER = new RegExp(`^/properties/(?:nodes|relationships)/prefixItems/\\d+(?:/${ALTERNATIVES}/\\d+)?`); +const ALTERNATIVE_SUFFIX = new RegExp(`/${ALTERNATIVES}/\\d+$`); function entryPath(calmType: CalmType): string { return `$.properties.${calmType}.prefixItems[*]`; } function alternativePaths(calmType: CalmType): string[] { - return ['oneOf', 'anyOf'].map(keyword => `${entryPath(calmType)}.${keyword}[*]`); + return ALTERNATIVE_KEYWORDS.map(keyword => `${entryPath(calmType)}.${keyword}[*]`); } /** * Shared so that the rules resolving declarations cannot disagree about where they are. + * + * The paths below find declarations. A query run with `resultType: 'all'` returns each hit + * with the JSON Pointer it was found at, and the `containing` helpers read that pointer + * back, because it is the only surviving trace of which entry the hit came from. */ export function declarationPaths(calmType: CalmType): string[] { return [entryPath(calmType), ...alternativePaths(calmType)]; @@ -32,9 +42,24 @@ export function declaredIdPaths(calmType: CalmType): string[] { } export function declaredInterfaceIdPaths(): string[] { - return declarationPaths('nodes').map(path => `${path}.properties.interfaces.prefixItems[*].${ID}`); + return declarationPaths('nodes').map(path => `${path}.${INTERFACES}.${ID}`); } export function declaredId(declaration: object): string | undefined { - return JSONPath({ path: '$.properties.unique-id.const', json: declaration })[0]; + return JSONPath({ path: `$.${ID}`, json: declaration })[0]; +} + +/** + * A pointer from outside these paths has no declaration, so it stands alone. + */ +export function containingDeclaration(pointer: string): string { + return pointer.match(DECLARATION_POINTER)?.[0] ?? pointer; +} + +export function containingEntry(pointer: string): string { + return containingDeclaration(pointer).split(ALTERNATIVE_SUFFIX)[0]; +} + +export function isAlternative(pointer: string): boolean { + return containingDeclaration(pointer) !== containingEntry(pointer); } diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts index f798c6490a..3b38f31bb1 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts @@ -495,4 +495,55 @@ describe('idsAreUnique', () => { expect(result.length).toBeGreaterThan(0); expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/1/oneOf/0/properties/interfaces/prefixItems/0/properties/unique-id/const'); }); + it('should blame the later declaration when an alternative comes before a fixed entry', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] }, + { 'properties': { 'unique-id': { 'const': 'dup' } } } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/prefixItems/1/properties/unique-id/const'); + }); + + it('should blame the later declaration when anyOf comes before oneOf', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'anyOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] }, + { 'oneOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/prefixItems/1/oneOf/0/properties/unique-id/const'); + }); + + it('should order prefixItems entries numerically, not as strings', () => { + const input = {}; + const prefixItems = Array.from({ length: 11 }, (_, index) => ({ 'properties': { 'unique-id': { 'const': `n${index}` } } })); + prefixItems.push({ 'properties': { 'unique-id': { 'const': 'n2' } } }); + const context = { document: { data: { properties: { nodes: { prefixItems } } } } }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/prefixItems/11/properties/unique-id/const'); + }); }); diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.ts b/shared/src/spectral/functions/pattern/ids-are-unique.ts index c2a176c953..0deea31c36 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -1,23 +1,21 @@ import { JSONPath } from 'jsonpath-plus'; +import { partition } from 'lodash'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; import { detectDuplicates } from '../helper-functions'; -import { declaredIdPaths, declaredInterfaceIdPaths } from './declaration-paths'; +import { containingDeclaration, containingEntry, declaredIdPaths, declaredInterfaceIdPaths, isAlternative } from './declaration-paths'; interface Match { value: unknown; pointer: string; } -function declarationKey(pointer: string): string { - return pointer.split('/properties/interfaces/')[0]; -} - -function entryKey(pointer: string): string { - return declarationKey(pointer).split(/\/(?:oneOf|anyOf)\/\d+$/)[0]; -} - -function isAlternative(pointer: string): boolean { - return declarationKey(pointer) !== entryKey(pointer); +/** + * The rule blames the second declaration it sees, but three queries per id kind arrive + * grouped by query rather than by position. Padding keeps prefixItems/2 before /10. + */ +function inDocumentOrder(matches: Match[]): Match[] { + const position = (pointer: string) => pointer.replace(/\d+/g, index => index.padStart(6, '0')); + return [...matches].sort((left, right) => position(left.pointer) < position(right.pointer) ? -1 : 1); } function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] { @@ -37,13 +35,12 @@ function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] * prefixItems entry is ever chosen, so alternatives may repeat an interface id. */ function detectDuplicateInterfaceIds(matches: Match[], seenIds: Set, messages: IFunctionResult[]) { - for (const entry of groupBy(matches, entryKey)) { - const always = entry.filter(match => !isAlternative(match.pointer)); - const choices = groupBy(entry.filter(match => isAlternative(match.pointer)), declarationKey); + for (const entry of groupBy(matches, containingEntry)) { + const [choices, fixed] = partition(entry, match => isAlternative(match.pointer)); - detectDuplicates(always, seenIds, messages); - choices.forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); - choices.flat().forEach(match => seenIds.add(match.value)); + detectDuplicates(fixed, seenIds, messages); + groupBy(choices, containingDeclaration).forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); + choices.forEach(match => seenIds.add(match.value)); } } @@ -54,8 +51,8 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF if (!input) { return []; } - const collect = (paths: string[]): Match[] => paths.flatMap(path => - JSONPath({ path, json: context.document.data as object, resultType: 'all' })); + const collect = (paths: string[]): Match[] => inDocumentOrder(paths.flatMap(path => + JSONPath({ path, json: context.document.data as object, resultType: 'all' }))); const nodeIdMatches = collect(declaredIdPaths('nodes')); const relationshipIdMatches = collect(declaredIdPaths('relationships')); @@ -70,4 +67,4 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF detectDuplicateInterfaceIds(interfaceIdMatches, seenIds, messages); return messages; -}; \ No newline at end of file +}; diff --git a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts index 3aa06e7ed3..9fb920046c 100644 --- a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts +++ b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts @@ -198,4 +198,48 @@ describe('interfaceIdExistsOnNode', () => { expect(result.length).toBe(1); expect(result[0].message).toBe('Referenced interface with ID \'missing-port\' was not defined on the node with ID \'queue\'.'); }); -}); \ No newline at end of file + it('should not accept an interface belonging to a sibling alternative', () => { + const input = { node: 'cache', interfaces: ['port-b'] }; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { + oneOf: [ + { + properties: { + 'unique-id': { const: 'cache' }, + 'interfaces': { + prefixItems: [ + { properties: { 'unique-id': { const: 'port-a' } } } + ] + } + } + }, + { + properties: { + 'unique-id': { const: 'queue' }, + 'interfaces': { + prefixItems: [ + { properties: { 'unique-id': { const: 'port-b' } } } + ] + } + } + } + ] + } + ] + } + } + } + }, + path: ['/relationships/0/connects/destination'] + }; + + const result = interfaceIdExistsOnNode(input, null, asContext(context)); + expect(result.length).toBe(1); + expect(result[0].message).toBe('Referenced interface with ID \'port-b\' was not defined on the node with ID \'cache\'.'); + }); +}); From 0284505c3f956ef2de36ae3f4a75e817730b8bae Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Fri, 11 Sep 2026 11:14:24 +0000 Subject: [PATCH 5/8] docs: narrow the connects guarantee to the source calm validate reads $..connects.source.node and never the destination, so a destination typo is not reported. The contract claimed both ends. --- PATTERN-DECISIONS.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md index dc41b99b04..052b7dfdd7 100644 --- a/PATTERN-DECISIONS.md +++ b/PATTERN-DECISIONS.md @@ -63,11 +63,14 @@ three declaration sites listed above. | Two declarations that can appear together share a `unique-id` | error | | Two alternatives of one entry share a node or relationship `unique-id` | error | | One name is used for more than one kind of thing | error | -| A relationship refers to a node that the pattern does not declare | error | +| The source of a connects relationship refers to a node that the pattern does not declare | error | | A connects relationship refers to an interface that the named node does not declare | error | | A `prefixItems` entry declares both `oneOf` and `anyOf` | error | | No relationship and no decision refers to a declared node | warning | +`calm validate` does not read the destination of a connects relationship. A typo there is +not reported. + `calm validate` reads one level of alternatives. It does not read alternatives declared inside another alternative. The keyword check reads node and relationship entries, not interface entries. From e1ac334bfaa162cccbd9cd75753564adddce69f4 Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Tue, 15 Sep 2026 21:12:52 +0000 Subject: [PATCH 6/8] fix(shared): order declarations by index, not by pointer text Sorting the pointer text put an alternative ahead of the entry that holds it, because "oneOf" precedes "properties". A node id declared in both was then blamed on the entry, which is the half an architecture always builds. The indices of the containing declaration decide the order instead, so a shorter key means the outer declaration and the padding that capped array indices at 999,999 goes. --- .../functions/pattern/declaration-paths.ts | 21 +++++++++++++++++ .../functions/pattern/ids-are-unique.spec.ts | 23 +++++++++++++++++++ .../functions/pattern/ids-are-unique.ts | 13 +++++------ 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts index fc8f5a2c29..c74e8be4af 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -63,3 +63,24 @@ export function containingEntry(pointer: string): string { export function isAlternative(pointer: string): boolean { return containingDeclaration(pointer) !== containingEntry(pointer); } + +function declarationIndices(pointer: string): number[] { + return (containingDeclaration(pointer).match(/\d+/g) ?? []).map(Number); +} + +/** + * Orders declarations as an architecture fills the array. The indices decide it, not the + * pointer text: sorting the text puts an alternative ahead of the entry that holds it, + * because "oneOf" precedes "properties". A declaration with fewer indices contains the + * other, so it comes first. + */ +export function byBuildOrder(left: string, right: string): number { + const [first, second] = [left, right].map(declarationIndices); + for (let depth = 0; depth < Math.max(first.length, second.length); depth++) { + const difference = (first[depth] ?? -1) - (second[depth] ?? -1); + if (difference !== 0) { + return difference; + } + } + return 0; +} diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts index 3b38f31bb1..8f836b7a8f 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts @@ -495,6 +495,29 @@ describe('idsAreUnique', () => { expect(result.length).toBeGreaterThan(0); expect(result[0].message).toContain('Duplicate unique-id detected. ID: intf1, path: /properties/nodes/prefixItems/1/oneOf/0/properties/interfaces/prefixItems/0/properties/unique-id/const'); }); + it('should blame the alternative when an entry and its own alternative share a node ID', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [ + { + 'properties': { 'unique-id': { 'const': 'dup' } }, + 'oneOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] + } + ] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/prefixItems/0/oneOf/0/properties/unique-id/const'); + }); + it('should blame the later declaration when an alternative comes before a fixed entry', () => { const input = {}; const context = { diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.ts b/shared/src/spectral/functions/pattern/ids-are-unique.ts index 0deea31c36..00bf9deac1 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -2,7 +2,7 @@ import { JSONPath } from 'jsonpath-plus'; import { partition } from 'lodash'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; import { detectDuplicates } from '../helper-functions'; -import { containingDeclaration, containingEntry, declaredIdPaths, declaredInterfaceIdPaths, isAlternative } from './declaration-paths'; +import { byBuildOrder, containingDeclaration, containingEntry, declaredIdPaths, declaredInterfaceIdPaths, isAlternative } from './declaration-paths'; interface Match { value: unknown; @@ -10,12 +10,11 @@ interface Match { } /** - * The rule blames the second declaration it sees, but three queries per id kind arrive - * grouped by query rather than by position. Padding keeps prefixItems/2 before /10. + * The rule blames the second declaration it sees, but one query per declaration site means + * matches arrive grouped by site rather than by position. */ -function inDocumentOrder(matches: Match[]): Match[] { - const position = (pointer: string) => pointer.replace(/\d+/g, index => index.padStart(6, '0')); - return [...matches].sort((left, right) => position(left.pointer) < position(right.pointer) ? -1 : 1); +function inBuildOrder(matches: Match[]): Match[] { + return [...matches].sort((left, right) => byBuildOrder(left.pointer, right.pointer)); } function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] { @@ -51,7 +50,7 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF if (!input) { return []; } - const collect = (paths: string[]): Match[] => inDocumentOrder(paths.flatMap(path => + const collect = (paths: string[]): Match[] => inBuildOrder(paths.flatMap(path => JSONPath({ path, json: context.document.data as object, resultType: 'all' }))); const nodeIdMatches = collect(declaredIdPaths('nodes')); From 457c01a17eb2f827e7088a64e7573ffc417764cd Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Tue, 15 Sep 2026 21:13:39 +0000 Subject: [PATCH 7/8] refactor(shared): reuse the shared match type and lodash groupBy The rule redeclared helper-functions' JSONPathMatch and hand-rolled a Map-based groupBy beside the lodash import it already had. --- .../spectral/functions/helper-functions.ts | 2 +- .../functions/pattern/ids-are-unique.ts | 30 ++++++------------- 2 files changed, 10 insertions(+), 22 deletions(-) diff --git a/shared/src/spectral/functions/helper-functions.ts b/shared/src/spectral/functions/helper-functions.ts index 4388217d94..f0fed25c49 100644 --- a/shared/src/spectral/functions/helper-functions.ts +++ b/shared/src/spectral/functions/helper-functions.ts @@ -1,6 +1,6 @@ import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; -interface JSONPathMatch { +export interface JSONPathMatch { value: unknown; pointer: string; } diff --git a/shared/src/spectral/functions/pattern/ids-are-unique.ts b/shared/src/spectral/functions/pattern/ids-are-unique.ts index 00bf9deac1..c7086ce720 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -1,31 +1,19 @@ import { JSONPath } from 'jsonpath-plus'; -import { partition } from 'lodash'; +import { groupBy, partition } from 'lodash'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; -import { detectDuplicates } from '../helper-functions'; +import { detectDuplicates, JSONPathMatch } from '../helper-functions'; import { byBuildOrder, containingDeclaration, containingEntry, declaredIdPaths, declaredInterfaceIdPaths, isAlternative } from './declaration-paths'; -interface Match { - value: unknown; - pointer: string; -} - /** * The rule blames the second declaration it sees, but one query per declaration site means * matches arrive grouped by site rather than by position. */ -function inBuildOrder(matches: Match[]): Match[] { +function inBuildOrder(matches: JSONPathMatch[]): JSONPathMatch[] { return [...matches].sort((left, right) => byBuildOrder(left.pointer, right.pointer)); } -function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] { - const groups = new Map(); - for (const match of matches) { - const groupKey = key(match.pointer); - const group = groups.get(groupKey) ?? []; - group.push(match); - groups.set(groupKey, group); - } - return [...groups.values()]; +function groupMatches(matches: JSONPathMatch[], key: (pointer: string) => string): JSONPathMatch[][] { + return Object.values(groupBy(matches, match => key(match.pointer))); } /** @@ -33,12 +21,12 @@ function groupBy(matches: Match[], key: (pointer: string) => string): Match[][] * unique among the nodes that can appear in one architecture. At most one alternative of a * prefixItems entry is ever chosen, so alternatives may repeat an interface id. */ -function detectDuplicateInterfaceIds(matches: Match[], seenIds: Set, messages: IFunctionResult[]) { - for (const entry of groupBy(matches, containingEntry)) { +function detectDuplicateInterfaceIds(matches: JSONPathMatch[], seenIds: Set, messages: IFunctionResult[]) { + for (const entry of groupMatches(matches, containingEntry)) { const [choices, fixed] = partition(entry, match => isAlternative(match.pointer)); detectDuplicates(fixed, seenIds, messages); - groupBy(choices, containingDeclaration).forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); + groupMatches(choices, containingDeclaration).forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); choices.forEach(match => seenIds.add(match.value)); } } @@ -50,7 +38,7 @@ export default (input: unknown, _: unknown, context: RulesetFunctionContext): IF if (!input) { return []; } - const collect = (paths: string[]): Match[] => inBuildOrder(paths.flatMap(path => + const collect = (paths: string[]): JSONPathMatch[] => inBuildOrder(paths.flatMap(path => JSONPath({ path, json: context.document.data as object, resultType: 'all' }))); const nodeIdMatches = collect(declaredIdPaths('nodes')); From 65c88bdc650b1f1ba0863c3b66a5d0eb3ceb6f33 Mon Sep 17 00:00:00 2001 From: YoofiTT96 Date: Tue, 15 Sep 2026 21:15:25 +0000 Subject: [PATCH 8/8] perf(shared): stop searching declaration sites at the first match Every declaration site was queried before the search began, so a node declared as a plain prefixItems entry paid for two queries it never needed. declaredId ran a JSONPath query per node inspected; a lodash path read off the same ID constant does the same work. --- .../functions/pattern/declaration-paths.ts | 4 ++-- .../pattern/interface-id-exists-on-node.ts | 23 ++++++++++++++----- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts index c74e8be4af..3ddad86dd8 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -1,4 +1,4 @@ -import { JSONPath } from 'jsonpath-plus'; +import { get } from 'lodash'; export type CalmType = 'nodes' | 'relationships'; @@ -46,7 +46,7 @@ export function declaredInterfaceIdPaths(): string[] { } export function declaredId(declaration: object): string | undefined { - return JSONPath({ path: `$.${ID}`, json: declaration })[0]; + return get(declaration, ID); } /** diff --git a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts index cbc6a7110d..2ba356c93d 100644 --- a/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts +++ b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.ts @@ -8,6 +8,20 @@ interface ConnectsRelationship { interfaces?: string[]; } +/** + * One declaration site at a time, because a node is nearly always a plain prefixItems + * entry and the later sites then never run. + */ +function findDeclaredNode(json: object, nodeId: string): object | undefined { + for (const path of declarationPaths('nodes')) { + const declarations: object[] = JSONPath({ path, json }); + const node = declarations.find(declaration => declaredId(declaration) === nodeId); + if (node) { + return node; + } + } +} + /** * Checks that the input value exists as an interface with matching unique ID defined under a node in the document. */ @@ -23,10 +37,7 @@ export function interfaceIdExistsOnNode(input: ConnectsRelationship | null | und }]; } - const nodeId = input.node; - const nodes: object[] = declarationPaths('nodes').flatMap(path => - JSONPath({ path, json: context.document.data as object })); - const node = nodes.find(declaration => declaredId(declaration) === nodeId); + const node = findDeclaredNode(context.document.data as object, input.node); if (!node) { // other rule will report undefined node return []; @@ -38,7 +49,7 @@ export function interfaceIdExistsOnNode(input: ConnectsRelationship | null | und const nodeInterfaces = JSONPath({ path: '$.properties.interfaces.prefixItems[*].properties.unique-id.const', json: node }); if (!nodeInterfaces || nodeInterfaces.length === 0) { return [ - { message: `Node with unique-id ${nodeId} has no interfaces defined, expected interfaces [${desiredInterfaces}]` } + { message: `Node with unique-id ${input.node} has no interfaces defined, expected interfaces [${desiredInterfaces}]` } ]; } @@ -52,7 +63,7 @@ export function interfaceIdExistsOnNode(input: ConnectsRelationship | null | und for (const missing of missingInterfaces) { results.push({ - message: `Referenced interface with ID '${missing}' was not defined on the node with ID '${nodeId}'.`, + message: `Referenced interface with ID '${missing}' was not defined on the node with ID '${input.node}'.`, path: [...context.path] }); }