Backup_Repos/komga
1
0
Fork 0
mirror of https://github.com/gotson/komga.git synced 2025-12-06 08:32:25 +01:00
Code Issues Projects Releases Wiki Activity

fix(api): better error handling for read list matching

Browse source
This commit is contained in:
Gauthier Roebroeck 2023-03-07 17:23:54 +08:00
parent 88abfcc733
commit 1961efe890
7 changed files with 434 additions and 152 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
komga/src/main/kotlin/org/gotson/komga/domain/model/Exceptions.kt
View file

@ -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)

8
komga/src/main/kotlin/org/gotson/komga/domain/service/ReadListLifecycle.kt
View file

@ -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())

24
komga/src/main/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProvider.kt
View file

@ -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" } }
}
}

7
komga/src/main/kotlin/org/gotson/komga/interfaces/api/rest/ReadListController.kt
View file

@ -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')")

6
komga/src/test/kotlin/org/gotson/komga/domain/service/ReadListMatcherTest.kt
View file

@ -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<ReadListRequestBook, Map<Series, Collection<Book>>>.mapIds(requestBooks: List<ReadListRequestBook>) = this
.mapKeys { requestBooks.indexOf(it.key) }
.mapValues { (_, v) -> v.mapKeys { it.key.id }.mapValues { it.value.map { book -> book.id } } }
private fun Collection<ReadListRequestBookMatches>.mapIds() = map {
it.matches
.mapKeys { (series, _) -> series.id }

387
komga/src/test/kotlin/org/gotson/komga/infrastructure/metadata/comicrack/ReadListProviderTest.kt
View file

@ -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<XmlMapper>()
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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), ReadingList::class.java) } returns cbl
// when
val request = readListProvider.importFromCbl(ByteArray(0))
// then
assertThat(request).isNull()
}
every { mockMapper.readValue(any<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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<ByteArray>(), 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")
}
}
}

153
komga/src/test/kotlin/org/gotson/komga/interfaces/api/rest/ReadListControllerTest.kt
View file

@ -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 = """
<?xml version="1.0"?>
<ReadingList>
<Name>RL</Name>
<Books>
</Books>
</ReadingList>
""".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 = """
<?xml version="1.0"?>
<ReadingList>
<Name></Name>
<Books>
<Book Series="Civil War" Number="1" Volume="2006" Year="2006"/>
</Books>
</ReadingList>
""".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 = """
<?xml version="1.0"?>
<ReadingList>
<Name>RL</Name>
<Books>
<Book Number="1" Volume="2006" Year="2006"/>
</Books>
</ReadingList>
""".trimIndent()
mockMvc.multipart("/api/v1/readlists/match/comicrack") {
file("file", content.toByteArray())
}
.andExpect {
status {
isBadRequest()
reason("ERR_1031")
}
}
}
}
}