From 4bd804df313390476b4b47b1a0c4dcee4e89ce0a Mon Sep 17 00:00:00 2001
From: Nicolas Pavie <pavie.nicolas@gmail.com>
Date: Thu, 25 Apr 2024 11:38:10 +0200
Subject: [PATCH] fix(#212): avoid empty download folder in loaded settings

Added a URL test in settings middleware to avoid empty or invalid download folder on loading the settings file.

Also
- Added some additionnal logging to report errors in the log on downloading results
- Handle a possible case of download results called on a job without results
- Avoid to delete the engine job if it has not been downloaded
---
 src/main/data/middlewares/pipeline.ts | 69 +++++++++++++--------------
 src/main/data/middlewares/settings.ts | 10 ++++
 2 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/src/main/data/middlewares/pipeline.ts b/src/main/data/middlewares/pipeline.ts
index 0155e12..3cbd0ce 100644
--- a/src/main/data/middlewares/pipeline.ts
+++ b/src/main/data/middlewares/pipeline.ts
@@ -93,22 +93,6 @@ const timestamp = () => {
         currentTime.getMilliseconds()}`
 }
 
-async function downloadResultFile(r: ResultFile, targetUrl: string) {
-    return pipelineAPI
-        .fetchResult(r)()
-        .then((buffer) =>
-            r.mimeType === 'application/zip'
-                ? unzipFile(buffer, targetUrl)
-                : saveFile(buffer, targetUrl)
-        )
-        .then(() => {
-            let newResult: ResultFile = Object.assign({}, r)
-            newResult.file = targetUrl
-            return newResult
-        })
-        .catch(() => r) // if a problem occured, return the original result
-}
-
 async function downloadNamedResult(r: NamedResult, targetUrl: string) {
     return pipelineAPI
         .fetchResult(r)()
@@ -133,7 +117,11 @@ async function downloadNamedResult(r: NamedResult, targetUrl: string) {
             })
             return newResult
         })
-        .catch(() => r) // if a problem occured, return the original result
+        .catch((e) => {
+            // if a problem occured, return the original result
+            error('Error downloading named result', r, e)
+            return r
+        })
 }
 
 async function downloadJobLog(j: Job, targetFolder: string) {
@@ -161,7 +149,7 @@ async function downloadJobLog(j: Job, targetFolder: string) {
         .catch((e) => {
             // Log is not accessible, revert back to default log url
             // and continue the chain of promise
-            error('Job log could not be downloaded', e)
+            error('Error downloading job log', e)
             return j
         })
 }
@@ -169,12 +157,12 @@ async function downloadJobLog(j: Job, targetFolder: string) {
 async function downloadJobResults(j: Job, targetFolder: string) {
     // Download log, named results, and unzip named results archives
     return Promise.all(
-        j.jobData.results.namedResults.map((r) =>
+        j.jobData.results?.namedResults?.map((r) =>
             downloadNamedResult(
                 r,
                 new URL(`${targetFolder}/${r.nicename ?? r.name}`).href
             )
-        )
+        ) || []
     )
         .then((downloadedNamedResults: NamedResult[]) => {
             return {
@@ -183,13 +171,17 @@ async function downloadJobResults(j: Job, targetFolder: string) {
                     ...j.jobData,
                     downloadedFolder: targetFolder,
                     results: {
-                        ...j.jobData.results,
+                        ...(j.jobData.results ?? {}),
                         namedResults: [...downloadedNamedResults],
                     },
                 },
             } as Job
         })
         .then(async (j: Job) => await downloadJobLog(j, targetFolder))
+        .catch((e) => {
+            error('Error downloading job results', e)
+            return j
+        })
 }
 
 /**
@@ -238,21 +230,28 @@ function startMonitor(j: Job, ws: Webservice, getState, dispatch) {
                         downloadJobResults(
                             updatedJob,
                             `${downloadFolder}/${newJobName}`
-                        ).then((downloadedJob) => {
-                            dispatch(updateJob(downloadedJob))
-                            const deleteJob =
-                                pipelineAPI.deleteJob(downloadedJob)
-                            deleteJob().then((response) => {
-                                console.log(
-                                    downloadedJob.jobData.jobId,
-                                    'delete response',
-                                    response.status,
-                                    response.statusText
-                                )
+                        )
+                            .then((downloadedJob) => {
+                                dispatch(updateJob(downloadedJob))
+                                // Only delete job if it has been downloaded
+                                if (downloadedJob.jobData.downloadedFolder) {
+                                    const deleteJob =
+                                        pipelineAPI.deleteJob(downloadedJob)
+                                    deleteJob().then((response) => {
+                                        info(
+                                            downloadedJob.jobData.jobId,
+                                            'delete response',
+                                            response.status,
+                                            response.statusText
+                                        )
+                                    })
+                                }
+                            })
+                            .catch((e) => {
+                                error('Error downloading job results', e)
                             })
-                        })
                     } else if (finished) {
-                        console.log('job is finished without results')
+                        info('job is finished without results')
                         // job is finished wihout results : keep the log
                         downloadJobLog(
                             updatedJob,
@@ -261,7 +260,7 @@ function startMonitor(j: Job, ws: Webservice, getState, dispatch) {
                             dispatch(updateJob(jobWithLog))
                             const deleteJob = pipelineAPI.deleteJob(jobWithLog)
                             deleteJob().then((response) => {
-                                console.log(
+                                info(
                                     jobWithLog.jobData.jobId,
                                     'delete response',
                                     response.status,
diff --git a/src/main/data/middlewares/settings.ts b/src/main/data/middlewares/settings.ts
index 6499a6b..bd1ad67 100644
--- a/src/main/data/middlewares/settings.ts
+++ b/src/main/data/middlewares/settings.ts
@@ -61,6 +61,16 @@ export function readSettings() {
             const loaded: ApplicationSettings = migrateSettings(
                 JSON.parse(readFileSync(settingsFile, 'utf8'))
             )
+            // NP 2024/04/25 : reset download folder on invalid url
+            try {
+                new URL(loaded.downloadFolder)
+            } catch (e) {
+                info(
+                    'Invalid download folder URL in settings file',
+                    'Falling back to default download folder'
+                )
+                loaded.downloadFolder = settings.downloadFolder
+            }
             settings = {
                 ...settings,
                 ...loaded,