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 23 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
64 changes: 64 additions & 0 deletions .github/scripts/detectRedirectCycle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import {parse} from 'csv-parse';
import fs from 'fs';

const parser = parse();
const adjacencyList: Record<string, string[]> = {};

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;
}
} else if (backEdges.has(node)) {
return true;
}
}
}

backEdges.delete(currentNode);

return false;
}

function detectCycle(): boolean {
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 (const [node] of Object.entries(adjacencyList)) {
if (!visited.has(node)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!visited.has(node)) {
if (visited.has(node)) {
continue;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

linter does not like continue, so i had to remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like that lint rule tbh, but this is fine for now

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

fs.createReadStream(`${process.cwd()}/docs/redirects.csv`)
.pipe(parser)
.on('data', (row) => {
// Create a directed graph of sourceURL -> targetURL
addEdge(row[0], row[1]);
})
.on('end', () => {
if (detectCycle()) {
process.exit(1);
}
process.exit(0);
});
19 changes: 15 additions & 4 deletions .github/scripts/verifyRedirect.sh
100644 → 100755
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 for duplicates and cycles 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