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

Added regex validation #20

Merged
merged 6 commits into from
Jul 17, 2023
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 src/file/validator.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Joi from "joi";

import { ActionLogger } from "../github/types";
import { ConfigurationFile, Rule } from "./types";

const ruleSchema = Joi.object<Rule>().keys({
Expand All @@ -20,3 +21,46 @@ export const schema = Joi.object<ConfigurationFile>().keys({
.optional()
.allow(null),
});

/** Evaluate if the regex expression inside a configuration are valid.
* @returns a tuple of type [boolean, string]. If the boolean is false, the string will contain an error message
* @example
* const [result, error] = validateRegularExpressions(myConfig);
* if (!result) {
* throw new Error(error);
* } else {
* runExpression(myConfig);
* }
*/
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): [true] | [false, string] => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};

for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
return [false, `Include condition '${condition}' is not a valid regex`];
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
return [false, `Exclude condition '${condition}' is not a valid regex`];
}
}
}
}

return [true];
};
Comment on lines +35 to +66
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could maybe change this to be the following:

Suggested change
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): [true] | [false, string] => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};
for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
return [false, `Include condition '${condition}' is not a valid regex`];
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
return [false, `Exclude condition '${condition}' is not a valid regex`];
}
}
}
}
return [true];
};
export const validateRegularExpressions = (
config: ConfigurationFile,
logger: ActionLogger,
): boolean => {
/** Regex evaluator */
const isRegexValid = (regex: string): boolean => {
try {
new RegExp(regex);
return true;
} catch (e) {
logger.error(e as Error);
return false;
}
};
for (const rule of config.rules) {
for (const condition of rule.condition.include) {
if (!isRegexValid(condition)) {
logger.error(`Include condition '${condition}' is not a valid regex`);
return false;
}
}
if (rule.condition.exclude) {
for (const condition of rule.condition.exclude) {
if (!isRegexValid(condition)) {
logger.error(`Exclude condition '${condition}' is not a valid regex`);
return false;
}
}
}
}
return true;
};

what do you think?

11 changes: 9 additions & 2 deletions src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { parse } from "yaml";

import { Inputs } from ".";
import { ConfigurationFile } from "./file/types";
import { schema } from "./file/validator";
import { schema, validateRegularExpressions } from "./file/validator";
import { PullRequestApi } from "./github/pullRequest";
import { ActionLogger } from "./github/types";

Expand All @@ -22,7 +22,14 @@ export class ActionRunner {

this.logger.info(`Obtained config at ${configLocation}`);

return validate<ConfigurationFile>(config, schema, { message: "Configuration file is invalid" });
const configFile = validate<ConfigurationFile>(config, schema, { message: "Configuration file is invalid" });

const [result, error] = validateRegularExpressions(configFile, this.logger);
if (!result) {
throw new Error(`Regular expression is invalid: ${error}`);
}

return configFile;
}

async runAction(inputs: Omit<Inputs, "repoToken">): Promise<boolean> {
Expand Down
26 changes: 21 additions & 5 deletions src/test/runner/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
import { mock, MockProxy } from "jest-mock-extended";

import { PullRequestApi } from "../../github/pullRequest";
import { ActionLogger } from "../../github/types";
import { ActionRunner } from "../../runner";
import { TestLogger } from "../logger";

describe("Config Parsing", () => {
let api: MockProxy<PullRequestApi>;
let runner: ActionRunner;
let logger: ActionLogger;
let logger: TestLogger;
beforeEach(() => {
logger = new TestLogger();
api = mock<PullRequestApi>();
Expand All @@ -28,14 +27,31 @@ describe("Config Parsing", () => {
- 'example'
`);
const config = await runner.getConfigFile("");
expect(config.preventReviewRequests).toBeNull;
expect(config.preventReviewRequests).toBeUndefined();
});

test("should call GitHub api with path", async () => {
await expect(runner.getConfigFile("example-location")).rejects.toThrowError();
expect(api.getConfigFile).toHaveBeenCalledWith("example-location");
});

describe("regular expressions validator", () => {
test("should fail with invalid regular expression", async () => {
const invalidRegex = "(?(";
api.getConfigFile.mockResolvedValue(`
rules:
- name: Default review
condition:
include:
- '${invalidRegex}'
`);
await expect(runner.getConfigFile("")).rejects.toThrowError(
`Regular expression is invalid: Include condition '${invalidRegex}' is not a valid regex`,
);
expect(logger.logHistory).toContainEqual(`Invalid regular expression: /${invalidRegex}/: Invalid group`);
});
});

describe("preventReviewRequests field", () => {
test("should get team", async () => {
api.getConfigFile.mockResolvedValue(`
Expand Down Expand Up @@ -86,7 +102,7 @@ describe("Config Parsing", () => {
- 'example'
`);
const config = await runner.getConfigFile("");
expect(config.preventReviewRequests).toBeNull;
expect(config.preventReviewRequests).toBeUndefined();
});
});

Expand Down Expand Up @@ -137,7 +153,7 @@ describe("Config Parsing", () => {
- '.*'
`);
const config = await runner.getConfigFile("");
expect(config.rules[0].condition.exclude).toBeNull;
expect(config.rules[0].condition.exclude).toBeUndefined();
});
});
});