From bb069512b41b3d2854dee292ba852f431b81d868 Mon Sep 17 00:00:00 2001 From: FyloZ Date: Fri, 31 Dec 2021 16:04:09 -0500 Subject: [PATCH] #18 Add cache for existing files --- Dockerfile | 4 +- build.gradle.kts | 2 +- .../fyloz/colorrecipesexplorer/TypeAliases.kt | 2 + .../service/config/ConfigurationSource.kt | 6 +- .../service/files/FileExistCache.kt | 14 +- .../service/files/FileService.kt | 22 +- .../fyloz/colorrecipesexplorer/utils/Files.kt | 36 ++- .../service/files/FileServiceTest.kt | 281 ++++++++++-------- 8 files changed, 207 insertions(+), 160 deletions(-) diff --git a/Dockerfile b/Dockerfile index 9fc34f3..f16b892 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,5 +1,5 @@ -ARG GRADLE_VERSION=7.1 -ARG JAVA_VERSION=11 +ARG GRADLE_VERSION=7.3 +ARG JAVA_VERSION=17 FROM gradle:$GRADLE_VERSION-jdk$JAVA_VERSION AS build WORKDIR /usr/src diff --git a/build.gradle.kts b/build.gradle.kts index 0d8dffd..4922681 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -98,7 +98,7 @@ tasks.test { } tasks.withType() { - options.compilerArgs.addAll(arrayOf("--release", "11")) + options.compilerArgs.addAll(arrayOf("--release", "17")) } tasks.withType().all { kotlinOptions { diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/TypeAliases.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/TypeAliases.kt index b56a00d..1433775 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/TypeAliases.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/TypeAliases.kt @@ -3,3 +3,5 @@ package dev.fyloz.colorrecipesexplorer typealias SpringUser = org.springframework.security.core.userdetails.User typealias SpringUserDetails = org.springframework.security.core.userdetails.UserDetails typealias SpringUserDetailsService = org.springframework.security.core.userdetails.UserDetailsService + +typealias JavaFile = java.io.File diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/config/ConfigurationSource.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/config/ConfigurationSource.kt index 6971f9e..f7db611 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/config/ConfigurationSource.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/config/ConfigurationSource.kt @@ -1,5 +1,6 @@ package dev.fyloz.colorrecipesexplorer.service.config +import dev.fyloz.colorrecipesexplorer.JavaFile import dev.fyloz.colorrecipesexplorer.SUPPORTED_DATABASE_VERSION import dev.fyloz.colorrecipesexplorer.config.properties.CreProperties import dev.fyloz.colorrecipesexplorer.emergencyMode @@ -8,14 +9,13 @@ import dev.fyloz.colorrecipesexplorer.model.Configuration import dev.fyloz.colorrecipesexplorer.model.ConfigurationType import dev.fyloz.colorrecipesexplorer.model.configuration import dev.fyloz.colorrecipesexplorer.repository.ConfigurationRepository -import dev.fyloz.colorrecipesexplorer.service.files.create +import dev.fyloz.colorrecipesexplorer.utils.create import dev.fyloz.colorrecipesexplorer.utils.excludeAll import org.slf4j.Logger import org.springframework.boot.info.BuildProperties import org.springframework.context.annotation.Lazy import org.springframework.data.repository.findByIdOrNull import org.springframework.stereotype.Component -import java.io.File import java.io.FileInputStream import java.io.FileOutputStream import java.time.LocalDate @@ -96,7 +96,7 @@ private class FileConfigurationSource( private val configFilePath: String ) : ConfigurationSource { private val properties = Properties().apply { - with(File(configFilePath)) { + with(JavaFile(configFilePath)) { if (!this.exists()) this.create() FileInputStream(this).use { this@apply.load(it) diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileExistCache.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileExistCache.kt index 46abd2e..bbeccb8 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileExistCache.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileExistCache.kt @@ -2,22 +2,26 @@ package dev.fyloz.colorrecipesexplorer.service.files import dev.fyloz.colorrecipesexplorer.utils.FilePath -class FileExistCache { +object FileExistCache { private val map = hashMapOf() + /** Checks if the given [path] is in the cache. */ operator fun contains(path: FilePath) = path in map + /** Checks if the file at the given [path] exists. */ fun exists(path: FilePath) = map[path] ?: false - fun set(path: FilePath, exists: Boolean) { - map[path] = exists - } - + /** Sets the file at the given [path] as existing. */ fun setExists(path: FilePath) = set(path, true) + /** Sets the file at the given [path] as not existing. */ fun setDoesNotExists(path: FilePath) = set(path, false) + + private fun set(path: FilePath, exists: Boolean) { + map[path] = exists + } } \ No newline at end of file diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt index 683cb74..d3a7615 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt @@ -2,8 +2,8 @@ package dev.fyloz.colorrecipesexplorer.service.files import dev.fyloz.colorrecipesexplorer.config.properties.CreProperties import dev.fyloz.colorrecipesexplorer.exception.RestException +import dev.fyloz.colorrecipesexplorer.utils.File import dev.fyloz.colorrecipesexplorer.utils.FilePath -import dev.fyloz.colorrecipesexplorer.utils.WrappedFile import dev.fyloz.colorrecipesexplorer.utils.withFileAt import org.slf4j.Logger import org.springframework.core.io.ByteArrayResource @@ -11,9 +11,7 @@ import org.springframework.core.io.Resource import org.springframework.http.HttpStatus import org.springframework.stereotype.Service import org.springframework.web.multipart.MultipartFile -import java.io.File import java.io.IOException -import java.nio.file.Files /** Banned path shards. These are banned because they can allow access to files outside the data directory. */ val BANNED_FILE_PATH_SHARDS = setOf( @@ -52,12 +50,10 @@ class FileServiceImpl( private val creProperties: CreProperties, private val logger: Logger ) : WriteableFileService { - private val existsCache = FileExistCache() - override fun exists(path: String): Boolean { val fullPath = path.fullPath() - return if (fullPath in existsCache) { - existsCache.exists(fullPath) + return if (fullPath in FileExistCache) { + FileExistCache.exists(fullPath) } else { withFileAt(fullPath) { this.exists() && this.isFile @@ -82,7 +78,7 @@ class FileServiceImpl( try { withFileAt(fullPath) { this.create() - existsCache.setExists(fullPath) + FileExistCache.setExists(fullPath) } } catch (ex: IOException) { FileCreateException(path).logAndThrow(ex, logger) @@ -107,7 +103,7 @@ class FileServiceImpl( if (!exists(path)) throw FileNotFoundException(path) this.delete() - existsCache.setDoesNotExists(fullPath) + FileExistCache.setDoesNotExists(fullPath) } } catch (ex: IOException) { FileDeleteException(path).logAndThrow(ex, logger) @@ -122,7 +118,7 @@ class FileServiceImpl( return FilePath("${creProperties.dataDirectory}/$this") } - private fun prepareWrite(path: String, overwrite: Boolean, op: WrappedFile.() -> Unit) { + private fun prepareWrite(path: String, overwrite: Boolean, op: File.() -> Unit) { val fullPath = path.fullPath() if (exists(path)) { @@ -141,12 +137,6 @@ class FileServiceImpl( } } -/** Shortcut to create a file and its parent directories. */ -fun File.create() { - Files.createDirectories(this.parentFile.toPath()) - Files.createFile(this.toPath()) -} - private const val FILE_IO_EXCEPTION_TITLE = "File IO error" class InvalidFilePathException(val path: String, val fragment: String) : diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/utils/Files.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/utils/Files.kt index 8e08f4f..7f3f2b3 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/utils/Files.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/utils/Files.kt @@ -1,44 +1,50 @@ package dev.fyloz.colorrecipesexplorer.utils -import dev.fyloz.colorrecipesexplorer.service.files.create -import java.io.File +import dev.fyloz.colorrecipesexplorer.JavaFile +import java.nio.file.Files import java.nio.file.Path /** Mockable file wrapper, to prevent issues when mocking [java.io.File]. */ -class WrappedFile(val file: File) { +class File(val file: JavaFile) { val isFile: Boolean get() = file.isFile fun toPath(): Path = - file.toPath() + file.toPath() fun exists() = - file.exists() + file.exists() fun readBytes() = - file.readBytes() + file.readBytes() fun writeBytes(array: ByteArray) = - file.writeBytes(array) + file.writeBytes(array) fun create() = - file.create() + file.create() fun delete(): Boolean = - file.delete() + file.delete() companion object { fun from(path: String) = - WrappedFile(File(path)) + File(JavaFile(path)) fun from(path: FilePath) = - from(path.path) + from(path.path) } } -@JvmInline -value class FilePath(val path: String) +// TODO: Move to value class when mocking them with mockk works +class FilePath(val path: String) /** Runs the given [block] in the context of a file with the given [fullPath]. */ -fun withFileAt(fullPath: FilePath, block: WrappedFile.() -> T) = - WrappedFile.from(fullPath).block() \ No newline at end of file +fun withFileAt(fullPath: FilePath, block: File.() -> T) = + File.from(fullPath).block() + +/** Shortcut to create a file and its parent directories. */ +fun JavaFile.create() { + Files.createDirectories(this.parentFile.toPath()) + Files.createFile(this.toPath()) +} \ No newline at end of file diff --git a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt index a68a93d..491257c 100644 --- a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt +++ b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt @@ -1,13 +1,13 @@ package dev.fyloz.colorrecipesexplorer.service.files import dev.fyloz.colorrecipesexplorer.config.properties.CreProperties -import dev.fyloz.colorrecipesexplorer.utils.WrappedFile +import dev.fyloz.colorrecipesexplorer.utils.File import io.mockk.* import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows import org.springframework.mock.web.MockMultipartFile -import java.io.File import java.io.IOException import java.nio.file.Path import kotlin.test.assertEquals @@ -21,54 +21,121 @@ private const val mockFilePath = "existingFile" private val mockFilePathPath = Path.of(mockFilePath) private val mockFileData = byteArrayOf(0x1, 0x8, 0xa, 0xf) -private class FileServiceTestContext { - val fileService = spyk(FileServiceImpl(creProperties, mockk { +class FileServiceTest { + private val fileService = spyk(FileServiceImpl(creProperties, mockk { every { error(any(), any()) } just Runs })) - val mockFile = mockk { + private val mockFile = mockk { every { file } returns mockk() every { exists() } returns true every { isFile } returns true every { toPath() } returns mockFilePathPath } - val mockMultipartFile = spyk(MockMultipartFile(mockFilePath, mockFileData)) + private val mockMultipartFile = spyk(MockMultipartFile(mockFilePath, mockFileData)) - init { - mockkObject(WrappedFile.Companion) - every { WrappedFile.from(any()) } returns mockFile + @BeforeEach + internal fun beforeEach() { + mockkObject(File.Companion) + every { File.from(any()) } returns mockFile } -} -class FileServiceTest { @AfterEach internal fun afterEach() { clearAllMocks() } + private fun whenFileCached(cached: Boolean = true, test: () -> Unit) { + mockkObject(FileExistCache) { + every { FileExistCache.contains(any()) } returns cached + + test() + } + } + + private fun whenFileNotCached(test: () -> Unit) { + whenFileCached(false, test) + } + + private fun whenMockFilePathExists(exists: Boolean = true, test: () -> Unit) { + every { fileService.exists(mockFilePath) } returns exists + test() + } + // exists() @Test fun `exists() returns true when the file at the given path exists and is a file`() { - test { + whenFileNotCached { assertTrue { fileService.exists(mockFilePath) } + + verify { + mockFile.exists() + mockFile.isFile + } + confirmVerified(mockFile) } } @Test fun `exists() returns false when the file at the given path does not exist`() { - test { + whenFileNotCached { every { mockFile.exists() } returns false assertFalse { fileService.exists(mockFilePath) } + + verify { + mockFile.exists() + } + confirmVerified(mockFile) } } @Test fun `exists() returns false when the file at the given path is not a file`() { - test { + whenFileNotCached { every { mockFile.isFile } returns false assertFalse { fileService.exists(mockFilePath) } + + verify { + mockFile.exists() + mockFile.isFile + } + confirmVerified(mockFile) + } + } + + @Test + fun `exists() returns true when the file at the given path is cached as existing`() { + whenFileCached { + every { FileExistCache.exists(any()) } returns true + + assertTrue { fileService.exists(mockFilePath) } + + verify { + FileExistCache.contains(any()) + FileExistCache.exists(any()) + + mockFile wasNot called + } + confirmVerified(FileExistCache, mockFile) + } + } + + @Test + fun `exists() returns false when the file at the given path is cached as not existing`() { + whenFileCached { + every { FileExistCache.exists(any()) } returns false + + assertFalse { fileService.exists(mockFilePath) } + + verify { + FileExistCache.contains(any()) + FileExistCache.exists(any()) + + mockFile wasNot called + } + confirmVerified(FileExistCache, mockFile) } } @@ -76,39 +143,33 @@ class FileServiceTest { @Test fun `read() returns a valid ByteArrayResource`() { - test { - whenMockFilePathExists { - mockkStatic(File::readBytes) - every { mockFile.readBytes() } returns mockFileData + whenMockFilePathExists { + mockkStatic(File::readBytes) + every { mockFile.readBytes() } returns mockFileData - val redResource = fileService.read(mockFilePath) + val redResource = fileService.read(mockFilePath) - assertEquals(mockFileData, redResource.byteArray) - } + assertEquals(mockFileData, redResource.byteArray) } } @Test fun `read() throws FileNotFoundException when no file exists at the given path`() { - test { - whenMockFilePathExists(false) { - with(assertThrows { fileService.read(mockFilePath) }) { - assertEquals(mockFilePath, this.path) - } + whenMockFilePathExists(false) { + with(assertThrows { fileService.read(mockFilePath) }) { + assertEquals(mockFilePath, this.path) } } } @Test fun `read() throws FileReadException when an IOException is thrown`() { - test { - whenMockFilePathExists { - mockkStatic(File::readBytes) - every { mockFile.readBytes() } throws IOException() + whenMockFilePathExists { + mockkStatic(File::readBytes) + every { mockFile.readBytes() } throws IOException() - with(assertThrows { fileService.read(mockFilePath) }) { - assertEquals(mockFilePath, this.path) - } + with(assertThrows { fileService.read(mockFilePath) }) { + assertEquals(mockFilePath, this.path) } } } @@ -117,15 +178,20 @@ class FileServiceTest { @Test fun `create() creates a file at the given path`() { - test { - whenMockFilePathExists(false) { - mockkStatic(File::create) - every { mockFile.create() } just Runs + whenMockFilePathExists(false) { + whenFileNotCached { + mockkStatic(File::create) { + every { mockFile.create() } just Runs + every { FileExistCache.setExists(any()) } just Runs - fileService.create(mockFilePath) + fileService.create(mockFilePath) - verify { - mockFile.create() + verify { + mockFile.create() + + FileExistCache.setExists(any()) + } + confirmVerified(mockFile, FileExistCache) } } } @@ -133,27 +199,23 @@ class FileServiceTest { @Test fun `create() does nothing when a file already exists at the given path`() { - test { - whenMockFilePathExists { - fileService.create(mockFilePath) + whenMockFilePathExists { + fileService.create(mockFilePath) - verify(exactly = 0) { - mockFile.create() - } + verify(exactly = 0) { + mockFile.create() } } } @Test fun `create() throws FileCreateException when the file creation throws an IOException`() { - test { - whenMockFilePathExists(false) { - mockkStatic(File::create) - every { mockFile.create() } throws IOException() + whenMockFilePathExists(false) { + mockkStatic(File::create) + every { mockFile.create() } throws IOException() - with(assertThrows { fileService.create(mockFilePath) }) { - assertEquals(mockFilePath, this.path) - } + with(assertThrows { fileService.create(mockFilePath) }) { + assertEquals(mockFilePath, this.path) } } } @@ -162,59 +224,51 @@ class FileServiceTest { @Test fun `write() creates and writes the given MultipartFile to the file at the given path`() { - test { - whenMockFilePathExists(false) { - every { fileService.create(mockFilePath) } just Runs - every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs + whenMockFilePathExists(false) { + every { fileService.create(mockFilePath) } just Runs + every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs - fileService.write(mockMultipartFile, mockFilePath, false) + fileService.write(mockMultipartFile, mockFilePath, false) - verify { - fileService.create(mockFilePath) - mockMultipartFile.transferTo(mockFilePathPath) - } + verify { + fileService.create(mockFilePath) + mockMultipartFile.transferTo(mockFilePathPath) } } } @Test fun `write() throws FileExistsException when a file at the given path already exists and overwrite is disabled`() { - test { - whenMockFilePathExists { - with(assertThrows { fileService.write(mockMultipartFile, mockFilePath, false) }) { - assertEquals(mockFilePath, this.path) - } + whenMockFilePathExists { + with(assertThrows { fileService.write(mockMultipartFile, mockFilePath, false) }) { + assertEquals(mockFilePath, this.path) } } } @Test fun `write() writes the given MultipartFile to an existing file when overwrite is enabled`() { - test { - whenMockFilePathExists { - every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs + whenMockFilePathExists { + every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs - fileService.write(mockMultipartFile, mockFilePath, true) + fileService.write(mockMultipartFile, mockFilePath, true) - verify { - mockMultipartFile.transferTo(mockFilePathPath) - } + verify { + mockMultipartFile.transferTo(mockFilePathPath) } } } @Test fun `write() throws FileWriteException when writing the given file throws an IOException`() { - test { - whenMockFilePathExists(false) { - every { fileService.create(mockFilePath) } just Runs - every { mockMultipartFile.transferTo(mockFilePathPath) } throws IOException() + whenMockFilePathExists(false) { + every { fileService.create(mockFilePath) } just Runs + every { mockMultipartFile.transferTo(mockFilePathPath) } throws IOException() - with(assertThrows { - fileService.write(mockMultipartFile, mockFilePath, false) - }) { - assertEquals(mockFilePath, this.path) - } + with(assertThrows { + fileService.write(mockMultipartFile, mockFilePath, false) + }) { + assertEquals(mockFilePath, this.path) } } } @@ -223,35 +277,39 @@ class FileServiceTest { @Test fun `delete() deletes the file at the given path`() { - test { - whenMockFilePathExists { + whenMockFilePathExists { + whenFileCached { every { mockFile.delete() } returns true + every { FileExistCache.setDoesNotExists(any()) } just Runs fileService.delete(mockFilePath) + + verify { + mockFile.delete() + + FileExistCache.setDoesNotExists(any()) + } + confirmVerified(mockFile, FileExistCache) } } } @Test fun `delete() throws FileNotFoundException when no file exists at the given path`() { - test { - whenMockFilePathExists(false) { - with(assertThrows { fileService.delete(mockFilePath) }) { - assertEquals(mockFilePath, this.path) - } + whenMockFilePathExists(false) { + with(assertThrows { fileService.delete(mockFilePath) }) { + assertEquals(mockFilePath, this.path) } } } @Test fun `delete() throws FileDeleteException when deleting throw and IOException`() { - test { - whenMockFilePathExists { - every { mockFile.delete() } throws IOException() + whenMockFilePathExists { + every { mockFile.delete() } throws IOException() - with(assertThrows { fileService.delete(mockFilePath) }) { - assertEquals(mockFilePath, this.path) - } + with(assertThrows { fileService.delete(mockFilePath) }) { + assertEquals(mockFilePath, this.path) } } } @@ -260,37 +318,24 @@ class FileServiceTest { @Test fun `fullPath() appends the given path to the given working directory`() { - test { - with(fileService) { - val fullFilePath = mockFilePath.fullPath() + with(fileService) { + val fullFilePath = mockFilePath.fullPath() - assertEquals("${creProperties.dataDirectory}/$mockFilePath", fullFilePath.path) - } + assertEquals("${creProperties.dataDirectory}/$mockFilePath", fullFilePath.path) } } @Test fun `fullPath() throws InvalidFilePathException when the given path contains invalid fragments`() { - test { - with(fileService) { - BANNED_FILE_PATH_SHARDS.forEach { - val maliciousPath = "$it/$mockFilePath" + with(fileService) { + BANNED_FILE_PATH_SHARDS.forEach { + val maliciousPath = "$it/$mockFilePath" - with(assertThrows { maliciousPath.fullPath() }) { - assertEquals(maliciousPath, this.path) - assertEquals(it, this.fragment) - } + with(assertThrows { maliciousPath.fullPath() }) { + assertEquals(maliciousPath, this.path) + assertEquals(it, this.fragment) } } } } - - private fun test(test: FileServiceTestContext.() -> Unit) { - FileServiceTestContext().test() - } - - private fun FileServiceTestContext.whenMockFilePathExists(exists: Boolean = true, test: () -> Unit) { - every { fileService.exists(mockFilePath) } returns exists - test() - } }