Bc 5170 no preview for larger files #4457

SevenWaysDP · 2023-10-05T18:45:54Z

Description

Links to Tickets or other pull requests

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
QA: In addition to review, the code has been manually tested (if manual testing is possible)
All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

apps/server/src/modules/files-storage/index.ts

SevenWaysDP

api-configmap
api-secret

apps/server/src/shared/infra/rabbitmq/rpc-message-producer.ts

…r-larger-files

apps/server/src/modules/files-storage-client/service/files-storage.producer.ts

ansible/roles/schulcloud-server-core/templates/amqp-preview-deployment.yml.j2

apps/server/src/modules/files-storage/controller/api-test/files-storage-copy-files.api.spec.ts

.../server/src/modules/files-storage/controller/api-test/files-storage-delete-files.api.spec.ts

...rver/src/modules/files-storage/controller/api-test/files-storage-download-upload.api.spec.ts

apps/server/src/modules/files-storage/controller/api-test/files-storage-list-files.api.spec.ts

apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts

apps/server/src/modules/files-storage/index.ts

apps/server/src/modules/files-storage/service/preview.service.ts

apps/server/src/shared/infra/preview-generator/preview-generator-consumer.module.ts

apps/server/src/shared/infra/preview-generator/preview-generator-producer.module.ts

apps/server/src/shared/infra/preview-generator/preview-generator.consumer.ts

apps/server/src/shared/infra/preview-generator/preview-generator.service.ts

CeEv · 2023-10-12T14:08:01Z

apps/server/src/shared/infra/preview-generator/preview.producer.ts

+		private readonly logger: LegacyLogger,
+		protected readonly configService: ConfigService<any, true>
+	) {
+		super(amqpConnection, FilesPreviewExchange, configService.get('INCOMING_REQUEST_TIMEOUT'));


Ist configService.get('INCOMING_REQUEST_TIMEOUT') inzwischen Typ sicher? Kannst du das vorher auf eine const schreiben?

apps/server/src/shared/infra/preview-generator/preview.producer.ts

CeEv · 2023-10-12T14:09:47Z

apps/server/src/shared/infra/rabbitmq/exchange/files-preview.ts

@@ -0,0 +1,7 @@
+import { Configuration } from '@hpi-schul-cloud/commons/lib';
+
+export const FilesPreviewExchange = Configuration.get('FILES_STORAGE__EXCHANGE') as string;


Können wir die direkte Verwendung von Configuration hier vermeiden?

Leider wollte Phillip, das wir exchanges konfigurierbar machen. Deswegen ist das so.

Todo: Follow up Ticket für Umstellung auf nest config.

https://ticketsystem.dbildungscloud.de/browse/BC-5661

CeEv · 2023-10-12T14:10:54Z

apps/server/src/shared/infra/rabbitmq/rabbitmq.module.ts

+			{
+				name: FilesPreviewExchange,
+				type: 'direct',
+			},
 		],
 		uri: Configuration.get('RABBITMQ_URI') as string,


Hier haben wir auch eine versteckte Zuweisung der Envirement.

Todo: Follow up Ticket für Umstellung auf nest config.

apps/server/src/shared/infra/s3-client/interface/index.ts

…les' into BC-5170-no-preview-for-larger-files

ansible/roles/schulcloud-server-core/tasks/main.yml

package.json

ansible/roles/schulcloud-server-core/templates/preview-generator-scaled-object.yml.j2

mamutmk5 · 2023-10-25T07:59:28Z

ansible/roles/schulcloud-server-core/templates/preview-generator-scaled-object.yml.j2

+  idleReplicaCount: 1
+  minReplicaCount:  1
+  maxReplicaCount:  5


Make this tree configurable

it so correct ?

Suggested change

idleReplicaCount: 1

minReplicaCount: 1

maxReplicaCount: 5

idleReplicaCount: {{ AMQP_FILE_PREVIEW_IDLE_REPLICA_COUNT|default("1", true) }}

minReplicaCount: {{ AMQP_FILE_PREVIEW_MIN_REPLICA_COUNT|default("1", true) }}

maxReplicaCount: {{ AMQP_FILE_PREVIEW_MAX_REPLICA_COUNT|default("5", true) }}

ansible/roles/schulcloud-server-core/templates/preview-generator-deployment.yml.j2

ansible/roles/schulcloud-server-core/templates/preview-generator-scaled-object.yml.j2

Co-authored-by: mamutmk5 <[email protected]>

…or-scaled-object.yml.j2 Co-authored-by: mamutmk5 <[email protected]>

…r-larger-files

ansible/roles/schulcloud-server-core/templates/preview-generator-deployment.yml.j2

…or-deployment.yml.j2 Co-authored-by: mamutmk5 <[email protected]>

CeEv · 2023-10-26T09:13:38Z

apps/server/src/modules/files-storage/helper/file-record.ts

@@ -38,3 +39,21 @@ export function createFileRecord(

 	return entity;
 }
+
+export function getFormat(mimeType: string): string {
+	const format = mimeType.split('/')[1];


validation fehlt ...was ist wenn [0] bzw. [1] undefined ist Das muss behandelt werden.

CeEv · 2023-10-26T09:53:25Z

apps/server/src/modules/files-storage/helper/file-record.ts

+		return fileRecord.name;
+	}
+
+	const fileNameWithoutExtension = fileRecord.name.split('.')[0];


Das gleiche validation von [0] !== undefined bzw. die Behandlung dessen.
Ich würde das gerne auch nach fileRecord verlagern. FileRecord.getFileNameWithoutExtension()

Das kann nicht geben

CeEv · 2023-10-26T11:35:45Z

apps/server/src/modules/files-storage/entity/filerecord.entity.ts

@@ -293,4 +293,10 @@ export class FileRecord extends BaseEntityWithTimestamps {

 		return PreviewStatus.PREVIEW_NOT_POSSIBLE_SCAN_STATUS_ERROR;
 	}
+
+	public get fileNameWithoutExtension(): string {
+		const fileNameWithoutExtension = this.name.split('.')[0];


Das kann auch ein undefined zurück liefern, wenn nach dem Split in [0] nichts vorhanden ist.
Wenn es schon nicht behandelt wird, dann müsste zumindest der return Wert string | undefined sein.

Nein? Wir haben immer ein string im name auch wenn das leer ist wird zu ""

im spec ist auch ein test dazu

Stimmt das liefert immer "" zurück.
Nur ".bild.123.jpg" als Test hinfügen und sollte dann ".bild.123" zurück liefern.

sonarqubecloud · 2023-10-26T12:52:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

SevenWaysDP added 2 commits October 5, 2023 19:42

BC-5170 - wip v1

1d5ff37

BC-5170 - wip v2

46203c9

SevenWaysDP added the WIP This feature branch is in progress, do not merge into master. label Oct 5, 2023

SevenWaysDP and others added 2 commits October 6, 2023 15:12

Merge branch 'main' into BC-5170-no-preview-for-larger-files

cd1ddfc

Adjust tests in file storage service and uc

2c73716

bischofmax reviewed Oct 9, 2023

View reviewed changes

apps/server/src/modules/files-storage/index.ts Show resolved Hide resolved

bischofmax and others added 3 commits October 9, 2023 13:17

Fix imports of testmodules

8b7c957

BC-5170 - fix api tests

13a53c3

BC-5170 - add test + code review

ec15838

SevenWaysDP requested review from bischofmax and CeEv October 9, 2023 13:44

SevenWaysDP commented Oct 11, 2023

View reviewed changes

apps/server/src/shared/infra/rabbitmq/rpc-message-producer.ts Show resolved Hide resolved

SevenWaysDP added 2 commits October 11, 2023 16:47

Merge remote-tracking branch 'origin/main' into BC-5170-no-preview-fo…

97daebc

…r-larger-files

BC-5170 devops code review

c8999a4

SevenWaysDP commented Oct 11, 2023

View reviewed changes

apps/server/src/modules/files-storage-client/service/files-storage.producer.ts Show resolved Hide resolved

ansible/roles/schulcloud-server-core/templates/amqp-preview-deployment.yml.j2 Outdated Show resolved Hide resolved

Merge branch 'main' into BC-5170-no-preview-for-larger-files

9b2708b

CeEv requested changes Oct 12, 2023

View reviewed changes

SevenWaysDP and others added 12 commits October 13, 2023 09:13

BC-5170 - code review

37bfd65

BC-5170 - rename method

75cb61c

BC-5170 - change logger

7565814

BC-5170 - add configmap

d609e9b

fixup! BC-5170 - add configmap

1f4ab96

fixup! fixup! BC-5170 - add configmap

6bad348

BC-5170 - remove required envs

1c63d61

BC-5170 - add scaled object

c827105

Merge branch 'main' into BC-5170-no-preview-for-larger-files

5939d0e

BC-5170 code review

9bda5ae

Merge remote-tracking branch 'origin/BC-5170-no-preview-for-larger-fi…

9af1a5a

…les' into BC-5170-no-preview-for-larger-files

BC-5170 - code review

24d1440

SevenWaysDP added 3 commits October 24, 2023 09:32

BC-5170 - fix lintner

5e05e10

BC-5170 - change keda config

1623cd5

BC-5170 - code review

36e4137

mamutmk5 requested changes Oct 25, 2023

View reviewed changes

SevenWaysDP and others added 6 commits October 25, 2023 10:10

Update ansible/roles/schulcloud-server-core/tasks/main.yml

da6cd81

Co-authored-by: mamutmk5 <[email protected]>

Update ansible/roles/schulcloud-server-core/tasks/main.yml

588e022

Co-authored-by: mamutmk5 <[email protected]>

Update ansible/roles/schulcloud-server-core/tasks/main.yml

634db78

Co-authored-by: mamutmk5 <[email protected]>

Update ansible/roles/schulcloud-server-core/tasks/main.yml

26563ed

Co-authored-by: mamutmk5 <[email protected]>

Update ansible/roles/schulcloud-server-core/templates/preview-generat…

6966569

…or-scaled-object.yml.j2 Co-authored-by: mamutmk5 <[email protected]>

Merge remote-tracking branch 'origin/main' into BC-5170-no-preview-fo…

20f5164

…r-larger-files

mamutmk5 requested changes Oct 25, 2023

View reviewed changes

ansible/roles/schulcloud-server-core/templates/preview-generator-deployment.yml.j2 Outdated Show resolved Hide resolved

SevenWaysDP and others added 3 commits October 25, 2023 16:09

BC-5170 - code review

bfc70c9

Update ansible/roles/schulcloud-server-core/templates/preview-generat…

bebde5c

…or-deployment.yml.j2 Co-authored-by: mamutmk5 <[email protected]>

BC-5170 - add label

44e0eaf

SevenWaysDP removed the WIP This feature branch is in progress, do not merge into master. label Oct 26, 2023

BC-5170 - add configs

4ebdb1d

SevenWaysDP requested review from CeEv and mamutmk5 October 26, 2023 09:09

CeEv requested changes Oct 26, 2023

View reviewed changes

BC-5170 - code review

96989b2

SevenWaysDP requested a review from CeEv October 26, 2023 10:45

CeEv reviewed Oct 26, 2023

View reviewed changes

BC-5170 - change logic for file name

a5f0fb0

CeEv approved these changes Oct 26, 2023

View reviewed changes

BC-5170 - because Cedric wishs ;o)

5ea34cc

mamutmk5 approved these changes Oct 26, 2023

View reviewed changes

SevenWaysDP merged commit 84c44c1 into main Oct 27, 2023
46 checks passed

SevenWaysDP deleted the BC-5170-no-preview-for-larger-files branch October 27, 2023 06:56

bergatco pushed a commit that referenced this pull request Nov 2, 2023

Bc 5170 no preview for larger files (#4457)

8f5df45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bc 5170 no preview for larger files #4457

Bc 5170 no preview for larger files #4457

SevenWaysDP commented Oct 5, 2023 •

edited

Loading

SevenWaysDP left a comment

CeEv Oct 12, 2023

CeEv Oct 12, 2023

SevenWaysDP Oct 12, 2023

CeEv Oct 26, 2023

SevenWaysDP Oct 26, 2023

CeEv Oct 12, 2023

CeEv Oct 26, 2023

mamutmk5 Oct 25, 2023

SevenWaysDP Oct 26, 2023

CeEv Oct 26, 2023

CeEv Oct 26, 2023

SevenWaysDP Oct 26, 2023

CeEv Oct 26, 2023 •

edited

Loading

SevenWaysDP Oct 26, 2023

SevenWaysDP Oct 26, 2023

CeEv Oct 26, 2023 •

edited

Loading

sonarqubecloud bot commented Oct 26, 2023

		@@ -0,0 +1,7 @@
		import { Configuration } from '@hpi-schul-cloud/commons/lib';

		export const FilesPreviewExchange = Configuration.get('FILES_STORAGE__EXCHANGE') as string;

Bc 5170 no preview for larger files #4457

Bc 5170 no preview for larger files #4457

Conversation

SevenWaysDP commented Oct 5, 2023 • edited Loading

Description

Links to Tickets or other pull requests

Changes

Datasecurity

Deployment

New Repos, NPM pakages or vendor scripts

Approval for review

SevenWaysDP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CeEv Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CeEv Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 26, 2023

SevenWaysDP commented Oct 5, 2023 •

edited

Loading

CeEv Oct 26, 2023 •

edited

Loading

CeEv Oct 26, 2023 •

edited

Loading