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

[HelpDot] detect infinite redirect cycles in redirects.csv #40434

Merged
merged 25 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
59 changes: 59 additions & 0 deletions .github/scripts/detectRedirectCycle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {parse} from 'csv-parse';
import fs from 'fs';

const parser = parse({skip_empty_lines: true});

Check failure on line 4 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Object Literal Property name `skip_empty_lines` must match one of the following formats: camelCase, UPPER_CASE, PascalCase
const adjacencyList: any = {};

Check failure on line 5 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Unexpected any. Specify a different type

function addEdge(source: string, target: string) {
if (!adjacencyList[source]) {
adjacencyList[source] = [];
}
adjacencyList[source].push(target);
}

function isCyclic(currentNode: string, visited: Map<string, boolean>, backEdges: Map<string, boolean>): boolean {
visited.set(currentNode, true);
backEdges.set(currentNode, true);

// Do a depth first search for all the neighbours. If a node is found in backedge, a cycle is detected.
const neighbours = adjacencyList[currentNode];
if (neighbours) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you could safely remove this if statement

Copy link
Member Author

Choose a reason for hiding this comment

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

No, there will be nodes that have no neighbours because this is a directed graph.

eg:

urlA, urlB

Graph will look like this
urlA -> urlB

urlB has no neighbours that we can go to. So we'll try to iterate over undefined in next line

for (const node of neighbours) {
if (!visited.has(node)) {
if (isCyclic(node, visited, backEdges)) return true;

Check failure on line 23 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Expected { after 'if' condition
} else if (backEdges.has(node)) return true;

Check failure on line 24 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Expected { after 'if' condition
}
}

backEdges.delete(currentNode);

return false;
}

function detectCycle() {
const visited: Map<string, boolean> = new Map<string, boolean>();
const backEdges: Map<string, boolean> = new Map<string, boolean>();
Copy link
Contributor

Choose a reason for hiding this comment

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

why not make them global variables?

Copy link
Member Author

Choose a reason for hiding this comment

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

no particular reason. done!


for (let node in adjacencyList) {

Check failure on line 37 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Using 'ForInStatement' is not allowed

Check failure on line 37 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

'node' is never reassigned. Use 'const' instead
if (visited.has(node)) {
continue;

Check failure on line 39 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Unexpected use of continue statement
}
if (isCyclic(node, visited, backEdges)) {
const cycle = Array.from(backEdges.keys());
console.log(`Infinite redirect found in the cycle: ${cycle.join(' -> ')} -> ${node}`);
return true;
}
}
}

fs.createReadStream(`${process.cwd()}/docs/redirects.csv`)
.pipe(parser)
.on('data', async (row) => {

Check failure on line 51 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Promise returned in function argument where a void return was expected

Check failure on line 51 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Async arrow function has no 'await' expression
addEdge(row[0], row[1]);
})
.on('end', async () => {

Check failure on line 54 in .github/scripts/detectRedirectCycle.ts

View workflow job for this annotation

GitHub Actions / Run ESLint

Promise returned in function argument where a void return was expected
if (detectCycle()) {
process.exit(1);
}
process.exit(0);
});
19 changes: 15 additions & 4 deletions .github/scripts/verifyRedirect.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,22 @@

declare -r REDIRECTS_FILE="docs/redirects.csv"

declare -r RED='\033[0;31m'
declare -r GREEN='\033[0;32m'
declare -r NC='\033[0m'

duplicates=$(awk -F, 'a[$1]++{print $1}' $REDIRECTS_FILE)
if [[ -n "$duplicates" ]]; then
echo "${RED}duplicate redirects are not allowed: $duplicates ${NC}"
exit 1
fi

if [[ -z "$duplicates" ]]; then
exit 0
npm run detectRedirectCycle
DETECT_CYCLE_EXIT_CODE=$?
if [[ DETECT_CYCLE_EXIT_CODE -eq 1 ]]; then
echo -e "${RED}The redirects.csv has a cycle. Please remove the redirect cycle because it will cause an infinite redirect loop ${NC}"
exit 1
fi

echo "duplicate redirects are not allowed: $duplicates"
exit 1
echo -e "${GREEN}The redirects.csv is valid!${NC}"
exit 0
4 changes: 2 additions & 2 deletions .github/workflows/deployExpensifyHelp.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ jobs:

- name: Create docs routes file
run: ./.github/scripts/createDocsRoutes.sh
- name: Check duplicates in redirect.csv

- name: Check duplicates in redirects.csv
run: ./.github/scripts/verifyRedirect.sh

- name: Build with Jekyll
Expand Down
3 changes: 2 additions & 1 deletion docs/redirects.csv
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ https://help.expensify.com/articles/expensify-classic/billing-and-subscriptions/
https://help.expensify.com/articles/expensify-classic/billing-and-subscriptions/Tax-Exempt,https://help.expensify.com/articles/expensify-classic/expensify-billing/Tax-Exempt
https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Approving-Reports,https://help.expensify.com/expensify-classic/hubs/reports/
https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Invite-Members,https://help.expensify.com/articles/expensify-classic/workspaces/Invite-members-and-assign-roles
https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Removing-Members,https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Removing-Members
https://help.expensify.com/articles/expensify-classic/expense-and-report-features/Attendee-Tracking,https://help.expensify.com/articles/expensify-classic/expenses/Track-group-expenses
https://help.expensify.com/articles/expensify-classic/expense-and-report-features/Currency,https://help.expensify.com/articles/expensify-classic/workspaces/Currency
https://help.expensify.com/articles/expensify-classic/expense-and-report-features/Expense-Rules,https://help.expensify.com/articles/expensify-classic/expenses/Expense-Rules
Expand Down Expand Up @@ -158,3 +157,5 @@ https://help.expensify.com/articles/expensify-classic/workspaces/Budgets,https:/
https://help.expensify.com/articles/expensify-classic/workspaces/Categories,https://help.expensify.com/articles/expensify-classic/workspaces/Create-categories
https://help.expensify.com/articles/expensify-classic/workspaces/Tags,https://help.expensify.com/articles/expensify-classic/workspaces/Create-tags
https://help.expensify.com/expensify-classic/hubs/manage-employees-and-report-approvals,https://help.expensify.com/articles/expensify-classic/copilots-and-delegates/Approval-Workflows
https://help.expensify.com/articles/expensify-classic/reports/Report-Audit-Log-and-Comments,https://help.expensify.com/articles/expensify-classic/reports/Print-or-download-a-report
https://help.expensify.com/articles/expensify-classic/reports/The-Reports-Page,https://help.expensify.com/articles/expensify-classic/reports/Report-statuses
7 changes: 7 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"desktop-build": "scripts/build-desktop.sh production",
"desktop-build-staging": "scripts/build-desktop.sh staging",
"createDocsRoutes": "ts-node .github/scripts/createDocsRoutes.ts",
"detectRedirectCycle": "ts-node .github/scripts/detectRedirectCycle.ts",
"desktop-build-adhoc": "scripts/build-desktop.sh adhoc",
"ios-build": "fastlane ios build",
"android-build": "fastlane android build",
Expand Down Expand Up @@ -252,6 +253,7 @@
"concurrently": "^8.2.2",
"copy-webpack-plugin": "^10.1.0",
"css-loader": "^6.7.2",
"csv-parse": "^5.5.5",
"diff-so-fancy": "^1.3.0",
"dotenv": "^16.0.3",
"electron": "^29.2.0",
Expand Down
Loading