diff --git a/komga/src/main/kotlin/org/gotson/komga/domain/model/Exceptions.kt b/komga/src/main/kotlin/org/gotson/komga/domain/model/Exceptions.kt index c3f5a18bb..312076317 100644 --- a/komga/src/main/kotlin/org/gotson/komga/domain/model/Exceptions.kt +++ b/komga/src/main/kotlin/org/gotson/komga/domain/model/Exceptions.kt @@ -22,3 +22,4 @@ class DuplicateNameException(message: String, code: String = "") : CodedExceptio class PathContainedInPath(message: String, code: String = "") : CodedException(message, code) class UserEmailAlreadyExistsException(message: String, code: String = "") : CodedException(message, code) class BookConversionException(message: String) : Exception(message) +class ComicRackListException(message: String, code: String = "") : CodedException(message, code) diff --git a/komga/src/main/kotlin/org/gotson/komga/domain/service/ReadListLifecycle.kt b/komga/src/main/kotlin/org/gotson/komga/domain/service/ReadListLifecycle.kt index aaea91469..9e661092d 100644 --- a/komga/src/main/kotlin/org/gotson/komga/domain/service/ReadListLifecycle.kt +++ b/komga/src/main/kotlin/org/gotson/komga/domain/service/ReadListLifecycle.kt @@ -5,7 +5,6 @@ import org.gotson.komga.application.events.EventPublisher import org.gotson.komga.domain.model.DomainEvent import org.gotson.komga.domain.model.DuplicateNameException import org.gotson.komga.domain.model.ReadList -import org.gotson.komga.domain.model.ReadListMatch import org.gotson.komga.domain.model.ReadListRequestMatch import org.gotson.komga.domain.model.ReadListRequestResult import org.gotson.komga.domain.model.ThumbnailReadList @@ -140,11 +139,7 @@ class ReadListLifecycle( } fun matchComicRackList(fileContent: ByteArray): ReadListRequestMatch { - val request = try { - readListProvider.importFromCbl(fileContent) ?: return ReadListRequestMatch(ReadListMatch(""), emptyList(), "ERR_1015") - } catch (e: Exception) { - return ReadListRequestMatch(ReadListMatch(""), emptyList(), "ERR_1015") - } + val request = readListProvider.importFromCblV2(fileContent) return readListMatcher.matchReadListRequest(request) } @@ -159,6 +154,7 @@ class ReadListLifecycle( logger.info { "More than one thumbnail is selected, removing extra ones" } thumbnailReadListRepository.markSelected(selected[0]) } + selected.isEmpty() && all.isNotEmpty() -> { logger.info { "Read list has no selected thumbnail, choosing one automatically" } thumbnailReadListRepository.markSelected(all.first()) diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProvider.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProvider.kt index 05b20d6e2..0fb214984 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProvider.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProvider.kt @@ -2,6 +2,7 @@ package org.gotson.komga.infrastructure.metadata.comicrack import com.fasterxml.jackson.dataformat.xml.XmlMapper import mu.KotlinLogging +import org.gotson.komga.domain.model.ComicRackListException import org.gotson.komga.domain.model.ReadListRequest import org.gotson.komga.domain.model.ReadListRequestBook import org.gotson.komga.infrastructure.metadata.comicrack.dto.ReadingList @@ -45,4 +46,27 @@ class ReadListProvider( return null } + + @Throws(ComicRackListException::class) + fun importFromCblV2(cbl: ByteArray): ReadListRequest { + val readingList = try { + mapper.readValue(cbl, ReadingList::class.java) + } catch (e: Exception) { + logger.error(e) { "Error while trying to parse ComicRack ReadingList" } + throw ComicRackListException("Error while trying to parse ComicRack ReadingList", "ERR_1015") + } + logger.debug { "Trying to convert ComicRack ReadingList to ReadListRequest: $readingList" } + + if (readingList.name.isNullOrBlank()) throw ComicRackListException("ReadingList has no Name element", "ERR_1030") + if (readingList.books.isEmpty()) throw ComicRackListException("ReadingList does not contain any Book element", "ERR_1029") + + val books = readingList.books.map { + val series = computeSeriesFromSeriesAndVolume(it.series, it.volume) + if (series.isNullOrBlank() || it.number == null) throw ComicRackListException("Book is missing series or number: $it", "ERR_1031") + else ReadListRequestBook(series, it.number!!.trim()) + } + + return ReadListRequest(name = readingList.name!!, books = books) + .also { logger.debug { "Converted request: $it" } } + } } diff --git a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ReadListController.kt b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ReadListController.kt index f2ab9fad2..274da1e59 100644 --- a/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ReadListController.kt +++ b/komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ReadListController.kt @@ -12,6 +12,7 @@ import org.apache.commons.io.IOUtils import org.gotson.komga.application.events.EventPublisher import org.gotson.komga.domain.model.Author import org.gotson.komga.domain.model.BookSearchWithReadProgress +import org.gotson.komga.domain.model.CodedException import org.gotson.komga.domain.model.DomainEvent import org.gotson.komga.domain.model.DuplicateNameException import org.gotson.komga.domain.model.Media @@ -254,7 +255,11 @@ class ReadListController( fun matchFromComicRackList( @RequestParam("file") file: MultipartFile, ): ReadListRequestMatchDto = - readListLifecycle.matchComicRackList(file.bytes).toDto() + try { + readListLifecycle.matchComicRackList(file.bytes).toDto() + } catch (e: CodedException) { + throw ResponseStatusException(HttpStatus.BAD_REQUEST, e.code) + } @PatchMapping("{id}") @PreAuthorize("hasRole('$ROLE_ADMIN')") diff --git a/komga/src/test/kotlin/org/gotson/komga/domain/service/ReadListMatcherTest.kt b/komga/src/test/kotlin/org/gotson/komga/domain/service/ReadListMatcherTest.kt index fca0eac08..cd56b8210 100644 --- a/komga/src/test/kotlin/org/gotson/komga/domain/service/ReadListMatcherTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/domain/service/ReadListMatcherTest.kt @@ -6,12 +6,10 @@ import io.mockk.every import io.mockk.just import org.assertj.core.api.Assertions.assertThat import org.gotson.komga.application.tasks.TaskEmitter -import org.gotson.komga.domain.model.Book import org.gotson.komga.domain.model.ReadList import org.gotson.komga.domain.model.ReadListRequest import org.gotson.komga.domain.model.ReadListRequestBook import org.gotson.komga.domain.model.ReadListRequestBookMatches -import org.gotson.komga.domain.model.Series import org.gotson.komga.domain.model.makeBook import org.gotson.komga.domain.model.makeLibrary import org.gotson.komga.domain.model.makeSeries @@ -278,10 +276,6 @@ class ReadListMatcherTest( @Nested inner class Match { - private fun Map>>.mapIds(requestBooks: List) = this - .mapKeys { requestBooks.indexOf(it.key) } - .mapValues { (_, v) -> v.mapKeys { it.key.id }.mapValues { it.value.map { book -> book.id } } } - private fun Collection.mapIds() = map { it.matches .mapKeys { (series, _) -> series.id } diff --git a/komga/src/test/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProviderTest.kt b/komga/src/test/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProviderTest.kt index 706aeec45..09f92f236 100644 --- a/komga/src/test/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProviderTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProviderTest.kt @@ -4,8 +4,11 @@ import com.fasterxml.jackson.dataformat.xml.XmlMapper import io.mockk.every import io.mockk.mockk import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.catchThrowable +import org.gotson.komga.domain.model.ComicRackListException import org.gotson.komga.infrastructure.metadata.comicrack.dto.Book import org.gotson.komga.infrastructure.metadata.comicrack.dto.ReadingList +import org.junit.jupiter.api.Nested import org.junit.jupiter.api.Test class ReadListProviderTest { @@ -13,146 +16,300 @@ class ReadListProviderTest { private val mockMapper = mockk() private val readListProvider = ReadListProvider(mockMapper) - @Test - fun `given CBL list with books when getting ReadListRequest then it is valid`() { - // given - val cbl = ReadingList().apply { - name = "my read list" - books = listOf( - Book().apply { - series = "series 1" - number = " 4 " - volume = 2005 - }, - Book().apply { - series = "series 2" - number = "1" - }, - ) - } - every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl - - // when - val request = readListProvider.importFromCbl(ByteArray(0)) - - // then - assertThat(request).isNotNull - with(request!!) { - assertThat(name).isEqualTo(cbl.name) - assertThat(books).hasSize(2) - - with(books[0]) { - assertThat(series).isEqualTo("series 1 (2005)") - assertThat(number).isEqualTo("4") + @Nested + inner class ImportFromCbl { + @Test + fun `given CBL list with books when getting ReadListRequest then it is valid`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = listOf( + Book().apply { + series = "series 1" + number = " 4 " + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) } - with(books[1]) { - assertThat(series).isEqualTo("series 2") - assertThat(number).isEqualTo("1") + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val request = readListProvider.importFromCbl(ByteArray(0)) + + // then + assertThat(request).isNotNull + with(request!!) { + assertThat(name).isEqualTo(cbl.name) + assertThat(books).hasSize(2) + + with(books[0]) { + assertThat(series).isEqualTo("series 1 (2005)") + assertThat(number).isEqualTo("4") + } + + with(books[1]) { + assertThat(series).isEqualTo("series 2") + assertThat(number).isEqualTo("1") + } } } - } - @Test - fun `given CBL list with invalid books when getting ReadListRequest then it is null`() { - // given - val cbl = ReadingList().apply { - name = "my read list" - books = listOf( - Book().apply { - series = " " - number = "4" - volume = 2005 - }, - Book().apply { - series = null - number = "1" - }, - Book().apply { - series = "Series" - number = null - }, - ) + @Test + fun `given CBL list with invalid books when getting ReadListRequest then it is null`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = listOf( + Book().apply { + series = " " + number = "4" + volume = 2005 + }, + Book().apply { + series = null + number = "1" + }, + Book().apply { + series = "Series" + number = null + }, + ) + } + + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val request = readListProvider.importFromCbl(ByteArray(0)) + + // then + assertThat(request).isNull() } - every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + @Test + fun `given CBL list without books when getting ReadListRequest then it is null`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = emptyList() + } - // when - val request = readListProvider.importFromCbl(ByteArray(0)) + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl - // then - assertThat(request).isNull() - } + // when + val request = readListProvider.importFromCbl(ByteArray(0)) - @Test - fun `given CBL list without books when getting ReadListRequest then it is null`() { - // given - val cbl = ReadingList().apply { - name = "my read list" - books = emptyList() + // then + assertThat(request).isNull() } - every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + @Test + fun `given CBL list without name when getting ReadListRequest then it is null`() { + // given + val cbl = ReadingList().apply { + name = null + books = listOf( + Book().apply { + series = "series 1" + number = "4" + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) + } - // when - val request = readListProvider.importFromCbl(ByteArray(0)) + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl - // then - assertThat(request).isNull() - } + // when + val request = readListProvider.importFromCbl(ByteArray(0)) - @Test - fun `given CBL list without name when getting ReadListRequest then it is null`() { - // given - val cbl = ReadingList().apply { - name = null - books = listOf( - Book().apply { - series = "series 1" - number = "4" - volume = 2005 - }, - Book().apply { - series = "series 2" - number = "1" - }, - ) + // then + assertThat(request).isNull() } - every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + @Test + fun `given CBL list with blank name when getting ReadListRequest then it is null`() { + // given + val cbl = ReadingList().apply { + name = " " + books = listOf( + Book().apply { + series = "series 1" + number = "4" + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) + } - // when - val request = readListProvider.importFromCbl(ByteArray(0)) + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl - // then - assertThat(request).isNull() + // when + val request = readListProvider.importFromCbl(ByteArray(0)) + + // then + assertThat(request).isNull() + } } - @Test - fun `given CBL list with blank name when getting ReadListRequest then it is null`() { - // given - val cbl = ReadingList().apply { - name = " " - books = listOf( - Book().apply { - series = "series 1" - number = "4" - volume = 2005 - }, - Book().apply { - series = "series 2" - number = "1" - }, - ) + @Nested + inner class ImportFromCblV2 { + @Test + fun `given CBL list with books when getting ReadListRequest then it is valid`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = listOf( + Book().apply { + series = "series 1" + number = " 4 " + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) + } + + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val request = readListProvider.importFromCblV2(ByteArray(0)) + + // then + assertThat(request).isNotNull + with(request!!) { + assertThat(name).isEqualTo(cbl.name) + assertThat(books).hasSize(2) + + with(books[0]) { + assertThat(series).isEqualTo("series 1 (2005)") + assertThat(number).isEqualTo("4") + } + + with(books[1]) { + assertThat(series).isEqualTo("series 2") + assertThat(number).isEqualTo("1") + } + } } - every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + @Test + fun `given CBL list with invalid books when getting ReadListRequest then exception is thrown`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = listOf( + Book().apply { + series = " " + number = "4" + volume = 2005 + }, + Book().apply { + series = null + number = "1" + }, + Book().apply { + series = "Series" + number = null + }, + ) + } - // when - val request = readListProvider.importFromCbl(ByteArray(0)) + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl - // then - assertThat(request).isNull() + // when + val thrown = catchThrowable { readListProvider.importFromCblV2(ByteArray(0)) } + + // then + assertThat(thrown).isInstanceOf(ComicRackListException::class.java) + assertThat(thrown).hasFieldOrPropertyWithValue("code", "ERR_1031") + } + + @Test + fun `given CBL list without books when getting ReadListRequest then exception is thrown`() { + // given + val cbl = ReadingList().apply { + name = "my read list" + books = emptyList() + } + + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val thrown = catchThrowable { readListProvider.importFromCblV2(ByteArray(0)) } + + // then + assertThat(thrown).isInstanceOf(ComicRackListException::class.java) + assertThat(thrown).hasFieldOrPropertyWithValue("code", "ERR_1029") + } + + @Test + fun `given CBL list without name when getting ReadListRequest then exception is thrown`() { + // given + val cbl = ReadingList().apply { + name = null + books = listOf( + Book().apply { + series = "series 1" + number = "4" + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) + } + + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val thrown = catchThrowable { readListProvider.importFromCblV2(ByteArray(0)) } + + // then + assertThat(thrown).isInstanceOf(ComicRackListException::class.java) + assertThat(thrown).hasFieldOrPropertyWithValue("code", "ERR_1030") + } + + @Test + fun `given CBL list with blank name when getting ReadListRequest then exception is thrown`() { + // given + val cbl = ReadingList().apply { + name = " " + books = listOf( + Book().apply { + series = "series 1" + number = "4" + volume = 2005 + }, + Book().apply { + series = "series 2" + number = "1" + }, + ) + } + + every { mockMapper.readValue(any(), ReadingList::class.java) } returns cbl + + // when + val thrown = catchThrowable { readListProvider.importFromCblV2(ByteArray(0)) } + + // then + assertThat(thrown).isInstanceOf(ComicRackListException::class.java) + assertThat(thrown).hasFieldOrPropertyWithValue("code", "ERR_1030") + } } } diff --git a/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ReadListControllerTest.kt b/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ReadListControllerTest.kt index c0c9c4261..558a46a07 100644 --- a/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ReadListControllerTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ReadListControllerTest.kt @@ -31,6 +31,7 @@ import org.springframework.test.context.junit.jupiter.SpringExtension import org.springframework.test.web.servlet.MockMvc import org.springframework.test.web.servlet.delete import org.springframework.test.web.servlet.get +import org.springframework.test.web.servlet.multipart import org.springframework.test.web.servlet.patch import org.springframework.test.web.servlet.post import java.net.URLEncoder @@ -1041,32 +1042,35 @@ class ReadListControllerTest( } } - @Test - @WithMockCustomUser - fun `given readlist with Unicode name when getting readlist file then attachment name is correct`() { - val name = "アキラ" - val tempFile = Files.createTempFile(name, ".cbz") - .also { it.toFile().deleteOnExit() } - val book = makeBook(name, libraryId = library1.id, url = tempFile.toUri().toURL()) - makeSeries(name = "series", libraryId = library1.id).let { series -> - seriesLifecycle.createSeries(series).let { created -> - val books = listOf(book) - seriesLifecycle.addBooks(created, books) + @Nested + inner class FileDownload { + @Test + @WithMockCustomUser + fun `given readlist with Unicode name when getting readlist file then attachment name is correct`() { + val name = "アキラ" + val tempFile = Files.createTempFile(name, ".cbz") + .also { it.toFile().deleteOnExit() } + val book = makeBook(name, libraryId = library1.id, url = tempFile.toUri().toURL()) + makeSeries(name = "series", libraryId = library1.id).let { series -> + seriesLifecycle.createSeries(series).let { created -> + val books = listOf(book) + seriesLifecycle.addBooks(created, books) + } } + + val readlist = readListLifecycle.addReadList( + ReadList( + name = name, + bookIds = listOf(book.id).toIndexedMap(), + ), + ) + + mockMvc.get("/api/v1/readlists/${readlist.id}/file") + .andExpect { + status { isOk() } + header { string("Content-Disposition", Matchers.containsString(URLEncoder.encode(name, StandardCharsets.UTF_8.name()))) } + } } - - val readlist = readListLifecycle.addReadList( - ReadList( - name = name, - bookIds = listOf(book.id).toIndexedMap(), - ), - ) - - mockMvc.get("/api/v1/readlists/${readlist.id}/file") - .andExpect { - status { isOk() } - header { string("Content-Disposition", Matchers.containsString(URLEncoder.encode(name, StandardCharsets.UTF_8.name()))) } - } } @Nested @@ -1238,4 +1242,105 @@ class ReadListControllerTest( .andExpect { status { isNotFound() } } } } + + @Nested + inner class Match { + @Test + @WithMockCustomUser + fun `given non-admin user when matching cbl file then return forbidden`() { + mockMvc.multipart("/api/v1/readlists/match/comicrack") { + file("file", byteArrayOf()) + } + .andExpect { + status { isForbidden() } + } + } + + @Test + @WithMockCustomUser(roles = [ROLE_ADMIN]) + fun `given invalid cbl file when matching then return bad request`() { + val content = "garbled" + + mockMvc.multipart("/api/v1/readlists/match/comicrack") { + file("file", content.toByteArray()) + } + .andExpect { + status { + isBadRequest() + reason("ERR_1015") + } + } + } + + @Test + @WithMockCustomUser(roles = [ROLE_ADMIN]) + fun `given cbl file without books when matching then return bad request`() { + val content = """ + + + RL + + + + """.trimIndent() + + mockMvc.multipart("/api/v1/readlists/match/comicrack") { + file("file", content.toByteArray()) + } + .andExpect { + status { + isBadRequest() + reason("ERR_1029") + } + } + } + + @Test + @WithMockCustomUser(roles = [ROLE_ADMIN]) + fun `given cbl file without name when matching then return bad request`() { + val content = """ + + + + + + + + """.trimIndent() + + mockMvc.multipart("/api/v1/readlists/match/comicrack") { + file("file", content.toByteArray()) + } + .andExpect { + status { + isBadRequest() + reason("ERR_1030") + } + } + } + + @Test + @WithMockCustomUser(roles = [ROLE_ADMIN]) + fun `given cbl file with book without series when matching then return bad request`() { + val content = """ + + + RL + + + + + """.trimIndent() + + mockMvc.multipart("/api/v1/readlists/match/comicrack") { + file("file", content.toByteArray()) + } + .andExpect { + status { + isBadRequest() + reason("ERR_1031") + } + } + } + } }