Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): improve alias validation #2164

Merged
merged 15 commits into from
Aug 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 43 additions & 1 deletion docs/guides/4-custom-rulesets.md
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ For now the JSON, YAML, and JS, are all being maintained, and there are no curre
Targeting certain parts of an OpenAPI spec is powerful but it can become cumbersome to write and repeat complex JSONPath expressions across various rules.
Define aliases for commonly used JSONPath expressions on a global level which can then be reused across the ruleset.

Aliases can be defined in an array of key-value pairs at the root level of the ruleset.
Aliases can be defined in an array of key-value pairs at the root level of the ruleset, or alternatively, within an override.
It's similar to `given`, with the notable difference being the possibility to distinguish between different formats.

**Example**
Expand Down Expand Up @@ -415,6 +415,48 @@ aliases:

Rulesets can then reference aliases in the [given](#given) keyword, either in full: `"given": "#Paths"`, or use it as a prefix for further JSONPath syntax, like dot notation: `"given": "#ParameterObject.name"`.

Bear in mind that an alias has to be explicitly defined in either at the top-level or inside an override.
This is to avoid ambiguity.

```yaml
aliases:
Stoplight:
- "$..stoplight"
overrides:
- files:
- "*.yaml"
rules:
value-matches-stoplight:
message: Value must contain Stoplight
given: "#Stoplight" # valid because declared at the root
severity: error
then:
field: description
function: pattern
functionOptions:
match: Stoplight
- files:
- "**/*.json"
aliases:
Value:
- "$..value"
Comment on lines +440 to +442
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a note re override behavior. E.g. - if an alias called Stoplight were defined here, would it take precedence over the Stoplight alias defined in the root?

rules:
truthy-stoplight-property:
message: Value must contain Stoplight
given: "#Value" # valid because declared within the override block
severity: error
then:
function: truthy
- files:
- legacy/**/*.json
rules:
falsy-value:
given: "#Value" # invalid because undeclared both at the top-level and the override. Note that this could be technically resolvable for some JSON documents, because the previous override block has the alias, but to spare some headaches, we demand an alias to be explicitly defined.
severity: error
then:
function: falsy
```

> This will be followed by our core rulesets providing a common set of aliases for OpenAPI and AsyncAPI so that our users don't have to do the work at all. If you have ideas about what kind of aliases could be useful leave your thoughts [here](https://roadmap.stoplight.io).

## Overrides
Expand Down
1 change: 1 addition & 0 deletions docs/reference/error-handling.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- a rule in the ruleset:
- had an invalid `given`, i.e. the JSON Path expression is not valid from syntax's standpoint
- the ruleset contains `except` entries and the input is passed through stdin
- a JSON Path alias cannot be resolved

### Runtime

Expand Down
16 changes: 0 additions & 16 deletions packages/cli/src/services/__tests__/linter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,6 @@ describe('Linter service', () => {
run(`lint ${validCustomOas3SpecPath} -r ${invalidNestedRulesetPath}`),
).rejects.toThrowAggregateError(
new AggregateError([
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-with-invalid-enum',
]),
new RulesetValidationError('the rule must have at least "given" and "then" properties', [
'rules',
'rule-without-given-nor-them',
Expand All @@ -242,10 +238,6 @@ describe('Linter service', () => {
'rule-with-invalid-enum',
'type',
]),
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError(
'the value has to be one of: 0, 1, 2, 3 or "error", "warn", "info", "hint", "off"',
['rules', 'rule-with-invalid-enum', 'severity'],
Expand All @@ -265,10 +257,6 @@ describe('Linter service', () => {
it('outputs "invalid ruleset" error', () => {
return expect(run(`lint ${validOas3SpecPath} -r ${invalidRulesetPath}`)).rejects.toThrowAggregateError(
new AggregateError([
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-with-invalid-enum',
]),
new RulesetValidationError('the rule must have at least "given" and "then" properties', [
'rules',
'rule-without-given-nor-them',
Expand All @@ -278,10 +266,6 @@ describe('Linter service', () => {
'rule-with-invalid-enum',
'type',
]),
new RulesetValidationError('must be equal to one of the allowed values', [
'rules',
'rule-without-given-nor-them',
]),
new RulesetValidationError(
'the value has to be one of: 0, 1, 2, 3 or "error", "warn", "info", "hint", "off"',
['rules', 'rule-with-invalid-enum', 'severity'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ const ruleset: RulesetDefinition = {
},
{
files: ['legacy/**/*.json'],
aliases: {
Value: ['$..value'],
},
rules: {
'falsy-value': {
given: '#Value',
Expand Down
30 changes: 26 additions & 4 deletions packages/core/src/ruleset/__tests__/ruleset.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import '@stoplight/spectral-test-utils/matchers';

import { oas2 } from '@stoplight/spectral-formats';
import { pattern, truthy } from '@stoplight/spectral-functions';
import * as path from '@stoplight/path';
import { DiagnosticSeverity } from '@stoplight/types';
import AggregateError = require('es-aggregate-error');

import { Ruleset } from '../ruleset';
import { RulesetDefinition } from '../types';
Expand Down Expand Up @@ -1266,7 +1269,11 @@ describe('Ruleset', () => {
},
},
}),
).toThrowError(ReferenceError('Alias "PathItem-" does not exist'));
).toThrowAggregateError(
new AggregateError([
new RulesetValidationError('Alias "PathItem-" does not exist', ['rules', 'valid-path', 'given']),
]),
);
});

it('given circular alias, should throw', () => {
Expand All @@ -1288,8 +1295,13 @@ describe('Ruleset', () => {
},
},
}),
).toThrowError(
ReferenceError('Alias "Test" is circular. Resolution stack: Test -> Contact -> Info -> Root -> Info'),
).toThrowAggregateError(
new AggregateError([
new RulesetValidationError(
'Alias "Test" is circular. Resolution stack: Test -> Contact -> Info -> Root -> Info',
['rules', 'valid-path', 'given'],
),
]),
);
});

Expand Down Expand Up @@ -1321,7 +1333,17 @@ describe('Ruleset', () => {
},
},
}),
).toThrowError(ReferenceError('Alias "PathItem" does not exist'));
).toThrowAggregateError(
new AggregateError([
new RulesetValidationError('Alias "PathItem" does not exist', ['rules', 'valid-path', 'given']),
new RulesetValidationError('Alias "Name" does not exist', ['rules', 'valid-name-and-description', 'given']),
new RulesetValidationError(`Alias "Description" does not exist`, [
'rules',
'valid-name-and-description',
'given',
]),
]),
);
});

describe('scoped aliases', () => {
Expand Down
83 changes: 83 additions & 0 deletions packages/core/src/ruleset/alias.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { isScopedAliasDefinition, isSimpleAliasDefinition } from './utils/guards';
import type { RulesetScopedAliasDefinition } from './types';

const ALIAS = /^#([A-Za-z0-9_-]+)/;

export function resolveAliasForFormats(
{ targets }: RulesetScopedAliasDefinition,
formats: Set<unknown> | null,
): string[] | null {
if (formats === null || formats.size === 0) {
return null;
}

// we start from the end to be consistent with overrides etc. - we generally tend to pick the "last" value.
for (let i = targets.length - 1; i >= 0; i--) {
const target = targets[i];
for (const format of target.formats) {
if (formats.has(format)) {
return target.given;
}
}
}

return null;
}

export function resolveAlias(
aliases: Record<string, unknown> | null,
expression: string,
formats: Set<unknown> | null,
): string[] {
return _resolveAlias(aliases, expression, formats, new Set());
}

function _resolveAlias(
aliases: Record<string, unknown> | null,
expression: string,
formats: Set<unknown> | null,
stack: Set<string>,
): string[] {
const resolvedExpressions: string[] = [];

if (expression.startsWith('#')) {
const alias = ALIAS.exec(expression)?.[1];

if (alias === void 0 || alias === null) {
throw new ReferenceError(`Alias must match /^#([A-Za-z0-9_-]+)/`);
}

if (stack.has(alias)) {
const _stack = [...stack, alias];
throw new ReferenceError(`Alias "${_stack[0]}" is circular. Resolution stack: ${_stack.join(' -> ')}`);
}

stack.add(alias);

if (aliases === null || !(alias in aliases)) {
throw new ReferenceError(`Alias "${alias}" does not exist`);
}

const aliasValue = aliases[alias];
let actualAliasValue: string[] | null;
if (isSimpleAliasDefinition(aliasValue)) {
actualAliasValue = aliasValue;
} else if (isScopedAliasDefinition(aliasValue)) {
actualAliasValue = resolveAliasForFormats(aliasValue, formats);
} else {
actualAliasValue = null;
}

if (actualAliasValue !== null) {
resolvedExpressions.push(
...actualAliasValue.flatMap(item =>
_resolveAlias(aliases, item + expression.slice(alias.length + 1), formats, new Set([...stack])),
),
);
}
} else {
resolvedExpressions.push(expression);
}

return resolvedExpressions;
}
120 changes: 63 additions & 57 deletions packages/core/src/ruleset/meta/rule.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,72 +21,78 @@
"$ref": "shared#severity"
}
},
"oneOf": [
{
"properties": {
"description": {
"type": "string"
},
"documentationUrl": {
"type": "string",
"format": "url",
"errorMessage": "must be a valid URL"
},
"recommended": {
"type": "boolean"
},
"given": {
"$ref": "shared#given"
},
"resolved": {
"type": "boolean"
},
"severity": {
"$ref": "#/$defs/Severity"
},
"message": {
"if": {
"type": "object"
},
"then": {
"type": "object",
"properties": {
"description": {
"type": "string"
},
"documentationUrl": {
"type": "string",
"format": "url",
"errorMessage": "must be a valid URL"
},
"recommended": {
"type": "boolean"
},
"given": {
"$ref": "shared#given"
},
"resolved": {
"type": "boolean"
},
"severity": {
"$ref": "#/$defs/Severity"
},
"message": {
"type": "string"
},
"tags": {
"items": {
"type": "string"
},
"tags": {
"items": {
"type": "string"
},
"type": "array"
},
"formats": {
"$ref": "shared#formats"
},
"then": {
"if": {
"type": "array"
},
"formats": {
"$ref": "shared#formats"
},
"then": {
"anyOf": [
{
"$ref": "#/$defs/Then"
},
{
"items": {
"$ref": "#/$defs/Then"
},
"type": "array"
}
]
"type": "array",
"items": {
"$ref": "#/$defs/Then"
}
},
"type": {
"enum": ["style", "validation"],
"type": "string",
"errorMessage": "allowed types are \"style\" and \"validation\""
"else": {
"$ref": "#/$defs/Then"
}
},
"required": ["given", "then"],
"type": "object",
"additionalProperties": false,
"errorMessage": {
"required": "the rule must have at least \"given\" and \"then\" properties"
"type": {
"enum": ["style", "validation"],
"type": "string",
"errorMessage": "allowed types are \"style\" and \"validation\""
}
},
{
"$ref": "shared#/$defs/HumanReadableSeverity"
},
{
"type": "boolean"
"required": ["given", "then"],
"additionalProperties": false,
"errorMessage": {
"required": "the rule must have at least \"given\" and \"then\" properties"
}
]
},
"else": {
"oneOf": [
{
"$ref": "shared#/$defs/HumanReadableSeverity"
},
{
"type": "boolean"
}
]
}
}
Loading