From efb7ac7823e69ed42bf2e84d7b7080a548bded7f Mon Sep 17 00:00:00 2001 From: Timothee Groleau Date: Mon, 1 Apr 2024 16:52:04 +0800 Subject: [PATCH] ISOM-824 feat: don't fetch redundant author info from db (#1255) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: don't fetch redundant author info from db * refactor: restore the old mapping assignment to show consistency * chore: log optimization results * chore: add test to validate there is no duplicate db queries * chore: improve test for computeShaMappings() * fest: optimize number of user queries for computeCommentData() * feat: track runtime metrics for review requests * fix: remove unecessary promise wrapper * fix: fix incorrect tests 😱 --- src/services/infra/StatsService.ts | 2 +- src/services/review/ReviewRequestService.ts | 83 ++++++++--- .../__tests__/ReviewRequestService.spec.ts | 133 ++++++++++++++---- 3 files changed, 165 insertions(+), 53 deletions(-) diff --git a/src/services/infra/StatsService.ts b/src/services/infra/StatsService.ts index 529d5713a..98b5a4bf3 100644 --- a/src/services/infra/StatsService.ts +++ b/src/services/infra/StatsService.ts @@ -24,7 +24,7 @@ const getNpsVariant = (rating: number): NpsVariant => { } export class StatsService { - private readonly statsD: StatsD + readonly statsD: StatsD private readonly usersRepo: ModelStatic diff --git a/src/services/review/ReviewRequestService.ts b/src/services/review/ReviewRequestService.ts index 9ae0fa49a..5d4669f89 100644 --- a/src/services/review/ReviewRequestService.ts +++ b/src/services/review/ReviewRequestService.ts @@ -1,4 +1,4 @@ -import _, { sortBy, unionBy } from "lodash" +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" @@ -25,6 +25,7 @@ import PageParseError from "@root/errors/PageParseError" import PlaceholderParseError from "@root/errors/PlaceholderParseError" import RequestNotFoundError from "@root/errors/RequestNotFoundError" import logger from "@root/logger/logger" +import { statsService } from "@root/services/infra/StatsService" import { BaseEditedItemDto, CommentItem, @@ -439,17 +440,41 @@ export default class ReviewRequestService { // NOTE: commits from github are capped at 300. // This implies that there might possibly be some files // whose commit isn't being returned. - await Promise.all( - commits.map(async ({ commit, sha }) => { - const { userId } = fromGithubCommitMessage(commit.message) - const author = await this.users.findByPk(userId) - const lastChangedTime = new Date(commit.author.date).getTime() - mappings[sha] = { - author: author?.email || commit.author.name, - unixTime: lastChangedTime, - } - }) + + // commitAuthorIds are linked to the commits by array index + const commitAuthorIds = commits.map( + ({ commit }) => fromGithubCommitMessage(commit.message).userId ) + + // Query DB for all valid author details + const validAuthorIds = new Set( + commitAuthorIds.filter(_.identity) as string[] + ) + const allAuthorIds = [...validAuthorIds] + const authorsById = zipObject( + allAuthorIds, + await Promise.all( + allAuthorIds.map((authorId) => this.users.findByPk(authorId)) + ) + ) + + statsService.statsD.increment("review_requests.commits", commits.length) + statsService.statsD.increment( + "review_requests.commits.users", + validAuthorIds.size + ) + + commits.forEach(({ commit, sha }, idx) => { + const authorId = commitAuthorIds[idx] + const author = authorId ? authorsById[authorId] : null + const lastChangedTime = new Date(commit.author.date).getTime() + + mappings[sha] = { + author: author?.email || commit.author.name, + unixTime: lastChangedTime, + } + }) + return mappings } @@ -457,18 +482,32 @@ export default class ReviewRequestService { comments: GithubCommentData[], viewedTime: Date | null ) => { - const mappings = await Promise.all( - comments.map(async ({ userId, message, createdAt }) => { - const createdTime = new Date(createdAt) - const author = await this.users.findByPk(userId) - return { - user: author?.email || "", - message, - createdAt: createdTime.getTime(), - isRead: viewedTime ? createdTime < viewedTime : false, - } - }) + // retrieve the comment users while minimizing the number of DB query + const userIds = [ + ...new Set(comments.map(({ userId }) => userId).filter(_.identity)), + ] + const userByIds = zipObject( + userIds, + await Promise.all(userIds.map((userId) => this.users.findByPk(userId))) + ) + + statsService.statsD.increment("review_requests.comments", comments.length) + statsService.statsD.increment( + "review_requests.comments.users", + userIds.length ) + + const mappings = comments.map(({ userId, message, createdAt }) => { + const createdTime = new Date(createdAt) + const author = userByIds[userId] + return { + user: author?.email || "", + message, + createdAt: createdTime.getTime(), + isRead: viewedTime ? createdTime < viewedTime : false, + } + }) + return mappings } diff --git a/src/services/review/__tests__/ReviewRequestService.spec.ts b/src/services/review/__tests__/ReviewRequestService.spec.ts index 8da08d42a..9347bc63a 100644 --- a/src/services/review/__tests__/ReviewRequestService.spec.ts +++ b/src/services/review/__tests__/ReviewRequestService.spec.ts @@ -257,7 +257,7 @@ describe("ReviewRequestService", () => { // Arrange const mockComments: GithubCommentData[] = [ MOCK_GITHUB_COMMENT_DATA_ONE, - MOCK_GITHUB_COMMENT_DATA_TWO, + MOCK_GITHUB_COMMENT_DATA_TWO, // same user id -_- ] const mockViewedTime = new Date("2022-09-23T00:00:00Z") const expected = [ @@ -268,18 +268,15 @@ describe("ReviewRequestService", () => { isRead: true, }, { - user: MOCK_GITHUB_EMAIL_ADDRESS_TWO, + user: MOCK_GITHUB_EMAIL_ADDRESS_ONE, message: MOCK_GITHUB_COMMENT_TWO, createdAt: new Date(MOCK_GITHUB_DATE_TWO).getTime(), isRead: false, }, ] - MockUsersRepository.findByPk.mockResolvedValueOnce( + MockUsersRepository.findByPk.mockResolvedValue( MOCK_GITHUB_COMMIT_AUTHOR_ONE ) - MockUsersRepository.findByPk.mockResolvedValueOnce( - MOCK_GITHUB_COMMIT_AUTHOR_TWO - ) // Act const actual = await ReviewRequestService.computeCommentData( @@ -289,14 +286,14 @@ describe("ReviewRequestService", () => { // Assert expect(actual).toEqual(expected) - expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(1) }) it("should return the correct comment objects with viewedTime being null", async () => { // Arrange const mockComments: GithubCommentData[] = [ MOCK_GITHUB_COMMENT_DATA_ONE, - MOCK_GITHUB_COMMENT_DATA_TWO, + MOCK_GITHUB_COMMENT_DATA_TWO, // same user id -_- ] const expected = [ { @@ -306,18 +303,15 @@ describe("ReviewRequestService", () => { isRead: false, }, { - user: MOCK_GITHUB_EMAIL_ADDRESS_TWO, + user: MOCK_GITHUB_EMAIL_ADDRESS_ONE, message: MOCK_GITHUB_COMMENT_TWO, createdAt: new Date(MOCK_GITHUB_DATE_TWO).getTime(), isRead: false, }, ] - MockUsersRepository.findByPk.mockResolvedValueOnce( + MockUsersRepository.findByPk.mockResolvedValue( MOCK_GITHUB_COMMIT_AUTHOR_ONE ) - MockUsersRepository.findByPk.mockResolvedValueOnce( - MOCK_GITHUB_COMMIT_AUTHOR_TWO - ) // Act const actual = await ReviewRequestService.computeCommentData( @@ -327,14 +321,14 @@ describe("ReviewRequestService", () => { // Assert expect(actual).toEqual(expected) - expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(1) }) it("should return empty email string if user is not found", async () => { // Arrange const mockComments: GithubCommentData[] = [ MOCK_GITHUB_COMMENT_DATA_ONE, - MOCK_GITHUB_COMMENT_DATA_TWO, + MOCK_GITHUB_COMMENT_DATA_TWO, // same user if -_- ] const expected = [ { @@ -350,8 +344,7 @@ describe("ReviewRequestService", () => { isRead: false, }, ] - MockUsersRepository.findByPk.mockResolvedValueOnce(null) - MockUsersRepository.findByPk.mockResolvedValueOnce(null) + MockUsersRepository.findByPk.mockResolvedValue(null) // Act const actual = await ReviewRequestService.computeCommentData( @@ -361,7 +354,7 @@ describe("ReviewRequestService", () => { // Assert expect(actual).toEqual(expected) - expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(1) }) it("should return empty array if there are no comments", async () => { @@ -512,7 +505,7 @@ describe("ReviewRequestService", () => { MOCK_REVIEW_REQUEST_ONE.reviewMeta.pullRequestNumber ) expect(SpyReviewRequestService.computeCommentData).toHaveBeenCalled() - expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(1) }) it("should return an array of basic review request objects with a mix of read and unread comments", async () => { @@ -553,12 +546,9 @@ describe("ReviewRequestService", () => { MockReviewRequestViewRepository.findOne.mockResolvedValueOnce( MOCK_REVIEW_REQUEST_VIEW_ONE ) - MockUsersRepository.findByPk.mockResolvedValueOnce({ + MockUsersRepository.findByPk.mockResolvedValue({ email: MOCK_IDENTITY_EMAIL_ONE, }) - MockUsersRepository.findByPk.mockResolvedValueOnce({ - email: MOCK_IDENTITY_EMAIL_TWO, - }) MockReviewCommentApi.getCommentsForReviewRequest([ MOCK_REVIEW_REQUEST_COMMENT, ]) @@ -587,7 +577,7 @@ describe("ReviewRequestService", () => { MOCK_REVIEW_REQUEST_ONE.reviewMeta.pullRequestNumber ) expect(SpyReviewRequestService.computeCommentData).toHaveBeenCalled() - expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(1) }) it("should return an array of basic review request objects with no comments", async () => { @@ -1244,7 +1234,7 @@ describe("ReviewRequestService", () => { // Arrange const mockComments: GithubCommentData[] = [ MOCK_GITHUB_COMMENT_DATA_ONE, - MOCK_GITHUB_COMMENT_DATA_TWO, + MOCK_GITHUB_COMMENT_DATA_TWO, // same user id -_- ] const expected = [ { @@ -1254,7 +1244,7 @@ describe("ReviewRequestService", () => { isRead: true, }, { - user: MOCK_GITHUB_EMAIL_ADDRESS_TWO, + user: MOCK_GITHUB_EMAIL_ADDRESS_ONE, message: MOCK_GITHUB_COMMENT_TWO, createdAt: new Date(MOCK_GITHUB_DATE_TWO).getTime(), isRead: false, @@ -1267,12 +1257,9 @@ describe("ReviewRequestService", () => { MockReviewRequestViewRepository.findOne.mockResolvedValueOnce( MOCK_REVIEW_REQUEST_VIEW_ONE ) - MockUsersRepository.findByPk.mockResolvedValueOnce( + MockUsersRepository.findByPk.mockResolvedValue( MOCK_GITHUB_COMMIT_AUTHOR_ONE ) - MockUsersRepository.findByPk.mockResolvedValueOnce( - MOCK_GITHUB_COMMIT_AUTHOR_TWO - ) // Act const actual = await ReviewRequestService.getComments( @@ -1294,4 +1281,90 @@ describe("ReviewRequestService", () => { ) }) }) + + describe("computeShaMappings()", () => { + const mockCommits = [ + { + sha: "123", + url: "", + commit: { + message: JSON.stringify({ userId: 1 }), + url: "", + author: { + ...MOCK_GITHUB_COMMIT_AUTHOR_ONE, + date: new Date().toISOString(), + }, + }, + }, + { + sha: "234", + url: "", + commit: { + message: JSON.stringify({ userId: 2 }), + url: "", + author: { + ...MOCK_GITHUB_COMMIT_AUTHOR_TWO, + date: new Date().toISOString(), + }, + }, + }, + { + sha: "345", + url: "", + commit: { + message: JSON.stringify({ userId: 1 }), + url: "", + author: { + ...MOCK_GITHUB_COMMIT_AUTHOR_ONE, + date: new Date().toISOString(), + }, + }, + }, + { + sha: "456", + url: "", + commit: { + message: JSON.stringify({ userId: 2 }), + url: "", + author: { + ...MOCK_GITHUB_COMMIT_AUTHOR_TWO, + date: new Date().toISOString(), + }, + }, + }, + ] + + it("should not issue duplicated DB queries for the same users", async () => { + // Arrange + const expected = { + "123": { + author: MOCK_GITHUB_COMMIT_AUTHOR_ONE.email, + unixTime: new Date(mockCommits[0].commit.author.date).getTime(), + }, + "234": { + author: MOCK_GITHUB_COMMIT_AUTHOR_TWO.email, + unixTime: new Date(mockCommits[1].commit.author.date).getTime(), + }, + "345": { + author: MOCK_GITHUB_COMMIT_AUTHOR_ONE.email, + unixTime: new Date(mockCommits[2].commit.author.date).getTime(), + }, + "456": { + author: MOCK_GITHUB_COMMIT_AUTHOR_TWO.email, + unixTime: new Date(mockCommits[3].commit.author.date).getTime(), + }, + } + + MockUsersRepository.findByPk + .mockResolvedValueOnce(MOCK_GITHUB_COMMIT_AUTHOR_ONE) + .mockResolvedValueOnce(MOCK_GITHUB_COMMIT_AUTHOR_TWO) + + // Act + const actual = await ReviewRequestService.computeShaMappings(mockCommits) + + // Assert + expect(actual).toEqual(expected) + expect(MockUsersRepository.findByPk).toHaveBeenCalledTimes(2) + }) + }) })