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

Remove file retention time limit #218

Merged
merged 1 commit into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .env
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,4 @@ RABBIT_PORT=5672
PROMETHEUS_PORT=9090
GRAFANA_PORT=3000

FILES_RETENTION_DAYS=
EXPORTS_RETENTION_DAYS=

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ interface FileService {
studyId: UUID,
)

fun deleteAllOlderThan(days: Int)

suspend fun deleteAllByStudyId(studyId: String)

/** The [createDEPRECATED] interface for creating a file. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@ import org.springframework.transaction.annotation.Transactional
import org.springframework.util.StringUtils
import org.springframework.web.multipart.MultipartFile
import org.springframework.web.util.UriComponentsBuilder
import java.io.IOException
import java.nio.file.Files
import java.nio.file.Path
import java.time.Instant

@Service
@Transactional
Expand Down Expand Up @@ -168,23 +166,6 @@ class FileServiceImpl(
LOGGER.info("File deleted, id = $id")
}

@Suppress("MagicNumber")
override fun deleteAllOlderThan(days: Int) {
val clockNow7DaysAgo = System.currentTimeMillis() - days * 24 * 60 * 60 * 1000
val filesToDelete = fileRepository.getAllByUpdatedAtIsBefore(Instant.ofEpochMilli(clockNow7DaysAgo))

filesToDelete.forEach { file ->
fileRepository.delete(file)
try {
fileStorage.deleteFileAtPath(file.fileName, Path.of(file.relativePath))
} catch (e: IOException) {
LOGGER.error("Failed to delete file with id = ${file.id}", e)
}
}

LOGGER.info("All files older than $days days deleted")
}

override suspend fun deleteAllByStudyId(studyId: String) {
val files =
withContext(Dispatchers.IO) {
Expand Down
2 changes: 0 additions & 2 deletions src/main/resources/config/application-local.yml
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,6 @@ storage:
days: 90

cleanup:
files:
retention-days: "${FILES_RETENTION_DAYS:7}"
exports:
retention-days: "${EXPORTS_RETENTION_DAYS:7}"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,10 @@ import dk.cachet.carp.webservices.security.authorization.Claim
import dk.cachet.carp.webservices.security.authorization.service.AuthorizationService
import io.mockk.*
import kotlinx.coroutines.test.runTest
import kotlinx.datetime.Instant
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows
import org.springframework.mock.web.MockMultipartFile
import org.springframework.util.StringUtils
import java.io.IOException
import java.util.*
import kotlin.test.assertEquals
import kotlin.test.assertNotEquals
Expand Down Expand Up @@ -195,106 +192,4 @@ class FileServiceTest {
assertEquals(expectedKey, actualKey)
}
}

@Nested
inner class DeleteAllOlderThan {
@Test
fun `deletes older than 7 days`() {
val fileMock1 =
mockk<File> {
every { id } returns 1
every { fileName } returns "file1"
every { relativePath } returns "foo/bar/baz"
}
val fileMock2 =
mockk<File> {
every { id } returns 2
every { fileName } returns "file2"
every { relativePath } returns "foo/bar/qux"
}
every { fileRepository.getAllByUpdatedAtIsBefore(any()) } returns mutableListOf(fileMock1, fileMock2)
every { fileRepository.delete(any<File>()) } just runs
every { fileStorage.deleteFileAtPath(any(), any()) } returns true
val sut =
FileServiceImpl(
fileRepository,
fileStorage,
messageBase,
s3Client,
authenticationService,
s3SpaceBucket,
s3SpaceEndpoint,
)
val sevenDaysAgo = System.currentTimeMillis() - 7 * 24 * 60 * 60 * 1000

sut.deleteAllOlderThan(7)

verify { fileRepository.getAllByUpdatedAtIsBefore(java.time.Instant.ofEpochMilli(sevenDaysAgo)) }
verify { fileRepository.delete(fileMock1) }
verify { fileRepository.delete(fileMock2) }
verify { fileStorage.deleteFileAtPath("file1", java.nio.file.Path.of("foo", "bar", "baz")) }
verify { fileStorage.deleteFileAtPath("file2", java.nio.file.Path.of("foo", "bar", "qux")) }
}

@Test
fun `should keep deleting even if one of the files could not be deleted from storage`() {
val fileMock1 =
mockk<File> {
every { id } returns 1
every { fileName } returns "file1"
every { relativePath } returns "foo/bar/baz"
}
val fileMock2 =
mockk<File> {
every { id } returns 2
every { fileName } returns "file2"
every { relativePath } returns "foo/bar/qux"
}
every { fileRepository.getAllByUpdatedAtIsBefore(any()) } returns mutableListOf(fileMock1, fileMock2)
every { fileRepository.delete(any<File>()) } just runs
every {
fileStorage.deleteFileAtPath(
"file1",
java.nio.file.Path.of("foo", "bar", "baz"),
)
} returns true
every {
fileStorage.deleteFileAtPath(
"file2",
java.nio.file.Path.of("foo", "bar", "qux"),
)
} throws IOException(":(")
val sut =
FileServiceImpl(
fileRepository,
fileStorage,
messageBase,
s3Client,
authenticationService,
s3SpaceBucket,
s3SpaceEndpoint,
)
val sevenDaysAgo = System.currentTimeMillis() - 7 * 24 * 60 * 60 * 1000

sut.deleteAllOlderThan(7)

verify { fileRepository.getAllByUpdatedAtIsBefore(any()) }
verify { fileRepository.delete(fileMock1) }
verify { fileRepository.delete(fileMock2) }
verify { fileStorage.deleteFileAtPath("file1", java.nio.file.Path.of("foo", "bar", "baz")) }
verify { fileStorage.deleteFileAtPath("file2", java.nio.file.Path.of("foo", "bar", "qux")) }
assertThrows<IOException> {
fileStorage.deleteFileAtPath("file2", java.nio.file.Path.of("foo", "bar", "qux"))
}

verify {
fileRepository.getAllByUpdatedAtIsBefore(
match {
val tolerance = 5000
Math.abs(it.toEpochMilli() - sevenDaysAgo) <= tolerance
},
)
}
}
}
}