From bea18c08f62cb8db0dfcbd14601649b6ed2693f6 Mon Sep 17 00:00:00 2001 From: Qilu Xie Date: Sat, 25 Nov 2023 21:09:05 +0800 Subject: [PATCH 01/10] Added swagger doc for GIG DNS API (#1017) * Added swagger doc for GIG DNS API * Modified the swagger doc to allow 1 DNS record per POST request * added 401 error response to all 3 API calls --- docs/gigDnsApi.yaml | 230 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 docs/gigDnsApi.yaml diff --git a/docs/gigDnsApi.yaml b/docs/gigDnsApi.yaml new file mode 100644 index 000000000..5c0b71705 --- /dev/null +++ b/docs/gigDnsApi.yaml @@ -0,0 +1,230 @@ +openapi: 3.0.0 +info: + version: "1.0.0" + title: "GIG DNS Management API" + description: "An API to manage DNS records for GovTech Domains" + +paths: + /dns-records: + post: + summary: Add a DNS record + description: Add a DNS record of various types to the DNS management system along with a contact email. + security: + - ApiKeyAuth: [] + parameters: + - in: header + name: X-API-KEY + required: true + schema: + type: string + description: API key required to authorize requests. + example: "1234567890abcdef" + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - email + - record + properties: + email: + type: string + format: email + description: The contact email address for this DNS record. + record: + $ref: "#/components/schemas/DNSRecord" + example: + email: "admin@example.com" + record: + type: "A" + name: "example.com" + content: "192.168.1.1" + responses: + "201": + description: DNS record successfully added. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: "DNS record added successfully." + "401": + description: Unauthorized. + "400": + description: Invalid request data. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: "Invalid data provided." + "500": + description: Server error. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: "Internal server error." + get: + summary: Retrieve DNS records + description: Get a list of DNS records for a specified name. + parameters: + - in: query + name: name + required: true + schema: + type: string + description: The name of the DNS record to retrieve. + responses: + "200": + description: A list of DNS records. + content: + application/json: + schema: + type: array + items: + $ref: "#/components/schemas/DNSRecord" + "401": + description: Unauthorized. + "404": + description: No records found for the specified name. + delete: + summary: Delete a DNS record by name and type + description: Delete a specific DNS record identified by its name and type. + parameters: + - in: query + name: name + required: true + schema: + type: string + description: The name of the DNS record to be deleted. + - in: query + name: type + required: true + schema: + type: string + enum: + [ + "A", + "AAAA", + "CNAME", + "TXT", + "SRV", + "LOC", + "MX", + "NS", + "SPF", + "CAA", + "DNSKEY", + "DS", + ] + description: The type of the DNS record to be deleted. + responses: + "200": + description: DNS record successfully deleted. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: "DNS record successfully deleted" + "401": + description: Unauthorized. + "404": + description: DNS record not found. +components: + securitySchemes: + ApiKeyAuth: + type: apiKey + in: header + name: X-API-KEY + description: API key required to authorize requests. + schemas: + DNSRecord: + type: object + required: + - type + - name + - content + properties: + type: + type: string + description: The type of the DNS record. + enum: + [ + "A", + "AAAA", + "CNAME", + "TXT", + "SRV", + "LOC", + "MX", + "NS", + "SPF", + "CAA", + "DNSKEY", + "DS", + ] + name: + type: string + description: The name of the DNS record. + content: + oneOf: + - type: string + - $ref: "#/components/schemas/DNSKEYData" + - $ref: "#/components/schemas/DSData" + description: The content of the DNS record. Varies depending on the record type. + priority: + type: integer + description: Priority of the record (used for MX and SRV records). + DNSKEYData: + type: object + required: + - flags + - protocol + - algorithm + - publicKey + properties: + flags: + type: integer + description: An unsigned 16-bit integer representing DNSKEY flags. + protocol: + type: integer + description: The protocol field. Should always be 3. + algorithm: + type: integer + description: An 8-bit integer identifying the DNSKEY's cryptographic algorithm. + publicKey: + type: string + description: The public key material, encoded in Base64. + DSData: + type: object + required: + - keyTag + - algorithm + - digestType + - digest + properties: + keyTag: + type: integer + description: An unsigned 16-bit integer representing the key tag. + algorithm: + type: integer + description: An 8-bit integer identifying the DS record's cryptographic algorithm. + digestType: + type: integer + description: An 8-bit integer representing the type of the digest. + digest: + type: string + description: The digest value, encoded in Base64. From 1b21e2b58ec00111826e2e2d777bc6753d3effb5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 1 Dec 2023 22:52:01 +0000 Subject: [PATCH 02/10] build(deps): bump sequelize-typescript from 2.1.5 to 2.1.6 (#1041) Bumps [sequelize-typescript](https://github.com/RobinBuschmann/sequelize-typescript) from 2.1.5 to 2.1.6. - [Release notes](https://github.com/RobinBuschmann/sequelize-typescript/releases) - [Changelog](https://github.com/sequelize/sequelize-typescript/blob/master/CHANGELOG.md) - [Commits](https://github.com/RobinBuschmann/sequelize-typescript/compare/v2.1.5...v2.1.6) --- updated-dependencies: - dependency-name: sequelize-typescript dependency-type: direct:production ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7de7e5b90..06d9d2354 100644 --- a/package-lock.json +++ b/package-lock.json @@ -66,7 +66,7 @@ "query-string": "^6.14.1", "reflect-metadata": "^0.1.13", "sequelize": "^6.29.0", - "sequelize-typescript": "^2.1.3", + "sequelize-typescript": "^2.1.6", "serialize-error": "^7.0.1", "simple-git": "^3.19.1", "slugify": "^1.6.0", @@ -14691,9 +14691,9 @@ } }, "node_modules/sequelize-typescript": { - "version": "2.1.5", - "resolved": "https://registry.npmjs.org/sequelize-typescript/-/sequelize-typescript-2.1.5.tgz", - "integrity": "sha512-x1CNODct8gJyfZPwEZBU5uVGNwgJI2Fda913ZxD5ZtCSRyTDPBTS/0uXciF+MlCpyqjpmoCAPtudQWzw579bzA==", + "version": "2.1.6", + "resolved": "https://registry.npmjs.org/sequelize-typescript/-/sequelize-typescript-2.1.6.tgz", + "integrity": "sha512-Vc2N++3en346RsbGjL3h7tgAl2Y7V+2liYTAOZ8XL0KTw3ahFHsyAUzOwct51n+g70I1TOUDgs06Oh6+XGcFkQ==", "dependencies": { "glob": "7.2.0" }, diff --git a/package.json b/package.json index e65f2e46e..a960d2e63 100644 --- a/package.json +++ b/package.json @@ -82,7 +82,7 @@ "query-string": "^6.14.1", "reflect-metadata": "^0.1.13", "sequelize": "^6.29.0", - "sequelize-typescript": "^2.1.3", + "sequelize-typescript": "^2.1.6", "serialize-error": "^7.0.1", "simple-git": "^3.19.1", "slugify": "^1.6.0", From 3217eed185637e06c736476b47e378c9905eb1a7 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:51:12 +0800 Subject: [PATCH 03/10] hotfix(repair-form): set remote url correctly (#1048) --- src/routes/formsg/formsgGGsRepair.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routes/formsg/formsgGGsRepair.ts b/src/routes/formsg/formsgGGsRepair.ts index c216879a4..6558acda5 100644 --- a/src/routes/formsg/formsgGGsRepair.ts +++ b/src/routes/formsg/formsgGGsRepair.ts @@ -116,7 +116,7 @@ export class FormsgGGsRepairRouter { const clonedStagingRepos: string[] = [] const syncedStagingAndStagingLiteRepos: string[] = [] repoNames.forEach((repoName) => { - const repoUrl = `git@github.com:isomerpages/${repoName}` + const repoUrl = `git@github.com:isomerpages/${repoName}.git` repairs.push( this.doesRepoNeedClone(repoName) From d84e63f5856438d43a8578c40fb1d0835ca28eb3 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 18:51:49 +0800 Subject: [PATCH 04/10] feat(quickie): only gitfile should have quickie (#1042) ## Problem Moving forward, since we have migrated most sites out of gh, we will only offer quickie for sites that are in ggs. As such, this pr removes the dependency of a no longer supported flow so as to allow for a simpler mental model. Additionally, there were weird abstractions bet GithubService + GithubCommitService -> removed the responsibilities to be isolated. Test cases will be added in the leaf node of these prs, cicd will also be fixed upstream. --- src/services/db/GitHubService.ts | 172 ++++++++++++++++++++++++- src/services/db/GithubCommitService.ts | 19 +-- src/services/db/RepoService.ts | 38 ++---- 3 files changed, 185 insertions(+), 44 deletions(-) diff --git a/src/services/db/GitHubService.ts b/src/services/db/GitHubService.ts index 285183a80..f4076e281 100644 --- a/src/services/db/GitHubService.ts +++ b/src/services/db/GitHubService.ts @@ -559,8 +559,7 @@ export default class GitHubService { async updateTree( sessionData: UserWithSiteSessionData, githubSessionData: GithubSessionData, - { gitTree, message }: { gitTree: any; message: any }, - isStaging: boolean + { gitTree, message }: { gitTree: any; message: any } ) { const { accessToken, siteName, isomerUserId: userId } = sessionData const { treeSha, currentCommitSha } = githubSessionData.getGithubState() @@ -604,16 +603,175 @@ export default class GitHubService { return newCommitSha } - async updateRepoState( + async deleteDirectory( sessionData: UserWithSiteSessionData, { - commitSha, - branchName = STAGING_BRANCH, - }: { commitSha: any; branchName?: string } + directoryName, + message, + githubSessionData, + }: { + directoryName: string + message: string + githubSessionData: GithubSessionData + } + ): Promise { + // GitHub flow + const gitTree = await this.getTree(sessionData, githubSessionData, { + isRecursive: true, + }) + + // Retrieve removed items and set their sha to null + const newGitTree = gitTree + .filter( + (item) => + item.path.startsWith(`${directoryName}/`) && item.type !== "tree" + ) + .map((item) => ({ + ...item, + sha: null, + })) + + const newCommitSha = await this.updateTree(sessionData, githubSessionData, { + gitTree: newGitTree, + message, + }) + + await this.updateRepoState(sessionData, { + commitSha: newCommitSha, + }) + } + + async moveFiles( + sessionData: UserWithSiteSessionData, + githubSessionData: GithubSessionData, + oldPath: string, + newPath: string, + targetFiles: string[], + message?: string + ): Promise { + const gitTree = await this.getTree(sessionData, githubSessionData, { + isRecursive: true, + }) + const newGitTree: any[] = [] + + gitTree.forEach((item: any) => { + if (item.path.startsWith(`${newPath}/`) && item.type !== "tree") { + const fileName = item.path + .split(`${newPath}/`) + .slice(1) + .join(`${newPath}/`) + if (targetFiles.includes(fileName)) { + // Conflicting file + throw new ConflictError("File already exists in target directory") + } + } + if (item.path.startsWith(`${oldPath}/`) && item.type !== "tree") { + const fileName = item.path + .split(`${oldPath}/`) + .slice(1) + .join(`${oldPath}/`) + if (targetFiles.includes(fileName)) { + // Add file to target directory + newGitTree.push({ + ...item, + path: `${newPath}/${fileName}`, + }) + // Delete old file + newGitTree.push({ + ...item, + sha: null, + }) + } + } + }) + + const newCommitSha = await this.updateTree(sessionData, githubSessionData, { + gitTree: newGitTree, + message, + }) + + await this.updateRepoState(sessionData, { + commitSha: newCommitSha, + }) + + return { newSha: newCommitSha } + } + + async renameSinglePath( + sessionData: UserWithSiteSessionData, + githubSessionData: GithubSessionData, + oldPath: string, + newPath: string, + message?: string, + isStaging = true + ): Promise { + const gitTree = await this.getTree( + sessionData, + githubSessionData, + { + isRecursive: true, + }, + !!isStaging + ) + const newGitTree: any[] = [] + const isMovingDirectory = + gitTree.find((item: any) => item.path === oldPath)?.type === "tree" || + false + + gitTree.forEach((item: any) => { + if (isMovingDirectory) { + if (item.path === newPath && item.type === "tree") { + throw new ConflictError("Target directory already exists") + } else if (item.path === oldPath && item.type === "tree") { + // Rename old subdirectory to new name + newGitTree.push({ + ...item, + path: newPath, + }) + } else if ( + item.path.startsWith(`${oldPath}/`) && + item.type !== "tree" + ) { + // Delete old files + newGitTree.push({ + ...item, + sha: null, + }) + } + } else if (item.path === newPath && item.type !== "tree") { + throw new ConflictError("Target file already exists") + } else if (item.path === oldPath && item.type !== "tree") { + // Add file to new directory + newGitTree.push({ + ...item, + path: newPath, + }) + // Delete old file + newGitTree.push({ + ...item, + sha: null, + }) + } + }) + + const newCommitSha = await this.updateTree(sessionData, githubSessionData, { + gitTree: newGitTree, + message, + }) + await this.updateRepoState(sessionData, { + commitSha: newCommitSha, + }) + + return { newSha: newCommitSha } + } + + async updateRepoState( + sessionData: UserWithSiteSessionData, + { commitSha }: { commitSha: string } ) { const { accessToken } = sessionData const { siteName } = sessionData - const refEndpoint = `${siteName}/git/refs/heads/${branchName}` + const refEndpoint = `${siteName}/git/refs/heads/${STAGING_BRANCH}` const headers = { Authorization: `token ${accessToken}`, } diff --git a/src/services/db/GithubCommitService.ts b/src/services/db/GithubCommitService.ts index 102639c69..3d0e386a0 100644 --- a/src/services/db/GithubCommitService.ts +++ b/src/services/db/GithubCommitService.ts @@ -149,15 +149,10 @@ export default class GitHubCommitService extends GitHubService { sha: null, })) - const newCommitSha = await this.updateTree( - sessionData, - githubSessionData, - { - gitTree: newGitTree, - message, - }, - !!isStaging - ) + const newCommitSha = await this.updateTree(sessionData, githubSessionData, { + gitTree: newGitTree, + message, + }) const deletePromises = [ this.updateRepoState(sessionData, { @@ -280,8 +275,7 @@ export default class GitHubCommitService extends GitHubService { { gitTree: newGitTree, message, - }, - !!isStaging + } ) await super.updateRepoState(sessionData, { commitSha: newCommitSha, @@ -362,8 +356,7 @@ export default class GitHubCommitService extends GitHubService { { gitTree: newGitTree, message, - }, - !!isStaging + } ) await super.updateRepoState(sessionData, { diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 7e386e04a..97f5646c3 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -20,7 +20,6 @@ import { getMediaFileInfo } from "@root/utils/media-utils" import GitFileCommitService from "./GitFileCommitService" import GitFileSystemService from "./GitFileSystemService" -import GitHubCommitService from "./GithubCommitService" import GitHubService from "./GitHubService" import * as ReviewApi from "./review" @@ -79,7 +78,6 @@ interface RepoServiceParams { isomerRepoAxiosInstance: AxiosCacheInstance gitFileSystemService: GitFileSystemService gitFileCommitService: GitFileCommitService - gitHubCommitService: GitHubCommitService } export default class RepoService extends GitHubService { @@ -87,18 +85,14 @@ export default class RepoService extends GitHubService { private readonly gitFileCommitService: GitFileCommitService - private readonly githubCommitService: GitHubCommitService - constructor({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }: RepoServiceParams) { super({ axiosInstance: isomerRepoAxiosInstance }) this.gitFileSystemService = gitFileSystemService this.gitFileCommitService = gitFileCommitService - this.githubCommitService = gitHubCommitService } getCommitDiff(siteName: string, base?: string, head?: string) { @@ -209,11 +203,12 @@ export default class RepoService extends GitHubService { isMedia, }) } - return this.githubCommitService.create(sessionData, { + return super.create(sessionData, { content, fileName, directoryName, isMedia, + branchName: STAGING_BRANCH, }) } @@ -419,11 +414,12 @@ export default class RepoService extends GitHubService { }) } - return this.githubCommitService.update(sessionData, { + return super.update(sessionData, { fileContent, sha, fileName, directoryName, + branchName: STAGING_BRANCH, }) } @@ -451,7 +447,7 @@ export default class RepoService extends GitHubService { return } - await this.githubCommitService.deleteDirectory(sessionData, { + super.deleteDirectory(sessionData, { directoryName, message, githubSessionData, @@ -486,7 +482,7 @@ export default class RepoService extends GitHubService { } // GitHub flow - await this.githubCommitService.delete(sessionData, { + await super.delete(sessionData, { sha, fileName, directoryName, @@ -514,7 +510,7 @@ export default class RepoService extends GitHubService { message ) } - return this.githubCommitService.renameSinglePath( + return super.renameSinglePath( sessionData, githubSessionData, oldPath, @@ -547,7 +543,7 @@ export default class RepoService extends GitHubService { ) } - return this.githubCommitService.moveFiles( + return super.moveFiles( sessionData, githubSessionData, oldPath, @@ -604,18 +600,12 @@ export default class RepoService extends GitHubService { async updateTree( sessionData: any, githubSessionData: any, - { gitTree, message }: any, - isStaging: boolean + { gitTree, message }: any ): Promise { - return super.updateTree( - sessionData, - githubSessionData, - { - gitTree, - message, - }, - isStaging - ) + return super.updateTree(sessionData, githubSessionData, { + gitTree, + message, + }) } async updateRepoState( @@ -646,7 +636,7 @@ export default class RepoService extends GitHubService { return } - await super.updateRepoState(sessionData, { commitSha, branchName }) + await super.updateRepoState(sessionData, { commitSha }) } async checkHasAccess(sessionData: any): Promise { From 7eb32fa4a0b382265d5314a8d61f9bf638b26c6a Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 18:53:26 +0800 Subject: [PATCH 05/10] chore(quickie): delete quickie for gh (#1043) ## Problem Actual removal of the no longer needed `GithubCommitService` file to reduce complexity + fix test cases. --- src/integration/Notifications.spec.ts | 3 - src/integration/Privatisation.spec.ts | 4 +- src/integration/Reviews.spec.ts | 4 +- src/integration/Sites.spec.ts | 4 +- src/server.js | 3 - src/services/db/GitHubService.ts | 32 +- src/services/db/GithubCommitService.ts | 383 ------------------ src/services/db/RepoService.ts | 1 - .../db/__tests__/GitHubService.spec.ts | 21 +- src/services/db/__tests__/RepoService.spec.ts | 372 +++++++++-------- 10 files changed, 213 insertions(+), 614 deletions(-) delete mode 100644 src/services/db/GithubCommitService.ts diff --git a/src/integration/Notifications.spec.ts b/src/integration/Notifications.spec.ts index b25a4b660..3fc89a30a 100644 --- a/src/integration/Notifications.spec.ts +++ b/src/integration/Notifications.spec.ts @@ -34,7 +34,6 @@ import { import { getAuthorizationMiddleware } from "@root/middleware" import { NotificationsRouter as _NotificationsRouter } from "@root/routes/v2/authenticated/notifications" import GitFileCommitService from "@root/services/db/GitFileCommitService" -import GitHubCommitService from "@root/services/db/GithubCommitService" import { BaseDirectoryService } from "@root/services/directoryServices/BaseDirectoryService" import { ResourceRoomDirectoryService } from "@root/services/directoryServices/ResourceRoomDirectoryService" import { CollectionPageService } from "@root/services/fileServices/MdPageServices/CollectionPageService" @@ -71,12 +70,10 @@ const MOCK_SITE_MEMBER_ID = "1" const gitFileSystemService = new GitFileSystemService(simpleGit()) const gitFileCommitService = new GitFileCommitService(gitFileSystemService) -const gitHubCommitService = new GitHubCommitService(isomerRepoAxiosInstance) const gitHubService = new RepoService({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }) const identityAuthService = getIdentityAuthService(gitHubService) const usersService = getUsersService(sequelize) diff --git a/src/integration/Privatisation.spec.ts b/src/integration/Privatisation.spec.ts index d47157bc0..c129a5569 100644 --- a/src/integration/Privatisation.spec.ts +++ b/src/integration/Privatisation.spec.ts @@ -45,7 +45,6 @@ import { AuthorizationMiddleware } from "@root/middleware/authorization" import { SettingsRouter as _SettingsRouter } from "@root/routes/v2/authenticatedSites/settings" import { SettingsService } from "@root/services/configServices/SettingsService" import GitFileCommitService from "@root/services/db/GitFileCommitService" -import GitHubCommitService from "@root/services/db/GithubCommitService" import { BaseDirectoryService } from "@root/services/directoryServices/BaseDirectoryService" import { ResourceRoomDirectoryService } from "@root/services/directoryServices/ResourceRoomDirectoryService" import { CollectionPageService } from "@root/services/fileServices/MdPageServices/CollectionPageService" @@ -100,12 +99,11 @@ jest.mock("../services/identity/DeploymentClient", () => ) const gitFileSystemService = new GitFileSystemService(simpleGit()) const gitFileCommitService = new GitFileCommitService(gitFileSystemService) -const gitHubCommitService = new GitHubCommitService(isomerRepoAxiosInstance) + const gitHubService = new RepoService({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }) const configYmlService = new ConfigYmlService({ gitHubService }) const usersService = getUsersService(sequelize) diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts index 927dc7956..a63d551a5 100644 --- a/src/integration/Reviews.spec.ts +++ b/src/integration/Reviews.spec.ts @@ -75,7 +75,6 @@ import { } from "@fixtures/users" import { ReviewRequestStatus } from "@root/constants" import GitFileCommitService from "@root/services/db/GitFileCommitService" -import GitHubCommitService from "@root/services/db/GithubCommitService" import { BaseDirectoryService } from "@root/services/directoryServices/BaseDirectoryService" import { ResourceRoomDirectoryService } from "@root/services/directoryServices/ResourceRoomDirectoryService" import { CollectionPageService } from "@root/services/fileServices/MdPageServices/CollectionPageService" @@ -105,12 +104,11 @@ import { sequelize } from "@tests/database" const gitFileSystemService = new GitFileSystemService(simpleGit()) const gitFileCommitService = new GitFileCommitService(gitFileSystemService) -const gitHubCommitService = new GitHubCommitService(isomerRepoAxiosInstance) + const gitHubService = new RepoService({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }) const configYmlService = new ConfigYmlService({ gitHubService }) const usersService = getUsersService(sequelize) diff --git a/src/integration/Sites.spec.ts b/src/integration/Sites.spec.ts index 0df835d79..61d3f5a1c 100644 --- a/src/integration/Sites.spec.ts +++ b/src/integration/Sites.spec.ts @@ -26,7 +26,6 @@ import { statsMiddleware } from "@root/middleware/stats" import { SitesRouter as _SitesRouter } from "@root/routes/v2/authenticated/sites" import { isomerRepoAxiosInstance } from "@root/services/api/AxiosInstance" import GitFileCommitService from "@root/services/db/GitFileCommitService" -import GitHubCommitService from "@root/services/db/GithubCommitService" import { BaseDirectoryService } from "@root/services/directoryServices/BaseDirectoryService" import { ResourceRoomDirectoryService } from "@root/services/directoryServices/ResourceRoomDirectoryService" import { CollectionPageService } from "@root/services/fileServices/MdPageServices/CollectionPageService" @@ -67,13 +66,12 @@ const mockPermissions = { push: true } const mockPrivate = true const gitFileSystemService = new GitFileSystemService(simpleGit()) -const gitHubCommitService = new GitHubCommitService(isomerRepoAxiosInstance) + const gitFileCommitService = new GitFileCommitService(gitFileSystemService) const gitHubService = new RepoService({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }) const configYmlService = new ConfigYmlService({ gitHubService }) const usersService = getUsersService(sequelize) diff --git a/src/server.js b/src/server.js index 73027b686..302e10ea9 100644 --- a/src/server.js +++ b/src/server.js @@ -79,7 +79,6 @@ import { SgidAuthRouter } from "./routes/v2/sgidAuth" import RepoManagementService from "./services/admin/RepoManagementService" import GitFileCommitService from "./services/db/GitFileCommitService" import GitFileSystemService from "./services/db/GitFileSystemService" -import GitHubCommitService from "./services/db/GithubCommitService" import RepoService from "./services/db/RepoService" import { PageService } from "./services/fileServices/MdPageServices/PageService" import { ConfigService } from "./services/fileServices/YmlFileServices/ConfigService" @@ -170,14 +169,12 @@ const simpleGitInstance = new simpleGit({ }) const gitFileSystemService = new GitFileSystemService(simpleGitInstance) -const gitHubCommitService = new GitHubCommitService(isomerRepoAxiosInstance) const gitFileCommitService = new GitFileCommitService(gitFileSystemService) const gitHubService = new RepoService({ isomerRepoAxiosInstance, gitFileSystemService, gitFileCommitService, - gitHubCommitService, }) const repoManagementService = new RepoManagementService({ diff --git a/src/services/db/GitHubService.ts b/src/services/db/GitHubService.ts index f4076e281..c5710b98a 100644 --- a/src/services/db/GitHubService.ts +++ b/src/services/db/GitHubService.ts @@ -132,13 +132,11 @@ export default class GitHubService { fileName, directoryName, isMedia = false, - branchName = STAGING_BRANCH, }: { content: string fileName: string directoryName: string isMedia: boolean - branchName: string } ) { const { accessToken, siteName, isomerUserId: userId } = sessionData @@ -185,7 +183,7 @@ export default class GitHubService { const params = { message, content: encodedContent, - branch: branchName, + branch: STAGING_BRANCH, } const resp = await this.axiosInstance.put(endpoint, params, { @@ -209,18 +207,14 @@ export default class GitHubService { async read( sessionData: UserWithSiteSessionData, - { - fileName, - directoryName, - branchName = STAGING_BRANCH, - }: { fileName: any; directoryName: any; branchName?: string } + { fileName, directoryName }: { fileName: any; directoryName: any } ) { const { accessToken } = sessionData const { siteName } = sessionData const endpoint = this.getFilePath({ siteName, fileName, directoryName }) const params = { - ref: branchName, + ref: STAGING_BRANCH, } const resp = await this.axiosInstance.get(endpoint, { @@ -241,10 +235,7 @@ export default class GitHubService { async readMedia( sessionData: UserWithSiteSessionData, - { - fileSha, - branchName = STAGING_BRANCH, - }: { fileSha: any; branchName?: string } + { fileSha }: { fileSha: any; branchName?: string } ) { /** * Files that are bigger than 1 MB needs to be retrieved @@ -254,7 +245,7 @@ export default class GitHubService { const { accessToken } = sessionData const { siteName } = sessionData const params = { - ref: branchName, + ref: STAGING_BRANCH, } const blobEndpoint = this.getBlobPath({ siteName, fileSha }) @@ -379,12 +370,10 @@ export default class GitHubService { sha, fileName, directoryName, - branchName = STAGING_BRANCH, }: { sha: string fileName: string directoryName: string - branchName?: string } ) { const { accessToken, siteName, isomerUserId: userId } = sessionData @@ -392,11 +381,10 @@ export default class GitHubService { const endpoint = this.getFilePath({ siteName, fileName, directoryName }) let fileSha = sha - if (!sha || branchName === STAGING_LITE_BRANCH) { + if (!sha) { const { sha: retrievedSha } = await this.read(sessionData, { fileName, directoryName, - branchName, }) fileSha = retrievedSha } @@ -408,7 +396,7 @@ export default class GitHubService { }) const params = { message, - branch: branchName, + branch: STAGING_BRANCH, sha: fileSha, } @@ -767,11 +755,13 @@ export default class GitHubService { async updateRepoState( sessionData: UserWithSiteSessionData, - { commitSha }: { commitSha: string } + { commitSha, branchName }: { commitSha: any; branchName?: string } ) { const { accessToken } = sessionData const { siteName } = sessionData - const refEndpoint = `${siteName}/git/refs/heads/${STAGING_BRANCH}` + const refEndpoint = `${siteName}/git/refs/heads/${ + branchName || STAGING_BRANCH + }` const headers = { Authorization: `token ${accessToken}`, } diff --git a/src/services/db/GithubCommitService.ts b/src/services/db/GithubCommitService.ts deleted file mode 100644 index 3d0e386a0..000000000 --- a/src/services/db/GithubCommitService.ts +++ /dev/null @@ -1,383 +0,0 @@ -import { AxiosCacheInstance } from "axios-cache-interceptor" - -import GithubSessionData from "@root/classes/GithubSessionData" -import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" -import { STAGING_BRANCH, STAGING_LITE_BRANCH } from "@root/constants" -import { ConflictError } from "@root/errors/ConflictError" -import { GitCommitResult } from "@root/types/gitfilesystem" -import isFileAsset from "@root/utils/commit-utils" -import { isReduceBuildTimesWhitelistedRepo } from "@root/utils/growthbook-utils" -import GitHubService from "@services/db/GitHubService" - -/** - * Responsibilities of this class - * 1. Creates all commits to staging - * 2. Creates non-asset related commits to staging-lite - */ -export default class GitHubCommitService extends GitHubService { - constructor(axiosInstance: AxiosCacheInstance) { - super({ axiosInstance }) - } - - async create( - sessionData: UserWithSiteSessionData, - { - content, - fileName, - directoryName, - isMedia = false, - }: { - content: string - fileName: string - directoryName: string - isMedia?: boolean - } - ): Promise<{ - sha: string - }> { - const createPromises = [ - super.create(sessionData, { - content, - fileName, - directoryName, - isMedia, - branchName: STAGING_BRANCH, - }), - ] - const shouldStagingLiteUpdate = - !isFileAsset({ directoryName, fileName }) && - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) - - if (shouldStagingLiteUpdate) { - createPromises.push( - super.create(sessionData, { - content, - directoryName, - fileName, - isMedia, - branchName: STAGING_LITE_BRANCH, - }) - ) - } - - // We still await and throw error if the commit to staging-lite fails, - // but the caller only gets the `resultToStaging` commit returned - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const [resultToStaging, _] = await Promise.all(createPromises) - - return resultToStaging - } - - async update( - sessionData: UserWithSiteSessionData, - { - fileContent, - sha, - fileName, - directoryName, - }: { - fileContent: string - sha: string - fileName: string - directoryName: string | undefined - } - ): Promise { - const updatePromises = [ - super.update(sessionData, { - fileContent, - sha, - fileName, - directoryName, - branchName: STAGING_BRANCH, - }), - ] - const shouldStagingLiteUpdate = - !isFileAsset({ directoryName, fileName }) && - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) - - if (shouldStagingLiteUpdate) { - updatePromises.push( - super.update(sessionData, { - fileContent, - sha, - fileName, - directoryName, - branchName: STAGING_LITE_BRANCH, - }) - ) - } - - // We still await and throw error if the commit to staging-lite fails, - // but the caller only gets the `resultToStaging` commit returned - const [resultToStaging, _] = await Promise.all(updatePromises) - - return resultToStaging - } - - async deleteDirectory( - sessionData: UserWithSiteSessionData, - { - directoryName, - message, - githubSessionData, - isStaging = true, - }: { - directoryName: string - message: string - githubSessionData: GithubSessionData - isStaging?: boolean - } - ): Promise { - // GitHub flow - const gitTree = await this.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - }, - isStaging - ) - - // Retrieve removed items and set their sha to null - const newGitTree = gitTree - .filter( - (item) => - item.path.startsWith(`${directoryName}/`) && item.type !== "tree" - ) - .map((item) => ({ - ...item, - sha: null, - })) - - const newCommitSha = await this.updateTree(sessionData, githubSessionData, { - gitTree: newGitTree, - message, - }) - - const deletePromises = [ - this.updateRepoState(sessionData, { - commitSha: newCommitSha, - }), - ] - - if ( - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName }) - ) { - deletePromises.push( - this.deleteDirectory(sessionData, { - directoryName, - message, - githubSessionData, - isStaging: false, - }) - ) - } - await Promise.all(deletePromises) - } - - async delete( - sessionData: UserWithSiteSessionData, - { - sha, - fileName, - directoryName, - }: { - sha: string - fileName: string - directoryName: string - } - ): Promise { - const deletePromises = [ - super.delete(sessionData, { - sha, - fileName, - directoryName, - }), - ] - const shouldStagingLiteUpdate = - !isFileAsset({ directoryName, fileName }) && - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) - if (shouldStagingLiteUpdate) { - deletePromises.push( - super.delete(sessionData, { - sha, - fileName, - directoryName, - branchName: STAGING_LITE_BRANCH, - }) - ) - } - - await Promise.all(deletePromises) - } - - async renameSinglePath( - sessionData: UserWithSiteSessionData, - githubSessionData: GithubSessionData, - oldPath: string, - newPath: string, - message?: string, - isStaging = true - ): Promise { - const gitTree = await super.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - }, - !!isStaging - ) - const newGitTree: any[] = [] - const isMovingDirectory = - gitTree.find((item: any) => item.path === oldPath)?.type === "tree" || - false - - gitTree.forEach((item: any) => { - if (isMovingDirectory) { - if (item.path === newPath && item.type === "tree") { - throw new ConflictError("Target directory already exists") - } else if (item.path === oldPath && item.type === "tree") { - // Rename old subdirectory to new name - newGitTree.push({ - ...item, - path: newPath, - }) - } else if ( - item.path.startsWith(`${oldPath}/`) && - item.type !== "tree" - ) { - // Delete old files - newGitTree.push({ - ...item, - sha: null, - }) - } - } else if (item.path === newPath && item.type !== "tree") { - throw new ConflictError("Target file already exists") - } else if (item.path === oldPath && item.type !== "tree") { - // Add file to new directory - newGitTree.push({ - ...item, - path: newPath, - }) - // Delete old file - newGitTree.push({ - ...item, - sha: null, - }) - } - }) - - const newCommitSha = await super.updateTree( - sessionData, - githubSessionData, - { - gitTree: newGitTree, - message, - } - ) - await super.updateRepoState(sessionData, { - commitSha: newCommitSha, - }) - - const shouldStagingLiteUpdate = - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName: oldPath }) - - if (shouldStagingLiteUpdate) { - // we await this call, but we do not need to return this result - await this.renameSinglePath( - sessionData, - githubSessionData, - oldPath, - newPath, - message, - false - ) - } - - return { newSha: newCommitSha } - } - - async moveFiles( - sessionData: UserWithSiteSessionData, - githubSessionData: GithubSessionData, - oldPath: string, - newPath: string, - targetFiles: string[], - message?: string, - isStaging = true - ): Promise { - const gitTree = await super.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - }, - isStaging - ) - const newGitTree: any[] = [] - - gitTree.forEach((item: any) => { - if (item.path.startsWith(`${newPath}/`) && item.type !== "tree") { - const fileName = item.path - .split(`${newPath}/`) - .slice(1) - .join(`${newPath}/`) - if (targetFiles.includes(fileName)) { - // Conflicting file - throw new ConflictError("File already exists in target directory") - } - } - if (item.path.startsWith(`${oldPath}/`) && item.type !== "tree") { - const fileName = item.path - .split(`${oldPath}/`) - .slice(1) - .join(`${oldPath}/`) - if (targetFiles.includes(fileName)) { - // Add file to target directory - newGitTree.push({ - ...item, - path: `${newPath}/${fileName}`, - }) - // Delete old file - newGitTree.push({ - ...item, - sha: null, - }) - } - } - }) - - const newCommitSha = await super.updateTree( - sessionData, - githubSessionData, - { - gitTree: newGitTree, - message, - } - ) - - await super.updateRepoState(sessionData, { - commitSha: newCommitSha, - }) - - const shouldUpdateStagingLite = - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName: oldPath }) - if (shouldUpdateStagingLite) { - // We don't have to return the sha, just update this should be ok - await this.moveFiles( - sessionData, - githubSessionData, - oldPath, - newPath, - targetFiles, - message, - false - ) - } - return { newSha: newCommitSha } - } -} diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 97f5646c3..1d48bbeda 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -208,7 +208,6 @@ export default class RepoService extends GitHubService { fileName, directoryName, isMedia, - branchName: STAGING_BRANCH, }) } diff --git a/src/services/db/__tests__/GitHubService.spec.ts b/src/services/db/__tests__/GitHubService.spec.ts index be83fe6da..03a1ced24 100644 --- a/src/services/db/__tests__/GitHubService.spec.ts +++ b/src/services/db/__tests__/GitHubService.spec.ts @@ -173,7 +173,6 @@ describe("Github Service", () => { fileName, directoryName, isMedia: false, - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ sha, @@ -200,7 +199,6 @@ describe("Github Service", () => { fileName: topLevelDirectoryFileName, directoryName, isMedia: false, - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ sha, @@ -228,7 +226,6 @@ describe("Github Service", () => { fileName: resourceCategoryFileName, directoryName, isMedia: false, - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ sha, @@ -257,7 +254,6 @@ describe("Github Service", () => { fileName, directoryName, isMedia: true, - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ sha, @@ -289,7 +285,6 @@ describe("Github Service", () => { fileName, directoryName, isMedia: false, - branchName: BRANCH_REF, }) ).rejects.toThrowError(ConflictError) expect(mockAxiosInstance.put).toHaveBeenCalledWith( @@ -309,7 +304,6 @@ describe("Github Service", () => { fileName, directoryName, isMedia: false, - branchName: BRANCH_REF, }) ).rejects.toThrowError(NotFoundError) expect(mockAxiosInstance.get).toHaveBeenCalledWith(folderParentEndpoint, { @@ -332,7 +326,6 @@ describe("Github Service", () => { fileName: subDirectoryFileName, directoryName: subDirectoryName, isMedia: false, - branchName: BRANCH_REF, }) ).rejects.toThrowError() expect(mockAxiosInstance.get).toHaveBeenCalledWith(fileParentEndpoint, { @@ -354,7 +347,6 @@ describe("Github Service", () => { fileName, directoryName: `${resourceCategoryName}/_posts`, isMedia: false, - branchName: BRANCH_REF, }) ).rejects.toThrowError(NotFoundError) expect(mockAxiosInstance.get).toHaveBeenCalledWith(resourceRoomEndpoint, { @@ -892,15 +884,10 @@ describe("Github Service", () => { .mockResolvedValueOnce(firstResp) .mockResolvedValueOnce(secondResp) await expect( - service.updateTree( - sessionData, - mockGithubSessionData, - { - gitTree, - message, - }, - true - ) + service.updateTree(sessionData, mockGithubSessionData, { + gitTree, + message, + }) ).resolves.toEqual(secondSha) expect(mockAxiosInstance.post).toHaveBeenCalledWith( url, diff --git a/src/services/db/__tests__/RepoService.spec.ts b/src/services/db/__tests__/RepoService.spec.ts index 7f482bf60..19f8b9fb7 100644 --- a/src/services/db/__tests__/RepoService.spec.ts +++ b/src/services/db/__tests__/RepoService.spec.ts @@ -8,7 +8,6 @@ import { mockGithubSessionData, mockGrowthBook, mockIsomerUserId, - mockSiteName, mockUserWithSiteSessionDataAndGrowthBook, } from "@fixtures/sessionData" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" @@ -24,7 +23,6 @@ import GitFileSystemService from "@services/db/GitFileSystemService" import _RepoService from "@services/db/RepoService" import GitFileCommitService from "../GitFileCommitService" -import GitHubCommitService from "../GithubCommitService" import GitHubService from "../GitHubService" const MockAxiosInstance = { @@ -58,7 +56,7 @@ const MockGitFileCommitService = { moveFiles: jest.fn(), } -const MockGitHubCommitService = { +const MockGitHubService = { create: jest.fn(), update: jest.fn(), delete: jest.fn(), @@ -71,7 +69,6 @@ const RepoService = new _RepoService({ isomerRepoAxiosInstance: (MockAxiosInstance as unknown) as AxiosCacheInstance, gitFileSystemService: (MockGitFileSystemService as unknown) as GitFileSystemService, gitFileCommitService: (MockGitFileCommitService as unknown) as GitFileCommitService, - gitHubCommitService: (MockGitHubCommitService as unknown) as GitHubCommitService, }) describe("RepoService", () => { @@ -164,7 +161,7 @@ describe("RepoService", () => { ) }) - it("should create files on GitHub directly if the repo is not ggs enabled", async () => { + it("should create files on GitHub directly if the repo is not whitelisted", async () => { const mockContent = "content" const mockFileName = "test.md" const mockDirectoryName = "" @@ -179,17 +176,18 @@ describe("RepoService", () => { const expected = { sha: "test-sha", } - MockGitHubCommitService.create.mockResolvedValueOnce(expected) + const gitHubServiceCreate = jest.spyOn(GitHubService.prototype, "create") + gitHubServiceCreate.mockResolvedValueOnce(expected) const actual = await RepoService.create(sessionData, { - content: mockContent, - fileName: mockFileName, - directoryName: mockDirectoryName, + content: "content", + fileName: "test.md", + directoryName: "", isMedia, }) expect(actual).toEqual(expected) - expect(MockGitHubCommitService.create).toHaveBeenCalledWith(sessionData, { + expect(gitHubServiceCreate).toHaveBeenCalledWith(sessionData, { content: mockContent, fileName: mockFileName, directoryName: mockDirectoryName, @@ -568,9 +566,8 @@ describe("RepoService", () => { email: mockEmail, siteName: "not-whitelisted", }) - MockGitHubCommitService.update.mockResolvedValueOnce({ - newSha: expectedSha, - }) + const gitHubServiceUpdate = jest.spyOn(GitHubService.prototype, "update") + gitHubServiceUpdate.mockResolvedValueOnce({ newSha: expectedSha }) const actual = await RepoService.update(sessionData, { fileContent: "test content", @@ -616,200 +613,221 @@ describe("RepoService", () => { siteName: "not-whitelisted", }) + const mockedGitHubService = jest + .spyOn(GitHubService.prototype, "delete") + .mockResolvedValueOnce(undefined) + await RepoService.delete(sessionData, { sha: "fake-original-sha", fileName: "test.md", directoryName: "pages", }) - expect(MockGitHubCommitService.delete).toBeCalledTimes(1) - expect(MockGitHubCommitService.delete).toBeCalledWith(sessionData, { + expect(mockedGitHubService).toBeCalledWith(sessionData, { sha: "fake-original-sha", fileName: "test.md", directoryName: "pages", }) }) - }) - - describe("renameSinglePath", () => { - it("should rename using the local Git file system if the repo is ggs enabled", async () => { - const expected: GitCommitResult = { newSha: "fake-commit-sha" } - MockGitFileCommitService.renameSinglePath.mockResolvedValueOnce(expected) - gbSpy.mockReturnValueOnce(true) - const actual = await RepoService.renameSinglePath( - mockUserWithSiteSessionDataAndGrowthBook, - mockGithubSessionData, - "fake-old-path", - "fake-new-path", - "fake-commit-message" - ) - - expect(actual).toEqual(expected) - }) - - it("should rename file using GitHub directly if the repo is not ggs enabled", async () => { - const expectedSha = "fake-commit-sha" - const fakeCommitMessage = "fake-commit-message" - const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ - githubId: mockGithubId, - accessToken: mockAccessToken, - isomerUserId: mockIsomerUserId, - email: mockEmail, - siteName: "not-whitelisted", + describe("renameSinglePath", () => { + it("should rename using the local Git file system if the repo is ggs enabled", async () => { + const expected: GitCommitResult = { newSha: "fake-commit-sha" } + MockGitFileCommitService.renameSinglePath.mockResolvedValueOnce( + expected + ) + gbSpy.mockReturnValueOnce(true) + + const actual = await RepoService.renameSinglePath( + mockUserWithSiteSessionDataAndGrowthBook, + mockGithubSessionData, + "fake-old-path", + "fake-new-path", + "fake-commit-message" + ) + + expect(actual).toEqual(expected) }) - MockGitHubCommitService.renameSinglePath.mockResolvedValueOnce({ - newSha: expectedSha, + it("should rename file using GitHub directly if the repo is not ggs enabled", async () => { + const expectedSha = "fake-commit-sha" + const fakeCommitMessage = "fake-commit-message" + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData( + { + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + } + ) + + const gitHubServiceRenameSinglePath = jest.spyOn( + GitHubService.prototype, + "renameSinglePath" + ) + gitHubServiceRenameSinglePath.mockResolvedValueOnce({ + newSha: expectedSha, + }) + + const actual = await RepoService.renameSinglePath( + sessionData, + mockGithubSessionData, + "fake-path/old-fake-file.md", + "fake-path/new-fake-file.md", + fakeCommitMessage + ) + + expect(actual).toEqual({ newSha: expectedSha }) }) - - const actual = await RepoService.renameSinglePath( - sessionData, - mockGithubSessionData, - "fake-path/old-fake-file.md", - "fake-path/new-fake-file.md", - fakeCommitMessage - ) - - expect(actual).toEqual({ newSha: expectedSha }) }) - }) - describe("moveFiles", () => { - it("should move files using the Git local file system if the repo is ggs enabled", async () => { - const expected = { newSha: "fake-commit-sha" } - MockGitFileCommitService.moveFiles.mockResolvedValueOnce(expected) - gbSpy.mockReturnValueOnce(true) - // MockCommitServiceGitFile.push.mockReturnValueOnce(undefined) - - const actual = await RepoService.moveFiles( - mockUserWithSiteSessionDataAndGrowthBook, - mockGithubSessionData, - "fake-old-path", - "fake-new-path", - ["fake-file1", "fake-file2"], - "fake-commit-message" - ) - - expect(actual).toEqual(expected) - }) - - it("should move files using GitHub directly if the repo is not ggs enabled", async () => { - const expected = { newSha: "fake-commit-sha" } - const fakeCommitMessage = "fake-commit-message" - const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ - githubId: mockGithubId, - accessToken: mockAccessToken, - isomerUserId: mockIsomerUserId, - email: mockEmail, - siteName: "not-whitelisted", + describe("moveFiles", () => { + it("should move files using the Git local file system if the repo is ggs enabled", async () => { + const expected = { newSha: "fake-commit-sha" } + MockGitFileCommitService.moveFiles.mockResolvedValueOnce(expected) + gbSpy.mockReturnValueOnce(true) + // MockCommitServiceGitFile.push.mockReturnValueOnce(undefined) + + const actual = await RepoService.moveFiles( + mockUserWithSiteSessionDataAndGrowthBook, + mockGithubSessionData, + "fake-old-path", + "fake-new-path", + ["fake-file1", "fake-file2"], + "fake-commit-message" + ) + + expect(actual).toEqual(expected) }) - MockGitHubCommitService.moveFiles.mockResolvedValueOnce(expected) - - const actual = await RepoService.moveFiles( - sessionData, - mockGithubSessionData, - "fake-path", - "fake-new-path", - ["old-fake-file.md", "old-fake-file-two.md"], - fakeCommitMessage - ) - - expect(actual).toEqual(expected) + it("should move files using GitHub directly if the repo is not ggs enabled", async () => { + const expected = { newSha: "fake-commit-sha" } + const fakeCommitMessage = "fake-commit-message" + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData( + { + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + } + ) + + const gitHubServiceMoveFiles = jest.spyOn( + GitHubService.prototype, + "moveFiles" + ) + gitHubServiceMoveFiles.mockResolvedValueOnce(expected) + + const actual = await RepoService.moveFiles( + sessionData, + mockGithubSessionData, + "fake-path", + "fake-new-path", + ["old-fake-file.md", "old-fake-file-two.md"], + fakeCommitMessage + ) + + expect(actual).toEqual(expected) + }) }) - }) - - describe("getLatestCommitOfBranch", () => { - it("should read the latest commit data from the local Git file system if the repo is ggs enabled", async () => { - const expected: GitHubCommitData = { - author: { - name: "test author", - email: "test@email.com", - date: "2023-07-20T11:25:05+08:00", - }, - sha: "test-sha", - message: "test message", - } - gbSpy.mockReturnValueOnce(true) - MockGitFileSystemService.getLatestCommitOfBranch.mockResolvedValueOnce( - okAsync(expected) - ) - const actual = await RepoService.getLatestCommitOfBranch( - mockUserWithSiteSessionDataAndGrowthBook, - "master" - ) - expect(actual).toEqual(expected) - }) + describe("getLatestCommitOfBranch", () => { + it("should read the latest commit data from the local Git file system if the repo is ggs enabled", async () => { + const expected: GitHubCommitData = { + author: { + name: "test author", + email: "test@email.com", + date: "2023-07-20T11:25:05+08:00", + }, + sha: "test-sha", + message: "test message", + } + gbSpy.mockReturnValueOnce(true) + MockGitFileSystemService.getLatestCommitOfBranch.mockResolvedValueOnce( + okAsync(expected) + ) + + const actual = await RepoService.getLatestCommitOfBranch( + mockUserWithSiteSessionDataAndGrowthBook, + "master" + ) + expect(actual).toEqual(expected) + }) - it("should read latest commit data from GitHub if the repo is not ggs enabled", async () => { - const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ - githubId: mockGithubId, - accessToken: mockAccessToken, - isomerUserId: mockIsomerUserId, - email: mockEmail, - siteName: "not-whitelisted", + it("should read latest commit data from GitHub if the repo is not ggs enabled", async () => { + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData( + { + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + } + ) + const expected: GitHubCommitData = { + author: { + name: "test author", + email: "test@email.com", + date: "2023-07-20T11:25:05+08:00", + }, + message: "test message", + } + const gitHubServiceReadDirectory = jest.spyOn( + GitHubService.prototype, + "getLatestCommitOfBranch" + ) + gitHubServiceReadDirectory.mockResolvedValueOnce(expected) + const actual = await RepoService.getLatestCommitOfBranch( + sessionData, + "master" + ) + expect(actual).toEqual(expected) }) - const expected: GitHubCommitData = { - author: { - name: "test author", - email: "test@email.com", - date: "2023-07-20T11:25:05+08:00", - }, - message: "test message", - } - const gitHubServiceReadDirectory = jest.spyOn( - GitHubService.prototype, - "getLatestCommitOfBranch" - ) - gitHubServiceReadDirectory.mockResolvedValueOnce(expected) - const actual = await RepoService.getLatestCommitOfBranch( - sessionData, - "master" - ) - expect(actual).toEqual(expected) }) - }) - describe("updateRepoState", () => { - it("should update the repo state on the local Git file system if the repo is ggs enabled", async () => { - MockGitFileSystemService.updateRepoState.mockResolvedValueOnce( - okAsync(undefined) - ) - gbSpy.mockReturnValueOnce(true) + describe("updateRepoState", () => { + it("should update the repo state on the local Git file system if the repo is ggs enabled", async () => { + MockGitFileSystemService.updateRepoState.mockResolvedValueOnce( + okAsync(undefined) + ) + gbSpy.mockReturnValueOnce(true) + + await RepoService.updateRepoState( + mockUserWithSiteSessionDataAndGrowthBook, + { + commitSha: "fake-sha", + branchName: "master", + } + ) + + expect(MockGitFileSystemService.updateRepoState).toBeCalledTimes(1) + }) - await RepoService.updateRepoState( - mockUserWithSiteSessionDataAndGrowthBook, - { + it("should update the repo state on GitHub if the repo is not ggs enabled", async () => { + const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData( + { + githubId: mockGithubId, + accessToken: mockAccessToken, + isomerUserId: mockIsomerUserId, + email: mockEmail, + siteName: "not-whitelisted", + } + ) + const gitHubServiceUpdateRepoState = jest.spyOn( + GitHubService.prototype, + "updateRepoState" + ) + gitHubServiceUpdateRepoState.mockResolvedValueOnce(undefined) + + await RepoService.updateRepoState(sessionData, { commitSha: "fake-sha", branchName: "master", - } - ) + }) - expect(MockGitFileSystemService.updateRepoState).toBeCalledTimes(1) - }) - - it("should update the repo state on GitHub if the repo is not ggs enabled", async () => { - const sessionData: UserWithSiteSessionData = new UserWithSiteSessionData({ - githubId: mockGithubId, - accessToken: mockAccessToken, - isomerUserId: mockIsomerUserId, - email: mockEmail, - siteName: "not-whitelisted", - }) - const gitHubServiceUpdateRepoState = jest.spyOn( - GitHubService.prototype, - "updateRepoState" - ) - gitHubServiceUpdateRepoState.mockResolvedValueOnce(undefined) - - await RepoService.updateRepoState(sessionData, { - commitSha: "fake-sha", - branchName: "master", + expect(gitHubServiceUpdateRepoState).toBeCalledTimes(1) }) - - expect(gitHubServiceUpdateRepoState).toBeCalledTimes(1) }) }) }) From a7654ffad9d524c7ff9e58d5d23facd003918e78 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 18:55:26 +0800 Subject: [PATCH 06/10] test(githubService): add tests (#1045) adding test cases for the two new functions `renameSinglePath` and `deleteDirectory` introduced into `GithubService` in #1042. Note that these functions already existed, but this pr adds the test cases to the right file. --- src/fixtures/sessionData.ts | 19 +++ src/services/db/GitHubService.ts | 34 ++-- src/services/db/RepoService.ts | 7 +- .../db/__tests__/GitHubService.spec.ts | 157 +++++++++++++++++- 4 files changed, 186 insertions(+), 31 deletions(-) diff --git a/src/fixtures/sessionData.ts b/src/fixtures/sessionData.ts index 661515caa..2e1c6e72d 100644 --- a/src/fixtures/sessionData.ts +++ b/src/fixtures/sessionData.ts @@ -4,6 +4,7 @@ import GithubSessionData from "@root/classes/GithubSessionData" import UserSessionData from "@root/classes/UserSessionData" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" import { FeatureFlags } from "@root/types/featureFlags" +import { RawGitTreeEntry } from "@root/types/github" import { MOCK_USER_EMAIL_ONE, @@ -25,6 +26,24 @@ export const mockCurrentCommitSha = "mockCurrentCommitSha" export const mockSiteName = "mockSiteName" export const mockGrowthBook = new GrowthBook() +export const gitTree: RawGitTreeEntry[] = [ + { + path: "directory/file1.txt", + type: "tree", + sha: "fake-sha-1", + mode: "100644", + url: "fake-url-1", + }, + { + path: "directory/file2.txt", + type: "file", + sha: "fake-sha-2", + mode: "100644", + url: "fake-url-2", + size: 100, + }, +] + export const mockGithubState = { treeSha: mockTreeSha, currentCommitSha: mockCurrentCommitSha, diff --git a/src/services/db/GitHubService.ts b/src/services/db/GitHubService.ts index c5710b98a..c8c7d1f46 100644 --- a/src/services/db/GitHubService.ts +++ b/src/services/db/GitHubService.ts @@ -10,7 +10,7 @@ import { isAxiosError, validateStatus } from "@utils/axios-utils" import GithubSessionData from "@root/classes/GithubSessionData" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" -import { STAGING_BRANCH, STAGING_LITE_BRANCH } from "@root/constants" +import { STAGING_BRANCH } from "@root/constants" import logger from "@root/logger/logger" import { GitCommitResult } from "@root/types/gitfilesystem" import { RawGitTreeEntry } from "@root/types/github" @@ -235,7 +235,7 @@ export default class GitHubService { async readMedia( sessionData: UserWithSiteSessionData, - { fileSha }: { fileSha: any; branchName?: string } + { fileSha }: { fileSha: string } ) { /** * Files that are bigger than 1 MB needs to be retrieved @@ -268,17 +268,14 @@ export default class GitHubService { async readDirectory( sessionData: UserWithSiteSessionData, - { - directoryName, - branchName = STAGING_BRANCH, - }: { directoryName: any; branchName?: string } + { directoryName }: { directoryName: string } ) { const { accessToken } = sessionData const { siteName } = sessionData const endpoint = this.getFolderPath({ siteName, directoryName }) const params = { - ref: branchName, + ref: STAGING_BRANCH, } const resp = await this.axiosInstance.get(endpoint, { @@ -300,13 +297,11 @@ export default class GitHubService { sha, fileName, directoryName, - branchName, }: { fileContent: string sha: string fileName: string directoryName: string | undefined - branchName: string } ): Promise { const { accessToken, siteName, isomerUserId: userId } = sessionData @@ -335,7 +330,7 @@ export default class GitHubService { const params = { message, content: encodedNewContent, - branch: branchName, + branch: STAGING_BRANCH, sha: fileSha, } @@ -519,8 +514,7 @@ export default class GitHubService { async getTree( sessionData: UserWithSiteSessionData, githubSessionData: GithubSessionData, - { isRecursive }: any, - isStaging = true + { isRecursive }: any ): Promise { const { accessToken } = sessionData const { siteName } = sessionData @@ -528,7 +522,7 @@ export default class GitHubService { const url = `${siteName}/git/trees/${treeSha}` const params = { - ref: isStaging ? STAGING_BRANCH : STAGING_LITE_BRANCH, + ref: STAGING_BRANCH, recursive: false, } @@ -690,17 +684,11 @@ export default class GitHubService { githubSessionData: GithubSessionData, oldPath: string, newPath: string, - message?: string, - isStaging = true + message?: string ): Promise { - const gitTree = await this.getTree( - sessionData, - githubSessionData, - { - isRecursive: true, - }, - !!isStaging - ) + const gitTree = await this.getTree(sessionData, githubSessionData, { + isRecursive: true, + }) const newGitTree: any[] = [] const isMovingDirectory = gitTree.find((item: any) => item.path === oldPath)?.type === "tree" || diff --git a/src/services/db/RepoService.ts b/src/services/db/RepoService.ts index 1d48bbeda..eacd93f07 100644 --- a/src/services/db/RepoService.ts +++ b/src/services/db/RepoService.ts @@ -363,9 +363,9 @@ export default class RepoService extends GitHubService { dirContent = result.value } else { - dirContent = (await super.readDirectory(sessionData, { + dirContent = await super.readDirectory(sessionData, { directoryName, - })) as GitDirectoryItem[] + }) } const { directories, files, total } = getPaginatedDirectoryContents( @@ -418,7 +418,6 @@ export default class RepoService extends GitHubService { sha, fileName, directoryName, - branchName: STAGING_BRANCH, }) } @@ -612,7 +611,7 @@ export default class RepoService extends GitHubService { { commitSha, branchName = BRANCH_REF, - }: { commitSha: string; branchName?: string } + }: { commitSha: string; branchName: string } ): Promise { const { siteName } = sessionData if ( diff --git a/src/services/db/__tests__/GitHubService.spec.ts b/src/services/db/__tests__/GitHubService.spec.ts index 03a1ced24..75866351a 100644 --- a/src/services/db/__tests__/GitHubService.spec.ts +++ b/src/services/db/__tests__/GitHubService.spec.ts @@ -15,9 +15,12 @@ import { mockCurrentCommitSha, mockGithubSessionData, mockIsomerUserId, + gitTree, } from "@fixtures/sessionData" +import { STAGING_BRANCH, STAGING_LITE_BRANCH } from "@root/constants" import { indexHtmlContent } from "@root/fixtures/markdown-fixtures" import { collectionYmlContent } from "@root/fixtures/yaml-fixtures" +import { RawGitTreeEntry } from "@root/types/github" import GitHubService from "@services/db/GitHubService" // using es6 gives some error @@ -531,7 +534,6 @@ describe("Github Service", () => { directoryName, fileContent: content, sha, - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ newSha: sha, @@ -560,7 +562,6 @@ describe("Github Service", () => { directoryName, fileContent: content, sha, - branchName: BRANCH_REF, }) ).rejects.toThrowError(NotFoundError) expect(mockAxiosInstance.put).toHaveBeenCalledWith( @@ -596,7 +597,6 @@ describe("Github Service", () => { directoryName, fileContent: content, sha: "", - branchName: BRANCH_REF, }) ).resolves.toMatchObject({ newSha: sha, @@ -628,7 +628,6 @@ describe("Github Service", () => { directoryName, fileContent: content, sha: "", - branchName: BRANCH_REF, }) ).rejects.toThrowError(NotFoundError) expect(mockAxiosInstance.get).toHaveBeenCalledWith(endpoint, { @@ -985,4 +984,154 @@ describe("Github Service", () => { expect(resp.isOk()).toEqual(true) }) }) + + describe("deleteDirectory", () => { + const message = JSON.stringify({ + message: `Delete directory: ${directoryName}`, + directoryName, + userId, + }) + const params = { + message, + branch: BRANCH_REF, + sha: treeSha, + force: true, + } + + const endpoint = `${siteName}/git/refs/heads/${STAGING_BRANCH}` + const stagingLiteEndpoint = `${siteName}/git/refs/heads/${STAGING_LITE_BRANCH}` + + it("should delete a directory correctly", async () => { + // Arrange + const getTreeSpy = jest.spyOn(service, "getTree") + getTreeSpy.mockResolvedValueOnce(gitTree) + mockAxiosInstance.get.mockImplementation(() => + Promise.resolve({ data: gitTree }) + ) + const updateTreeSpy = jest.spyOn(service, "updateTree") + updateTreeSpy.mockResolvedValueOnce(treeSha) + // Act + await service.deleteDirectory(sessionData, { + directoryName, + message, + githubSessionData: mockGithubSessionData, + }) + + // Assert + expect(mockAxiosInstance.patch).toHaveBeenCalledWith( + endpoint, + { + force: true, + sha: treeSha, + }, + { + headers: { Authorization: `token ${accessToken}` }, + } + ) + }) + + it("should throw the correct error if directory cannot be found", async () => { + // Arrange + const getTreeSpy = jest.spyOn(service, "getTree") + getTreeSpy.mockResolvedValueOnce(gitTree) + const updateTreeSpy = jest.spyOn(service, "updateTree") + updateTreeSpy.mockResolvedValueOnce(treeSha) + mockAxiosInstance.patch.mockImplementation(() => { + const err = { + response: { + status: 404, + }, + isAxiosError: true, + } + throw err + }) + + // Act + await expect( + service.deleteDirectory(sessionData, { + directoryName, + message, + githubSessionData: mockGithubSessionData, + }) + ).rejects.toStrictEqual({ isAxiosError: true, response: { status: 404 } }) + + // Assert + expect(mockAxiosInstance.patch).toHaveBeenCalledWith( + endpoint, + { + force: true, + sha: "mockTreeSha", + }, + { + headers: authHeader.headers, + } + ) + }) + }) + + describe("renameSinglePath", () => { + it("should rename a file correctly", async () => { + // Arrange + + const oldPath = "old/path.txt" + const newPath = "new/path.txt" + const message = "Renaming file" + + const newGitTree: RawGitTreeEntry[] = [ + { + path: oldPath, + type: "file", + sha: "new-sha2", + mode: "100644", + url: "", + }, + ] + + const resolvedTree = [ + { + path: newPath, + type: "file", + sha: "new-sha2", + mode: "100644", + url: "", + }, + { + path: oldPath, + type: "file", + sha: null, + mode: "100644", + url: "", + }, + ] + const newCommitSha = "new-commit-sha" + jest.spyOn(service, "getTree").mockResolvedValueOnce(newGitTree) + jest.spyOn(service, "updateTree").mockResolvedValueOnce(newCommitSha) + jest.spyOn(service, "updateRepoState").mockResolvedValueOnce() + + // Act + const result = await service.renameSinglePath( + sessionData, + mockGithubSessionData, + oldPath, + newPath, + message + ) + + // Assert + expect(service.getTree).toHaveBeenCalledWith( + sessionData, + mockGithubSessionData, + { isRecursive: true } + ) + expect(service.updateTree).toHaveBeenCalledWith( + sessionData, + mockGithubSessionData, + { gitTree: resolvedTree, message } + ) + expect(service.updateRepoState).toHaveBeenCalledWith(sessionData, { + commitSha: newCommitSha, + }) + expect(result).toEqual({ newSha: newCommitSha }) + }) + }) }) From 22f239c1ae5ff24293cf256383f9b3d6e556c992 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 18:57:29 +0800 Subject: [PATCH 07/10] feat(gitFileSystem): safer api (#1046) ## Problem 1. the presence of default arguments lead to sneaky bugs introduced due to caller unknowingly calling the wrong branch. 2. there is a scary instance whereby the lack of update to staging-lite could occur, but the update to staging does not, leading to issues downstream. Closes [insert issue #] ## Solution 1. to keep things simple, we make all the args compulsory. 2. make staging update first before updating staging lite. --- src/routes/formsg/formsgGGsRepair.ts | 2 +- src/services/db/GitFileCommitService.ts | 359 ++++++++---------- src/services/db/GitFileSystemService.ts | 46 ++- .../db/__tests__/GitFileSystemService.spec.ts | 25 +- 4 files changed, 205 insertions(+), 227 deletions(-) diff --git a/src/routes/formsg/formsgGGsRepair.ts b/src/routes/formsg/formsgGGsRepair.ts index 6558acda5..869902145 100644 --- a/src/routes/formsg/formsgGGsRepair.ts +++ b/src/routes/formsg/formsgGGsRepair.ts @@ -230,7 +230,7 @@ ${syncedRepos.map((repo) => `
  • ${repo}
    • `)} doesRepoNeedClone(repoName: string): ResultAsync { return this.gitFileSystemService - .isGitInitialized(repoName) + .isGitInitialized(repoName, true) .andThen((isRepoInEfs) => { if (isRepoInEfs) { return errAsync(false) diff --git a/src/services/db/GitFileCommitService.ts b/src/services/db/GitFileCommitService.ts index d89b47cf2..4dc640dff 100644 --- a/src/services/db/GitFileCommitService.ts +++ b/src/services/db/GitFileCommitService.ts @@ -1,6 +1,6 @@ import GithubSessionData from "@root/classes/GithubSessionData" import UserWithSiteSessionData from "@root/classes/UserWithSiteSessionData" -import { STAGING_BRANCH } from "@root/constants" +import { STAGING_BRANCH, STAGING_LITE_BRANCH } from "@root/constants" import logger from "@root/logger/logger" import { GitCommitResult } from "@root/types/gitfilesystem" import isFileAsset from "@root/utils/commit-utils" @@ -14,14 +14,34 @@ import GitFileSystemService from "./GitFileSystemService" * 2. Creates non-asset related commits to staging-lite */ export default class GitFileCommitService { - private readonly STAGING_LITE_BRANCH = "staging-lite" - private readonly gitFileSystemService: GitFileSystemService constructor(gitFileSystemService: GitFileSystemService) { this.gitFileSystemService = gitFileSystemService } + async pushToGithub( + sessionData: UserWithSiteSessionData, + shouldUpdateStagingLite: boolean + ) { + // We await the push to staging FIRST, and then push to staging-lite + // We don't want a case when staging lite updates but staging doesn't + const res = this.gitFileSystemService.push( + sessionData.siteName, + STAGING_BRANCH + ) + + if (shouldUpdateStagingLite) { + res.andThen(() => + this.gitFileSystemService.push( + sessionData.siteName, + STAGING_LITE_BRANCH + ) + ) + } + return res + } + async create( sessionData: UserWithSiteSessionData, { @@ -36,51 +56,43 @@ export default class GitFileCommitService { isMedia?: boolean } ): Promise<{ sha: string }> { - const createPromises = [ - this.gitFileSystemService.create( + const stagingCreateResult = await this.gitFileSystemService.create( + sessionData.siteName, + sessionData.isomerUserId, + content, + directoryName, + fileName, + isMedia ? "base64" : "utf-8", + STAGING_BRANCH + ) + const shouldUpdateStagingLite = + isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && + !isFileAsset({ directoryName, fileName }) + + let stagingLiteCreateResult + if (shouldUpdateStagingLite) { + stagingLiteCreateResult = await this.gitFileSystemService.create( sessionData.siteName, sessionData.isomerUserId, content, directoryName, fileName, isMedia ? "base64" : "utf-8", - STAGING_BRANCH - ), - ] - const shouldUpdateStagingLite = - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName, fileName }) - - if (shouldUpdateStagingLite) { - createPromises.push( - this.gitFileSystemService.create( - sessionData.siteName, - sessionData.isomerUserId, - content, - directoryName, - fileName, - isMedia ? "base64" : "utf-8", - this.STAGING_LITE_BRANCH - ) + STAGING_LITE_BRANCH ) } - const [stagingCreateResult, stagingLiteCreateResult] = await Promise.all( - createPromises - ) if (stagingCreateResult.isErr()) { throw stagingCreateResult.error - } else if (shouldUpdateStagingLite && stagingLiteCreateResult.isErr()) { + } else if ( + shouldUpdateStagingLite && + stagingLiteCreateResult && + stagingLiteCreateResult.isErr() + ) { throw stagingLiteCreateResult.error } - this.gitFileSystemService.push(sessionData.siteName, STAGING_BRANCH) - if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( - sessionData.siteName, - this.STAGING_LITE_BRANCH - ) - } + this.pushToGithub(sessionData, shouldUpdateStagingLite) return { sha: stagingCreateResult.value.newSha } } @@ -101,51 +113,43 @@ export default class GitFileCommitService { const defaultBranch = STAGING_BRANCH logger.info("Updating file in local Git file system") const filePath = directoryName ? `${directoryName}/${fileName}` : fileName - const updatePromises = [ - this.gitFileSystemService.update( - sessionData.siteName, - filePath, - fileContent, - sha, - sessionData.isomerUserId, - defaultBranch - ), - ] + const stagingUpdateResult = await this.gitFileSystemService.update( + sessionData.siteName, + filePath, + fileContent, + sha, + sessionData.isomerUserId, + defaultBranch + ) const shouldUpdateStagingLite = - filePath && + !!filePath && isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && !isFileAsset({ fileName, directoryName }) + let stagingLiteUpdateResult if (shouldUpdateStagingLite) { - updatePromises.push( - this.gitFileSystemService.update( - sessionData.siteName, - filePath, - fileContent, - sha, - sessionData.isomerUserId, - this.STAGING_LITE_BRANCH - ) + stagingLiteUpdateResult = await this.gitFileSystemService.update( + sessionData.siteName, + filePath, + fileContent, + sha, + sessionData.isomerUserId, + STAGING_LITE_BRANCH ) } - const results = await Promise.all(updatePromises) - const [stagingUpdateResult, stagingLiteUpdateResult] = results - if (stagingUpdateResult.isErr()) { throw stagingUpdateResult.error - } else if (shouldUpdateStagingLite && stagingLiteUpdateResult.isErr()) { + } else if ( + shouldUpdateStagingLite && + stagingLiteUpdateResult && + stagingLiteUpdateResult.isErr() + ) { throw stagingLiteUpdateResult.error } - this.gitFileSystemService.push(sessionData.siteName, defaultBranch) - if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( - sessionData.siteName, - this.STAGING_LITE_BRANCH - ) - } + this.pushToGithub(sessionData, shouldUpdateStagingLite) return { newSha: stagingUpdateResult.value } } @@ -161,50 +165,41 @@ export default class GitFileCommitService { logger.info( `Deleting directory in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}` ) - const deletePromises = [ - this.gitFileSystemService.delete( - sessionData.siteName, - directoryName, - "", - sessionData.isomerUserId, - true, - STAGING_BRANCH - ), - ] + const stagingDeleteResult = await this.gitFileSystemService.delete( + sessionData.siteName, + directoryName, + "", + sessionData.isomerUserId, + true, + defaultBranch + ) const shouldUpdateStagingLite = isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && !isFileAsset({ directoryName }) - + let stagingLiteDeleteResult if (shouldUpdateStagingLite) { - deletePromises.push( - this.gitFileSystemService.delete( - sessionData.siteName, - directoryName, - "", - sessionData.isomerUserId, - true, - this.STAGING_LITE_BRANCH - ) + stagingLiteDeleteResult = await this.gitFileSystemService.delete( + sessionData.siteName, + directoryName, + "", + sessionData.isomerUserId, + true, + STAGING_LITE_BRANCH ) } - const results = await Promise.all(deletePromises) - const [stagingDeleteResult, stagingLiteDeleteResult] = results - if (stagingDeleteResult.isErr()) { throw stagingDeleteResult.error - } else if (shouldUpdateStagingLite && stagingLiteDeleteResult.isErr()) { + } else if ( + shouldUpdateStagingLite && + stagingLiteDeleteResult && + stagingLiteDeleteResult.isErr() + ) { throw stagingLiteDeleteResult.error } - this.gitFileSystemService.push(sessionData.siteName, defaultBranch) - if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( - sessionData.siteName, - this.STAGING_LITE_BRANCH - ) - } + this.pushToGithub(sessionData, shouldUpdateStagingLite) } async delete( @@ -222,55 +217,45 @@ export default class GitFileCommitService { logger.info( `Deleting file in local Git file system for repo: ${sessionData.siteName}, directory name: ${directoryName}, file name: ${fileName}` ) - const defaultBranch = STAGING_BRANCH const filePath = directoryName ? `${directoryName}/${fileName}` : fileName - const deletePromises = [ - this.gitFileSystemService.delete( - sessionData.siteName, - filePath, - sha, - sessionData.isomerUserId, - false, - STAGING_BRANCH - ), - ] + const stagingDeleteResult = await this.gitFileSystemService.delete( + sessionData.siteName, + filePath, + sha, + sessionData.isomerUserId, + false, + STAGING_BRANCH + ) const shouldUpdateStagingLite = isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && !isFileAsset({ directoryName }) + let stagingLiteDeleteResult if (shouldUpdateStagingLite) { - deletePromises.push( - this.gitFileSystemService.delete( - sessionData.siteName, - filePath, - sha, - sessionData.isomerUserId, - false, - this.STAGING_LITE_BRANCH - ) + stagingLiteDeleteResult = await this.gitFileSystemService.delete( + sessionData.siteName, + filePath, + sha, + sessionData.isomerUserId, + false, + STAGING_LITE_BRANCH ) } - const [stagingDeleteResult, stagingLiteDeleteResult] = await Promise.all( - deletePromises - ) - if (stagingDeleteResult.isErr()) { throw stagingDeleteResult.error - } else if (shouldUpdateStagingLite && stagingLiteDeleteResult.isErr()) { + } else if ( + shouldUpdateStagingLite && + stagingLiteDeleteResult && + stagingLiteDeleteResult.isErr() + ) { throw stagingLiteDeleteResult.error } - this.gitFileSystemService.push(sessionData.siteName, defaultBranch) - if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( - sessionData.siteName, - this.STAGING_LITE_BRANCH - ) - } + this.pushToGithub(sessionData, shouldUpdateStagingLite) } async renameSinglePath( @@ -283,50 +268,38 @@ export default class GitFileCommitService { const defaultBranch = STAGING_BRANCH logger.info("Renaming file/directory in local Git file system") - const renamePromises = [ - this.gitFileSystemService.renameSinglePath( - sessionData.siteName, - oldPath, - newPath, - sessionData.isomerUserId, - defaultBranch, - message - ), - ] - - const shouldUpdateStagingLite = - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName: oldPath }) - - if (shouldUpdateStagingLite) { - renamePromises.push( - this.gitFileSystemService.renameSinglePath( - sessionData.siteName, - oldPath, - newPath, - sessionData.isomerUserId, - this.STAGING_LITE_BRANCH, - message - ) - ) - } - - const results = await Promise.all(renamePromises) - const [stagingRenameResult, stagingLiteRenameResult] = results + const stagingRenameResult = await this.gitFileSystemService.renameSinglePath( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + defaultBranch, + message + ) if (stagingRenameResult.isErr()) { throw stagingRenameResult.error - } else if (shouldUpdateStagingLite && stagingLiteRenameResult.isErr()) { - throw stagingLiteRenameResult.error } - this.gitFileSystemService.push(sessionData.siteName, defaultBranch) + const shouldUpdateStagingLite = + isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && + !isFileAsset({ directoryName: oldPath }) + if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( + const stagingLiteRenameResult = await this.gitFileSystemService.renameSinglePath( sessionData.siteName, - this.STAGING_LITE_BRANCH + oldPath, + newPath, + sessionData.isomerUserId, + STAGING_LITE_BRANCH, + message ) + if (stagingLiteRenameResult.isErr()) { + throw stagingLiteRenameResult.error + } } + + this.pushToGithub(sessionData, shouldUpdateStagingLite) return { newSha: stagingRenameResult.value } } @@ -340,51 +313,39 @@ export default class GitFileCommitService { ): Promise { logger.info("Moving files in local Git file system") const defaultBranch = STAGING_BRANCH - const mvFilesResults = [ - this.gitFileSystemService.moveFiles( + const stagingMvFilesResult = await this.gitFileSystemService.moveFiles( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + targetFiles, + defaultBranch, + message + ) + if (stagingMvFilesResult.isErr()) { + throw stagingMvFilesResult.error + } + + const shouldUpdateStagingLite = + isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && + !isFileAsset({ directoryName: oldPath }) + + if (shouldUpdateStagingLite) { + const stagingLiteMvFilesResult = await this.gitFileSystemService.moveFiles( sessionData.siteName, oldPath, newPath, sessionData.isomerUserId, targetFiles, - defaultBranch, + STAGING_LITE_BRANCH, message - ), - ] - const shouldUpdateStagingLite = - isReduceBuildTimesWhitelistedRepo(sessionData.growthbook) && - !isFileAsset({ directoryName: oldPath }) - - if (shouldUpdateStagingLite) { - mvFilesResults.push( - this.gitFileSystemService.moveFiles( - sessionData.siteName, - oldPath, - newPath, - sessionData.isomerUserId, - targetFiles, - this.STAGING_LITE_BRANCH, - message - ) ) + if (stagingLiteMvFilesResult.isErr()) { + throw stagingLiteMvFilesResult.error + } } - const results = await Promise.all(mvFilesResults) - const [stagingMvFilesResult, stagingLiteMvFilesResult] = results - - if (stagingMvFilesResult.isErr()) { - throw stagingMvFilesResult.error - } else if (shouldUpdateStagingLite && stagingLiteMvFilesResult.isErr()) { - throw stagingLiteMvFilesResult.error - } - - this.gitFileSystemService.push(sessionData.siteName, defaultBranch) - if (shouldUpdateStagingLite) { - this.gitFileSystemService.push( - sessionData.siteName, - this.STAGING_LITE_BRANCH - ) - } + this.pushToGithub(sessionData, shouldUpdateStagingLite) return { newSha: stagingMvFilesResult.value } } } diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 9c8b42a9b..5f4cbe402 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -46,16 +46,20 @@ export default class GitFileSystemService { this.git = git } - private getEfsVolPathFromBranch(branchName: string) { + private getEfsVolPathFromBranch(branchName: string): string { return branchName === STAGING_LITE_BRANCH ? EFS_VOL_PATH_STAGING_LITE : EFS_VOL_PATH_STAGING } - private getEfsVolPath(isStaging: boolean) { + private getEfsVolPath(isStaging: boolean): string { return isStaging ? EFS_VOL_PATH_STAGING : EFS_VOL_PATH_STAGING_LITE } + private isStagingFromBranchName(branchName: string): boolean { + return branchName !== STAGING_LITE_BRANCH + } + /** * NOTE: We can do concurrent writes to the staging branch and the staging lite branch * since they exist in different folders. @@ -97,7 +101,7 @@ export default class GitFileSystemService { isGitInitialized( repoName: string, - isStaging = true + isStaging: boolean ): ResultAsync { const repoPath = isStaging ? `${EFS_VOL_PATH_STAGING}/${repoName}` @@ -122,7 +126,7 @@ export default class GitFileSystemService { isOriginRemoteCorrect( repoName: string, - isStaging = true + isStaging: boolean ): ResultAsync { const originUrl = `git@github.com:${ISOMER_GITHUB_ORG_NAME}/${repoName}.git` const repoPath = isStaging @@ -149,11 +153,9 @@ export default class GitFileSystemService { repoName: string, branchName: string ): ResultAsync { - return this.getFilePathStats( - repoName, - "", - branchName !== STAGING_LITE_BRANCH - ) + const isStaging = this.isStagingFromBranchName(branchName) + + return this.getFilePathStats(repoName, "", isStaging) .andThen((stats) => { if (!stats.isDirectory()) { // Return as an error to prevent further processing @@ -168,14 +170,14 @@ export default class GitFileSystemService { } return err(error) }) - .andThen(() => this.isGitInitialized(repoName)) + .andThen(() => this.isGitInitialized(repoName, isStaging)) .andThen((isGitInitialized) => { if (!isGitInitialized) { return err(false) } return ok(true) }) - .andThen(() => this.isOriginRemoteCorrect(repoName)) + .andThen(() => this.isOriginRemoteCorrect(repoName, isStaging)) .andThen((isOriginRemoteCorrect) => { if (!isOriginRemoteCorrect) { return err(false) @@ -235,7 +237,7 @@ export default class GitFileSystemService { getGitBlobHash( repoName: string, filePath: string, - isStaging = true + isStaging: boolean ): ResultAsync { const efsVolPath = isStaging ? EFS_VOL_PATH_STAGING @@ -403,7 +405,7 @@ export default class GitFileSystemService { }).map(() => `${efsVolPath}/${repoName}`) } - return this.isGitInitialized(repoName) + return this.isGitInitialized(repoName, isStaging) .andThen((isGitInitialized) => { if (!isGitInitialized) { return errAsync( @@ -416,7 +418,7 @@ export default class GitFileSystemService { } return okAsync(true) }) - .andThen(() => this.isOriginRemoteCorrect(repoName)) + .andThen(() => this.isOriginRemoteCorrect(repoName, isStaging)) .andThen((isOriginRemoteCorrect) => { if (!isOriginRemoteCorrect) { return errAsync( @@ -791,7 +793,7 @@ export default class GitFileSystemService { return new GitFileSystemError("An unknown error occurred") } ), - this.getGitBlobHash(repoName, filePath), + this.getGitBlobHash(repoName, filePath, true), ]).map((contentAndHash) => { const [content, sha] = contentAndHash const result: GitFile = { @@ -858,7 +860,7 @@ export default class GitFileSystemService { mediaUrl: `${dataUrlPrefix},${file.content}`, mediaPath: `${directoryName}/${fileName}`, type: fileType, - addedTime: stats.ctimeMs, + addedTime: stats.birthtimeMs, size: stats.size, }) }) @@ -871,6 +873,7 @@ export default class GitFileSystemService { branchName: string ): ResultAsync { const efsVolPath = this.getEfsVolPathFromBranch(branchName) + const isStaging = this.isStagingFromBranchName(branchName) return this.getFilePathStats( repoName, directoryPath, @@ -909,7 +912,7 @@ export default class GitFileSystemService { const path = directoryPath === "" ? name : `${directoryPath}/${name}` const type = isDirectory ? "dir" : "file" - return this.getGitBlobHash(repoName, path) + return this.getGitBlobHash(repoName, path, isStaging) .orElse(() => okAsync("")) .andThen((sha) => ResultAsync.combine([ @@ -929,7 +932,7 @@ export default class GitFileSystemService { sha, path, size: type === "dir" ? 0 : stats.size, - addedTime: stats.ctimeMs, + addedTime: stats.birthtimeMs, } return okAsync(result) @@ -955,6 +958,7 @@ export default class GitFileSystemService { ): ResultAsync { let oldStateSha = "" const efsVolPath = this.getEfsVolPathFromBranch(branchName) + const isStaging = this.isStagingFromBranchName(branchName) return this.getLatestCommitOfBranch(repoName, branchName) .andThen((latestCommit) => { // It is guaranteed that the latest commit contains the SHA hash @@ -979,7 +983,7 @@ export default class GitFileSystemService { return okAsync(true) }) .andThen(() => - this.getGitBlobHash(repoName, filePath).andThen((sha) => { + this.getGitBlobHash(repoName, filePath, isStaging).andThen((sha) => { if (sha !== oldSha) { return errAsync( new ConflictError( @@ -1044,6 +1048,8 @@ export default class GitFileSystemService { ): ResultAsync { let oldStateSha = "" const efsVolPath = this.getEfsVolPathFromBranch(branchName) + const isStaging = this.isStagingFromBranchName(branchName) + return this.getLatestCommitOfBranch(repoName, branchName) .andThen((latestCommit) => { if (!latestCommit.sha) { @@ -1084,7 +1090,7 @@ export default class GitFileSystemService { if (isDir) { return okAsync(true) // If it's a directory, skip the blob hash verification } - return this.getGitBlobHash(repoName, path).andThen((sha) => { + return this.getGitBlobHash(repoName, path, isStaging).andThen((sha) => { if (sha !== oldSha) { return errAsync( new ConflictError( diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index a6887c2c5..c38cbabe1 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -248,7 +248,10 @@ describe("GitFileSystemService", () => { checkIsRepo: jest.fn().mockResolvedValueOnce(true), }) - const result = await GitFileSystemService.isGitInitialized("fake-repo") + const result = await GitFileSystemService.isGitInitialized( + "fake-repo", + true + ) expect(result._unsafeUnwrap()).toBeTrue() }) @@ -258,7 +261,10 @@ describe("GitFileSystemService", () => { checkIsRepo: jest.fn().mockResolvedValueOnce(false), }) - const result = await GitFileSystemService.isGitInitialized("fake-repo") + const result = await GitFileSystemService.isGitInitialized( + "fake-repo", + true + ) expect(result._unsafeUnwrap()).toBeFalse() }) @@ -288,7 +294,8 @@ describe("GitFileSystemService", () => { }) const result = await GitFileSystemService.isOriginRemoteCorrect( - "fake-repo" + "fake-repo", + true ) expect(result._unsafeUnwrap()).toBeTrue() @@ -304,7 +311,8 @@ describe("GitFileSystemService", () => { }) const result = await GitFileSystemService.isOriginRemoteCorrect( - "fake-repo" + "fake-repo", + true ) expect(result._unsafeUnwrap()).toBeFalse() @@ -316,7 +324,8 @@ describe("GitFileSystemService", () => { }) const result = await GitFileSystemService.isOriginRemoteCorrect( - "fake-repo" + "fake-repo", + true ) expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) @@ -515,7 +524,8 @@ describe("GitFileSystemService", () => { const result = await GitFileSystemService.getGitBlobHash( "fake-repo", - "fake-dir/fake-file" + "fake-dir/fake-file", + true ) expect(result._unsafeUnwrap()).toBe("fake-hash") @@ -528,7 +538,8 @@ describe("GitFileSystemService", () => { const result = await GitFileSystemService.getGitBlobHash( "fake-repo", - "fake-dir/fake-file" + "fake-dir/fake-file", + true ) expect(result._unsafeUnwrapErr()).toBeInstanceOf(GitFileSystemError) From 6bd471d4245db5d599ec53bd0fe296ac66a852d6 Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 18:59:40 +0800 Subject: [PATCH 08/10] test(gitCommitService): add test cases (#1047) add test cases for `GitFileCommitService`. --- .../db/__tests__/GitFileCommitService.spec.ts | 625 ++++++++++++++++++ 1 file changed, 625 insertions(+) create mode 100644 src/services/db/__tests__/GitFileCommitService.spec.ts diff --git a/src/services/db/__tests__/GitFileCommitService.spec.ts b/src/services/db/__tests__/GitFileCommitService.spec.ts new file mode 100644 index 000000000..3eb4914e7 --- /dev/null +++ b/src/services/db/__tests__/GitFileCommitService.spec.ts @@ -0,0 +1,625 @@ +import fs, { Stats } from "fs" + +import mockFs from "mock-fs" +import { ResultAsync, ok, okAsync } from "neverthrow" +import { GitError, SimpleGit } from "simple-git" + +import config from "@config/config" + +import { BadRequestError } from "@errors/BadRequestError" +import { ConflictError } from "@errors/ConflictError" +import GitFileSystemError from "@errors/GitFileSystemError" +import GitFileSystemNeedsRollbackError from "@errors/GitFileSystemNeedsRollbackError" +import { NotFoundError } from "@errors/NotFoundError" + +import { + EFS_VOL_PATH_STAGING, + EFS_VOL_PATH_STAGING_LITE, + ISOMER_GITHUB_ORG_NAME, + STAGING_BRANCH, + STAGING_LITE_BRANCH, +} from "@constants/constants" + +import { + MOCK_GITHUB_FILENAME_ALPHA_ONE, + MOCK_GITHUB_COMMIT_MESSAGE_ALPHA_ONE, +} from "@fixtures/github" +import { MOCK_USER_ID_ONE } from "@fixtures/users" +import { MediaTypeError } from "@root/errors/MediaTypeError" +import { + mockGithubSessionData, + mockUserWithSiteSessionData, + mockUserWithSiteSessionDataAndGrowthBook, +} from "@root/fixtures/sessionData" +import { MediaFileOutput } from "@root/types" +import { GitHubCommitData } from "@root/types/commitData" +import { GitDirectoryItem, GitFile } from "@root/types/gitfilesystem" +import _GitFileCommitService from "@services/db/GitFileCommitService" +import _GitFileSystemService from "@services/db/GitFileSystemService" + +import * as gbUtils from "../../../utils/growthbook-utils" + +const MockSimpleGit = { + clone: jest.fn(), + cwd: jest.fn(), +} + +const gitFileSystemService = new _GitFileSystemService( + (MockSimpleGit as unknown) as SimpleGit +) + +const gitFileCommitService = new _GitFileCommitService(gitFileSystemService) + +const BRANCH_REF = config.get("github.branchRef") +const DEFAULT_BRANCH = "staging" +const sessionData = mockUserWithSiteSessionData +describe("GitFileCommitService", () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + describe("pushToGithub", () => { + it("should push to staging and staging-lite", async () => { + // Arrange + const pushSpy = jest + .spyOn(gitFileSystemService, "push") + .mockImplementation((siteName, branchName) => + ResultAsync.fromPromise( + Promise.resolve("push ok"), + () => new GitFileSystemError("push failed") + ) + ) + + // Act + await gitFileCommitService.pushToGithub(sessionData, true) + + // Assert + expect(pushSpy).toHaveBeenCalledWith(sessionData.siteName, STAGING_BRANCH) + expect(pushSpy).toHaveBeenCalledWith( + sessionData.siteName, + STAGING_LITE_BRANCH + ) + }) + + it("should only push to staging if shouldUpdateStagingLite is false", async () => { + // Arrange + const pushSpy = jest + .spyOn(gitFileSystemService, "push") + .mockResolvedValue(okAsync("push ok")) + + // Act + await gitFileCommitService.pushToGithub(sessionData, false) + + // Assert + expect(pushSpy).toHaveBeenCalledWith(sessionData.siteName, STAGING_BRANCH) + expect(pushSpy).toHaveBeenCalledTimes(1) + }) + + describe("create", () => { + it("should create a file and push to GitHub", async () => { + // Arrange + + const content = "file content" + const fileName = "file.txt" + const directoryName = "directory" + const isMedia = false + + const createSpy = jest + .spyOn(gitFileSystemService, "create") + .mockResolvedValue(okAsync({ newSha: "new-sha" })) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + // Act + const result = await gitFileCommitService.create(sessionData, { + content, + fileName, + directoryName, + isMedia, + }) + + // Assert + expect(createSpy).toHaveBeenCalledWith( + sessionData.siteName, + sessionData.isomerUserId, + content, + directoryName, + fileName, + "utf-8", + STAGING_BRANCH + ) + expect(createSpy).toHaveBeenCalledWith( + sessionData.siteName, + sessionData.isomerUserId, + content, + directoryName, + fileName, + "utf-8", + STAGING_LITE_BRANCH + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + expect(result).toEqual({ sha: "new-sha" }) + }) + + it("should create a file and commit only to staging branch when not whitelisted", async () => { + const content = "file content" + const fileName = "file.txt" + const directoryName = "directory" + const isMedia = false + + const createSpy = jest + .spyOn(gitFileSystemService, "create") + .mockResolvedValue(okAsync({ newSha: "new-sha" })) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + const result = await gitFileCommitService.create(sessionData, { + content, + fileName, + directoryName, + isMedia, + }) + + // Assert + expect(createSpy).toHaveBeenCalledWith( + sessionData.siteName, + sessionData.isomerUserId, + content, + directoryName, + fileName, + isMedia ? "base64" : "utf-8", + STAGING_BRANCH + ) + expect(createSpy).toHaveBeenCalledTimes(1) // create is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + expect(result).toEqual({ sha: "new-sha" }) + }) + }) + + describe("update", () => { + it("should update a file and push to GitHub", async () => { + // Arrange + const fileContent = "updated file content" + const sha = "old-sha" + const fileName = "file.txt" + const directoryName = "directory" + + const updateSpy = jest + .spyOn(gitFileSystemService, "update") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + + // Act + const result = await gitFileCommitService.update(sessionData, { + fileContent, + sha, + fileName, + directoryName, + }) + + // Assert + const filePath = directoryName + ? `${directoryName}/${fileName}` + : fileName + expect(updateSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + fileContent, + sha, + sessionData.isomerUserId, + STAGING_BRANCH + ) + + expect(updateSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + fileContent, + sha, + sessionData.isomerUserId, + STAGING_LITE_BRANCH + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + expect(result).toEqual({ newSha: "new-sha" }) + }) + + it("should update a file and commit only to staging branch when not whitelisted", async () => { + const fileContent = "updated file content" + const sha = "old-sha" + const fileName = "file.txt" + const directoryName = "directory" + + const updateSpy = jest + .spyOn(gitFileSystemService, "update") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + const result = await gitFileCommitService.update(sessionData, { + fileContent, + sha, + fileName, + directoryName, + }) + + // Assert + const filePath = directoryName + ? `${directoryName}/${fileName}` + : fileName + expect(updateSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + fileContent, + sha, + sessionData.isomerUserId, + STAGING_BRANCH + ) + expect(updateSpy).toHaveBeenCalledTimes(1) // update is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + expect(result).toEqual({ newSha: "new-sha" }) + }) + }) + + describe("deleteDirectory", () => { + it("should delete a directory and push to GitHub", async () => { + // Arrange + const directoryName = "directory" + + const deleteSpy = jest + .spyOn(gitFileSystemService, "delete") + .mockResolvedValue(okAsync("")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + + // Act + await gitFileCommitService.deleteDirectory(sessionData, { + directoryName, + }) + + // Assert + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + directoryName, + "", + sessionData.isomerUserId, + true, + STAGING_BRANCH + ) + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + directoryName, + "", + sessionData.isomerUserId, + true, + STAGING_LITE_BRANCH + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + }) + + it("should delete a directory and commit only to staging branch when not whitelisted", async () => { + const directoryName = "directory" + + const deleteSpy = jest + .spyOn(gitFileSystemService, "delete") + .mockResolvedValue(okAsync("")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + await gitFileCommitService.deleteDirectory(sessionData, { + directoryName, + }) + + // Assert + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + directoryName, + "", + sessionData.isomerUserId, + true, + STAGING_BRANCH + ) + expect(deleteSpy).toHaveBeenCalledTimes(1) // delete is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + }) + }) + + describe("delete", () => { + it("should delete a file and push to GitHub", async () => { + // Arrange + const sha = "file-sha" + const fileName = "file.txt" + const directoryName = "directory" + + const deleteSpy = jest + .spyOn(gitFileSystemService, "delete") + .mockResolvedValue(okAsync("")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + + // Act + await gitFileCommitService.delete(sessionData, { + sha, + fileName, + directoryName, + }) + + // Assert + const filePath = directoryName + ? `${directoryName}/${fileName}` + : fileName + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + sha, + sessionData.isomerUserId, + false, + STAGING_BRANCH + ) + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + sha, + sessionData.isomerUserId, + false, + STAGING_LITE_BRANCH + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + }) + + it("should delete a file and commit only to staging branch when not whitelisted", async () => { + const sha = "file-sha" + const fileName = "file.txt" + const directoryName = "directory" + + const deleteSpy = jest + .spyOn(gitFileSystemService, "delete") + .mockResolvedValue(okAsync("")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + await gitFileCommitService.delete(sessionData, { + sha, + fileName, + directoryName, + }) + + // Assert + const filePath = directoryName + ? `${directoryName}/${fileName}` + : fileName + expect(deleteSpy).toHaveBeenCalledWith( + sessionData.siteName, + filePath, + sha, + sessionData.isomerUserId, + false, + STAGING_BRANCH + ) + expect(deleteSpy).toHaveBeenCalledTimes(1) // delete is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + }) + }) + + describe("renameSinglePath", () => { + it("should rename a file or directory and push to GitHub", async () => { + // Arrange + const oldPath = "old-path" + const newPath = "new-path" + const message = "rename message" + + const renameSpy = jest + .spyOn(gitFileSystemService, "renameSinglePath") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + + // Act + const result = await gitFileCommitService.renameSinglePath( + sessionData, + mockGithubSessionData, + oldPath, + newPath, + message + ) + + // Assert + expect(renameSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + STAGING_BRANCH, + message + ) + expect(renameSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + STAGING_LITE_BRANCH, + message + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + expect(result).toEqual({ newSha: "new-sha" }) + }) + + it("should rename a file or directory and commit only to staging branch when not whitelisted", async () => { + const oldPath = "old-path" + const newPath = "new-path" + const message = "rename message" + + const renameSpy = jest + .spyOn(gitFileSystemService, "renameSinglePath") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + const result = await gitFileCommitService.renameSinglePath( + sessionData, + mockGithubSessionData, + oldPath, + newPath, + message + ) + + // Assert + expect(renameSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + STAGING_BRANCH, + message + ) + expect(renameSpy).toHaveBeenCalledTimes(1) // renameSinglePath is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + expect(result).toEqual({ newSha: "new-sha" }) + }) + }) + + describe("moveFiles", () => { + it("should move files and push to GitHub", async () => { + // Arrange + const oldPath = "old-path" + const newPath = "new-path" + const targetFiles = ["file1.txt", "file2.txt"] + const message = "move files message" + + const moveFilesSpy = jest + .spyOn(gitFileSystemService, "moveFiles") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(true) + + // Act + const result = await gitFileCommitService.moveFiles( + sessionData, + mockGithubSessionData, + oldPath, + newPath, + targetFiles, + message + ) + + // Assert + expect(moveFilesSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + targetFiles, + STAGING_BRANCH, + message + ) + expect(moveFilesSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + targetFiles, + STAGING_LITE_BRANCH, + message + ) + expect(pushSpy).toHaveBeenCalledWith(sessionData, expect.any(Boolean)) + expect(result).toEqual({ newSha: "new-sha" }) + }) + + it("should move files and commit only to staging branch when not whitelisted", async () => { + const oldPath = "old-path" + const newPath = "new-path" + const targetFiles = ["file1.txt", "file2.txt"] + const message = "move files message" + + const moveFilesSpy = jest + .spyOn(gitFileSystemService, "moveFiles") + .mockResolvedValue(okAsync("new-sha")) + const pushSpy = jest + .spyOn(gitFileCommitService, "pushToGithub") + .mockResolvedValue(ok("")) + + jest + .spyOn(gbUtils, "isReduceBuildTimesWhitelistedRepo") + .mockReturnValue(false) + + // Act + const result = await gitFileCommitService.moveFiles( + sessionData, + mockGithubSessionData, + oldPath, + newPath, + targetFiles, + message + ) + + // Assert + expect(moveFilesSpy).toHaveBeenCalledWith( + sessionData.siteName, + oldPath, + newPath, + sessionData.isomerUserId, + targetFiles, + STAGING_BRANCH, + message + ) + expect(moveFilesSpy).toHaveBeenCalledTimes(1) // moveFiles is called only once for the staging branch + expect(pushSpy).toHaveBeenCalledWith(sessionData, false) // push is called with shouldUpdateStagingLite as false + expect(result).toEqual({ newSha: "new-sha" }) + }) + }) + }) +}) From 00c532e1d9242a874d2d1ff94b7424472ff420dc Mon Sep 17 00:00:00 2001 From: Kishore <42832651+kishore03109@users.noreply.github.com> Date: Wed, 6 Dec 2023 19:02:59 +0800 Subject: [PATCH 09/10] fix(rollback handler): convert to ts for safety (#1044) * fix(rollback handler): convert to ts for safety * Untitled commit * Untitled commit * Untitled commit * Untitled commit * fix(retry): add max 5 retries) * feat(loggin): add logging for possibility of alarm * feat(style): use resultAsync for clarity + refactor * fix(privatisation): should be write handler --- src/middleware/routeHandler.js | 202 ------------- src/middleware/routeHandler.ts | 284 ++++++++++++++++++ src/routes/v2/authenticatedSites/contactUs.js | 4 +- .../v2/authenticatedSites/navigation.js | 4 +- src/routes/v2/authenticatedSites/settings.js | 2 +- src/services/db/GitFileSystemService.ts | 27 +- .../db/__tests__/GitFileSystemService.spec.ts | 3 + src/utils/neverthrow.ts | 15 + 8 files changed, 332 insertions(+), 209 deletions(-) delete mode 100644 src/middleware/routeHandler.js create mode 100644 src/middleware/routeHandler.ts create mode 100644 src/utils/neverthrow.ts diff --git a/src/middleware/routeHandler.js b/src/middleware/routeHandler.js deleted file mode 100644 index 147c75534..000000000 --- a/src/middleware/routeHandler.js +++ /dev/null @@ -1,202 +0,0 @@ -const { backOff } = require("exponential-backoff") -const SimpleGit = require("simple-git") - -const { config } = require("@config/config") - -const logger = require("@logger/logger").default - -const { default: GithubSessionData } = require("@classes/GithubSessionData") - -const { lock, unlock } = require("@utils/mutex-utils") -const { getCommitAndTreeSha, revertCommit } = require("@utils/utils.js") - -const { MAX_CONCURRENT_GIT_PROCESSES } = require("@constants/constants") - -const { FEATURE_FLAGS } = require("@root/constants/featureFlags") -const GitFileSystemError = require("@root/errors/GitFileSystemError").default -const LockedError = require("@root/errors/LockedError").default -const { - default: GitFileSystemService, -} = require("@services/db/GitFileSystemService") - -const BRANCH_REF = config.get("github.branchRef") - -const gitFileSystemService = new GitFileSystemService( - new SimpleGit({ maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES }) -) - -const handleGitFileLock = async (repoName, next) => { - const result = await gitFileSystemService.hasGitFileLock(repoName) - if (result.isErr()) { - next(result.err) - return false - } - const isGitLocked = result.value - if (isGitLocked) { - logger.error(`Failed to lock repo ${repoName}: git file system in use.`) - next( - new LockedError( - `Someone else is currently modifying repo ${repoName}. Please try again later.` - ) - ) - return false - } - return true -} - -// Used when there are no write API calls to the repo on GitHub -const attachReadRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - routeHandler(req, res).catch((err) => { - next(err) - }) -} - -// Used when there are write API calls to the repo on GitHub -const attachWriteRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - const { siteName } = req.params - const { growthbook } = req - - let isGitAvailable = true - - // only check git file lock if the repo is ggs enabled - if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { - isGitAvailable = await handleGitFileLock(siteName, next) - } - - if (!isGitAvailable) return - - try { - await lock(siteName) - } catch (err) { - next(err) - return - } - - await routeHandler(req, res, next).catch(async (err) => { - await unlock(siteName) - next(err) - }) - - try { - await unlock(siteName) - } catch (err) { - next(err) - } -} - -const attachRollbackRouteHandlerWrapper = (routeHandler) => async ( - req, - res, - next -) => { - const { userSessionData } = res.locals - const { siteName } = req.params - - const { accessToken } = userSessionData - const { growthbook } = req - - const shouldUseGitFileSystem = !!growthbook?.getFeatureValue( - FEATURE_FLAGS.IS_GGS_ENABLED, - false - ) - - const isGitAvailable = await handleGitFileLock(siteName, next) - if (!isGitAvailable) return - try { - await lock(siteName) - } catch (err) { - next(err) - return - } - - let originalCommitSha - if (shouldUseGitFileSystem) { - const result = await gitFileSystemService.getLatestCommitOfBranch( - siteName, - BRANCH_REF - ) - if (result.isErr()) { - await unlock(siteName) - next(result.err) - return - } - originalCommitSha = result.value.sha - if (!originalCommitSha) { - await unlock(siteName) - next(result.err) - return - } - // Unused for git file system, but to maintain existing structure - res.locals.githubSessionData = new GithubSessionData({ - currentCommitSha: "", - treeSha: "", - }) - } else { - try { - const { currentCommitSha, treeSha } = await getCommitAndTreeSha( - siteName, - accessToken - ) - - const githubSessionData = new GithubSessionData({ - currentCommitSha, - treeSha, - }) - res.locals.githubSessionData = githubSessionData - - originalCommitSha = currentCommitSha - } catch (err) { - await unlock(siteName) - next(err) - return - } - } - await routeHandler(req, res, next).catch(async (err) => { - try { - if (shouldUseGitFileSystem) { - await backOff(() => { - const rollbackRes = gitFileSystemService - .rollback(siteName, originalCommitSha) - .unwrapOr(false) - if (!rollbackRes) throw new GitFileSystemError("Rollback failure") - }) - await backOff(() => { - const pushRes = gitFileSystemService - .push(siteName, true) - .unwrapOr(false) - if (!pushRes) throw new GitFileSystemError("Push failure") - }) - } else { - await backOff(() => { - revertCommit(originalCommitSha, siteName, accessToken) - }) - } - } catch (retryErr) { - await unlock(siteName) - next(retryErr) - return - } - await unlock(siteName) - next(err) - }) - - try { - await unlock(siteName) - } catch (err) { - next(err) - } -} - -module.exports = { - attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, - attachRollbackRouteHandlerWrapper, -} diff --git a/src/middleware/routeHandler.ts b/src/middleware/routeHandler.ts new file mode 100644 index 000000000..46d9352a0 --- /dev/null +++ b/src/middleware/routeHandler.ts @@ -0,0 +1,284 @@ +import { GrowthBook } from "@growthbook/growthbook" +import { BackoffOptions, backOff } from "exponential-backoff" +import { ResultAsync } from "neverthrow" +import simpleGit from "simple-git" + +import { config } from "@config/config" + +import GithubSessionData from "@classes/GithubSessionData" + +import { lock, unlock } from "@utils/mutex-utils" +import { getCommitAndTreeSha, revertCommit } from "@utils/utils.js" + +import { + MAX_CONCURRENT_GIT_PROCESSES, + STAGING_BRANCH, + STAGING_LITE_BRANCH, +} from "@constants/constants" + +import { FEATURE_FLAGS } from "@root/constants/featureFlags" +import LockedError from "@root/errors/LockedError" +import logger from "@root/logger/logger" +import { FeatureFlags } from "@root/types/featureFlags" +import convertNeverThrowToPromise from "@root/utils/neverthrow" +import GitFileSystemService from "@services/db/GitFileSystemService" + +const BRANCH_REF = config.get("github.branchRef") + +const backoffOptions: BackoffOptions = { + numOfAttempts: 5, +} +const simpleGitInstance = simpleGit({ + maxConcurrentProcesses: MAX_CONCURRENT_GIT_PROCESSES, +}) +const gitFileSystemService = new GitFileSystemService(simpleGitInstance) + +const handleGitFileLock = async ( + repoName: string, + next: (arg0: any) => void +) => { + const result = await gitFileSystemService.hasGitFileLock(repoName, true) + if (result.isErr()) { + next(result.error) + return false + } + const isGitLocked = result.value + if (isGitLocked) { + logger.error(`Failed to lock repo ${repoName}: git file system in use.`) + next( + new LockedError( + `Someone else is currently modifying repo ${repoName}. Please try again later.` + ) + ) + return false + } + return true +} + +const isGitFileAndIsGitAvail = async ( + growthbook: GrowthBook, + siteName: string, + next: any +) => { + let isGitAvailable = true + + // only check git file lock if the repo is ggs enabled + if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { + isGitAvailable = await handleGitFileLock(siteName, next) + } + return isGitAvailable +} + +// Used when there are no write API calls to the repo on GitHub +export const attachReadRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + routeHandler(req, res).catch((err: any) => { + next(err) + }) +} + +// Used when there are write API calls to the repo on GitHub +export const attachWriteRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + const { siteName } = req.params + const { growthbook } = req + + let isGitAvailable = true + + // only check git file lock if the repo is ggs enabled + if (growthbook?.getFeatureValue(FEATURE_FLAGS.IS_GGS_ENABLED, false)) { + isGitAvailable = await handleGitFileLock(siteName, next) + } + + if (!isGitAvailable) return + + try { + await lock(siteName) + } catch (err) { + next(err) + return + } + + await routeHandler(req, res, next).catch(async (err: any) => { + await unlock(siteName) + next(err) + }) + + try { + await unlock(siteName) + } catch (err) { + next(err) + } +} + +export const attachRollbackRouteHandlerWrapper = (routeHandler: any) => async ( + req: any, + res: any, + next: any +) => { + const { userSessionData } = res.locals + const { siteName } = req.params + + const { accessToken } = userSessionData + const { growthbook } = req + + const isGitAvailable = await isGitFileAndIsGitAvail( + growthbook, + siteName, + next + ) + + if (!isGitAvailable) return + try { + await lock(siteName) + } catch (err) { + next(err) + return + } + + let originalStagingCommitSha: string + let originalStagingLiteCommitSha: string + + const shouldUseGitFileSystem = growthbook?.getFeatureValue( + FEATURE_FLAGS.IS_GGS_ENABLED, + false + ) + if (shouldUseGitFileSystem) { + const results = await ResultAsync.combine([ + gitFileSystemService.getLatestCommitOfBranch(siteName, STAGING_BRANCH), + gitFileSystemService.getLatestCommitOfBranch( + siteName, + STAGING_LITE_BRANCH + ), + ]) + + if (results.isErr()) { + await unlock(siteName) + next(results.error) + return + } + const [stagingResult, stagingLiteResult] = results.value + if (!stagingResult.sha || !stagingLiteResult.sha) { + await unlock(siteName) + return + } + + originalStagingCommitSha = stagingResult.sha + originalStagingLiteCommitSha = stagingLiteResult.sha + // Unused for git file system, but to maintain existing structure + res.locals.githubSessionData = new GithubSessionData({ + currentCommitSha: "", + treeSha: "", + }) + } else { + try { + const { + currentCommitSha: currentStgCommitSha, + treeSha: stgTreeSha, + } = await getCommitAndTreeSha(siteName, accessToken, STAGING_BRANCH) + + const { + currentCommitSha: currentStgLiteCommitSha, + } = await getCommitAndTreeSha(siteName, accessToken, STAGING_LITE_BRANCH) + + const githubSessionData = new GithubSessionData({ + currentCommitSha: currentStgCommitSha, + treeSha: stgTreeSha, + }) + res.locals.githubSessionData = githubSessionData + + originalStagingCommitSha = currentStgCommitSha + originalStagingLiteCommitSha = currentStgLiteCommitSha + } catch (err) { + await unlock(siteName) + logger.error(`Failed to rollback repo ${siteName}: ${err}`) + next(err) + return + } + } + + await routeHandler(req, res, next).catch(async (err: any) => { + try { + if (shouldUseGitFileSystem) { + await backOff( + () => + convertNeverThrowToPromise( + gitFileSystemService.rollback( + siteName, + originalStagingCommitSha, + STAGING_BRANCH + ) + ), + backoffOptions + ) + + await backOff( + () => + convertNeverThrowToPromise( + gitFileSystemService.rollback( + siteName, + originalStagingLiteCommitSha, + STAGING_LITE_BRANCH + ) + ), + backoffOptions + ) + + await backOff(() => { + let pushRes = gitFileSystemService.push( + siteName, + STAGING_BRANCH, + true + ) + if (originalStagingLiteCommitSha) { + pushRes = pushRes.andThen(() => + gitFileSystemService.push(siteName, STAGING_LITE_BRANCH, true) + ) + } + + return convertNeverThrowToPromise(pushRes) + }, backoffOptions) + } else { + await backOff( + () => + revertCommit( + originalStagingCommitSha, + siteName, + accessToken, + STAGING_BRANCH + ), + backoffOptions + ) + await backOff( + () => + revertCommit( + originalStagingLiteCommitSha, + siteName, + accessToken, + STAGING_LITE_BRANCH + ), + backoffOptions + ) + } + } catch (retryErr) { + await unlock(siteName) + logger.error(`Failed to rollback repo ${siteName}: ${retryErr}`) + next(retryErr) + return + } + await unlock(siteName) + next(err) + }) + + try { + await unlock(siteName) + } catch (err) { + next(err) + } +} diff --git a/src/routes/v2/authenticatedSites/contactUs.js b/src/routes/v2/authenticatedSites/contactUs.js index b146aa1f9..2da7c57dc 100644 --- a/src/routes/v2/authenticatedSites/contactUs.js +++ b/src/routes/v2/authenticatedSites/contactUs.js @@ -6,7 +6,7 @@ const { BadRequestError } = require("@errors/BadRequestError") const { attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, + attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") const { UpdateContactUsSchema } = require("@validators/RequestSchema") @@ -53,7 +53,7 @@ class ContactUsRouter { const router = express.Router({ mergeParams: true }) router.get("/", attachReadRouteHandlerWrapper(this.readContactUs)) - router.post("/", attachWriteRouteHandlerWrapper(this.updateContactUs)) + router.post("/", attachRollbackRouteHandlerWrapper(this.updateContactUs)) return router } diff --git a/src/routes/v2/authenticatedSites/navigation.js b/src/routes/v2/authenticatedSites/navigation.js index 291942a04..0092fa1b8 100644 --- a/src/routes/v2/authenticatedSites/navigation.js +++ b/src/routes/v2/authenticatedSites/navigation.js @@ -6,7 +6,7 @@ const { BadRequestError } = require("@errors/BadRequestError") const { attachReadRouteHandlerWrapper, - attachWriteRouteHandlerWrapper, + attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") const { UpdateNavigationRequestSchema } = require("@validators/RequestSchema") @@ -52,7 +52,7 @@ class NavigationRouter { const router = express.Router({ mergeParams: true }) router.get("/", attachReadRouteHandlerWrapper(this.readNavigation)) - router.post("/", attachWriteRouteHandlerWrapper(this.updateNavigation)) + router.post("/", attachRollbackRouteHandlerWrapper(this.updateNavigation)) return router } diff --git a/src/routes/v2/authenticatedSites/settings.js b/src/routes/v2/authenticatedSites/settings.js index 3e9f2a1f1..4005a3977 100644 --- a/src/routes/v2/authenticatedSites/settings.js +++ b/src/routes/v2/authenticatedSites/settings.js @@ -6,8 +6,8 @@ const { BadRequestError } = require("@errors/BadRequestError") // Import middleware const { - attachReadRouteHandlerWrapper, attachWriteRouteHandlerWrapper, + attachReadRouteHandlerWrapper, attachRollbackRouteHandlerWrapper, } = require("@middleware/routeHandler") diff --git a/src/services/db/GitFileSystemService.ts b/src/services/db/GitFileSystemService.ts index 5f4cbe402..348dd1ad2 100644 --- a/src/services/db/GitFileSystemService.ts +++ b/src/services/db/GitFileSystemService.ts @@ -542,10 +542,33 @@ export default class GitFileSystemService { isForce ? this.git .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - .push(["--force"]) + .push([...gitOptions, "--force"]) + : this.git + .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) + .push(gitOptions), + (error) => { + logger.error(`Error when pushing ${repoName}: ${error}`) + + if (error instanceof GitError) { + return new GitFileSystemError( + "Unable to push latest changes of repo" + ) + } + + return new GitFileSystemError("An unknown error occurred") + } + ) + ) + .orElse(() => + // Retry push twice + ResultAsync.fromPromise( + isForce + ? this.git + .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) + .push([...gitOptions, "--force"]) : this.git .cwd({ path: `${efsVolPath}/${repoName}`, root: false }) - .push(), + .push(gitOptions), (error) => { logger.error(`Error when pushing ${repoName}: ${error}`) diff --git a/src/services/db/__tests__/GitFileSystemService.spec.ts b/src/services/db/__tests__/GitFileSystemService.spec.ts index c38cbabe1..f7fa089bf 100644 --- a/src/services/db/__tests__/GitFileSystemService.spec.ts +++ b/src/services/db/__tests__/GitFileSystemService.spec.ts @@ -901,6 +901,9 @@ describe("GitFileSystemService", () => { MockSimpleGit.cwd.mockReturnValueOnce({ push: jest.fn().mockRejectedValueOnce(new GitError()), }) + MockSimpleGit.cwd.mockReturnValueOnce({ + push: jest.fn().mockRejectedValueOnce(new GitError()), + }) const result = await GitFileSystemService.push("fake-repo", BRANCH_REF) diff --git a/src/utils/neverthrow.ts b/src/utils/neverthrow.ts new file mode 100644 index 000000000..5f29b15c5 --- /dev/null +++ b/src/utils/neverthrow.ts @@ -0,0 +1,15 @@ +import { ResultAsync } from "neverthrow" + +/** + * This function is only used when integrating with third party libraries that + * expect a .catch() method on the returned promise. This should not be used in most + * control flows as it removes the benefits that neverthrow provides. + */ +const convertNeverThrowToPromise = async ( + x: ResultAsync +): Promise => { + const res = await x + return res._unsafeUnwrap() +} + +export default convertNeverThrowToPromise From 0639fedba576b3e069172c7ea72a73b14a5d0def Mon Sep 17 00:00:00 2001 From: seaerchin Date: Wed, 6 Dec 2023 19:05:34 +0800 Subject: [PATCH 10/10] 0.56.0 --- CHANGELOG.md | 17 ++++++++++++++++- package-lock.json | 4 ++-- package.json | 2 +- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c43a7c116..43d9673b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,8 +4,23 @@ All notable changes to this project will be documented in this file. Dates are d Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). +#### [v0.56.0](https://github.com/isomerpages/isomercms-backend/compare/v0.55.0...v0.56.0) + +- fix(rollback handler): convert to ts for safety [`#1044`](https://github.com/isomerpages/isomercms-backend/pull/1044) +- test(gitCommitService): add test cases [`#1047`](https://github.com/isomerpages/isomercms-backend/pull/1047) +- feat(gitFileSystem): safer api [`#1046`](https://github.com/isomerpages/isomercms-backend/pull/1046) +- test(githubService): add tests [`#1045`](https://github.com/isomerpages/isomercms-backend/pull/1045) +- chore(quickie): delete quickie for gh [`#1043`](https://github.com/isomerpages/isomercms-backend/pull/1043) +- feat(quickie): only gitfile should have quickie [`#1042`](https://github.com/isomerpages/isomercms-backend/pull/1042) +- hotfix(repair-form): set remote url correctly [`#1048`](https://github.com/isomerpages/isomercms-backend/pull/1048) +- build(deps): bump sequelize-typescript from 2.1.5 to 2.1.6 [`#1041`](https://github.com/isomerpages/isomercms-backend/pull/1041) +- Added swagger doc for GIG DNS API [`#1017`](https://github.com/isomerpages/isomercms-backend/pull/1017) +- 0.55.0 [`#1039`](https://github.com/isomerpages/isomercms-backend/pull/1039) + #### [v0.55.0](https://github.com/isomerpages/isomercms-backend/compare/v0.54.0...v0.55.0) +> 16 November 2023 + - fix(siteCreate): add redirect rules [`#1036`](https://github.com/isomerpages/isomercms-backend/pull/1036) - chore: remove extra and unused submodules [`#1031`](https://github.com/isomerpages/isomercms-backend/pull/1031) - release/0.54.0 [`#1033`](https://github.com/isomerpages/isomercms-backend/pull/1033) @@ -21,6 +36,7 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). - fix(pagination): images fix pagination [`#1026`](https://github.com/isomerpages/isomercms-backend/pull/1026) - fix(media): change media sorting to addedTime descending [`#1019`](https://github.com/isomerpages/isomercms-backend/pull/1019) - 0.53.0 [`#1024`](https://github.com/isomerpages/isomercms-backend/pull/1024) +- fix(mediafileservice): disable sneky cloudmersive [`#1025`](https://github.com/isomerpages/isomercms-backend/pull/1025) - release(0.52.0): merge to prod [`#1013`](https://github.com/isomerpages/isomercms-backend/pull/1013) - Release/0.51.0 [`#1002`](https://github.com/isomerpages/isomercms-backend/pull/1002) - * refactor(formsg-site-clone): remove and add to site creation (#971) [`#992`](https://github.com/isomerpages/isomercms-backend/pull/992) @@ -94,7 +110,6 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog). > 9 November 2023 -- fix(mediafileservice): disable sneky cloudmersive [`#1025`](https://github.com/isomerpages/isomercms-backend/pull/1025) - fix(file ext): fix casing + better logging [`#1020`](https://github.com/isomerpages/isomercms-backend/pull/1020) - fix(rr): capture file extensions that are in uppercase [`#1016`](https://github.com/isomerpages/isomercms-backend/pull/1016) - release(0.52.0): merge to develop [`#1012`](https://github.com/isomerpages/isomercms-backend/pull/1012) diff --git a/package-lock.json b/package-lock.json index 06d9d2354..0491adcf8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "isomercms", - "version": "0.55.0", + "version": "0.56.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "isomercms", - "version": "0.55.0", + "version": "0.56.0", "dependencies": { "@aws-sdk/client-amplify": "^3.370.0", "@aws-sdk/client-cloudwatch-logs": "^3.370.0", diff --git a/package.json b/package.json index a960d2e63..11bb185e5 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "isomercms", - "version": "0.55.0", + "version": "0.56.0", "private": true, "scripts": { "build": "tsc -p tsconfig.build.json",