diff --git a/PATTERN-DECISIONS.md b/PATTERN-DECISIONS.md index 052b7dfdd..3bdb62f09 100644 --- a/PATTERN-DECISIONS.md +++ b/PATTERN-DECISIONS.md @@ -13,15 +13,30 @@ gap. | 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: +A pattern declares a node at five 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 | +| `items.oneOf` | nodes an architecture may add, none of them required | +| `items.anyOf` | nodes an architecture may add, none of them required | -A pattern declares a relationship at the same three sites. +`maxItems` counts the whole array and the `prefixItems` entries fill it from the front, so +a pattern that wants `items` must leave room for them. + +A node declared under `items` must sit inside `oneOf` or `anyOf`. `items` applies one +schema to every position after the entries, so a node declared directly would force every +added node to be that same node. An `items` schema that declares a node directly is not +read. + +A pattern declares a relationship at the same five sites. + +`prefixItems` and `items` differ in how many of their declarations reach one architecture. +A `prefixItems` entry is one position, so exactly one of its alternatives is built. `items` +describes every position after the last entry, so an architecture may build any number of +its members, or none. ## Rules that hold across all tools @@ -43,6 +58,10 @@ Neither keyword controls how many alternatives an architecture includes. A `pref entry is one position, so it takes one alternative. `minItems` and `maxItems` on the array set the bounds. +`prefixItems` is positional. An architecture lists the elements a pattern declares there +first, and in the order the pattern declares them. Members built from `items` follow. An +architecture that reorders the positional elements does not match the pattern. + `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 @@ -50,11 +69,8 @@ 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. +five declaration sites listed above. `calm validate` reports these faults: @@ -65,20 +81,53 @@ three declaration sites listed above. | 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 | +| A `prefixItems` entry or an `items` block declares both `oneOf` and `anyOf` | error | +| A decision is declared in `items` rather than in `prefixItems` | error | +| A decision declares its options in `items` rather than in `prefixItems` | error | +| `maxItems` leaves no room for an `items` member | 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. +Declare a decision as a plain entry in `relationships.prefixItems`. An architecture +contains every relationship declared at a fixed position there, so the decision is always +asked. A decision declared in `items`, or as one alternative of an entry, can be left out, +and an answer never gets to decline it. `calm validate` reports the `items` case only. + +A decision holds its answers in an `options` array. Declare those option blocks in +`options.prefixItems`. `calm generate` reads that array alone, so a block declared in +`options.items` is never built and the decision loses the answers it holds. + +Two `items` members may both be built, so they must not share a `unique-id`, and two nodes +declared there must not share an interface id. Two alternatives of one `prefixItems` entry +may, because only one of them is built. + +An `items` block cannot limit how many times one member is used. `items` constrains every +position after the entries, so two positions may both match the same member, and the +architecture then holds one `unique-id` twice. `calm validate` cannot report that from the +pattern, because the pattern is correct. The architecture check +`unique-ids-must-be-unique-in-architecture` reports it against the architecture. + +Leave `minItems` out rather than writing `minItems: 0`. Zero is the default, and +`pattern-has-no-empty-properties` reads a zero as a placeholder and reports an error. + `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 +inside another alternative. The keyword check reads node and relationship sites, 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. +Do not give a `prefixItems` entry its own `properties` as well as alternatives. Both halves +apply to the same element. If each half pins a `unique-id`, no element can satisfy both, so +nothing can ever fill that position. If they do not conflict, `calm generate` keeps the +selected alternative and discards the entry's own `properties`, so whatever the entry +declares is lost. `calm validate` reports this only when the two halves share a `unique-id`, +and then it reports a duplicate rather than naming 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. + +## What generation guarantees + +`calm generate` does not read `items`. It builds one node per `prefixItems` entry and +nothing else, so a node declared in `items` never reaches the architecture. `calm validate` +accepts the pattern, and the missing node is silent. diff --git a/cli/test_fixtures/validate_output_junit.xml b/cli/test_fixtures/validate_output_junit.xml index a482fbe73..bbed7fb21 100644 --- a/cli/test_fixtures/validate_output_junit.xml +++ b/cli/test_fixtures/validate_output_junit.xml @@ -1,10 +1,10 @@ - + - @@ -41,7 +41,10 @@ - + + + + \ No newline at end of file diff --git a/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.spec.ts b/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.spec.ts new file mode 100644 index 000000000..33174ae3a --- /dev/null +++ b/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.spec.ts @@ -0,0 +1,44 @@ +import { RulesetFunctionContext } from '@stoplight/spectral-core'; +import { decisionIsDeclaredInPrefixItems } from './decision-is-declared-in-prefix-items'; + +const asContext = () => ({ path: ['properties', 'relationships', 'items', 'oneOf', 0] } as unknown as RulesetFunctionContext); + +const decision = (id: string) => ({ + properties: { + 'unique-id': { const: id }, + 'relationship-type': { properties: { options: { prefixItems: [] } } } + } +}); + +const connects = (id: string) => ({ + properties: { + 'unique-id': { const: id }, + 'relationship-type': { const: { connects: { source: { node: 'a' }, destination: { node: 'b' } } } } + } +}); + +describe('decisionIsDeclaredInPrefixItems', () => { + it('reports a decision', () => { + const results = decisionIsDeclaredInPrefixItems(decision('add-ons'), null, asContext()); + expect(results).toHaveLength(1); + expect(results[0].message).toContain('The decision \'add-ons\' is declared in items'); + }); + + it('reports at the path of the declaration', () => { + const results = decisionIsDeclaredInPrefixItems(decision('add-ons'), null, asContext()); + expect(results[0].path).toEqual(['properties', 'relationships', 'items', 'oneOf', 0]); + }); + + it('names the decision as unknown when it declares no id', () => { + const anonymous = { properties: { 'relationship-type': { properties: { options: {} } } } }; + expect(decisionIsDeclaredInPrefixItems(anonymous, null, asContext())[0].message).toContain('\'unknown\''); + }); + + it('accepts a relationship that is not a decision', () => { + expect(decisionIsDeclaredInPrefixItems(connects('w-d'), null, asContext())).toEqual([]); + }); + + it('accepts no input', () => { + expect(decisionIsDeclaredInPrefixItems(undefined, null, asContext())).toEqual([]); + }); +}); diff --git a/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.ts b/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.ts new file mode 100644 index 000000000..ca04c78df --- /dev/null +++ b/shared/src/spectral/functions/pattern/decision-is-declared-in-prefix-items.ts @@ -0,0 +1,19 @@ +import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; +import { declaredId, declaresOptions } from './declaration-paths'; + +/** + * An architecture always contains every relationship a pattern declares in prefixItems, so + * a decision declared there is always asked. An items member may be left out, so a + * decision declared there can vanish, and declining it stops being a choice. + */ +export function decisionIsDeclaredInPrefixItems(input: unknown, _: unknown, context: RulesetFunctionContext): IFunctionResult[] { + const relationship = input as object; + if (!relationship || !declaresOptions(relationship)) { + return []; + } + + return [{ + message: `The decision '${declaredId(relationship) ?? 'unknown'}' is declared in items. Declare a decision in relationships prefixItems, so that an answer can decline it.`, + path: [...context.path], + }]; +} diff --git a/shared/src/spectral/functions/pattern/declaration-paths.spec.ts b/shared/src/spectral/functions/pattern/declaration-paths.spec.ts index 4413d9532..5e3efe80d 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.spec.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.spec.ts @@ -1,4 +1,4 @@ -import { alternativeIdPaths, containingDeclaration, containingEntry, declarationPaths, declaredId, declaredIdPaths, declaredInterfaceIdPaths, fixedIdPath, isAlternative } from './declaration-paths'; +import { choiceIdPaths, containingDeclaration, declarationPaths, declaredId, declaredIdPaths, declaredInterfaceIdPaths, exclusiveGroup, fixedIdPath, isAlternative, twoKeywordSites } from './declaration-paths'; const ENTRY = '/properties/nodes/prefixItems/0'; const ALTERNATIVE = `${ENTRY}/oneOf/1`; @@ -6,16 +6,27 @@ 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', () => { + it('covers the fixed entry, both entry keywords and both items keywords', () => { expect(declarationPaths('relationships')).toEqual([ '$.properties.relationships.prefixItems[*]', '$.properties.relationships.prefixItems[*].oneOf[*]', - '$.properties.relationships.prefixItems[*].anyOf[*]' + '$.properties.relationships.prefixItems[*].anyOf[*]', + '$.properties.relationships.items.oneOf[*]', + '$.properties.relationships.items.anyOf[*]' + ]); + }); + + it('names every site a choice can declare both keywords at', () => { + expect(twoKeywordSites()).toEqual([ + '$.properties.nodes.prefixItems[?(@.oneOf && @.anyOf)]', + '$.properties.nodes[?(@property === "items" && @.oneOf && @.anyOf)]', + '$.properties.relationships.prefixItems[?(@.oneOf && @.anyOf)]', + '$.properties.relationships[?(@property === "items" && @.oneOf && @.anyOf)]' ]); }); it('builds id paths from the fixed entry followed by the alternatives', () => { - expect(declaredIdPaths('nodes')).toEqual([fixedIdPath('nodes'), ...alternativeIdPaths('nodes')]); + expect(declaredIdPaths('nodes')).toEqual([fixedIdPath('nodes'), ...choiceIdPaths('nodes')]); }); it('reaches interfaces on every node declaration site', () => { @@ -39,7 +50,7 @@ describe('declaration pointers', () => { '/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(exclusiveGroup(pointer)).toBe(entry); expect(isAlternative(pointer)).toBe(alternative); }); diff --git a/shared/src/spectral/functions/pattern/declaration-paths.ts b/shared/src/spectral/functions/pattern/declaration-paths.ts index 3ddad86dd..17a161d04 100644 --- a/shared/src/spectral/functions/pattern/declaration-paths.ts +++ b/shared/src/spectral/functions/pattern/declaration-paths.ts @@ -4,75 +4,102 @@ export type CalmType = 'nodes' | 'relationships'; const ALTERNATIVE_KEYWORDS = ['oneOf', 'anyOf']; const ID = 'properties.unique-id.const'; +const OPTIONS = 'properties.relationship-type.properties.options'; 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+$`); +const CALM_TYPE = '(?:nodes|relationships)'; +const DECLARATION = new RegExp(`^/properties/${CALM_TYPE}/(?:prefixItems/\\d+(?:/${ALTERNATIVES}/\\d+)?|items/${ALTERNATIVES}/\\d+)`); +const ENTRY_ALTERNATIVE = new RegExp(`^(/properties/${CALM_TYPE}/prefixItems/\\d+)/${ALTERNATIVES}/\\d+$`); -function entryPath(calmType: CalmType): string { +function fixedPath(calmType: CalmType): string { return `$.properties.${calmType}.prefixItems[*]`; } -function alternativePaths(calmType: CalmType): string[] { - return ALTERNATIVE_KEYWORDS.map(keyword => `${entryPath(calmType)}.${keyword}[*]`); +function choicePaths(calmType: CalmType): string[] { + return [fixedPath(calmType), `$.properties.${calmType}.items`].flatMap(base => + ALTERNATIVE_KEYWORDS.map(keyword => `${base}.${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)]; + return [fixedPath(calmType), ...choicePaths(calmType)]; } export function fixedIdPath(calmType: CalmType): string { - return `${entryPath(calmType)}.${ID}`; + return `${fixedPath(calmType)}.${ID}`; } -export function alternativeIdPaths(calmType: CalmType): string[] { - return alternativePaths(calmType).map(path => `${path}.${ID}`); +export function choiceIdPaths(calmType: CalmType): string[] { + return choicePaths(calmType).map(path => `${path}.${ID}`); } export function declaredIdPaths(calmType: CalmType): string[] { - return [fixedIdPath(calmType), ...alternativeIdPaths(calmType)]; + return [fixedIdPath(calmType), ...choiceIdPaths(calmType)]; } export function declaredInterfaceIdPaths(): string[] { return declarationPaths('nodes').map(path => `${path}.${INTERFACES}.${ID}`); } +/** + * Every site at which a pattern can declare both keywords, as Spectral `given` selectors. + */ +export function twoKeywordSites(): string[] { + return (['nodes', 'relationships'] as CalmType[]).flatMap(calmType => [ + `$.properties.${calmType}.prefixItems[?(@.oneOf && @.anyOf)]`, + `$.properties.${calmType}[?(@property === "items" && @.oneOf && @.anyOf)]`, + ]); +} + export function declaredId(declaration: object): string | undefined { return get(declaration, ID); } +/** + * A relationship that carries options is a decision: it asks which alternatives to include. + */ +export function declaresOptions(relationship: object): boolean { + return get(relationship, OPTIONS) !== undefined; +} + +// Reading a pointer back. A query run with `resultType: 'all'` returns each hit with the +// JSON Pointer it was found at, the only surviving trace of which site the hit came from. + /** * 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; + return pointer.match(DECLARATION)?.[0] ?? pointer; } -export function containingEntry(pointer: string): string { - return containingDeclaration(pointer).split(ALTERNATIVE_SUFFIX)[0]; +/** + * Declarations sharing a group never appear in the same architecture. Only the + * alternatives of one prefixItems entry qualify, because the entry is one position and one + * of them wins. An items member competes with nothing, since items admits any number. + */ +export function exclusiveGroup(pointer: string): string { + const declaration = containingDeclaration(pointer); + return declaration.match(ENTRY_ALTERNATIVE)?.[1] ?? declaration; } export function isAlternative(pointer: string): boolean { - return containingDeclaration(pointer) !== containingEntry(pointer); + return ENTRY_ALTERNATIVE.test(containingDeclaration(pointer)); } function declarationIndices(pointer: string): number[] { - return (containingDeclaration(pointer).match(/\d+/g) ?? []).map(Number); + const declaration = containingDeclaration(pointer); + const indices = (declaration.match(/\d+/g) ?? []).map(Number); + return [declaration.includes('/items/') ? 1 : 0, ...indices]; } /** - * 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. + * Orders declarations as an architecture fills the array: every prefixItems entry, then + * every items member. The indices decide it, not the pointer text, which sorts "items" + * ahead of "prefixItems" and an alternative ahead of the entry that holds it. 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); 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 8f836b7a8..a839fac15 100644 --- a/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts +++ b/shared/src/spectral/functions/pattern/ids-are-unique.spec.ts @@ -569,4 +569,60 @@ describe('idsAreUnique', () => { const result = idsAreUnique(input, null, asContext(context)); expect(result[0].message).toContain('path: /properties/nodes/prefixItems/11/properties/unique-id/const'); }); + it('should blame the items member when a fixed entry declares the same id', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + prefixItems: [{ 'properties': { 'unique-id': { 'const': 'dup' } } }], + items: { 'oneOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] } + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/items/oneOf/0/properties/unique-id/const'); + }); + + it('should blame the items member however the pattern orders its keys', () => { + const input = {}; + const context = { + document: { + data: { + properties: { + nodes: { + items: { 'oneOf': [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] }, + prefixItems: [{ 'properties': { 'unique-id': { 'const': 'dup' } } }] + } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/items/oneOf/0/properties/unique-id/const'); + }); + + it('should blame the items member for an interface id a fixed node already declares', () => { + const input = {}; + const withPort = (id: string) => ({ 'properties': { + 'unique-id': { 'const': id }, + 'interfaces': { prefixItems: [{ 'properties': { 'unique-id': { 'const': 'port' } } }] } } }); + const context = { + document: { + data: { + properties: { + nodes: { prefixItems: [withPort('database')], items: { 'oneOf': [withPort('cache')] } } + } + } + } + }; + + const result = idsAreUnique(input, null, asContext(context)); + expect(result[0].message).toContain('path: /properties/nodes/items/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 c7086ce72..0a0d82c68 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 { groupBy, partition } from 'lodash'; import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; import { detectDuplicates, JSONPathMatch } from '../helper-functions'; -import { byBuildOrder, containingDeclaration, containingEntry, declaredIdPaths, declaredInterfaceIdPaths, isAlternative } from './declaration-paths'; +import { byBuildOrder, containingDeclaration, declaredIdPaths, declaredInterfaceIdPaths, exclusiveGroup, isAlternative } from './declaration-paths'; /** * The rule blames the second declaration it sees, but one query per declaration site means @@ -22,8 +22,8 @@ function groupMatches(matches: JSONPathMatch[], key: (pointer: string) => string * 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)); + for (const group of groupMatches(matches, exclusiveGroup)) { + const [choices, fixed] = partition(group, match => isAlternative(match.pointer)); detectDuplicates(fixed, seenIds, messages); groupMatches(choices, containingDeclaration).forEach(choice => detectDuplicates(choice, new Set(seenIds), messages)); 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 index a78e1be41..e0802b05b 100644 --- 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 @@ -35,6 +35,16 @@ describe('isDefinedInOneOfOrAnyOf', () => { expect(messagesFor(data, 'redis')).toEqual([]); }); + it('accepts an id declared in an items catalogue', () => { + const data = { properties: { nodes: { prefixItems: [], items: { oneOf: [declaration('redis')] } } } }; + expect(messagesFor(data, 'redis')).toEqual([]); + }); + + it('accepts a relationship id declared in an items catalogue', () => { + const data = { properties: { relationships: { prefixItems: [], items: { anyOf: [declaration('cache-link')] } } } }; + expect(messagesFor(data, 'cache-link', 'relationships')).toEqual([]); + }); + it('accepts an id the pattern does not declare', () => { const data = pattern('nodes', [declaration('webapp')]); expect(messagesFor(data, 'redis')).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 265e22408..61f45773e 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 { alternativeIdPaths, CalmType, fixedIdPath } from './declaration-paths'; +import { CalmType, choiceIdPaths, fixedIdPath } from './declaration-paths'; /** * Checks that the input value should be defined in a oneOf or anyOf block. */ @@ -10,12 +10,12 @@ export function isDefinedInOneOfOrAnyOf(input: unknown, { calmType }: { calmType } const fixed = JSONPath({ path: fixedIdPath(calmType), json: context.document.data as object }); - const inAlternatives = alternativeIdPaths(calmType).flatMap(path => + const inChoices = choiceIdPaths(calmType).flatMap(path => JSONPath({ path, json: context.document.data as object })); const results: IFunctionResult[] = []; - if (fixed.includes(input) && !inAlternatives.includes(input)) { + if (fixed.includes(input) && !inChoices.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/items-fit-within-max-items.spec.ts b/shared/src/spectral/functions/pattern/items-fit-within-max-items.spec.ts new file mode 100644 index 000000000..af49bb117 --- /dev/null +++ b/shared/src/spectral/functions/pattern/items-fit-within-max-items.spec.ts @@ -0,0 +1,52 @@ +import { RulesetFunctionContext } from '@stoplight/spectral-core'; +import { itemsFitWithinMaxItems } from './items-fit-within-max-items'; + +const asContext = () => ({ path: ['properties', 'nodes'] } as unknown as RulesetFunctionContext); +const run = (array: unknown) => itemsFitWithinMaxItems(array, null, asContext()); + +const choice = { oneOf: [{}] }; + +describe('itemsFitWithinMaxItems', () => { + it('reports an array whose entries already fill maxItems', () => { + const result = run({ prefixItems: [{}], maxItems: 1, items: choice }); + expect(result).toHaveLength(1); + expect(result[0].message).toContain('maxItems is 1 and 1 prefixItems entries already fill it'); + }); + + it('reports an array with no entries and no room', () => { + expect(run({ maxItems: 0, items: choice })).toHaveLength(1); + }); + + it('reports at the path of the array', () => { + expect(run({ prefixItems: [{}], maxItems: 1, items: choice })[0].path).toEqual(['properties', 'nodes']); + }); + + it('accepts an array with room to spare', () => { + expect(run({ prefixItems: [{}], maxItems: 2, items: choice })).toEqual([]); + }); + + it('accepts an array that declares no maxItems', () => { + expect(run({ prefixItems: [{}], items: choice })).toEqual([]); + }); + + it('accepts an array with no items block', () => { + expect(run({ prefixItems: [{}], maxItems: 1 })).toEqual([]); + }); + + it('accepts an items block that offers no choice', () => { + expect(run({ prefixItems: [{}], maxItems: 1, items: { $ref: 'core.json#/defs/node' } })).toEqual([]); + }); + + it('accepts an items block whose choice is empty, because it declares no member', () => { + expect(run({ prefixItems: [{}], maxItems: 1, items: { oneOf: [] } })).toEqual([]); + expect(run({ prefixItems: [{}], maxItems: 1, items: { anyOf: [] } })).toEqual([]); + }); + + it('accepts an anyOf items block with room', () => { + expect(run({ prefixItems: [{}], maxItems: 3, items: { anyOf: [{}] } })).toEqual([]); + }); + + it('accepts no input', () => { + expect(run(undefined)).toEqual([]); + }); +}); diff --git a/shared/src/spectral/functions/pattern/items-fit-within-max-items.ts b/shared/src/spectral/functions/pattern/items-fit-within-max-items.ts new file mode 100644 index 000000000..8b7ed2fdb --- /dev/null +++ b/shared/src/spectral/functions/pattern/items-fit-within-max-items.ts @@ -0,0 +1,26 @@ +import { IFunctionResult, RulesetFunctionContext } from '@stoplight/spectral-core'; + +interface CalmArray { + maxItems?: number; + prefixItems?: unknown[]; + items?: { oneOf?: unknown[]; anyOf?: unknown[] }; +} + +/** + * `maxItems` counts the whole array, and the `prefixItems` entries fill it from the front. + * An `items` member can only be built in what is left over. + */ +export function itemsFitWithinMaxItems(input: unknown, _: unknown, context: RulesetFunctionContext): IFunctionResult[] { + const array = input as CalmArray; + const offersChoice = Boolean(array?.items?.oneOf?.length || array?.items?.anyOf?.length); + const positions = array?.prefixItems?.length ?? 0; + + if (!offersChoice || array.maxItems === undefined || array.maxItems > positions) { + return []; + } + + return [{ + message: `maxItems is ${array.maxItems} and ${positions} prefixItems entries already fill it, so no items member can ever be built. Raise maxItems or remove items.`, + path: [...context.path], + }]; +} diff --git a/shared/src/spectral/rules-pattern.spec.ts b/shared/src/spectral/rules-pattern.spec.ts index 98533acde..2a5eebf4c 100644 --- a/shared/src/spectral/rules-pattern.spec.ts +++ b/shared/src/spectral/rules-pattern.spec.ts @@ -75,7 +75,7 @@ describe('pattern ruleset', () => { }); }); - describe('pattern-prefix-items-must-declare-one-keyword', () => { + describe('pattern-choice-must-declare-one-keyword', () => { it('accepts an entry that declares one keyword', async () => { const codes = await codesFor({ properties: { @@ -83,7 +83,7 @@ describe('pattern ruleset', () => { relationships: { prefixItems: [] } } }); - expect(codes).not.toContain('pattern-prefix-items-must-declare-one-keyword'); + expect(codes).not.toContain('pattern-choice-must-declare-one-keyword'); }); it('rejects an entry that declares both keywords', async () => { @@ -93,7 +93,7 @@ describe('pattern ruleset', () => { relationships: { prefixItems: [] } } }); - expect(codes).toContain('pattern-prefix-items-must-declare-one-keyword'); + expect(codes).toContain('pattern-choice-must-declare-one-keyword'); }); it('rejects a relationships entry that declares both keywords', async () => { @@ -103,7 +103,7 @@ describe('pattern ruleset', () => { relationships: { prefixItems: [{ oneOf: [node('a')], anyOf: [node('b')] }] } } }); - expect(codes).toContain('pattern-prefix-items-must-declare-one-keyword'); + expect(codes).toContain('pattern-choice-must-declare-one-keyword'); }); it('reports the location of the offending entry', async () => { @@ -112,7 +112,7 @@ describe('pattern ruleset', () => { nodes: { prefixItems: [{ oneOf: [node('cache')], anyOf: [node('queue')] }] }, relationships: { prefixItems: [] } } - }, 'pattern-prefix-items-must-declare-one-keyword'); + }, 'pattern-choice-must-declare-one-keyword'); expect(path).toBe('/properties/nodes/prefixItems/0'); }); }); @@ -138,4 +138,192 @@ describe('pattern ruleset', () => { expect(codes).toContain('connects-relationship-references-existing-nodes-in-pattern'); }); }); + describe('items', () => { + it('resolves a node declared in items', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [connects('w-c', 'cache', 'webapp'), decision('add-ons', ['cache'])] } + } + }); + expect(codes).not.toContain('connects-relationship-references-existing-nodes-in-pattern'); + expect(codes).not.toContain('pattern-nodes-must-be-referenced'); + }); + + it('warns about an items member that nothing references', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), node('database')], items: { oneOf: [node('orphan')] } }, + relationships: { prefixItems: [connects('w-d', 'webapp', 'database')] } + } + }); + expect(codes).toContain('pattern-nodes-must-be-referenced'); + }); + + it('reports an id shared by two items members, which can both appear', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('dup'), node('dup')] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).toContain('unique-ids-must-be-unique-in-pattern'); + }); + + it('reports an interface id shared by two items members, which can both appear', async () => { + const cache = { properties: { 'unique-id': { const: 'cache' }, interfaces: { prefixItems: [{ properties: { 'unique-id': { const: 'port' } } }] } } }; + const queue = { properties: { 'unique-id': { const: 'queue' }, interfaces: { prefixItems: [{ properties: { 'unique-id': { const: 'port' } } }] } } }; + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [cache, queue] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).toContain('unique-ids-must-be-unique-in-pattern'); + }); + + it('rejects an items block declaring both keywords', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')], anyOf: [node('queue')] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).toContain('pattern-choice-must-declare-one-keyword'); + }); + + it('reports the location of the offending items block', async () => { + const path = await pathFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')], anyOf: [node('queue')] } }, + relationships: { prefixItems: [] } + } + }, 'pattern-choice-must-declare-one-keyword'); + expect(path).toBe('/properties/nodes/items'); + }); + }); + + describe('pattern-decision-must-be-declared-in-prefix-items', () => { + it('accepts a decision declared in relationships prefixItems', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [decision('add-ons', ['cache'])] } + } + }); + expect(codes).not.toContain('pattern-decision-must-be-declared-in-prefix-items'); + }); + + it('rejects a decision declared in relationships items', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [], items: { oneOf: [decision('add-ons', ['cache'])] } } + } + }); + expect(codes).toContain('pattern-decision-must-be-declared-in-prefix-items'); + }); + + it('accepts a plain relationship declared in relationships items', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), node('database')] }, + relationships: { prefixItems: [], items: { anyOf: [connects('w-d', 'webapp', 'database')] } } + } + }); + expect(codes).not.toContain('pattern-decision-must-be-declared-in-prefix-items'); + }); + }); + describe('nodes and relationships referenced by a decision', () => { + it('accepts a decision option referencing a node declared in an items catalogue', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [decision('add-ons', ['cache'])] } + } + }); + expect(codes).not.toContain('nodes-referenced-in-pattern-decision-must-be-in-oneof-or-anyof-block'); + }); + + it('reports a decision option referencing a node declared only as a fixed entry', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp'), node('cache')] }, + relationships: { prefixItems: [decision('add-ons', ['cache'])] } + } + }); + expect(codes).toContain('nodes-referenced-in-pattern-decision-must-be-in-oneof-or-anyof-block'); + }); + }); + + describe('pattern-decision-options-must-be-declared-in-prefix-items', () => { + const decisionWithOptions = (options: object) => ({ + properties: { + 'unique-id': { const: 'add-ons' }, + description: { const: 'Pick an alternative' }, + 'relationship-type': { properties: { options } } + } + }); + const optionBlock = { + anyOf: [{ + properties: { + description: { const: 'Use cache' }, + nodes: { const: ['cache'] }, + relationships: { const: [] } + } + }] + }; + + it('accepts option blocks declared in options prefixItems', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [decisionWithOptions({ prefixItems: [optionBlock] })] } + } + }); + expect(codes).not.toContain('pattern-decision-options-must-be-declared-in-prefix-items'); + }); + + it('rejects option blocks declared in options items, which calm generate never reads', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [decisionWithOptions({ items: optionBlock })] } + } + }); + expect(codes).toContain('pattern-decision-options-must-be-declared-in-prefix-items'); + }); + }); + + describe('pattern-items-must-fit-within-max-items', () => { + it('rejects a pattern whose maxItems leaves no room for items', async () => { + const codes = await codesFor({ + properties: { + nodes: { minItems: 1, maxItems: 1, prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).toContain('pattern-items-must-fit-within-max-items'); + }); + + it('accepts a pattern whose maxItems leaves room', async () => { + const codes = await codesFor({ + properties: { + nodes: { minItems: 1, maxItems: 2, prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).not.toContain('pattern-items-must-fit-within-max-items'); + }); + + it('accepts a pattern that declares no maxItems', async () => { + const codes = await codesFor({ + properties: { + nodes: { prefixItems: [node('webapp')], items: { oneOf: [node('cache')] } }, + relationships: { prefixItems: [] } + } + }); + expect(codes).not.toContain('pattern-items-must-fit-within-max-items'); + }); + }); }); diff --git a/shared/src/spectral/rules-pattern.ts b/shared/src/spectral/rules-pattern.ts index b2cdbb48a..d3cb7a2d1 100644 --- a/shared/src/spectral/rules-pattern.ts +++ b/shared/src/spectral/rules-pattern.ts @@ -7,7 +7,9 @@ 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'; +import { decisionIsDeclaredInPrefixItems } from './functions/pattern/decision-is-declared-in-prefix-items'; +import { itemsFitWithinMaxItems } from './functions/pattern/items-fit-within-max-items'; +import { declaredIdPaths, twoKeywordSites } from './functions/pattern/declaration-paths'; const patternRules: RulesetDefinition = { @@ -188,15 +190,40 @@ const patternRules: RulesetDefinition = { }, }, }, - 'pattern-prefix-items-must-declare-one-keyword': { - description: 'A prefixItems entry must declare either oneOf or anyOf, not both', + 'pattern-choice-must-declare-one-keyword': { + description: 'A choice 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)]', - ], + message: 'A choice declares both \'oneOf\' and \'anyOf\'. An element must satisfy both, so some alternatives can never be selected. Declare one keyword.', + given: twoKeywordSites(), + then: { + function: falsy, + }, + }, + 'pattern-items-must-fit-within-max-items': { + description: 'maxItems must leave room for an items member to be built', + severity: 'error', + message: '{{error}}', + given: ['$.properties.nodes', '$.properties.relationships'], + then: { + function: itemsFitWithinMaxItems, + }, + }, + 'pattern-decision-must-be-declared-in-prefix-items': { + description: 'A decision must be declared in relationships prefixItems, not in items', + severity: 'error', + message: '{{error}}', + given: ['$.properties.relationships.items.oneOf[*]', '$.properties.relationships.items.anyOf[*]'], + then: { + function: decisionIsDeclaredInPrefixItems, + }, + }, + 'pattern-decision-options-must-be-declared-in-prefix-items': { + description: 'Decision options must be declared in options prefixItems, not in items', + severity: 'error', + message: 'Declare decision options in options prefixItems. calm generate reads prefixItems only, so an options block declared in items is never built.', + given: '$..relationship-type.properties.options', then: { + field: 'items', function: falsy, }, },