perf(h5p-server): reduce memory and IO when uploading package (#2100)

* perf(h5p-server): reduce memory and IO when uploading package

Reference a single file for identical filepaths rather than one file for each.

* refactor: fix variable name casing

* test: add a couple package upload tests

* test: make sure only 2 files were added to permanent storage

packages/h5p-server/src/ContentStorer.ts

@@ -236,12 +236,18 @@ export default class ContentStorer {
                 )
             );
 
+        const filePathToNewFilenameMap = new Map<string, string>();
         for (const reference of fileReferencesInParams) {
             const filepath = path.join(
                 packageDirectory,
                 'content',
                 reference.filePath
             );
+            let newFilename = filePathToNewFilenameMap.get(filepath);
+            if (newFilename) {
+                reference.context.params.path = `${newFilename}#tmp`;
+                continue;
+            }
             // If the file referenced in the parameters isn't included in the
             // package, we first check if the path is actually a URL and if not,
             // we delete the reference.
@@ -257,12 +263,13 @@ export default class ContentStorer {
                 continue;
             }
             const readStream = fsExtra.createReadStream(filepath);
-            const newFilename = await this.temporaryFileManager.addFile(
+            newFilename = await this.temporaryFileManager.addFile(
                 reference.filePath,
                 readStream,
                 user
             );
             reference.context.params.path = `${newFilename}#tmp`;
+            filePathToNewFilenameMap.set(filepath, newFilename);
         }
         return { metadata, parameters };
     }
@@ -487,6 +494,7 @@ export default class ContentStorer {
         oldFiles: string[]
     ): Promise<IFileReference[]> {
         const filesToCopyFromTemporaryStorage: IFileReference[] = [];
+        const hashSet = new Set<string>();
 
         for (const ref of fileReferencesInNewParams) {
             // We mark the file to be copied over from temporary storage if the
@@ -495,11 +503,15 @@ export default class ContentStorer {
                 // We only save temporary file for later copying, however, if
                 // the there isn't already a file with the exact name. This
                 // might be the case if the user presses "save" twice.
-                if (!oldFiles.some((f) => f === ref.filePath)) {
+                if (
+                    !hashSet.has(ref.filePath) &&
+                    !oldFiles.some((f) => f === ref.filePath)
+                ) {
                     filesToCopyFromTemporaryStorage.push(ref);
                 }
                 // remove temporary file marker from parameters
                 ref.context.params.path = ref.filePath;
+                hashSet.add(ref.filePath);
             }
         }
         return filesToCopyFromTemporaryStorage;

packages/h5p-server/test/H5PEditor.uploadingPackages.test.ts

@@ -127,7 +127,7 @@ describe('H5PEditor', () => {
                     await packageFinishedPromise;
                     writeStream.close();
 
-                    // CHeck if filename remains short
+                    // Check if filename remains short
                     expect(
                         /^earth-[0-9a-z]+\.jpg$/i.test(parameters?.image?.path)
                     ).toBe(true);
@@ -136,4 +136,119 @@ describe('H5PEditor', () => {
             { keep: false, unsafeCleanup: true }
         );
     });
+
+    // context: activity contains 3 images; image1 uploaded, image2
+    // copy/pasted from image1, image3 uploaded
+    it(
+        'returns parameters containing items 1 and 2 referencing file 1, and ' +
+            'item 3 referencing file 2',
+        async () => {
+            await withDir(
+                async ({ path: tempDirPath }) => {
+                    const { h5pEditor, temporaryStorage } =
+                        createH5PEditor(tempDirPath);
+                    const user = new User();
+
+                    const fileBuffer = fsExtra.readFileSync(
+                        path.resolve('test/data/validator/valid3-3-images.h5p')
+                    );
+                    const { parameters } = await h5pEditor.uploadPackage(
+                        fileBuffer,
+                        user
+                    );
+
+                    const tmpFilePath1 =
+                        parameters.items[0].image.params.file.path;
+                    const tmpFilePath2 =
+                        parameters.items[1].image.params.file.path;
+                    const tmpFilePath3 =
+                        parameters.items[2].image.params.file.path;
+                    expect(tmpFilePath1).toEqual(tmpFilePath2);
+                    expect(tmpFilePath2).not.toEqual(tmpFilePath3);
+
+                    const files = await temporaryStorage.listFiles(user);
+                    expect(files).toHaveLength(2);
+
+                    expect(
+                        temporaryStorage.fileExists(
+                            tmpFilePath1.substr(0, tmpFilePath1.length - 4),
+                            user
+                        )
+                    ).resolves.toEqual(true);
+
+                    expect(
+                        temporaryStorage.fileExists(
+                            tmpFilePath3.substr(0, tmpFilePath3.length - 4),
+                            user
+                        )
+                    ).resolves.toEqual(true);
+                },
+                { keep: false, unsafeCleanup: true }
+            );
+        }
+    );
+
+    // context: activity contains 3 images; image1 uploaded, image2
+    // copy/pasted from image1, image3 uploaded
+    it('saves uploaded package as new content; 2 images saved to permanent storage', async () => {
+        await withDir(
+            async ({ path: tempDirPath }) => {
+                const { h5pEditor, contentStorage } =
+                    createH5PEditor(tempDirPath);
+                const user = new User();
+
+                const fileBuffer = fsExtra.readFileSync(
+                    path.resolve('test/data/validator/valid3-3-images.h5p')
+                );
+                const { metadata, parameters } = await h5pEditor.uploadPackage(
+                    fileBuffer,
+                    user
+                );
+
+                const tmpFilePath1 = parameters.items[0].image.params.file.path;
+                const tmpFilePath3 = parameters.items[2].image.params.file.path;
+
+                const contentId = await h5pEditor.saveOrUpdateContent(
+                    undefined,
+                    parameters,
+                    metadata,
+                    ContentMetadata.toUbername(metadata),
+                    user
+                );
+
+                const files = await contentStorage.listFiles(contentId, user);
+                expect(files).toHaveLength(2);
+
+                // get data we've stored and check if the #tmp tag has been removed from the image
+                const { params } = await h5pEditor.getContent(contentId, user);
+
+                // FIRST IMAGE =======================
+                const newFilename1 = tmpFilePath1.substr(
+                    0,
+                    tmpFilePath1.length - 4
+                );
+                expect(params.params.items[0].image.params.file.path).toEqual(
+                    newFilename1
+                );
+                // check if image is now in permanent storage
+                await expect(
+                    contentStorage.fileExists(contentId, newFilename1)
+                ).resolves.toEqual(true);
+
+                // SECOND IMAGE =======================
+                const newFilename2 = tmpFilePath3.substr(
+                    0,
+                    tmpFilePath3.length - 4
+                );
+                expect(params.params.items[2].image.params.file.path).toEqual(
+                    newFilename2
+                );
+                // check if image is now in permanent storage
+                await expect(
+                    contentStorage.fileExists(contentId, newFilename2)
+                ).resolves.toEqual(true);
+            },
+            { keep: false, unsafeCleanup: true }
+        );
+    });
 });