Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BC-3961-move-s3-client-to-infra #4369

Merged
merged 10 commits into from
Sep 1, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Test, TestingModule } from '@nestjs/testing';
import { ApiValidationError } from '@shared/common';
import { EntityId, Permission } from '@shared/domain';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import {
cleanupCollections,
courseFactory,
Expand All @@ -25,7 +26,6 @@ import {
} from '@src/modules/files-storage/controller/dto';
import { Request } from 'express';
import request from 'supertest';
import { S3ClientAdapter } from '../../client/s3-client.adapter';
import { FileRecordParentType } from '../../entity';
import { availableParentTypes } from './mocks';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Test, TestingModule } from '@nestjs/testing';
import { ApiValidationError } from '@shared/common';
import { EntityId, Permission } from '@shared/domain';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import {
cleanupCollections,
fileRecordFactory,
Expand All @@ -19,7 +20,6 @@ import { FilesStorageTestModule } from '@src/modules/files-storage';
import { FileRecordListResponse, FileRecordResponse } from '@src/modules/files-storage/controller/dto';
import { Request } from 'express';
import request from 'supertest';
import { S3ClientAdapter } from '../../client/s3-client.adapter';
import { FileRecordParentType, PreviewStatus } from '../../entity';
import { availableParentTypes } from './mocks';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import { Test, TestingModule } from '@nestjs/testing';
import { ApiValidationError } from '@shared/common';
import { EntityId, Permission } from '@shared/domain';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import { cleanupCollections, mapUserToCurrentUser, roleFactory, schoolFactory, userFactory } from '@shared/testing';
import { ICurrentUser } from '@src/modules/authentication';
import { JwtAuthGuard } from '@src/modules/authentication/guard/jwt-auth.guard';
import { FilesStorageTestModule } from '@src/modules/files-storage';
import { FileRecordResponse } from '@src/modules/files-storage/controller/dto';
import { Request } from 'express';
import request from 'supertest';
import { S3ClientAdapter } from '../../client/s3-client.adapter';
import { FileRecord } from '../../entity';
import { ErrorType } from '../../error';
import { TestHelper } from '../../helper/test-helper';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import { Test, TestingModule } from '@nestjs/testing';
import { ApiValidationError } from '@shared/common';
import { EntityId, Permission } from '@shared/domain';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import { cleanupCollections, mapUserToCurrentUser, roleFactory, schoolFactory, userFactory } from '@shared/testing';
import { ICurrentUser } from '@src/modules/authentication';
import { JwtAuthGuard } from '@src/modules/authentication/guard/jwt-auth.guard';
import { FilesStorageTestModule } from '@src/modules/files-storage';
import { FileRecordResponse } from '@src/modules/files-storage/controller/dto';
import { Request } from 'express';
import request from 'supertest';
import { S3ClientAdapter } from '../../client/s3-client.adapter';
import { FileRecord, ScanStatus } from '../../entity';
import { ErrorType } from '../../error';
import { TestHelper } from '../../helper/test-helper';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Test, TestingModule } from '@nestjs/testing';
import { ApiValidationError } from '@shared/common';
import { EntityId, Permission } from '@shared/domain';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import {
cleanupCollections,
fileRecordFactory,
Expand All @@ -19,7 +20,6 @@ import { FilesStorageTestModule } from '@src/modules/files-storage';
import { FileRecordListResponse, FileRecordResponse } from '@src/modules/files-storage/controller/dto';
import { Request } from 'express';
import request from 'supertest';
import { S3ClientAdapter } from '../../client/s3-client.adapter';
import { FileRecordParentType, PreviewStatus } from '../../entity';
import { availableParentTypes } from './mocks';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { PaginationParams } from '@shared/controller';
import { ICurrentUser } from '@src/modules/authentication';
import { Authenticate, CurrentUser } from '@src/modules/authentication/decorator/auth.decorator';
import { Request, Response } from 'express';
import { IGetFileResponse } from '../interface';
import { GetFileResponse } from '../interface';
import { FilesStorageMapper } from '../mapper';
import { FileRecordMapper } from '../mapper/file-record.mapper';
import { FilesStorageUC } from '../uc';
Expand Down Expand Up @@ -149,7 +149,7 @@ export class FilesStorageController {

private streamFileToClient(
req: Request,
fileResponse: IGetFileResponse,
fileResponse: GetFileResponse,
httpResponse: Response,
bytesRange?: string
): StreamableFile {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Configuration } from '@hpi-schul-cloud/commons';
import { S3Config } from '@shared/infra/s3-file-storage';
import { ICoreModuleConfig } from '@src/core';
import { S3Config } from './interface';

export interface IFileStorageConfig extends ICoreModuleConfig {
MAX_FILE_SIZE: number;
Expand Down
31 changes: 3 additions & 28 deletions apps/server/src/modules/files-storage/files-storage.module.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { S3Client } from '@aws-sdk/client-s3';
import { Configuration } from '@hpi-schul-cloud/commons';
import { Dictionary, IPrimaryKey } from '@mikro-orm/core';
import { MikroOrmModule, MikroOrmModuleSyncOptions } from '@mikro-orm/nestjs';
Expand All @@ -7,12 +6,11 @@ import { ConfigModule } from '@nestjs/config';
import { ALL_ENTITIES } from '@shared/domain';
import { AntivirusModule } from '@shared/infra/antivirus/antivirus.module';
import { RabbitMQWrapperModule } from '@shared/infra/rabbitmq/rabbitmq.module';
import { S3FileStorageModule } from '@shared/infra/s3-file-storage';
import { DB_PASSWORD, DB_URL, DB_USERNAME, createConfigModuleOptions } from '@src/config';
import { LoggerModule } from '@src/core/logger';
import { S3ClientAdapter } from './client/s3-client.adapter';
import { FileRecord, FileSecurityCheck } from './entity';
import { config, s3Config } from './files-storage.config';
import { S3Config } from './interface/config';
import { FileRecordRepo } from './repo';
import { FilesStorageService } from './service/files-storage.service';
import { PreviewService } from './service/preview.service';
Expand All @@ -26,32 +24,9 @@ const imports = [
exchange: Configuration.get('ANTIVIRUS_EXCHANGE') as string,
routingKey: Configuration.get('ANTIVIRUS_ROUTING_KEY') as string,
}),
S3FileStorageModule.register(s3Config),
];
const providers = [
FilesStorageService,
PreviewService,
{
provide: 'S3_Client',
useFactory: (configProvider: S3Config) =>
new S3Client({
region: configProvider.region,
credentials: {
accessKeyId: configProvider.accessKeyId,
secretAccessKey: configProvider.secretAccessKey,
},
endpoint: configProvider.endpoint,
forcePathStyle: true,
tls: true,
}),
inject: ['S3_Config'],
},
{
provide: 'S3_Config',
useValue: s3Config,
},
S3ClientAdapter,
FileRecordRepo,
];
const providers = [FilesStorageService, PreviewService, FileRecordRepo];

const defaultMikroOrmOptions: MikroOrmModuleSyncOptions = {
findOneOrFailHandler: (entityName: string, where: Dictionary | IPrimaryKey) =>
Expand Down
4 changes: 2 additions & 2 deletions apps/server/src/modules/files-storage/helper/path.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { EntityId } from '@shared/domain';
import { CopyFiles } from '@shared/infra/s3-file-storage';
import { FileRecord } from '../entity';
import { ErrorType } from '../error';
import { ICopyFiles } from '../interface';

export function createPath(schoolId: EntityId, fileRecordId: EntityId): string {
if (!schoolId || !fileRecordId) {
Expand Down Expand Up @@ -32,7 +32,7 @@ export function getPaths(fileRecords: FileRecord[]): string[] {
return paths;
}

export function createICopyFiles(sourceFile: FileRecord, targetFile: FileRecord): ICopyFiles {
export function createICopyFiles(sourceFile: FileRecord, targetFile: FileRecord): CopyFiles {
dyedwiper marked this conversation as resolved.
Show resolved Hide resolved
const iCopyFiles = {
sourcePath: createPath(sourceFile.getSchoolId(), sourceFile.id),
targetPath: createPath(targetFile.getSchoolId(), targetFile.id),
Expand Down
7 changes: 4 additions & 3 deletions apps/server/src/modules/files-storage/helper/test-helper.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { GetFile } from '@shared/infra/s3-file-storage';
import { Readable } from 'stream';
import { IGetFile, IGetFileResponse } from '../interface';
import { GetFileResponse } from '../interface';

export class TestHelper {
public static createFile = (contentRange?: string): IGetFile => {
public static createFile = (contentRange?: string): GetFile => {
const text = 'testText';
const readable = Readable.from(text);

Expand All @@ -17,7 +18,7 @@ export class TestHelper {
return fileResponse;
};

public static createFileResponse = (contentRange?: string): IGetFileResponse => {
public static createFileResponse = (contentRange?: string): GetFileResponse => {
const name = 'testName';
const file = this.createFile(contentRange);
const fileResponse = { ...file, name };
Expand Down
7 changes: 0 additions & 7 deletions apps/server/src/modules/files-storage/interface/config.ts

This file was deleted.

3 changes: 1 addition & 2 deletions apps/server/src/modules/files-storage/interface/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export * from './config';
export * from './interfaces';
export * from './preview-input-mime-types.enum';
export * from './preview-output-mime-types.enum';
export * from './preview-width.enum';
export * from './storage-client';
21 changes: 21 additions & 0 deletions apps/server/src/modules/files-storage/interface/interfaces.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { Readable } from 'stream';
import type { DownloadFileParams, PreviewParams } from '../controller/dto';
import { FileRecord } from '../entity';

export interface GetFileResponse {
data: Readable;
etag: string | undefined;
dyedwiper marked this conversation as resolved.
Show resolved Hide resolved
contentType: string | undefined;
contentLength: number | undefined;
contentRange: string | undefined;
name: string;
}

export interface PreviewFileParams {
fileRecord: FileRecord;
downloadParams: DownloadFileParams;
previewParams: PreviewParams;
hash: string;
filePath: string;
bytesRange?: string;
}
31 changes: 0 additions & 31 deletions apps/server/src/modules/files-storage/interface/storage-client.ts

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { IGetFile, IGetFileResponse } from '../interface';
import { GetFile } from '@shared/infra/s3-file-storage';
import { GetFileResponse } from '../interface';

export class FileResponseBuilder {
public static build(file: IGetFile, name: string): IGetFileResponse {
public static build(file: GetFile, name: string): GetFileResponse {
const fileResponse = { ...file, data: file.data, name };

return fileResponse;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
SingleFileParams,
} from '../controller/dto';
import { FileRecord, FileRecordParentType } from '../entity';
import { IGetFileResponse } from '../interface';
import { GetFileResponse } from '../interface';

export class FilesStorageMapper {
static mapToAllowedAuthorizationEntityType(type: FileRecordParentType): AuthorizableReferenceType {
Expand Down Expand Up @@ -62,7 +62,7 @@ export class FilesStorageMapper {
return response;
}

static mapToStreamableFile(fileResponse: IGetFileResponse): StreamableFile {
static mapToStreamableFile(fileResponse: GetFileResponse): StreamableFile {
const streamableFile = new StreamableFile(fileResponse.data, {
type: fileResponse.contentType,
disposition: `inline; filename="${encodeURI(fileResponse.name)}"`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { ObjectId } from '@mikro-orm/mongodb';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import { fileRecordFactory, setupEntities } from '@shared/testing';
import { LegacyLogger } from '@src/core/logger';
import { S3ClientAdapter } from '../client/s3-client.adapter';
import { FileRecordParams } from '../controller/dto';
import { FileRecord, FileRecordParentType, ScanStatus } from '../entity';
import { createICopyFiles } from '../helper';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import { InternalServerErrorException } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { S3ClientAdapter } from '@shared/infra/s3-file-storage';
import { fileRecordFactory, setupEntities } from '@shared/testing';
import { LegacyLogger } from '@src/core/logger';
import { S3ClientAdapter } from '../client/s3-client.adapter';
import { FileRecordParams } from '../controller/dto';
import { FileRecord, FileRecordParentType } from '../entity';
import { getPaths } from '../helper';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@ import { NotAcceptableException, NotFoundException } from '@nestjs/common';
import { ConfigService } from '@nestjs/config';
import { Test, TestingModule } from '@nestjs/testing';
import { AntivirusService } from '@shared/infra/antivirus/antivirus.service';
import { GetFile, S3ClientAdapter } from '@shared/infra/s3-file-storage';
import { fileRecordFactory, setupEntities } from '@shared/testing';
import { LegacyLogger } from '@src/core/logger';
import { S3ClientAdapter } from '../client/s3-client.adapter';
import { FileRecordParams } from '../controller/dto';
import { FileRecord, FileRecordParentType, ScanStatus } from '../entity';
import { ErrorType } from '../error';
import { createPath } from '../helper';
import { IGetFile, IGetFileResponse } from '../interface';
import { FileResponseBuilder } from '../mapper';
import { FileRecordRepo } from '../repo';
import { FilesStorageService } from './files-storage.service';
Expand Down Expand Up @@ -101,10 +100,10 @@ describe('FilesStorageService download methods', () => {
fileName: fileRecord.name,
};

const fileResponse = createMock<IGetFile>();
const fileResponse = createMock<GetFile>();
const expectedResponse = FileResponseBuilder.build(fileResponse, fileRecord.getName());

spy = jest.spyOn(service, 'downloadFile').mockResolvedValueOnce(fileResponse);
spy = jest.spyOn(service, 'downloadFile').mockResolvedValueOnce(expectedResponse);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist das hier richtig so? fileResponse wird nicht mehr benutzt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mapping passiert jetzt dort


return { fileRecord, params, expectedResponse };
};
Expand All @@ -114,7 +113,7 @@ describe('FilesStorageService download methods', () => {

await service.download(fileRecord, params);

expect(service.downloadFile).toHaveBeenCalledWith(fileRecord.schoolId, fileRecord.id, undefined);
expect(service.downloadFile).toHaveBeenCalledWith(fileRecord, undefined);
});

it('returns correct response', async () => {
Expand Down Expand Up @@ -204,9 +203,10 @@ describe('FilesStorageService download methods', () => {
const { fileRecords } = buildFileRecordsWithParams();
const fileRecord = fileRecords[0];

const expectedResponse = createMock<IGetFileResponse>();
const fileResponse = createMock<GetFile>();

storageClient.get.mockResolvedValueOnce(expectedResponse);
storageClient.get.mockResolvedValueOnce(fileResponse);
const expectedResponse = FileResponseBuilder.build(fileResponse, fileRecord.getName());

return { fileRecord, expectedResponse };
};
Expand All @@ -216,15 +216,15 @@ describe('FilesStorageService download methods', () => {

const path = createPath(fileRecord.schoolId, fileRecord.id);

await service.downloadFile(fileRecord.schoolId, fileRecord.id);
await service.downloadFile(fileRecord);

expect(storageClient.get).toHaveBeenCalledWith(path, undefined);
});

it('returns correct response', async () => {
const { fileRecord, expectedResponse } = setup();

const response = await service.downloadFile(fileRecord.schoolId, fileRecord.id);
const response = await service.downloadFile(fileRecord);

expect(response).toEqual(expectedResponse);
});
Expand All @@ -244,7 +244,7 @@ describe('FilesStorageService download methods', () => {
it('passes error', async () => {
const { fileRecord, error } = setup();

await expect(service.downloadFile(fileRecord.schoolId, fileRecord.id)).rejects.toThrowError(error);
await expect(service.downloadFile(fileRecord)).rejects.toThrowError(error);
});
});
});
Expand Down
Loading