Skip to content

Commit

Permalink
ISOM-824 feat: don't fetch redundant author info from db (#1255)
Browse files Browse the repository at this point in the history 
* 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 😱
  • Loading branch information
@timotheeg
timotheeg authored Apr 1, 2024
1 parent 5647ae7 commit efb7ac7
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 53 deletions.
2 changes: 1 addition & 1 deletion src/services/infra/StatsService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ const getNpsVariant = (rating: number): NpsVariant => {
}

export class StatsService {
private readonly statsD: StatsD
readonly statsD: StatsD

private readonly usersRepo: ModelStatic<User>

Expand Down
83 changes: 61 additions & 22 deletions src/services/review/ReviewRequestService.ts
View file
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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,
Expand Down Expand Up @@ -439,36 +440,74 @@ 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
}

computeCommentData = async (
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
}

Expand Down
133 changes: 103 additions & 30 deletions src/services/review/__tests__/ReviewRequestService.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand All @@ -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(
Expand All @@ -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 = [
{
Expand All @@ -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(
Expand All @@ -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 = [
{
Expand All @@ -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(
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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,
])
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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 = [
{
Expand All @@ -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,
Expand All @@ -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(
Expand All @@ -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)
})
})
})

0 comments on commit efb7ac7

Please sign in to comment.