From c05de8e176d7930fb959bc1a893cad9f3b984458 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 27 Mar 2022 10:04:21 +0200 Subject: [PATCH] feat(ng-dev/pullapprove): handle references to `author` in condition * feat(ng-dev/pullapprove): handle references to `author` in condition Necessary change for: https://github.com/angular/angular/pull/44866 --- ng-dev/pullapprove/BUILD.bazel | 1 + ng-dev/pullapprove/condition_errors.ts | 11 ++++ ng-dev/pullapprove/condition_evaluator.ts | 65 +++++++++++++---------- ng-dev/pullapprove/group.ts | 50 +++++++++-------- ng-dev/pullapprove/pullapprove_arrays.ts | 4 +- ng-dev/pullapprove/verify.spec.ts | 14 +++++ 6 files changed, 93 insertions(+), 52 deletions(-) create mode 100644 ng-dev/pullapprove/condition_errors.ts diff --git a/ng-dev/pullapprove/BUILD.bazel b/ng-dev/pullapprove/BUILD.bazel index 7c87e8187..6d1ff714e 100644 --- a/ng-dev/pullapprove/BUILD.bazel +++ b/ng-dev/pullapprove/BUILD.bazel @@ -4,6 +4,7 @@ ts_library( name = "pullapprove", srcs = [ "cli.ts", + "condition_errors.ts", "condition_evaluator.ts", "group.ts", "logging.ts", diff --git a/ng-dev/pullapprove/condition_errors.ts b/ng-dev/pullapprove/condition_errors.ts new file mode 100644 index 000000000..a7d6cb570 --- /dev/null +++ b/ng-dev/pullapprove/condition_errors.ts @@ -0,0 +1,11 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +export class PullApproveGroupStateDependencyError extends Error {} + +export class PullApproveAuthorStateDependencyError extends Error {} diff --git a/ng-dev/pullapprove/condition_evaluator.ts b/ng-dev/pullapprove/condition_evaluator.ts index 2d307663d..cb90ca4cf 100644 --- a/ng-dev/pullapprove/condition_evaluator.ts +++ b/ng-dev/pullapprove/condition_evaluator.ts @@ -6,23 +6,39 @@ * found in the LICENSE file at https://angular.io/license */ -import {PullApproveGroup} from './group'; import {PullApproveGroupArray, PullApproveStringArray} from './pullapprove_arrays'; + +import {PullApproveAuthorStateDependencyError} from './condition_errors'; +import {PullApproveGroup} from './group'; import {getOrCreateGlob} from './utils'; +import {runInNewContext} from 'vm'; /** - * Context that is provided to conditions. Conditions can use various helpers - * that PullApprove provides. We try to mock them here. Consult the official - * docs for more details: https://docs.pullapprove.com/config/conditions. + * Context object that will be used as global context in condition evaluation. + * + * Conditions can use various helpers that PullApprove provides. We try to + * mock them here. Consult the official docs for more details: + * https://docs.pullapprove.com/config/conditions. */ -const conditionContext = { - 'len': (value: any[]) => value.length, - 'contains_any_globs': (files: PullApproveStringArray, patterns: string[]) => { - // Note: Do not always create globs for the same pattern again. This method - // could be called for each source file. Creating glob's is expensive. - return files.some((f) => patterns.some((pattern) => getOrCreateGlob(pattern).match(f))); - }, -}; +const conditionEvaluationContext: object = (() => { + const context = { + 'len': (value: any[]) => value.length, + 'contains_any_globs': (files: PullApproveStringArray, patterns: string[]) => { + // Note: Do not always create globs for the same pattern again. This method + // could be called for each source file. Creating glob's is expensive. + return files.some((f) => patterns.some((pattern) => getOrCreateGlob(pattern).match(f))); + }, + }; + + // We cannot process references to `author` in conditions. + Object.defineProperty(context, 'author', { + get: () => { + throw new PullApproveAuthorStateDependencyError(); + }, + }); + + return context; +})(); /** * Converts a given condition to a function that accepts a set of files. The returned @@ -31,28 +47,19 @@ const conditionContext = { export function convertConditionToFunction( expr: string, ): (files: string[], groups: PullApproveGroup[]) => boolean { - // Creates a dynamic function with the specified expression. - // The first parameter will be `files` as that corresponds to the supported `files` variable that - // can be accessed in PullApprove condition expressions. The second parameter is the list of - // PullApproveGroups that are accessible in the condition expressions. The followed parameters - // correspond to other context variables provided by PullApprove for conditions. - const evaluateFn = new Function( - 'files', - 'groups', - ...Object.keys(conditionContext), - ` - return (${transformExpressionToJs(expr)}); - `, - ); + const jsExpression = ` + (files, groups) => { + return (${transformExpressionToJs(expr)}); + } + `; + const isMatchingFn = runInNewContext(jsExpression, conditionEvaluationContext); - // Create a function that calls the dynamically constructed function which mimics - // the condition expression that is usually evaluated with Python in PullApprove. return (files, groups) => { - const result = evaluateFn( + const result = isMatchingFn( new PullApproveStringArray(...files), new PullApproveGroupArray(...groups), - ...Object.values(conditionContext), ); + // If an array is returned, we consider the condition as active if the array is not // empty. This matches PullApprove's condition evaluation that is based on Python. if (Array.isArray(result)) { diff --git a/ng-dev/pullapprove/group.ts b/ng-dev/pullapprove/group.ts index fe4fc02bb..6462c2c65 100644 --- a/ng-dev/pullapprove/group.ts +++ b/ng-dev/pullapprove/group.ts @@ -6,10 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ -import {error} from '../utils/console'; -import {convertConditionToFunction} from './condition_evaluator'; +import { + PullApproveAuthorStateDependencyError, + PullApproveGroupStateDependencyError, +} from './condition_errors'; + import {PullApproveGroupConfig} from './parse-yaml'; -import {PullApproveGroupStateDependencyError} from './pullapprove_arrays'; +import {convertConditionToFunction} from './condition_evaluator'; +import {error} from '../utils/console'; /** A condition for a group. */ interface GroupCondition { @@ -34,9 +38,6 @@ export interface PullApproveGroupResult { unverifiableConditions: GroupCondition[]; } -// Regular expression that matches conditions for the global approval. -const GLOBAL_APPROVAL_CONDITION_REGEX = /^"global-(docs-)?approvers" not in groups.approved$/; - /** A PullApprove group to be able to test files against. */ export class PullApproveGroup { /** List of conditions for the group. */ @@ -58,11 +59,6 @@ export class PullApproveGroup { return config.conditions.forEach((condition) => { const expression = condition.trim(); - if (expression.match(GLOBAL_APPROVAL_CONDITION_REGEX)) { - // Currently a noop as we don't take any action for global approval conditions. - return; - } - try { this.conditions.push({ expression, @@ -85,23 +81,31 @@ export class PullApproveGroup { * the pull approve group's conditions. */ testFile(filePath: string): boolean { - return this.conditions.every((condition) => { + let allConditionsMet: boolean | null = null; + + for (const condition of this.conditions) { const {matchedFiles, checkFn, expression} = condition; try { const matchesFile = checkFn([filePath], this.precedingGroups); + if (matchesFile) { matchedFiles.add(filePath); } - return matchesFile; + + allConditionsMet = (allConditionsMet ?? true) && matchesFile; } catch (e) { - // In the case of a condition that depends on the state of groups we want to - // ignore that the verification can't accurately evaluate the condition and then - // continue processing. Other types of errors fail the verification, as conditions - // should otherwise be able to execute without throwing. - if (e instanceof PullApproveGroupStateDependencyError) { + // If a group relies on the author state, we assume this group to never match + // or own a file. This is a strict assumption but prevents false-positives. + if (e instanceof PullApproveAuthorStateDependencyError) { + condition.unverifiable = true; + allConditionsMet = false; + } + // In the case of a condition that depends on the state of groups, we want to ignore + // that the verification can't accurately evaluate the condition and continue processing. + // Other types of errors fail the verification, as conditions should otherwise be able to + // execute without throwing. + else if (e instanceof PullApproveGroupStateDependencyError) { condition.unverifiable = true; - // Return true so that `this.conditions.every` can continue evaluating. - return true; } else { const errMessage = `Condition could not be evaluated: \n\n` + @@ -111,7 +115,11 @@ export class PullApproveGroup { process.exit(1); } } - }); + } + + // A file matches the group when all conditions are met. A group is not considered + // as matching when all conditions have been skipped. + return allConditionsMet === true; } /** Retrieve the results for the Group, all matched and unmatched conditions. */ diff --git a/ng-dev/pullapprove/pullapprove_arrays.ts b/ng-dev/pullapprove/pullapprove_arrays.ts index c515124d7..9886ac8c0 100644 --- a/ng-dev/pullapprove/pullapprove_arrays.ts +++ b/ng-dev/pullapprove/pullapprove_arrays.ts @@ -5,11 +5,11 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ + import {PullApproveGroup} from './group'; +import {PullApproveGroupStateDependencyError} from './condition_errors'; import {getOrCreateGlob} from './utils'; -export class PullApproveGroupStateDependencyError extends Error {} - /** * Superset of a native array. The superset provides methods which mimic the * list data structure used in PullApprove for files in conditions. diff --git a/ng-dev/pullapprove/verify.spec.ts b/ng-dev/pullapprove/verify.spec.ts index 1e975fe9f..53f298fd7 100644 --- a/ng-dev/pullapprove/verify.spec.ts +++ b/ng-dev/pullapprove/verify.spec.ts @@ -78,6 +78,20 @@ describe('group parsing', () => { const fwCore = getGroupByName(groups, 'fw-core')!; expect(() => fwCore.testFile('any')).not.toThrow(); }); + + it('should never match groups with conditions that rely on author state', () => { + const groups = getGroupsFromYaml(` + groups: + renovate-notify-group: + conditions: + - author in ["renovate-bot"] + - contains_any_globs(files, ['packages/core/**']) + `); + const renovateGroup = getGroupByName(groups, 'renovate-notify-group')!; + + expect(renovateGroup.testFile('packages/core/index.ts')).toBe(false); + expect(renovateGroup.conditions[0].unverifiable).toBe(true); + }); }); function getGroupByName(groups: PullApproveGroup[], name: string): PullApproveGroup | undefined {