Repository: stoplightio/spectral
Pull request #1300 (2020-08-03 22:15:16)

perf(pattern): improve general perf
Leverages the cache mechanism we added back in 5.4 so that each regular expression does not to be compiled each time.
Moreover, we avoid an allocation of an array if not required.
It's not like the perf gain is huge, but it's like 2-4% on average, which is still something.

Oh, and I expose cache to built-in (core) functions, because they didn't have access to it beforehand.

**Checklist**

- [x] Tests added / updated
- [x] Docs added / updated

**Does this PR introduce a breaking change?**

- [ ] Yes
- [x] No




Patch:
diff --git a/src/functions/pattern.ts b/src/functions/pattern.ts
index d8da21fc..9791f8d1 100644
--- a/src/functions/pattern.ts
+++ b/src/functions/pattern.ts
@@ -1,4 +1,5 @@
-import { IFunction, IFunctionResult } from '../types';
+import type { IFunction, IFunctionContext, IFunctionResult } from '../types';
+import type { Optional } from '@stoplight/types';
 
 export interface IRulePatternOptions {
   /** regex that target must match */
@@ -8,46 +9,65 @@ export interface IRulePatternOptions {
   notMatch?: string;
 }
 
-function test(value: string, regex: RegExp | string) {
-  let re;
-  if (typeof regex === 'string') {
-    // regex in a string like {"match": "/[a-b]+/im"} or {"match": "[a-b]+"} in a json ruleset
-    // the available flags are "gimsuy" as described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
-    const splitRegex = /^\/(.+)\/([a-z]*)$/.exec(regex);
-    if (splitRegex) {
-      // with slashes like /[a-b]+/ and possibly with flags like /[a-b]+/im
-      re = new RegExp(splitRegex[1], splitRegex[2]);
-    } else {
-      // without slashes like [a-b]+
-      re = new RegExp(regex);
-    }
+// regex in a string like {"match": "/[a-b]+/im"} or {"match": "[a-b]+"} in a json ruleset
+// the available flags are "gimsuy" as described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
+const REGEXP_PATTERN = /^\/(.+)\/([a-z]*)$/;
+
+function getFromCache(cache: Map<string, RegExp>, pattern: string): RegExp {
+  const existingPattern = cache.get(pattern);
+  if (existingPattern !== void 0) {
+    return existingPattern;
+  }
+
+  const newPattern = createRegex(pattern);
+  cache.set(pattern, newPattern);
+  return newPattern;
+}
+
+function createRegex(pattern: string): RegExp {
+  const splitRegex = REGEXP_PATTERN.exec(pattern);
+  if (splitRegex !== null) {
+    // with slashes like /[a-b]+/ and possibly with flags like /[a-b]+/im
+    return new RegExp(splitRegex[1], splitRegex[2]);
   } else {
-    // true regex
-    re = new RegExp(regex);
+    // without slashes like [a-b]+
+    return new RegExp(pattern);
   }
-  return re.test(value);
 }
 
-export const pattern: IFunction<IRulePatternOptions> = (targetVal, opts) => {
+export const pattern: IFunction<IRulePatternOptions> = function (this: IFunctionContext, targetVal, opts) {
   if (typeof targetVal !== 'string') return;
 
-  const results: IFunctionResult[] = [];
+  let results: Optional<IFunctionResult[]>;
 
   const { match, notMatch } = opts;
+  const cache = this.cache as Map<string, RegExp>;
+
+  if (match !== void 0) {
+    const pattern = getFromCache(cache, match);
 
-  if (match) {
-    if (test(targetVal, match) !== true) {
-      results.push({
-        message: `must match the pattern '${match}'`,
-      });
+    if (!pattern.test(targetVal)) {
+      results = [
+        {
+          message: `must match the pattern '${match}'`,
+        },
+      ];
     }
   }
 
-  if (notMatch) {
-    if (test(targetVal, notMatch) === true) {
-      results.push({
+  if (notMatch !== void 0) {
+    const pattern = getFromCache(cache, notMatch);
+
+    if (pattern.test(targetVal)) {
+      const result = {
         message: `must not match the pattern '${notMatch}'`,
-      });
+      };
+
+      if (results === void 0) {
+        results = [result];
+      } else {
+        results.push(result);
+      }
     }
   }
 
diff --git a/src/rulesets/evaluators.ts b/src/rulesets/evaluators.ts
index 781a6836..377f1af5 100644
--- a/src/rulesets/evaluators.ts
+++ b/src/rulesets/evaluators.ts
@@ -165,8 +165,12 @@ export const compileExportedFunction = ({ code, name, source, schema, inject }:
 };
 
 export function setFunctionContext(context: unknown, fn: Function) {
-  return Function.prototype.bind.call(
+  const boundFn = Function.prototype.bind.call(
     fn,
     Object.freeze(Object.defineProperties({}, Object.getOwnPropertyDescriptors(context))),
   );
+
+  Object.assign(boundFn, fn);
+
+  return boundFn;
 }
diff --git a/src/spectral.ts b/src/spectral.ts
index b407be93..31828540 100644
--- a/src/spectral.ts
+++ b/src/spectral.ts
@@ -44,10 +44,10 @@ export class Spectral {
   private readonly _resolver: IResolver;
   private readonly agent: Agent | undefined;
 
-  public functions: FunctionCollection & CoreFunctions = { ...coreFunctions };
-  public rules: RunRuleCollection = {};
-  public exceptions: RulesetExceptionCollection = {};
-  public formats: RegisteredFormats;
+  public readonly functions: FunctionCollection & CoreFunctions = { ...coreFunctions };
+  public readonly rules: RunRuleCollection = {};
+  public readonly exceptions: RulesetExceptionCollection = {};
+  public readonly formats: RegisteredFormats;
 
   protected readonly runtime: RunnerRuntime;
 
@@ -70,6 +70,8 @@ export class Spectral {
 
     this.formats = {};
     this.runtime = new RunnerRuntime();
+
+    this.setFunctions(coreFunctions);
   }
 
   public static registerStaticAssets(assets: Dictionary<string, string>) {
@@ -141,7 +143,16 @@ export class Spectral {
   public setFunctions(functions: FunctionCollection): void {
     empty(this.functions);
 
-    Object.assign(this.functions, { ...coreFunctions, ...functions });
+    const mergedFunctions = { ...coreFunctions, ...functions };
+
+    for (const key of Object.keys(mergedFunctions)) {
+      const context: IFunctionContext = {
+        functions: this.functions,
+        cache: new Map(),
+      };
+
+      this.functions[key] = setFunctionContext(context, mergedFunctions[key]);
+    }
   }
 
   public setRules(rules: RuleCollection): void {
@@ -198,29 +209,20 @@ export class Spectral {
             return fns;
           }
 
-          const context: IFunctionContext = {
-            functions: this.functions,
-            cache: new Map(),
-          };
-
-          fns[key] = setFunctionContext(
-            context,
-            compileExportedFunction({
-              code,
-              name,
-              source,
-              schema,
-              inject: {
-                fetch: request,
-                spectral: this.runtime.spawn(),
-              },
-            }),
-          );
+          fns[key] = compileExportedFunction({
+            code,
+            name,
+            source,
+            schema,
+            inject: {
+              fetch: request,
+              spectral: this.runtime.spawn(),
+            },
+          });
+
           return fns;
         },
-        {
-          ...coreFunctions,
-        },
+        {},
       ),
     );
 


Test patch:
diff --git a/src/functions/__tests__/pattern.test.ts b/src/functions/__tests__/pattern.test.ts
index a79bfa06..29b2bfe5 100644
--- a/src/functions/__tests__/pattern.test.ts
+++ b/src/functions/__tests__/pattern.test.ts
@@ -1,7 +1,10 @@
 import { pattern } from '../pattern';
 
 function runPattern(targetVal: any, options?: any) {
-  return pattern(
+  return pattern.call(
+    {
+      cache: new Map(),
+    },
     targetVal,
     options,
     {
@@ -15,37 +18,17 @@ function runPattern(targetVal: any, options?: any) {
 }
 
 describe('pattern', () => {
-  describe('given a true regex', () => {
-    test('should return empty array when given value matches the given match regex', () => {
-      expect(runPattern('abc', { match: /[abc]+/ })).toEqual([]);
-    });
-
-    test('should return a message when given value does not match the given notMatch regex', () => {
-      expect(runPattern('def', { match: /[abc]+/ })).toEqual([{ message: "must match the pattern '/[abc]+/'" }]);
-    });
-
-    test('should return empty array when given value does not match the given notMatch regex', () => {
-      expect(runPattern('dEf', { notMatch: /[abc]+/i })).toEqual([]);
-    });
-
-    test('should return a message when given value does match the given notMatch regex', () => {
-      expect(runPattern('aBc', { notMatch: /[abc]+/i })).toEqual([
-        { message: "must not match the pattern '/[abc]+/i'" },
-      ]);
-    });
-  });
-
   describe('given a string regex', () => {
     test('should return empty array when given value matches the given match string regex without slashes', () => {
-      expect(runPattern('abc', { match: '[abc]+' })).toEqual([]);
+      expect(runPattern('abc', { match: '[abc]+' })).toBeUndefined();
     });
 
     test('should return empty array when given value matches the given match string regex with slashes', () => {
-      expect(runPattern('abc', { match: '/[abc]+/' })).toEqual([]);
+      expect(runPattern('abc', { match: '/[abc]+/' })).toBeUndefined();
     });
 
     test('should return empty array when given value matches the given match string regex with slashes and modifier', () => {
-      expect(runPattern('aBc', { match: '/[abc]+/im' })).toEqual([]);
+      expect(runPattern('aBc', { match: '/[abc]+/im' })).toBeUndefined();
     });
 
     test('should throw an exception when given string regex contains invalid flags', () => {
@@ -55,13 +38,13 @@ describe('pattern', () => {
     });
 
     test('should return empty array when given value does not match the given notMatch string regex with slashes and modifier', () => {
-      expect(runPattern('def', { notMatch: '/[abc]+/i' })).toEqual([]);
+      expect(runPattern('def', { notMatch: '/[abc]+/i' })).toBeUndefined();
     });
   });
 
   describe('given match and notMatch regexes', () => {
     test('should return empty array when given value match the given match and does not match the given notMatch regexes', () => {
-      expect(runPattern('def', { match: /[def]+/, notMatch: /[abc]+/ })).toEqual([]);
+      expect(runPattern('def', { match: /[def]+/, notMatch: /[abc]+/ })).toBeUndefined();
     });
   });
 });
diff --git a/src/rulesets/__tests__/evaluators.test.ts b/src/rulesets/__tests__/evaluators.test.ts
index c420ef06..7a2ff185 100644
--- a/src/rulesets/__tests__/evaluators.test.ts
+++ b/src/rulesets/__tests__/evaluators.test.ts
@@ -96,5 +96,18 @@ describe('Code evaluators', () => {
       const fn2 = setFunctionContext(context, jest.fn().mockReturnThis());
       expect(fn()).not.toBe(fn2());
     });
+
+    it('copies enumerable properties', () => {
+      const context = { a: true };
+      const prop = Object.freeze({});
+      const fn = function () {
+        return;
+      };
+
+      fn.foo = prop;
+
+      const boundFn = setFunctionContext(context, fn);
+      expect(boundFn.foo).toBe(prop);
+    });
   });
 });