Pivotal ID # 184772659: Delete only common files when required on re submission

submission/persistence-filesystem/src/main/kotlin/ac/uk/ebi/biostd/persistence/filesystem/api/FileStorageService.kt

@@ -13,5 +13,5 @@ interface FileStorageService {
 
     fun deleteSubmissionFile(sub: ExtSubmission, file: ExtFile)
 
-    fun deleteFtpLinks(sub: ExtSubmission)
+    fun deleteFtpFile(sub: ExtSubmission, file: ExtFile)
 }

submission/persistence-filesystem/src/main/kotlin/ac/uk/ebi/biostd/persistence/filesystem/api/FilesService.kt

@@ -10,5 +10,5 @@ internal interface FilesService {
 
     fun deleteSubmissionFile(sub: ExtSubmission, file: ExtFile)
 
-    fun deleteFtpLinks(sub: ExtSubmission)
+    fun deleteFtpFile(sub: ExtSubmission, file: ExtFile)
 }

submission/persistence-filesystem/src/main/kotlin/ac/uk/ebi/biostd/persistence/filesystem/fire/FireFilesService.kt

@@ -96,15 +96,15 @@ class FireFilesService(
         return file.asFireFile(fireOid, expectedPath, published)
     }
 
-    override fun deleteFtpLinks(sub: ExtSubmission) {
-        // No need to delete FTP links on FIRE
-    }
-
     override fun deleteSubmissionFile(sub: ExtSubmission, file: ExtFile) {
         require(file is FireFile) { "FireFilesService should only handle FireFile" }
         client.delete(file.fireId)
     }
 
+    override fun deleteFtpFile(sub: ExtSubmission, file: ExtFile) {
+        // No need to delete FTP links on FIRE as file deleting complete this
+    }
+
     override fun deleteSubmissionFiles(sub: ExtSubmission) {
         fun deleteFile(index: Int, file: FireFile) {
             logger.info { "${sub.accNo} ${sub.owner} Deleting file $index, path='${file.filePath}'" }

submission/persistence-filesystem/src/main/kotlin/ac/uk/ebi/biostd/persistence/filesystem/nfs/NfsFilesService.kt

@@ -63,7 +63,7 @@ class NfsFilesService(
     }
 
     override fun deleteSubmissionFiles(sub: ExtSubmission) {
-        deleteFtpLinks(sub)
+        deleteFtpFolder(sub)
         deleteSubFolder(sub)
     }
 
@@ -72,9 +72,10 @@ class NfsFilesService(
         FileUtils.deleteFile(folderResolver.getSubFolder(sub.relPath).resolve(file.relPath).toFile())
     }
 
-    override fun deleteFtpLinks(sub: ExtSubmission) {
+    override fun deleteFtpFile(sub: ExtSubmission, file: ExtFile) {
         logger.info { "${sub.accNo} ${sub.owner} Started un-publishing files of submission ${sub.accNo} on NFS" }
-        FileUtils.deleteFile(folderResolver.getSubmissionFtpFolder(sub.relPath).toFile())
+        val subFolder = folderResolver.getSubmissionFtpFolder(sub.relPath)
+        FileUtils.deleteFile(subFolder.resolve(file.relPath).toFile())
         logger.info { "${sub.accNo} ${sub.owner} Finished un-publishing files of submission ${sub.accNo} on NFS" }
     }
 
@@ -83,4 +84,10 @@ class NfsFilesService(
         FileUtils.deleteFile(folderResolver.getSubFolder(sub.relPath).toFile())
         logger.info { "${sub.accNo} ${sub.owner} Finished deleting files of submission ${sub.accNo} on NFS" }
     }
+
+    private fun deleteFtpFolder(sub: ExtSubmission) {
+        logger.info { "${sub.accNo} ${sub.owner} Started un-publishing files of submission ${sub.accNo} on NFS" }
+        FileUtils.deleteFile(folderResolver.getSubmissionFtpFolder(sub.relPath).toFile())
+        logger.info { "${sub.accNo} ${sub.owner} Finished un-publishing files of submission ${sub.accNo} on NFS" }
+    }
 }

submission/persistence-filesystem/src/main/kotlin/ac/uk/ebi/biostd/persistence/filesystem/service/StorageService.kt

@@ -31,10 +31,10 @@ class StorageService(
         }
     }
 
-    override fun deleteFtpLinks(sub: ExtSubmission) =
+    override fun deleteFtpFile(sub: ExtSubmission, file: ExtFile) =
         when (sub.storageMode) {
-            FIRE -> fireFilesService.deleteFtpLinks(sub)
-            NFS -> nfsFilesService.deleteFtpLinks(sub)
+            FIRE -> fireFilesService.deleteFtpFile(sub, file)
+            NFS -> nfsFilesService.deleteFtpFile(sub, file)
         }
 
     override fun deleteSubmissionFile(sub: ExtSubmission, file: ExtFile) =

submission/persistence-filesystem/src/test/kotlin/ac/uk/ebi/biostd/persistence/filesystem/nfs/NfsFilesServiceTest.kt

@@ -74,11 +74,14 @@ class NfsFilesServiceTest(
     @Test
     fun `delete ftp links`() {
         val ftpFolder = ftpFolder.createDirectory("S-BSST2")
+        val filesFtpFolder = ftpFolder.createDirectory("Files")
+        val ftpFile = filesFtpFolder.createFile("file1.txt")
+        val nfsFile = createNfsFile("file1.txt", "Files/file1.txt", tempFolder.createFile("file1.txt"))
         val sub = basicExtSubmission.copy(relPath = "S-BSST2")
 
-        testInstance.deleteFtpLinks(sub)
+        testInstance.deleteFtpFile(sub, nfsFile)
 
-        assertThat(Files.exists(ftpFolder.toPath())).isFalse()
+        assertThat(Files.exists(ftpFile.toPath())).isFalse()
     }
 
     @Test

submission/submission-webapp/src/itest/kotlin/ac/uk/ebi/biostd/itest/test/submission/submit/SubmissionFileSourceTest.kt

@@ -35,6 +35,7 @@ import ebi.ac.uk.model.extensions.title
 import ebi.ac.uk.util.collections.second
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.condition.EnabledIfSystemProperty
 import org.junit.jupiter.api.extension.ExtendWith
@@ -196,47 +197,55 @@ class SubmissionFileSourceTest(
         assertThat(referencedFileFireId).isEqualTo(fireFile3.fireOid)
     }
 
-    @Test
-    @EnabledIfSystemProperty(named = "enableFire", matches = "true")
-    fun `6-3 submission with directory with files on FIRE`() {
-        val submission = tsv {
-            line("Submission", "S-FSTST3")
-            line("Title", "Simple Submission With directory")
-            line()
+    @Nested
+    inner class SubmissionsWithFolders {
+        @Test
+        @EnabledIfSystemProperty(named = "enableFire", matches = "true")
+        fun `6-3-1 submission with directory with files on FIRE`() {
+            val submission = tsv {
+                line("Submission", "S-FSTST3")
+                line("Title", "Simple Submission With directory")
+                line()
+
+                line("Study")
+                line()
+
+                line("File", "directory")
+                line("Type", "test")
+                line()
+            }.toString()
 
-            line("Study")
-            line()
+            val file1 = tempFolder.createFile("file1.txt", "content-1")
+            val file2 = tempFolder.createFile(".file2.txt", "content-2")
 
-            line("File", "directory")
-            line("Type", "test")
-            line()
-        }.toString()
-
-        val file1 = tempFolder.createFile("file1.txt", "content-1")
-        val file2 = tempFolder.createFile("file2.txt", "content-2")
+            webClient.uploadFiles(listOf(file1), "directory")
+            webClient.uploadFiles(listOf(file2), "directory/subdirectory")
 
-        webClient.uploadFiles(listOf(file1), "directory")
-        webClient.uploadFiles(listOf(file2), "directory/subdirectory")
+            assertThat(webClient.submitSingle(submission, TSV)).isSuccessful()
 
-        assertThat(webClient.submitSingle(submission, TSV)).isSuccessful()
+            val submitted = submissionRepository.getExtByAccNo("S-FSTST3")
+            assertThat(submitted.section.files).hasSize(1)
+            assertThat(submitted.section.files.first()).hasLeftValueSatisfying {
+                assertThat(it.type).isEqualTo(ExtFileType.DIR)
+                assertThat(it.size).isEqualTo(328L)
+                assertThat(it.md5).isEqualTo("18CF763D0BBA08E1AE232C191A3B58CF")
 
-        val submitted = submissionRepository.getExtByAccNo("S-FSTST3")
-        assertThat(submitted.section.files).hasSize(1)
-        assertThat(submitted.section.files.first()).hasLeftValueSatisfying {
-            assertThat(it.type).isEqualTo(ExtFileType.DIR)
-            assertThat(it.size).isEqualTo(326L)
-            assertThat(it.md5).isEqualTo("8BD1F30C5389037D06A3CA20E5549B45")
+                val files = getZipFiles("$submissionPath/${submitted.relPath}/Files/directory.zip")
+                assertThat(files).containsExactly(
+                    "file1.txt" to file1.readText(),
+                    "subdirectory/.file2.txt" to file2.readText()
+                )
+            }
+        }
 
+        private fun getZipFiles(filePath: String): List<Pair<String, String>> {
             val subZip = tempFolder.createDirectory("target")
-            ZipUtil.unpack(File("$submissionPath/${submitted.relPath}/Files/directory.zip"), subZip)
+            ZipUtil.unpack(File(filePath), subZip)
             val files = subZip.allSubFiles()
                 .filter { file -> file.isDirectory.not() }
                 .map { file -> file.toRelativeString(subZip) to file.readText() }
-
-            assertThat(files).containsExactly(
-                "file1.txt" to file1.readText(),
-                "subdirectory/file2.txt" to file2.readText()
-            )
+            subZip.delete()
+            return files
         }
     }
 

submission/submitter/src/main/kotlin/ac/uk/ebi/biostd/submission/submitter/request/SubmissionRequestCleaner.kt

@@ -7,6 +7,8 @@ import ac.uk.ebi.biostd.persistence.common.service.SubmissionRequestPersistenceS
 import ac.uk.ebi.biostd.persistence.filesystem.api.FileStorageService
 import ebi.ac.uk.extended.model.ExtFile
 import ebi.ac.uk.extended.model.ExtSubmission
+import ebi.ac.uk.extended.model.StorageMode
+import ebi.ac.uk.extended.model.storageMode
 import mu.KotlinLogging
 import uk.ac.ebi.extended.serialization.service.ExtSerializationService
 import uk.ac.ebi.extended.serialization.service.fileSequence
@@ -28,7 +30,6 @@ class SubmissionRequestCleaner(
         if (current != null) {
             logger.info { "${new.accNo} ${new.owner} Started cleaning common files of version ${new.version}" }
             deleteCommonFiles(new, current)
-            storageService.deleteFtpLinks(current)
             logger.info { "${new.accNo} ${new.owner} Finished cleaning common files of version ${new.version}" }
         }
 
@@ -39,18 +40,28 @@ class SubmissionRequestCleaner(
         fun deleteFile(index: Int, file: ExtFile) {
             logger.info { "${current.accNo} ${current.owner} Deleting file $index, path='${file.filePath}'" }
             storageService.deleteSubmissionFile(current, file)
+            storageService.deleteFtpFile(current, file)
         }
 
+        fun shouldDelete(newFiles: Map<String, FileEntry>, existing: ExtFile): Boolean =
+            when (val newFile = newFiles[existing.filePath]) {
+                null -> false
+                else -> newFile.md5 != existing.md5 && new.storageMode == existing.storageMode
+            }
+
         val newFiles = newFilesMap(new)
         logger.info { "${current.accNo} ${current.owner} Started cleaning common submission files" }
         serializationService.fileSequence(current)
-            .filter { newFiles.containsKey(it.filePath) && newFiles[it.filePath] != it.md5 }
+            .filter { shouldDelete(newFiles, it) }
             .forEachIndexed { index, file -> deleteFile(index, file) }
         logger.info { "${current.accNo} ${current.owner} Finished cleaning common submission files" }
     }
 
-    private fun newFilesMap(new: ExtSubmission) =
-        filesRequestService
+    private fun newFilesMap(new: ExtSubmission): Map<String, FileEntry> {
+        return filesRequestService
             .getSubmissionRequestFiles(new.accNo, new.version, 0)
-            .associate { it.path to it.file.md5 }
+            .associate { it.path to FileEntry(it.file.md5, it.file.storageMode) }
+    }
+
+    private data class FileEntry(val md5: String, val storageMode: StorageMode)
 }

submission/submitter/src/test/kotlin/ac/uk/ebi/biostd/submission/submitter/request/SubmissionRequestCleanerTest.kt

@@ -7,8 +7,8 @@ import ac.uk.ebi.biostd.persistence.common.service.SubmissionPersistenceQuerySer
 import ac.uk.ebi.biostd.persistence.common.service.SubmissionRequestFilesPersistenceService
 import ac.uk.ebi.biostd.persistence.common.service.SubmissionRequestPersistenceService
 import ac.uk.ebi.biostd.persistence.filesystem.service.StorageService
-import ebi.ac.uk.extended.model.ExtFile
 import ebi.ac.uk.extended.model.ExtSubmission
+import ebi.ac.uk.extended.model.FireFile
 import ebi.ac.uk.extended.model.StorageMode.FIRE
 import io.mockk.clearAllMocks
 import io.mockk.every
@@ -64,29 +64,32 @@ class SubmissionRequestCleanerTest(
 
         verify(exactly = 1) { requestService.saveSubmissionRequest(cleanedRequest) }
         verify(exactly = 0) {
-            storageService.deleteFtpLinks(any())
+            storageService.deleteFtpFile(any(), any())
             storageService.deleteSubmissionFile(any(), any())
         }
     }
 
     @Test
     fun `clean common files with current version`(
-        @MockK newFile: ExtFile,
-        @MockK currentFile: ExtFile,
+        @MockK newFile: FireFile,
+        @MockK currentFile: FireFile,
         @MockK loadedRequest: SubmissionRequest,
         @MockK cleanedRequest: SubmissionRequest,
         @MockK requestFile: SubmissionRequestFile,
     ) {
         val new = mockSubmission()
+        every { newFile.md5 } returns "new-md5"
+        every { newFile.filePath } returns "a/b/file.txt"
+
         val current = mockSubmission()
+        every { requestFile.path } returns "a/b/file.txt"
         every { requestFile.file } returns newFile
-        every { loadedRequest.submission } returns new
-        every { newFile.md5 } returns "new-md5"
         every { currentFile.md5 } returns "current-md5"
-        every { requestFile.path } returns "a/b/file.txt"
-        every { newFile.filePath } returns "a/b/file.txt"
         every { currentFile.filePath } returns "a/b/file.txt"
-        every { storageService.deleteFtpLinks(current) } answers { nothing }
+
+        every { loadedRequest.submission } returns new
+
+        every { storageService.deleteFtpFile(current, currentFile) } answers { nothing }
         every { loadedRequest.withNewStatus(CLEANED) } returns cleanedRequest
         every { queryService.findExtByAccNo("S-BSST1", true) } returns current
         every { requestService.getLoadedRequest("S-BSST1", 2) } returns loadedRequest
@@ -98,7 +101,7 @@ class SubmissionRequestCleanerTest(
         testInstance.cleanCurrentVersion("S-BSST1", 2)
 
         verify(exactly = 1) {
-            storageService.deleteFtpLinks(current)
+            storageService.deleteFtpFile(current, currentFile)
             requestService.saveSubmissionRequest(cleanedRequest)
             storageService.deleteSubmissionFile(current, currentFile)
         }