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.

Sign up for GitHub

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

🐛 Add url validation to rules step #1180

Merged
merged 3 commits into from
Jul 21, 2023
Merged

🐛 Add url validation to rules step #1180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2151685
:bug: Add url validation to rules step
ibolton336 Jul 20, 2023
c015493
Add unit tests for regex
ibolton336 Jul 21, 2023
98ec036
Merge branch 'main' into mta-985
ibolton336 Jul 21, 2023
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
10 changes: 4 additions & 6 deletions client/src/app/pages/applications/analysis-wizard/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "@app/api/models";
import { useTranslation } from "react-i18next";
import { useAnalyzableApplicationsByMode } from "./utils";
import { customURLValidation } from "@app/utils/utils";

export const ANALYSIS_MODES = [
"binary",
Expand Down Expand Up @@ -156,13 +157,10 @@ const useCustomRulesStepSchema = (): yup.SchemaOf<CustomRulesStepValues> => {
is: "repository",
then: yup.mixed<string>().required(),
}),
sourceRepository: yup.mixed<string>().when("rulesKind", {
sourceRepository: yup.string().when("rulesKind", {
is: "repository",
then: yup
.string()
.required("This value is required")
.min(3, t("validation.minLength", { length: 3 }))
.max(120, t("validation.maxLength", { length: 120 })),
then: (schema) =>
customURLValidation(schema).required("Enter repository url."),
}),
branch: yup.mixed<string>().when("rulesKind", {
is: "repository",
Expand Down
66 changes: 66 additions & 0 deletions client/src/app/utils/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
import { act, render } from "@testing-library/react";
import { renderHook } from "@testing-library/react-hooks";
import { AxiosError } from "axios";
import {
getAxiosErrorMessage,
getValidatedFromError,
getValidatedFromErrors,
getToolbarChipKey,
customURLValidation,
gitUrlRegex,
standardURLRegex,
} from "./utils";

describe("utils", () => {
Expand Down Expand Up @@ -89,4 +94,65 @@ describe("utils", () => {
const result = getToolbarChipKey({ key: "myKey", node: "myNode" });
expect(result).toBe("myKey");
});

//URL Regex tests
it("Regex should validate git URLs", () => {
const testGitURLs: string[] = [
"git@github.com:konveyor/tackle2-ui",
"http://git@github.com:konveyor/tackle2-ui",
];

for (const url of testGitURLs) {
const gitTestResult = gitUrlRegex.test(url);
expect(gitTestResult).toBe(true);
}
});

it("Regex should validate standard URLs", () => {
const testStandardURLs: string[] = [
"http://www.foo.bar",
"www.foo.bar",
"https://www.github.com/ibolton336/tackle-testapp.git",
];

for (const url of testStandardURLs) {
const standardTestResult = standardURLRegex.test(url);
expect(standardTestResult).toBe(true);
}
});

it("Regex should fail when validating broken standard URLs", () => {
const testBrokenURLs: string[] = [
"",
" http://www.foo.bar ",
" http://www.foo",
" http://wrong",
"wwwfoo.bar",
"foo.bar",
"www.foo.b",
"foo.ba",
"git@github.com:konveyor/tackle2-ui",
];

for (const url of testBrokenURLs) {
const testResult = standardURLRegex.test(url);
expect(testResult).toBe(false);
}
});

it("URL should match the same multiple times in a row", () => {
// Motivation for this test:
// https://stackoverflow.com/a/21373261/1183614
// https://stackoverflow.com/a/1520853/1183614
// When using the global flag on a regex, subsequent results will return a result from the last used index.
// In sum, we should never be using a global flag in combination with the .test method.

const url = "https://github.com/ibolton336/tackle-testapp.git";

expect(standardURLRegex.test(url)).toBe(true);
expect(standardURLRegex.test(url)).toBe(true);
expect(standardURLRegex.test(url)).toBe(true);
expect(standardURLRegex.test(url)).toBe(true);
expect(standardURLRegex.test(url)).toBe(true);
});
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding a few more strings to the tests, and have an it() section for gitUrlRegex and standardURLRegex.

A few url string suggestions:

  • ""
  • " http://www.foo.bar"
  • " http://www.foo.bar "
  • "www.foo.bar"
  • "foo.bar"
  • "www.foo.b"
  • "foo.ba"

Having a separate it() just for a double test would be good as well:

it("URL should match the same multiple times in a row", () => {
  const url = "https://github.com/ibolton336/tackle-testapp.git";

  expect(standardURLRegex.test(url).toBe(true);
  expect(standardURLRegex.test(url).toBe(true);
  expect(standardURLRegex.test(url).toBe(true);
  expect(standardURLRegex.test(url).toBe(true);
  expect(standardURLRegex.test(url).toBe(true);
});

And that test should fail if you put the /g back on the pattern...

Copy link
Collaborator

Choose a reason for hiding this comment

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

jeez, I would not have expected a regex test to have state like that

});
9 changes: 5 additions & 4 deletions client/src/app/utils/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,15 @@ export const getValidatedFromError = (error: unknown | undefined) => {
};

export const standardURLRegex =
/(https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|www\.[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9]+\.[^\s]{2,}|www\.[a-zA-Z0-9]+\.[^\s]{2,})/gi;
/^(https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|www\.[a-zA-Z0-9][a-zA-Z0-9-]+[a-zA-Z0-9]\.[^\s]{2,}|https?:\/\/(?:www\.|(?!www))[a-zA-Z0-9]+\.[^\s]{2,}|www\.[a-zA-Z0-9]+\.[^\s]{2,})$/;

export const gitUrlRegex =
/^(?:git|ssh|https?|git@[-\w.]+):(\/\/)?(.*?)(\/?|\#[-\d\w._]+?)$/;

export const standardStrictURLRegex =
/https:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,4}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)/gi;
/https:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,4}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)/;

export const customURLValidation = (schema: StringSchema) => {
const gitUrlRegex =
/(?:git|ssh|https?|git@[-\w.]+):(\/\/)?(.*?)(\/?|\#[-\d\w._]+?)$/;
const containsURL = (string: string) =>
gitUrlRegex.test(string) || standardURLRegex.test(string);

Expand Down