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: support async custom functions #1058

Merged
merged 11 commits into from
Apr 16, 2020
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: 44 additions & 0 deletions docs/guides/custom-functions.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

If the built-in functions are not enough for your [custom ruleset](../getting-started/rulesets.md), Spectral allows you to write and use your own custom functions.

As of Spectral 5.4.0, custom functions can also be asynchronous.

<!-- theme: warning -->

> Ideally linting should always be deterministic, which means if its run 10 times it should return the same results 10 times. To ensure this is the case, please refrain from introducing any logic that is prone to non-deterministic behavior. Examples of this might be contacting external service you have no control over, or that might be unstable, or change the way it responds over time.
> While, it may seem tempting to have a function that does so, the primary use case is to support libraries that makes async fs calls or exchanging information, i.e. obtaining a dictionary file, with a locally running server, etc.

Please, do keep in mind that for the time being, the code is **not** executed in a sandboxed environment, so be very careful when including external rulesets.
This indicates that almost any arbitrary code can be executed.
Potential risks include:
Expand Down Expand Up @@ -107,6 +114,43 @@ rules:
function: "abc"
```

#### Async Function Example

**functions/dictionary.js**

```js
const CACHE_KEY = 'dictionary';

module.exports = async function (targetVal) {
if (!this.cache.has(CACHE_KEY)) {
const res = await fetch('https://dictionary.com/evil');
if (res.ok) {
this.cache.set(CACHE_KEY, await res.json());
} else {
// you can either re-try or just throw an error
Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip What's the effect from throwing from within a function? Does the whole process crash down? Do we get a special kind of result or a console log?

From the function writer standpoint, what kind of code should be written

  • if I don't want this failure being unnoticed
  • if I prefer to see a CI build fail rather than having it pass for the wrong reasons

Note: I know this is kind off-topic with regards to the scope of the PR but... well... this is documentation and, to be honest, the doco actually lacks guidance about this.

Note 2: If you prefer this to be dealt with later, could you please log an issue to keep track of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@P0lip What's the effect from throwing from within a function? Does the whole process crash down? Do we get a special kind of result or a console log?

We'd get a console.log, same as we do when any other custom function throws an exception.
The whole process would still keep on going.

if I prefer to see a CI build fail rather than having it pass for the wrong reasons

I'd return a regular error, i.e.

return [{ message: 'Network error. The check could not be performed' }]

Copy link
Contributor

Choose a reason for hiding this comment

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

@P0lip That's very valuable info. I've logged #1096 as I think this deserves a doco section of its own.

}
}

const dictionary = this.cache.get(CACHE_KEY);

if (dictionary.includes(targetVal)) {
return [{ message: `\`${targetVal}\` is a forbidden word.` }];
}
};
```

**my-ruleset.yaml**

```yaml
functions: [dictionary]
rules:
no-evil-words:
message: "{{error}}"
given: ["$.info.title", "$.info.description"]
then:
function: "dictionary"
```

If you are writing a function that accepts options, you should provide a JSON Schema that describes those options.

You can do it as follows:
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@
"lodash": ">=4.17.5",
"nanoid": "^2.1.11",
"node-fetch": "^2.6",
"promise.allsettled": "^1.0.2",
"proxy-agent": "^3.1.1",
"strip-ansi": "^6.0",
"text-table": "^0.2",
Expand Down
148 changes: 148 additions & 0 deletions src/__tests__/linter.jest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { normalize } from '@stoplight/path';
import { DiagnosticSeverity } from '@stoplight/types';
import * as nock from 'nock';
import * as path from 'path';
import * as timers from 'timers';

import { isOpenApiv3 } from '../formats';
import { functions } from '../functions';
Expand Down Expand Up @@ -230,6 +231,153 @@ console.log(this.cache.get('test') || this.cache.set('test', []).get('test'));

expect(scope.isDone()).toBe(true);
});

describe('async functions', () => {
const fnName = 'asyncFn';

beforeEach(() => {
jest.useFakeTimers();

spectral.setRules({
'async-foo': {
given: '$',
severity: DiagnosticSeverity.Warning,
then: {
function: fnName,
},
},
});
});

afterEach(() => {
jest.useRealTimers();
});

it('should handle basic example', async () => {
spectral.setFunctions({
[fnName]() {
return new Promise(resolve => {
setTimeout(resolve, 200, [
{
message: 'Error reported by async fn',
},
]);
});
},
});

const result = spectral.run({
swagger: '2.0',
});

await new Promise(timers.setImmediate);

jest.advanceTimersByTime(200);

expect(await result).toEqual([
{
code: 'async-foo',
message: 'Error reported by async fn',
path: [],
range: expect.any(Object),
severity: DiagnosticSeverity.Warning,
},
]);
});

it('should handle rejections', async () => {
spectral.setFunctions({
[fnName]() {
return new Promise((resolve, reject) => {
setTimeout(reject, 1000, new Error('Some unknown error'));
});
},
});

const result = spectral.run({
swagger: '2.0',
});

await new Promise(timers.setImmediate);

jest.advanceTimersByTime(1000);

expect(await result).toEqual([]);
});

it('should be able to make actual requests', async () => {
spectral.setRuleset({
exceptions: {},
functions: {
[fnName]: {
name: fnName,
schema: null,
source: null,
code: `module.exports = async function (targetVal) {
if (!this.cache.has('dictionary')) {
const res = await fetch('https://dictionary.com/evil');
if (res.ok) {
this.cache.set('dictionary', await res.json());
} else {
// you can either re-try or just throw an error
}
}

const dictionary = this.cache.get('dictionary');

if (dictionary.includes(targetVal)) {
return [{ message: '\`' + targetVal + '\`' + ' is a forbidden word.' }];
}
}`,
},
},
rules: {
'no-evil-words': {
given: '$..*@string()',
severity: DiagnosticSeverity.Warning,
then: {
function: fnName,
},
},
},
});

nock('https://dictionary.com')
.persist()
.get('/evil')
.reply(200, JSON.stringify(['foo', 'bar', 'baz']));

const results = await spectral.run({
swagger: '2.0',
info: {
contact: {
email: 'foo',
author: 'baz',
},
},
paths: {
'/user': {},
},
});

expect(results).toEqual([
{
code: 'no-evil-words',
message: '`foo` is a forbidden word.',
path: ['info', 'contact', 'email'],
range: expect.any(Object),
severity: DiagnosticSeverity.Warning,
},
{
code: 'no-evil-words',
message: '`baz` is a forbidden word.',
path: ['info', 'contact', 'author'],
range: expect.any(Object),
severity: DiagnosticSeverity.Warning,
},
]);
});
});
});

it('should respect the scope of defined functions (ruleset-based)', async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/functions/schema-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export const schemaPath: IFunction<ISchemaPathOptions> = (targetVal, opts, paths
otherValues,
);

if (result !== void 0) {
if (Array.isArray(result)) {
results.push(...result);
}
}
Expand Down
85 changes: 48 additions & 37 deletions src/linter.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { decodePointerFragment } from '@stoplight/json';
import { get } from 'lodash';

import { JsonPath } from '@stoplight/types';
import { JsonPath, Optional } from '@stoplight/types';
import { Document } from './document';
import { DocumentInventory } from './documentInventory';
import { IMessageVars, message } from './rulesets/message';
import { getDiagnosticSeverity } from './rulesets/severity';
import { IFunction, IGivenNode, IRuleResult, IRunRule, IThen } from './types';
import { IRunningContext } from './runner';
import { IGivenNode, IRuleResult, IRunRule } from './types';
import { getClosestJsonPath, getLintTargets, printPath, PrintStyle } from './utils';
import { IExceptionLocation } from './utils/pivotExceptions';

Expand All @@ -24,38 +24,45 @@ const arePathsEqual = (one: JsonPath, another: JsonPath): boolean => {
return true;
};

const isAKnownException = (violation: IRuleResult, locations: IExceptionLocation[]): boolean => {
const isAKnownException = (
path: JsonPath,
source: Optional<string | null>,
locations: IExceptionLocation[],
): boolean => {
for (const location of locations) {
if (violation.source !== location.source) {
if (source !== location.source) {
continue;
}

if (arePathsEqual(violation.path, location.path)) {
if (arePathsEqual(path, location.path)) {
return true;
}
}

return false;
};

// TODO(SO-23): unit test but mock whatShouldBeLinted
export const lintNode = (
export const lintNode = async (
context: IRunningContext,
node: IGivenNode,
rule: IRunRule,
then: IThen<string, any>,
apply: IFunction,
inventory: DocumentInventory,
exceptionLocations: IExceptionLocation[] | undefined,
): IRuleResult[] => {
const givenPath = node.path[0] === '$' ? node.path.slice(1) : node.path;
const targets = getLintTargets(node.value, then.field);
exceptionLocations: Optional<IExceptionLocation[]>,
): Promise<IRuleResult[]> => {
const results: IRuleResult[] = [];

for (const target of targets) {
const targetPath = givenPath.concat(target.path);
for (const then of Array.isArray(rule.then) ? rule.then : [rule.then]) {
const func = context.functions[then.function];
if (typeof func !== 'function') {
throw new ReferenceError(`Function ${then.function} not found. Called by rule ${rule.name}.`);
}

const givenPath = node.path[0] === '$' ? node.path.slice(1) : node.path;
const targets = getLintTargets(node.value, then.field);

for (const target of targets) {
const targetPath = [...givenPath, ...target.path];

const targetResults =
apply(
const targetResults = await func(
target.value,
then.functionOptions || {},
{
Expand All @@ -65,19 +72,28 @@ export const lintNode = (
{
original: node.value,
given: node.value,
documentInventory: inventory,
documentInventory: context.documentInventory,
},
) || [];
);

results.push(
...targetResults.map<IRuleResult>(result => {
if (targetResults === void 0) continue;

for (const result of targetResults) {
const escapedJsonPath = (result.path || targetPath).map(segment => decodePointerFragment(String(segment)));
const associatedItem = inventory.findAssociatedItemForPath(escapedJsonPath, rule.resolved !== false);
const path = associatedItem?.path || getClosestJsonPath(inventory.resolved, escapedJsonPath);
const document = associatedItem?.document || inventory.document;
const associatedItem = context.documentInventory.findAssociatedItemForPath(
escapedJsonPath,
rule.resolved !== false,
);
const path = associatedItem?.path || getClosestJsonPath(context.documentInventory.resolved, escapedJsonPath);
const source = associatedItem?.document.source;

if (exceptionLocations !== void 0 && isAKnownException(path, source, exceptionLocations)) {
continue;
}

const document = associatedItem?.document || context.documentInventory.document;
const range = document.getRangeForJsonPath(path, true) || Document.DEFAULT_RANGE;
const value = path.length === 0 ? document.data : get(document.data, path);
const source = associatedItem?.document.source;

const vars: IMessageVars = {
property:
Expand All @@ -95,22 +111,17 @@ export const lintNode = (
const resultMessage = message(result.message, vars);
vars.error = resultMessage;

return {
results.push({
code: rule.name,
message: (rule.message === void 0 ? rule.description ?? resultMessage : message(rule.message, vars)).trim(),
path,
severity: getDiagnosticSeverity(rule.severity),
...(source !== null && { source }),
range,
};
}),
);
}

if (exceptionLocations === undefined) {
return results;
});
}
}
}

const filtered = results.filter(r => !isAKnownException(r, exceptionLocations));
return filtered;
return results;
};
Loading