From fd03040d737e158223df68003ae38142867a5e5c Mon Sep 17 00:00:00 2001 From: Anton Shepilov Date: Mon, 1 May 2023 19:45:28 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20#23=20Fix=20throw=20error=20when?= =?UTF-8?q?=20file=20doesn't=20exist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * #23 Fix throw error when file doesn't exist Run all the tests in once without special runner Add positive scenario Move coverage to the backend build workflow --- .github/workflows/backend.yml | 15 ++-- .github/workflows/coverage.yml | 32 -------- tdrive/backend/node/package.json | 1 + .../orm/connectors/mongodb/mongodb.ts | 3 +- .../storage/connectors/local/service.ts | 7 +- .../core/platform/services/storage/index.ts | 7 +- .../node/src/services/files/services/index.ts | 8 +- .../services/files/web/controllers/files.ts | 17 ++-- tdrive/backend/node/src/utils/users.ts | 3 + .../backend/node/test/e2e/files/files.spec.ts | 80 +++++++++++++++---- tdrive/backend/node/test/e2e/setup/index.ts | 10 ++- .../node/test/e2e/users/users.search.spec.ts | 2 +- .../backend/node/test/e2e/utils.prepare.db.ts | 12 +-- .../e2e/workspaces/workspace-users.spec.ts | 4 - .../test/e2e/workspaces/workspaces.spec.ts | 2 +- 15 files changed, 121 insertions(+), 82 deletions(-) delete mode 100644 .github/workflows/coverage.yml diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml index 75bd5a22..8d57b60f 100644 --- a/.github/workflows/backend.yml +++ b/.github/workflows/backend.yml @@ -19,12 +19,15 @@ jobs: runs-on: ubuntu-20.04 steps: - uses: actions/checkout@v2 - - name: build-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn node npm run build - - name: unit-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn node npm run test:unit - name: e2e-mongo-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=mongodb -e DB_DRIVER=mongodb -e PUBSUB_TYPE=local node npm run test:e2e + run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=mongodb -e DB_DRIVER=mongodb -e PUBSUB_TYPE=local node npm run test:all - name: e2e-cassandra-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml up -d scylladb elasticsearch rabbitmq && sleep 60 && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=elasticsearch -e DB_DRIVER=cassandra node npm run test:e2e + run: cd tdrive && docker-compose -f docker-compose.tests.yml up -d scylladb elasticsearch rabbitmq && sleep 60 && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=elasticsearch -e DB_DRIVER=cassandra node npm run test:all + - name: coverage + uses: adRise/jest-cov-reporter@main + with: + branch-coverage-report-path: ./tdrive/coverage/coverage-summary.json + base-coverage-report-path: ./tdrive/coverage/coverage-summary.json + delta: 0.3 + fullCoverageDiff: true diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml deleted file mode 100644 index 170644e1..00000000 --- a/.github/workflows/coverage.yml +++ /dev/null @@ -1,32 +0,0 @@ -name: backend-coverage - -on: - pull_request_target: - types: [assigned, opened, synchronize, reopened] - branches: [main] - paths: - - "tdrive/backend/node/**" - -jobs: - test: - runs-on: ubuntu-20.04 - steps: - - uses: actions/checkout@v2 - with: - ref: "refs/pull/${{ github.event.number }}/merge" - - name: unit-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn node npm run test:unit - - name: e2e-mongo-test - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn -e SEARCH_DRIVER=mongodb -e DB_DRIVER=mongodb -e PUBSUB_TYPE=local node npm run test:e2e - - name: generate coverage summary json - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn node npm run test:merge:json - - name: generate coverage summary text - run: cd tdrive && docker-compose -f docker-compose.tests.yml run -e NODE_OPTIONS=--unhandled-rejections=warn node npm run test:merge:text - - name: Coverage - uses: adRise/jest-cov-reporter@main - with: - branch-coverage-report-path: ./tdrive/coverage/merged/coverage-summary.json - base-coverage-report-path: ./tdrive/coverage/merged/coverage-summary.json - delta: 0.3 - fullCoverageDiff: true - diff --git a/tdrive/backend/node/package.json b/tdrive/backend/node/package.json index 2a448719..f7c73f79 100644 --- a/tdrive/backend/node/package.json +++ b/tdrive/backend/node/package.json @@ -28,6 +28,7 @@ "test:unit:watch": "npm run test:unit -- --watchAll --verbose false | pino-pretty", "test:merge:json": "npx istanbul report --dir coverage/merged --include 'coverage/**/coverage-final.json' json-summary", "test:merge:text": "npx istanbul report --dir coverage/merged --include 'coverage/**/coverage-final.json' text > coverage/merged/coverage-report.txt", + "test:all": "jest test --forceExit --coverage --detectOpenHandles --testTimeout=30000 --verbose false --runInBand", "kill": "kill $(lsof -t -i:3000) | exit 0" }, "jest": { diff --git a/tdrive/backend/node/src/core/platform/services/database/services/orm/connectors/mongodb/mongodb.ts b/tdrive/backend/node/src/core/platform/services/database/services/orm/connectors/mongodb/mongodb.ts index cb0a9333..26aced6d 100644 --- a/tdrive/backend/node/src/core/platform/services/database/services/orm/connectors/mongodb/mongodb.ts +++ b/tdrive/backend/node/src/core/platform/services/database/services/orm/connectors/mongodb/mongodb.ts @@ -44,14 +44,13 @@ export class MongoConnector extends AbstractConnector { await this.connect(); - return this.client.db(this.options.database); } async drop(): Promise { const db = await this.getDatabase(); - db.dropDatabase(); + await db.dropDatabase(); return this; } diff --git a/tdrive/backend/node/src/core/platform/services/storage/connectors/local/service.ts b/tdrive/backend/node/src/core/platform/services/storage/connectors/local/service.ts index d7150234..e68f0759 100644 --- a/tdrive/backend/node/src/core/platform/services/storage/connectors/local/service.ts +++ b/tdrive/backend/node/src/core/platform/services/storage/connectors/local/service.ts @@ -3,6 +3,7 @@ import { createWriteStream, createReadStream, existsSync, mkdirSync, statSync, r import p from "path"; import { rm } from "fs/promises"; // Do not change the import, this is not the same function import { rm } from "fs" import { StorageConnectorAPI, WriteMetadata } from "../../provider"; +import fs from "fs"; export type LocalConfiguration = { path: string; @@ -42,7 +43,11 @@ export default class LocalConnectorService implements StorageConnectorAPI { } async read(path: string): Promise { - return createReadStream(this.getFullPath(path)); + const fullPath = this.getFullPath(path); + if (!fs.existsSync(fullPath)) { + throw new Error("File doesn't not exists"); + } + return createReadStream(fullPath); } async remove(path: string): Promise { diff --git a/tdrive/backend/node/src/core/platform/services/storage/index.ts b/tdrive/backend/node/src/core/platform/services/storage/index.ts index 9fe35a2a..7c2c3ed5 100644 --- a/tdrive/backend/node/src/core/platform/services/storage/index.ts +++ b/tdrive/backend/node/src/core/platform/services/storage/index.ts @@ -73,7 +73,7 @@ export default class StorageService extends TdriveService implements return await this.getConnector().write(path, stream); } catch (err) { logger.error(err); - return null; + throw err; } } @@ -107,8 +107,7 @@ export default class StorageService extends TdriveService implements } } catch (err) { logger.error(err); - callback(); - return; + callback(err, null); } callback(null, stream); return; @@ -117,7 +116,7 @@ export default class StorageService extends TdriveService implements return new Multistream(factory); } catch (err) { logger.error(err); - return null; + throw err; } } diff --git a/tdrive/backend/node/src/services/files/services/index.ts b/tdrive/backend/node/src/services/files/services/index.ts index e8ff1ac8..cb422ee6 100644 --- a/tdrive/backend/node/src/services/files/services/index.ts +++ b/tdrive/backend/node/src/services/files/services/index.ts @@ -67,7 +67,7 @@ export class FileServiceImpl { entity.application_id = applicationId; entity.upload_data = null; - this.repository.save(entity, context); + await this.repository.save(entity, context); } if (file) { @@ -89,7 +89,7 @@ export class FileServiceImpl { size: options.totalSize, chunks: options.totalChunks || 1, }; - this.repository.save(entity, context); + await this.repository.save(entity, context); } } @@ -109,7 +109,9 @@ export class FileServiceImpl { } } - return entity; + return await this.getFile({ id: entity.id, company_id: entity.company_id }, context, { + waitForThumbnail: options.waitForThumbnail, + }); } async exists(id: string, companyId: string, context?: CompanyExecutionContext): Promise { diff --git a/tdrive/backend/node/src/services/files/web/controllers/files.ts b/tdrive/backend/node/src/services/files/web/controllers/files.ts index 35a20242..8098dff6 100644 --- a/tdrive/backend/node/src/services/files/web/controllers/files.ts +++ b/tdrive/backend/node/src/services/files/web/controllers/files.ts @@ -44,13 +44,18 @@ export class FileController { ): Promise { const context = getCompanyExecutionContext(request); const params = request.params; - const data = await gr.services.files.download(params.id, context); - const filename = data.name.replace(/[^a-zA-Z0-9 -_.]/g, ""); + try { + const data = await gr.services.files.download(params.id, context); + const filename = data.name.replace(/[^a-zA-Z0-9 -_.]/g, ""); - response.header("Content-disposition", `attachment; filename="${filename}"`); - if (data.size) response.header("Content-Length", data.size); - response.type(data.mime); - response.send(data.file); + response.header("Content-disposition", `attachment; filename="${filename}"`); + if (data.size) response.header("Content-Length", data.size); + response.type(data.mime); + response.send(data.file); + } catch (e) { + console.log("!!!" + e); + throw e; + } } async thumbnail( diff --git a/tdrive/backend/node/src/utils/users.ts b/tdrive/backend/node/src/utils/users.ts index 2d360350..50d65a1f 100644 --- a/tdrive/backend/node/src/utils/users.ts +++ b/tdrive/backend/node/src/utils/users.ts @@ -50,6 +50,9 @@ export async function formatUser( const companies = await Promise.all( userCompanies.map(async uc => { const company = await gr.services.companies.getCompany({ id: uc.group_id }); + if (!company) { + throw new Error(`Company with id ${uc.group_id} doesn't exists!`); + } return { role: uc.role as CompanyUserRole, status: "active" as CompanyUserStatus, // FIXME: with real status diff --git a/tdrive/backend/node/test/e2e/files/files.spec.ts b/tdrive/backend/node/test/e2e/files/files.spec.ts index 32f8da32..9f00e252 100644 --- a/tdrive/backend/node/test/e2e/files/files.spec.ts +++ b/tdrive/backend/node/test/e2e/files/files.spec.ts @@ -8,15 +8,18 @@ import fs from "fs"; import { File } from "../../../src/services/files/entities/file"; import { deserialize } from "class-transformer"; import formAutoContent from "form-auto-content"; +import LocalConnectorService from "../../../src/core/platform/services/storage/connectors/local/service"; -describe.skip("The Files feature", () => { + +describe("The Files feature", () => { const url = "/internal/services/files/v1"; let platform: TestPlatform; beforeAll(async () => { platform = await init({ - services: ["webserver", "database", "storage", "message-queue", "files", "previews"], + services: ["webserver", "database", "storage", "files", "previews"], }); + await platform.database.getConnector().init(); }); afterAll(async done => { @@ -25,6 +28,22 @@ describe.skip("The Files feature", () => { done(); }); + async function uploadFile(file: string) { + const form = formAutoContent({file: fs.createReadStream(file)}); + form.headers["authorization"] = `Bearer ${await platform.auth.getJWTToken()}`; + + const filesUploadRaw = await platform.app.inject({ + method: "POST", + url: `${url}/companies/${platform.workspace.company_id}/files?thumbnail_sync=1`, + ...form, + }); + const filesUpload: ResourceUpdateResponse = deserialize( + ResourceUpdateResponse, + filesUploadRaw.body, + ); + return filesUpload; + } + describe("On user send files", () => { const files = [ "assets/sample.png", @@ -36,22 +55,49 @@ describe.skip("The Files feature", () => { ].map(p => `${__dirname}/${p}`); const thumbnails = [1, 1, 2, 5, 0, 1]; - it("should save file and generate previews", async done => { + it("Download file should return 500 if file doesn't exists", async () => { + //given file + const filesUpload = await uploadFile(files[0]); + expect(filesUpload.resource.id).toBeTruthy(); + //clean files directory + expect(platform.storage.getConnector()).toBeInstanceOf(LocalConnectorService) + const path = (platform.storage.getConnector()).configuration.path; + fs.readdirSync(path).forEach(f => fs.rmSync(`${path}/${f}`, {recursive: true, force: true})); + //when try to download the file + const fileDownloadResponse = await platform.app.inject({ + method: "GET", + url: `${url}/companies/${platform.workspace.company_id}/files/${filesUpload.resource.id}/download`, + }); + //then file should be not found with 404 error and "File not found message" + expect(fileDownloadResponse).toBeTruthy(); + expect(fileDownloadResponse.statusCode).toBe(500); + + }, 120000); + + it("Download file should return 200 if file exists", async () => { + //given file + const filesUpload = await uploadFile(files[0]); + expect(filesUpload.resource.id).toBeTruthy(); + //clean files directory + expect(platform.storage.getConnector()).toBeInstanceOf(LocalConnectorService) + + //when try to download the file + const fileDownloadResponse = await platform.app.inject({ + method: "GET", + url: `${url}/companies/${platform.workspace.company_id}/files/${filesUpload.resource.id}/download`, + }); + //then file should be not found with 404 error and "File not found message" + expect(fileDownloadResponse).toBeTruthy(); + expect(fileDownloadResponse.statusCode).toBe(200); + + }, 120000); + + + it.skip("should save file and generate previews", async done => { for (const i in files) { const file = files[i]; - const form = formAutoContent({ file: fs.createReadStream(file) }); - form.headers["authorization"] = `Bearer ${await platform.auth.getJWTToken()}`; - - const filesUploadRaw = await platform.app.inject({ - method: "POST", - url: `${url}/companies/${platform.workspace.company_id}/files?thumbnail_sync=1`, - ...form, - }); - const filesUpload: ResourceUpdateResponse = deserialize( - ResourceUpdateResponse, - filesUploadRaw.body, - ); + const filesUpload = await uploadFile(file); expect(filesUpload.resource.id).not.toBeFalsy(); expect(filesUpload.resource.encryption_key).toBeFalsy(); //This must not be disclosed @@ -59,6 +105,7 @@ describe.skip("The Files feature", () => { for (const thumb of filesUpload.resource.thumbnails) { const thumbnails = await platform.app.inject({ + headers: {"authorization": `Bearer ${await platform.auth.getJWTToken()}`}, method: "GET", url: `${url}/companies/${platform.workspace.company_id}/files/${filesUpload.resource.id}/thumbnails/${thumb.index}`, }); @@ -67,6 +114,7 @@ describe.skip("The Files feature", () => { } done(); - }, 120000); + }, 1200000); }); }); + diff --git a/tdrive/backend/node/test/e2e/setup/index.ts b/tdrive/backend/node/test/e2e/setup/index.ts index 10d17e61..6e5e94da 100644 --- a/tdrive/backend/node/test/e2e/setup/index.ts +++ b/tdrive/backend/node/test/e2e/setup/index.ts @@ -11,6 +11,8 @@ import { MessageQueueServiceAPI } from "../../../src/core/platform/services/mess // @ts-ignore import config from "config"; import globalResolver from "../../../src/services/global-resolver"; +import {FileServiceImpl} from "../../../src/services/files/services"; +import StorageAPI from "../../../src/core/platform/services/storage/provider"; type TokenPayload = { sub: string; @@ -32,8 +34,10 @@ export interface TestPlatform { workspace: Workspace; app: FastifyInstance; database: DatabaseServiceAPI; + storage: StorageAPI; messageQueue: MessageQueueServiceAPI; authService: AuthServiceAPI; + filesService: FileServiceImpl; auth: { getJWTToken(payload?: TokenPayload): Promise; }; @@ -68,17 +72,21 @@ export async function init( await platform.start(); const database = platform.getProvider("database"); + await database.getConnector().drop(); const messageQueue = platform.getProvider("message-queue"); const auth = platform.getProvider("auth"); + const storage: StorageAPI = platform.getProvider("storage"); testPlatform = { platform, app, messageQueue, database, + storage, workspace: { company_id: "", workspace_id: "" }, currentUser: { id: "" }, authService: auth, + filesService: globalResolver.services.files, auth: { getJWTToken, }, @@ -104,7 +112,7 @@ export async function init( payload.sub = testPlatform.currentUser.id; } - if (testPlatform.currentUser.isWorkspaceModerator) { + if (testPlatform .currentUser.isWorkspaceModerator) { payload.org = {}; payload.org[testPlatform.workspace.company_id] = { role: "", diff --git a/tdrive/backend/node/test/e2e/users/users.search.spec.ts b/tdrive/backend/node/test/e2e/users/users.search.spec.ts index e096e02a..bc3813f1 100644 --- a/tdrive/backend/node/test/e2e/users/users.search.spec.ts +++ b/tdrive/backend/node/test/e2e/users/users.search.spec.ts @@ -123,7 +123,7 @@ describe("The /users API", () => { expect(resources.length).toBe(0); done(); - }); + }, 1200000); }); async function search(search: string, companyId?: string): Promise { diff --git a/tdrive/backend/node/test/e2e/utils.prepare.db.ts b/tdrive/backend/node/test/e2e/utils.prepare.db.ts index f91d6bdf..b3f6ffb3 100644 --- a/tdrive/backend/node/test/e2e/utils.prepare.db.ts +++ b/tdrive/backend/node/test/e2e/utils.prepare.db.ts @@ -156,11 +156,13 @@ export class TestDbService { } this.users.push(createdUser); - await gr.services.companies.setUserRole( - this.company ? this.company.id : workspacesPk[0].company_id, - createdUser.id, - options.companyRole ? options.companyRole : "member", - ); + if (workspacesPk && workspacesPk.length) { + await gr.services.companies.setUserRole( + this.company ? this.company.id : workspacesPk[0].company_id, + createdUser.id, + options.companyRole ? options.companyRole : "member", + ); + } if (workspacesPk && workspacesPk.length) { for (const workspacePk of workspacesPk) { diff --git a/tdrive/backend/node/test/e2e/workspaces/workspace-users.spec.ts b/tdrive/backend/node/test/e2e/workspaces/workspace-users.spec.ts index c70f2e83..274a4dbf 100644 --- a/tdrive/backend/node/test/e2e/workspaces/workspace-users.spec.ts +++ b/tdrive/backend/node/test/e2e/workspaces/workspace-users.spec.ts @@ -264,7 +264,6 @@ describe("The /workspace users API", () => { const anotherUserId = testDbService.workspaces[0].users[0].id; let workspaceUsersCount = await testDbService.getWorkspaceUsersCountFromDb(workspaceId); - let companyUsersCount = await testDbService.getCompanyUsersCountFromDb(companyId); console.log(testDbService.workspaces[2].users); console.log(workspaceUsersCount); @@ -290,10 +289,7 @@ describe("The /workspace users API", () => { checkUserObject(resource); workspaceUsersCount = await testDbService.getWorkspaceUsersCountFromDb(workspaceId); - companyUsersCount = await testDbService.getCompanyUsersCountFromDb(companyId); expect(workspaceUsersCount).toBe(5); - // expect(companyUsersCount).toBe(6); - done(); }); }); diff --git a/tdrive/backend/node/test/e2e/workspaces/workspaces.spec.ts b/tdrive/backend/node/test/e2e/workspaces/workspaces.spec.ts index d251e304..429d2f21 100644 --- a/tdrive/backend/node/test/e2e/workspaces/workspaces.spec.ts +++ b/tdrive/backend/node/test/e2e/workspaces/workspaces.spec.ts @@ -35,7 +35,7 @@ describe("The /workspaces API", () => { await platform.database.getConnector().init(); testDbService = new TestDbService(platform); - await testDbService.createCompany(companyId); + await testDbService.createCompany(companyId, "Company name"); const ws0pk = { id: uuidv1(), company_id: companyId }; const ws1pk = { id: uuidv1(), company_id: companyId }; const ws2pk = { id: uuidv1(), company_id: companyId };