From 50ca31f4f8433590dafee8646a14e9faae346b81 Mon Sep 17 00:00:00 2001 From: Jacob Kwan Date: Thu, 25 Apr 2024 15:46:22 +0800 Subject: [PATCH 1/5] fix: use faster diff-tree to get files changed between master and staging --- src/services/db/GitFileSystemService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 223ca1450..4246dab8f 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -396,7 +396,7 @@ export default class GitFileSystemService { return ResultAsync.fromPromise( this.git .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - .diff(["master..staging", "--name-only"]), + .raw(["diff-tree", "-r", "--name-only", "master..staging"]), (error) => { logger.error( `Error when getting diff files between master and staging: ${error}, when trying to access ${efsVolPath}/${repoName}` From f827833ae876a4645add79133880e7a6a6e326c3 Mon Sep 17 00:00:00 2001 From: Jacob Kwan Date: Thu, 25 Apr 2024 15:55:00 +0800 Subject: [PATCH 2/5] fix: GitFileSystemService tests --- src/services/db/__tests__/GitFileSystemService.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index f23d80192..ca491ce2d 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -753,7 +753,7 @@ describe("GitFileSystemService", () => { it("should return the files changed and defensively try creating local branches", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ - diff: jest + raw: jest .fn() .mockResolvedValueOnce("fake-dir/fake-file\nanother-fake-file\n"), }) @@ -768,7 +768,7 @@ describe("GitFileSystemService", () => { it("should return GitFileSystemError if an error occurred when getting the git diff", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ - diff: jest.fn().mockRejectedValueOnce(new GitError()), + raw: jest.fn().mockRejectedValueOnce(new GitError()), }) const actual = await GitFileSystemService.getFilesChanged("fake-repo") From fb420e713edc37dc6c68ebd01dbd54564d21e68b Mon Sep 17 00:00:00 2001 From: Jacob Kwan Date: Thu, 25 Apr 2024 20:58:58 +0800 Subject: [PATCH 3/5] fix: get all logs once instead of for each file and dedup queries for Users --- src/services/db/GitFileSystemService.ts | 59 ++++----- src/services/db/RepoService.ts | 4 +- src/services/review/ReviewRequestService.ts | 131 +++++++++++++------- 3 files changed, 112 insertions(+), 82 deletions(-) diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 4246dab8f..0048afd43 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -385,7 +385,7 @@ export default class GitFileSystemService { } /** - * Wrapper over `git diff --name-only` that also creates `master` branch if it does not exist. + * Wrapper over `git diff-tree -r --name-only master..staging` that also creates `master` branch if it does not exist. */ getFilesChanged(repoName: string): ResultAsync { return this.createLocalTrackingBranchIfNotExists( @@ -419,42 +419,31 @@ export default class GitFileSystemService { }) } - /** - * Get latest commit for a file path on a branch (including deleted files) - */ - getLatestCommitOfPath( - repoName: string, - path: string, - branch = "staging" - ): ResultAsync { - const efsVolPath = this.getEfsVolPathFromBranch(branch) - return ResultAsync.fromPromise( - this.git - .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - // -1 to return latest commit only, -- to get logs even for deleted files - .log(["-1", branch, "--", path]), - (error) => { - logger.error( - `Error when getting latest commit for "${path}" on "${branch}": ${error}, when trying to access ${efsVolPath}/${repoName}` - ) - - if (error instanceof GitError) { - return new GitFileSystemError( - "Unable to retrieve latest log info from disk" + getCommitsBetweenMasterAndStaging( + repoName: string + ): ResultAsync { + return this.createLocalTrackingBranchIfNotExists( + repoName, + "master" + ).andThen(() => { + const efsVolPath = this.getEfsVolPathFromBranch("staging") + return ResultAsync.fromPromise( + this.git + .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) + .log(["master..staging", "--name-only"]), + (error) => { + logger.error( + `Error when getting commits between master and staging: ${error}, when trying to access ${efsVolPath}/${repoName}` ) - } - return new GitFileSystemError("An unknown error occurred") - } - ).andThen((logs) => { - if (logs.latest === null) { - return errAsync( - new GitFileSystemError( - `No commit was found for "${path}" on "${branch}"` - ) - ) - } - return okAsync(logs.latest) + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to retrieve git logs info from disk" + ) + } + return new GitFileSystemError("An unknown error occurred") + } + ) }) } diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 89244e988..dfc30dd47 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -58,8 +58,8 @@ export default class RepoService extends GitHubService { return this.gitFileSystemService.getFilesChanged(siteName) } - getLatestLocalCommitOfPath(repoName: string, path: string) { - return this.gitFileSystemService.getLatestCommitOfPath(repoName, path) + getCommitsBetweenMasterAndStaging(siteName: string) { + return this.gitFileSystemService.getCommitsBetweenMasterAndStaging(siteName) } getCommitDiff(siteName: string, base?: string, head?: string) { diff --git a/src/services/review/ReviewRequestService.ts b/src/services/review/ReviewRequestService.ts index 5d4669f89..a76e76670 100644 --- a/src/services/review/ReviewRequestService.ts +++ b/src/services/review/ReviewRequestService.ts @@ -2,6 +2,7 @@ import _, { sortBy, unionBy, zipObject } from "lodash" import { err, errAsync, ok, okAsync, Result, ResultAsync } from "neverthrow" import { Op, ModelStatic } from "sequelize" import { Sequelize } from "sequelize-typescript" +import { DefaultLogFields, ListLogLine } from "simple-git" import UserWithSiteSessionData from "@classes/UserWithSiteSessionData" @@ -150,15 +151,65 @@ export default class ReviewRequestService { this.apiService .getFilesChanged(userWithSiteSessionData.siteName) .andThen((filenames) => { - // map each filename to its edit metadata - const editMetadata = filenames.map((filename) => - this.createEditedItemDtoWithEditMeta( - filename, - userWithSiteSessionData, - stagingLink - ) - ) - return ResultAsync.combine(editMetadata) + if (filenames.length === 0) { + return okAsync([]) + } + const filenamesSet = new Set(filenames) + return this.apiService + .getCommitsBetweenMasterAndStaging(userWithSiteSessionData.siteName) + .andThen((logs) => { + const filenameToLogMap = new Map< + string, + DefaultLogFields & ListLogLine + >() + const userIds = new Set() + logs.all.forEach((log) => { + const { userId } = fromGithubCommitMessage(log.message) + if (userId) { + userIds.add(userId) + } + log.diff?.files.forEach((file) => { + // Skip if we already have a log for file since we only want the latest log + if ( + filenamesSet.has(file.file) && + !filenameToLogMap.has(file.file) + ) { + filenameToLogMap.set(file.file, log) + } + }) + }) + + return ResultAsync.combine( + Array.from(userIds).map((userId) => + ResultAsync.fromPromise(this.users.findByPk(userId), () => { + logger.warn(`Error while finding userId: ${userId}`) + return new DatabaseError( + `Error while finding userId: ${userId}` + ) + }).orElse(() => okAsync(null)) + ) + ) + .map((users) => { + const userIdToUserMap = new Map() + users.forEach((user) => { + if (user) { + userIdToUserMap.set(user.id.toString(), user) + } + }) + // map each filename to its edit metadata + const editMetadata = filenames.map((filename) => + this.createEditedItemDtoWithEditMeta( + filename, + filenameToLogMap, + userIdToUserMap, + userWithSiteSessionData, + stagingLink + ) + ) + return ResultAsync.combine(editMetadata) + }) + .andThen((res) => res) + }) }) .map((changedItems) => changedItems.filter( @@ -169,11 +220,16 @@ export default class ReviewRequestService { createEditedItemDtoWithEditMeta = ( filename: string, + filenameToLogMap: Map, + userIdToUserMap: Map, sessionData: UserWithSiteSessionData, stagingLink: StagingPermalink ): ResultAsync, never> => { - const { siteName } = sessionData - const editMeta = this.extractEditMeta(siteName, filename) + const editMeta = this.extractEditMeta( + filename, + filenameToLogMap, + userIdToUserMap + ) const editedItemInfo = this.extractEditedItemInfo( filename, sessionData, @@ -189,41 +245,26 @@ export default class ReviewRequestService { } extractEditMeta = ( - siteName: string, - filename: string - ): ResultAsync, never> => - this.apiService - .getLatestLocalCommitOfPath(siteName, filename) - .andThen((latestLog) => { - const lastEditedTime = new Date(latestLog.date).getTime() - const { userId } = fromGithubCommitMessage(latestLog.message) - return ResultAsync.fromPromise( - this.users.findByPk(userId), - () => new DatabaseError(`Error while finding userId: ${userId}`) - ) - .map((author) => ({ - lastEditedBy: author?.email || latestLog.author_email, - lastEditedTime, - })) - .orElse((error) => { - logger.warn( - `Error getting edit metadata for ${filename} in ${siteName}: ${error}` - ) - return ok({ - lastEditedBy: "Unknown", - lastEditedTime, - }) - }) - }) - .orElse((error) => { - logger.warn( - `Error getting edit metadata for ${filename} in ${siteName}: ${error}` - ) - return ok({ - lastEditedBy: "Unknown", - lastEditedTime: 0, - }) + filename: string, + filenameToLogMap: Map, + userIdToUserMap: Map + ): ResultAsync, never> => { + const log = filenameToLogMap.get(filename) + if (!log) { + return okAsync({ + lastEditedBy: "Unknown", + lastEditedTime: 0, }) + } + const lastEditedTime = new Date(log.date).getTime() + const { userId } = fromGithubCommitMessage(log.message) + const user = userIdToUserMap.get(userId ?? "") + const lastEditedBy = user?.email ?? log.author_email + return okAsync({ + lastEditedBy, + lastEditedTime, + }) + } extractEditedItemInfo = ( filename: string, From 2b8c15c0e051b038ac5d9a59bc179b311a611d3a Mon Sep 17 00:00:00 2001 From: Jacob Kwan Date: Thu, 25 Apr 2024 21:00:39 +0800 Subject: [PATCH 4/5] chore: fix tests --- src/fixtures/review.ts | 10 +++++ .../db/__tests__/GitFileSystemService.spec.ts | 43 ------------------- .../__tests__/ReviewRequestService.spec.ts | 18 +++++--- 3 files changed, 22 insertions(+), 49 deletions(-) diff --git a/src/fixtures/review.ts b/src/fixtures/review.ts index b0148244d..84cd99519 100644 --- a/src/fixtures/review.ts +++ b/src/fixtures/review.ts @@ -47,6 +47,9 @@ export const MOCK_LATEST_LOG_ONE = { body: "body", author_name: "name", author_email: "email", + diff: { + files: [{ file: MOCK_COMMIT_FILEPATH_ONE + MOCK_COMMIT_FILENAME_ONE }], + }, } export const MOCK_LATEST_LOG_TWO = { @@ -57,6 +60,13 @@ export const MOCK_LATEST_LOG_TWO = { body: "body", author_name: "name", author_email: "email", + diff: { + files: [{ file: MOCK_COMMIT_FILEPATH_TWO + MOCK_COMMIT_FILENAME_TWO }], + }, +} + +export const MOCK_LATEST_LOGS = { + all: [MOCK_LATEST_LOG_ONE, MOCK_LATEST_LOG_TWO], } export const MOCK_FILENAME_TO_LATEST_LOG_MAP = { diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index ca491ce2d..55dd23acf 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -777,49 +777,6 @@ describe("GitFileSystemService", () => { }) }) - describe("getLatestLocalCommitOfPath", () => { - it("should return the latest commit for a valid path", async () => { - MockSimpleGit.cwd.mockReturnValueOnce({ - log: jest.fn().mockResolvedValueOnce({ - latest: MOCK_LATEST_LOG_ONE, - }), - }) - - const result = await GitFileSystemService.getLatestCommitOfPath( - "fake-repo", - "fake-dir/fake-file" - ) - - expect(result._unsafeUnwrap()).toEqual(MOCK_LATEST_LOG_ONE) - }) - - it("should return GitFileSystemError if an error occurred when getting the git log", async () => { - MockSimpleGit.cwd.mockReturnValueOnce({ - log: jest.fn().mockRejectedValueOnce(new GitError()), - }) - - const result = await GitFileSystemService.getLatestCommitOfPath( - "fake-repo", - "fake-dir/fake-file" - ) - - expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) - }) - - it("should return GitFileSystemError if there were no commits found", async () => { - MockSimpleGit.cwd.mockReturnValueOnce({ - log: jest.fn().mockResolvedValueOnce({ latest: null }), - }) - - const result = await GitFileSystemService.getLatestCommitOfPath( - "fake-repo", - "fake-dir/fake-file" - ) - - expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) - }) - }) - describe("getGitLog", () => { it("should return the Git log for a valid branch", async () => { MockSimpleGit.cwd.mockReturnValueOnce({ diff --git a/src/services/review/__tests__/ReviewRequestService.spec.ts b/src/services/review/__tests__/ReviewRequestService.spec.ts index 9347bc63a..6c8fbf529 100644 --- a/src/services/review/__tests__/ReviewRequestService.spec.ts +++ b/src/services/review/__tests__/ReviewRequestService.spec.ts @@ -54,10 +54,12 @@ import { MOCK_FILENAME_TO_LATEST_LOG_MAP, MOCK_REVIEW_REQUEST_META, MOCK_REVIEW_REQUEST_COMMENT, + MOCK_LATEST_LOGS, } from "@root/fixtures/review" import { mockEmail, mockGrowthBook, + mockIsomerUserId, mockUserWithSiteSessionData, mockUserWithSiteSessionDataAndGrowthBook, } from "@root/fixtures/sessionData" @@ -90,7 +92,7 @@ const MockReviewApi = { getCommitDiff: jest.fn(), getPullRequest: jest.fn(), getFilesChanged: jest.fn(), - getLatestLocalCommitOfPath: jest.fn(), + getCommitsBetweenMasterAndStaging: jest.fn(), fastForwardMaster: jest.fn(), } @@ -178,11 +180,11 @@ describe("ReviewRequestService", () => { MockReviewApi.getFilesChanged.mockReturnValue( okAsync(MOCK_PULL_REQUEST_FILES_CHANGED) ) - MockReviewApi.getLatestLocalCommitOfPath = jest.fn( - (repoName: string, path: string) => - okAsync(MOCK_FILENAME_TO_LATEST_LOG_MAP[path]) + MockReviewApi.getCommitsBetweenMasterAndStaging = jest.fn(() => + okAsync(MOCK_LATEST_LOGS) ) MockUsersRepository.findByPk.mockResolvedValue({ + id: mockIsomerUserId, email: mockEmail, }) MockPageService.parsePageName.mockReturnValue(okAsync("mock page name")) @@ -229,7 +231,9 @@ describe("ReviewRequestService", () => { // Assert expect(actual).toEqual(expected) expect(MockReviewApi.getFilesChanged).toHaveBeenCalled() - expect(MockReviewApi.getLatestLocalCommitOfPath).toHaveBeenCalledTimes(2) + expect( + MockReviewApi.getCommitsBetweenMasterAndStaging + ).toHaveBeenCalledTimes(1) expect(MockPageService.retrieveStagingPermalink).toHaveBeenCalled() }) @@ -247,7 +251,9 @@ describe("ReviewRequestService", () => { // Assert expect(actual).toEqual(expected) expect(MockReviewApi.getFilesChanged).toHaveBeenCalled() - expect(MockReviewApi.getLatestLocalCommitOfPath).not.toHaveBeenCalled() + expect( + MockReviewApi.getCommitsBetweenMasterAndStaging + ).not.toHaveBeenCalled() expect(MockPageService.retrieveStagingPermalink).not.toHaveBeenCalled() }) }) From 30a9257f9f9e42d17276e1943698e668dc2a7c87 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Thu, 9 May 2024 21:57:05 +0800 Subject: [PATCH 5/5] chore(gitfileSystem): add trivial test cases --- .env.test | 2 +- .../db/__tests__/GitFileSystemService.spec.ts | 36 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/.env.test b/.env.test index ae1637e94..5cbc2f366 100644 --- a/.env.test +++ b/.env.test @@ -35,7 +35,7 @@ export E2E_TEST_SECRET="test" export E2E_TEST_GH_TOKEN="test" # Database -export DB_URI="postgres://isomer:password@localhost:54321/isomercms_test" +export DB_URI="postgres://isomer:password@127.0.0.1:54321/isomercms_test" export DB_MIN_POOL="1" export DB_MAX_POOL="10" export DB_ENABLE_LOGGING="true" diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index 55dd23acf..99af59853 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -1,7 +1,7 @@ import fs, { Stats } from "fs" import mockFs from "mock-fs" -import { okAsync } from "neverthrow" +import { errAsync, okAsync } from "neverthrow" import { GitError, SimpleGit } from "simple-git" import config from "@config/config" @@ -3324,4 +3324,38 @@ describe("GitFileSystemService", () => { expect(branch).not.toHaveBeenCalled() }) }) + + describe("getCommitsBetweenMasterAndStaging", () => { + it("should get commits between staging and master branches", async () => { + jest + .spyOn(GitFileSystemService, "createLocalTrackingBranchIfNotExists") + .mockReturnValueOnce(okAsync(false)) + + const log = jest.fn().mockResolvedValueOnce({}) + + MockSimpleGit.cwd.mockReturnValueOnce({ + log, + }) + + await GitFileSystemService.getCommitsBetweenMasterAndStaging("fake-repo") + expect(log).toHaveBeenCalledWith(["master..staging", "--name-only"]) + }) + + it("should throw an error if there exists an error is getting log", async () => { + jest + .spyOn(GitFileSystemService, "createLocalTrackingBranchIfNotExists") + .mockReturnValueOnce(okAsync(false)) + + const log = jest.fn().mockRejectedValueOnce(new Error()) + + MockSimpleGit.cwd.mockReturnValueOnce({ + log, + }) + const result = await GitFileSystemService.getCommitsBetweenMasterAndStaging( + "fake-repo" + ) + + expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) + }) + }) })