diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md new file mode 100644 index 000000000..052b7dfdd --- /dev/null +++ b/PATTERN-DECISIONS.md @@ -0,0 +1,84 @@ +# 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 + +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. + +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 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 | +| 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. + +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/cli/test_fixtures/validate_output_junit.xml b/cli/test_fixtures/validate_output_junit.xml index da3078e73..a482fbe73 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/helper-functions.ts b/shared/src/spectral/functions/helper-functions.ts index 4388217d9..f0fed25c4 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/declaration-paths.spec.ts b/shared/src/spectral/functions/pattern/declaration-paths.spec.ts new file mode 100644 index 000000000..4413d9532 --- /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 new file mode 100644 index 000000000..3ddad86dd --- /dev/null +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -0,0 +1,86 @@ +import { get } from 'lodash'; + +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 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)]; +} + +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 [fixedIdPath(calmType), ...alternativeIdPaths(calmType)]; +} + +export function declaredInterfaceIdPaths(): string[] { + return declarationPaths('nodes').map(path => `${path}.${INTERFACES}.${ID}`); +} + +export function declaredId(declaration: object): string | undefined { + return get(declaration, ID); +} + +/** + * 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); +} + +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 91f27e3b6..8f836b7a8 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,426 @@ 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'); }); -}); \ No newline at end of file + + 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 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: { + data: { + properties: { + nodes: { + prefixItems: [ + { 'oneOf': [ + { '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 an empty array when anyOf alternatives of one entry share an interface ID', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + 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' } } }] } } + } + ] } + ] + } + } + } + } + }; + + 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'); + }); + + 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' } } }] } } + } + ] } + ] + } + } + } + } + }; + + 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'); + }); + 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 = { + 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 7a04b2400..c7086ce72 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.ts @@ -1,17 +1,49 @@ import { JSONPath } from 'jsonpath-plus'; +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'; + +/** + * 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: JSONPathMatch[]): JSONPathMatch[] { + return [...matches].sort((left, right) => byBuildOrder(left.pointer, right.pointer)); +} + +function groupMatches(matches: JSONPathMatch[], key: (pointer: string) => string): JSONPathMatch[][] { + return Object.values(groupBy(matches, match => key(match.pointer))); +} + /** - * Checks that the input value exists as a node with a matching unique ID. + * 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: 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); + groupMatches(choices, containingDeclaration).forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); + choices.forEach(match => seenIds.add(match.value)); + } +} + +/** + * 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[]): JSONPathMatch[] => inBuildOrder(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(); @@ -19,7 +51,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/interface-id-exists-on-node.spec.ts b/shared/src/spectral/functions/pattern/interface-id-exists-on-node.spec.ts index 44218b4f3..9fb920046 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,87 @@ 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']); }); -}); \ No newline at end of file + + + + + 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\'.'); + }); + 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\'.'); + }); +}); 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 19c765bdf..2ba356c93 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,12 +1,27 @@ 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; 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. */ @@ -22,14 +37,7 @@ 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 node = findDeclaredNode(context.document.data as object, input.node); if (!node) { // other rule will report undefined node return []; @@ -39,11 +47,9 @@ 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}]` } + { message: `Node with unique-id ${input.node} has no interfaces defined, expected interfaces [${desiredInterfaces}]` } ]; } @@ -57,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] }); } 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 000000000..a78e1be41 --- /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 41121acd9..265e22408 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,21 @@ import { JSONPath } from 'jsonpath-plus'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; +import { alternativeIdPaths, CalmType, fixedIdPath } 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 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 (names.includes(input) && !oneofs.includes(input) && !anyofs.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], 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 000000000..852381a64 --- /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 b34820b48..d955cef88 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/rules-pattern.spec.ts b/shared/src/spectral/rules-pattern.spec.ts new file mode 100644 index 000000000..98533acde --- /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 8bec5c21d..b2cdbb48a 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,6 +7,7 @@ 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 { declaredIdPaths } from './functions/pattern/declaration-paths'; const patternRules: RulesetDefinition = { @@ -134,7 +135,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 +188,18 @@ 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: '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: falsy, + }, + }, 'pattern-option-relationship-must-have-max-one-item': { description: 'Options relationships must have max one item', severity: 'error',