From 0dd4b270efb56a4614ff780b8fa729a754a25da6 Mon Sep 17 00:00:00 2001 From: Gauthier Roebroeck Date: Thu, 2 Jan 2025 13:27:19 +0800 Subject: [PATCH] feat(api): readlist books are not always sorted by number Closes: #1803 --- .../infrastructure/jooq/BookSearchHelper.kt | 27 ++++++---- .../komga/infrastructure/jooq/RequiredJoin.kt | 4 ++ .../gotson/komga/infrastructure/jooq/Utils.kt | 2 + .../komga/infrastructure/jooq/main/BookDao.kt | 9 ++++ .../infrastructure/jooq/main/BookDtoDao.kt | 33 ++++++++---- .../infrastructure/jooq/main/SeriesDao.kt | 1 + .../infrastructure/jooq/main/SeriesDtoDao.kt | 3 ++ .../infrastructure/jooq/main/SyncPointDao.kt | 2 + .../jooq/main/BookSearchTest.kt | 50 +++++++++++++++++++ 9 files changed, 110 insertions(+), 21 deletions(-) diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/BookSearchHelper.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/BookSearchHelper.kt index 83e67cf8..a6c0d851 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/BookSearchHelper.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/BookSearchHelper.kt @@ -58,18 +58,23 @@ class BookSearchHelper( is SearchCondition.SeriesId -> searchCondition.operator.toCondition(Tables.BOOK.SERIES_ID) to emptySet() is SearchCondition.ReadListId -> - Tables.BOOK.ID.let { field -> - val inner = { readListId: String -> - DSL - .select(Tables.READLIST_BOOK.BOOK_ID) - .from(Tables.READLIST_BOOK) - .where(Tables.READLIST_BOOK.READLIST_ID.eq(readListId)) + when (searchCondition.operator) { + // for IS condition we have to do a join, so as to order the books by readList number + is SearchOperator.Is -> + Tables.READLIST_BOOK + .`as`("RLB_${searchCondition.operator.value}") + .READLIST_ID + .eq(searchCondition.operator.value) to setOf(RequiredJoin.ReadList(searchCondition.operator.value)) + is SearchOperator.IsNot -> { + val inner = { readListId: String -> + DSL + .select(Tables.READLIST_BOOK.BOOK_ID) + .from(Tables.READLIST_BOOK) + .where(Tables.READLIST_BOOK.READLIST_ID.eq(readListId)) + } + Tables.BOOK.ID.notIn(inner(searchCondition.operator.value)) to emptySet() } - when (searchCondition.operator) { - is SearchOperator.Is -> field.`in`(inner(searchCondition.operator.value)) - is SearchOperator.IsNot -> field.notIn(inner(searchCondition.operator.value)) - } - } to emptySet() + } is SearchCondition.Title -> searchCondition.operator.toCondition(Tables.BOOK_METADATA.TITLE) to diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/RequiredJoin.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/RequiredJoin.kt index 978e75e3..d1443076 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/RequiredJoin.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/RequiredJoin.kt @@ -12,6 +12,10 @@ sealed class RequiredJoin { val userId: String, ) : RequiredJoin() + data class ReadList( + val readListId: String, + ) : RequiredJoin() + data object BookMetadataAggregation : RequiredJoin() data object SeriesMetadata : RequiredJoin() diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/Utils.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/Utils.kt index e8589b66..e576819d 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/Utils.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/Utils.kt @@ -147,3 +147,5 @@ fun ObjectMapper.deserializeMediaExtension( null } } + +fun rlbAlias(readListId: String) = Tables.READLIST_BOOK.`as`("RLB_$readListId") diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDao.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDao.kt index ff3fce7c..aa478c12 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDao.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDao.kt @@ -7,6 +7,7 @@ import org.gotson.komga.domain.persistence.BookRepository import org.gotson.komga.infrastructure.jooq.BookSearchHelper import org.gotson.komga.infrastructure.jooq.RequiredJoin import org.gotson.komga.infrastructure.jooq.insertTempStrings +import org.gotson.komga.infrastructure.jooq.rlbAlias import org.gotson.komga.infrastructure.jooq.selectTempStrings import org.gotson.komga.infrastructure.jooq.toOrderBy import org.gotson.komga.jooq.main.Tables @@ -140,6 +141,10 @@ class BookDao( RequiredJoin.SeriesMetadata -> innerJoin(sd).on(b.SERIES_ID.eq(sd.SERIES_ID)) RequiredJoin.Media -> innerJoin(m).on(b.ID.eq(m.BOOK_ID)) is RequiredJoin.ReadProgress -> leftJoin(r).on(b.ID.eq(r.BOOK_ID)).and(r.USER_ID.eq(join.userId)) + is RequiredJoin.ReadList -> { + val rlbAlias = rlbAlias(join.readListId) + leftJoin(rlbAlias).on(rlbAlias.BOOK_ID.eq(b.ID).and(rlbAlias.READLIST_ID.eq(join.readListId))) + } // shouldn't be required for books RequiredJoin.BookMetadataAggregation -> Unit } @@ -160,6 +165,10 @@ class BookDao( RequiredJoin.SeriesMetadata -> innerJoin(sd).on(b.SERIES_ID.eq(sd.SERIES_ID)) RequiredJoin.Media -> innerJoin(m).on(b.ID.eq(m.BOOK_ID)) is RequiredJoin.ReadProgress -> leftJoin(r).on(b.ID.eq(r.BOOK_ID)).and(r.USER_ID.eq(join.userId)) + is RequiredJoin.ReadList -> { + val rlbAlias = rlbAlias(join.readListId) + leftJoin(rlbAlias).on(rlbAlias.BOOK_ID.eq(b.ID).and(rlbAlias.READLIST_ID.eq(join.readListId))) + } // shouldn't be required for books RequiredJoin.BookMetadataAggregation -> Unit } diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDtoDao.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDtoDao.kt index 44cbf2ee..b72f80bd 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDtoDao.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDtoDao.kt @@ -9,6 +9,7 @@ import org.gotson.komga.infrastructure.jooq.BookSearchHelper import org.gotson.komga.infrastructure.jooq.RequiredJoin import org.gotson.komga.infrastructure.jooq.insertTempStrings import org.gotson.komga.infrastructure.jooq.noCase +import org.gotson.komga.infrastructure.jooq.rlbAlias import org.gotson.komga.infrastructure.jooq.selectTempStrings import org.gotson.komga.infrastructure.jooq.sortByValues import org.gotson.komga.infrastructure.jooq.toCondition @@ -128,10 +129,17 @@ class BookDtoDao( val orderBy = pageable.sort.mapNotNull { - if (it.property == "relevance" && !bookIds.isNullOrEmpty()) + if (it.property == "relevance" && !bookIds.isNullOrEmpty()) { b.ID.sortByValues(bookIds, it.isAscending) - else - it.toSortField(sorts) + } else { + if (it.property == "readList.number") { + val readListId = joins.filterIsInstance().firstOrNull()?.readListId ?: return@mapNotNull null + val f = rlb.`as`("RLB_$readListId").NUMBER + if (it.isAscending) f.asc() else f.desc() + } else { + it.toSortField(sorts) + } + } } val (count, dtos) = @@ -156,6 +164,10 @@ class BookDtoDao( .apply { joins.forEach { join -> when (join) { + is RequiredJoin.ReadList -> { + val rlbAlias = rlbAlias(join.readListId) + leftJoin(rlbAlias).on(rlbAlias.BOOK_ID.eq(b.ID).and(rlbAlias.READLIST_ID.eq(join.readListId))) + } // always joined RequiredJoin.BookMetadata -> Unit RequiredJoin.Media -> Unit @@ -171,7 +183,7 @@ class BookDtoDao( ) val dtos = - selectBase(userId, joins, pageable.sort.any { it.property == "readList.number" }) + selectBase(userId, joins) .where(conditions) .and(searchCondition) .orderBy(orderBy) @@ -334,11 +346,10 @@ class BookDtoDao( .apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } } .fetchOne(rlb.NUMBER) - return selectBase(userId, joinOnReadList = true) - .where(rlb.READLIST_ID.eq(readList.id)) + return selectBase(userId, setOf(RequiredJoin.ReadList(readList.id))) .apply { if (restrictions.isRestricted) and(restrictions.toCondition()) } .apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } } - .orderBy(rlb.NUMBER.let { if (next) it.asc() else it.desc() }) + .orderBy(rlbAlias(readList.id).NUMBER.let { if (next) it.asc() else it.desc() }) .seek(numberSort) .limit(1) .fetchAndMap() @@ -377,7 +388,6 @@ class BookDtoDao( private fun selectBase( userId: String, joins: Set = emptySet(), - joinOnReadList: Boolean = false, ): SelectOnConditionStep { val selectFields = listOf( @@ -389,7 +399,7 @@ class BookDtoDao( ) return dsl - .let { if (joinOnReadList) it.selectDistinct(selectFields) else it.select(selectFields) } + .select(selectFields) .from(b) .leftJoin(m) .on(b.ID.eq(m.BOOK_ID)) @@ -401,9 +411,12 @@ class BookDtoDao( .leftJoin(sd) .on(b.SERIES_ID.eq(sd.SERIES_ID)) .apply { - if (joinOnReadList) leftJoin(rlb).on(b.ID.eq(rlb.BOOK_ID)) joins.forEach { join -> when (join) { + is RequiredJoin.ReadList -> { + val rlbAlias = rlbAlias(join.readListId) + leftJoin(rlbAlias).on(rlbAlias.BOOK_ID.eq(b.ID).and(rlbAlias.READLIST_ID.eq(join.readListId))) + } // always joined RequiredJoin.BookMetadata -> Unit RequiredJoin.Media -> Unit diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDao.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDao.kt index cf15c7f8..92666787 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDao.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDao.kt @@ -137,6 +137,7 @@ class SeriesDao( // Book joins - not needed RequiredJoin.BookMetadata -> Unit RequiredJoin.Media -> Unit + is RequiredJoin.ReadList -> Unit } } }.where(conditions) diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDtoDao.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDtoDao.kt index 6af28e4a..be095421 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDtoDao.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDtoDao.kt @@ -163,6 +163,7 @@ class SeriesDtoDao( RequiredJoin.Media -> Unit RequiredJoin.BookMetadata -> Unit RequiredJoin.BookMetadataAggregation -> Unit + is RequiredJoin.ReadList -> Unit } } }.where(conditionsRefined) @@ -209,6 +210,7 @@ class SeriesDtoDao( RequiredJoin.BookMetadata -> Unit RequiredJoin.BookMetadataAggregation -> Unit RequiredJoin.Media -> Unit + is RequiredJoin.ReadList -> Unit } } } @@ -244,6 +246,7 @@ class SeriesDtoDao( RequiredJoin.BookMetadata -> Unit RequiredJoin.BookMetadataAggregation -> Unit RequiredJoin.Media -> Unit + is RequiredJoin.ReadList -> Unit } } }.where(conditions) diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SyncPointDao.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SyncPointDao.kt index 7fa499d4..8eec1ba8 100644 --- a/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SyncPointDao.kt +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SyncPointDao.kt @@ -100,6 +100,8 @@ class SyncPointDao( .apply { joins.forEach { when (it) { + // for future work + is RequiredJoin.ReadList -> Unit // we don't have to handle those since we already join on those tables anyway, the 'when' is here for future proofing RequiredJoin.BookMetadata -> Unit RequiredJoin.SeriesMetadata -> Unit diff --git a/komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/main/BookSearchTest.kt b/komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/main/BookSearchTest.kt index bfb4c46f..0b4d9bd4 100644 --- a/komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/main/BookSearchTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/main/BookSearchTest.kt @@ -189,6 +189,56 @@ class BookSearchTest( } } + @Test + fun `given some books in multiple read lists when searching by read list then results are accurate`() { + val book1 = makeBook("1", libraryId = library1.id, seriesId = series1.id) + val book2 = makeBook("2", libraryId = library2.id, seriesId = series2.id) + seriesLifecycle.addBooks(series1, listOf(book1)) + seriesLifecycle.addBooks(series2, listOf(book2)) + val readList1 = ReadList("rl1", bookIds = mapOf(1 to book1.id, 2 to book2.id).toSortedMap()) + val readList2 = ReadList("rl2", bookIds = mapOf(1 to book2.id, 2 to book1.id).toSortedMap()) + readListRepository.insert(readList1) + readListRepository.insert(readList2) + + // search by readList 1 + run { + val search = BookSearch(SearchCondition.ReadListId(SearchOperator.Is(readList1.id))) + val found = bookDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + val foundDto = bookDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + + // order not guaranteed for bookDao + assertThat(found.map { it.name }).containsExactlyInAnyOrder("1", "2") + assertThat(foundDto.map { it.name }).containsExactly("1", "2") + } + + // search by readList 2 + run { + val search = BookSearch(SearchCondition.ReadListId(SearchOperator.Is(readList2.id))) + val found = bookDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + val foundDto = bookDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + + // order not guaranteed for bookDao + assertThat(found.map { it.name }).containsExactlyInAnyOrder("2", "1") + assertThat(foundDto.map { it.name }).containsExactly("2", "1") + } + + // search by readList 1 or 2 - order is not guaranteed in that case + run { + val search = + BookSearch( + SearchCondition.AnyOfBook( + SearchCondition.ReadListId(SearchOperator.Is(readList1.id)), + SearchCondition.ReadListId(SearchOperator.Is(readList2.id)), + ), + ) + val found = bookDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + val foundDto = bookDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by(Sort.Order.asc("readList.number")))).content + + assertThat(found.map { it.name }).containsExactlyInAnyOrder("2", "1") + assertThat(foundDto.map { it.name }).containsExactlyInAnyOrder("2", "1") + } + } + @Test fun `given some books when searching by deleted then results are accurate`() { val book1 = makeBook("1", libraryId = library1.id, seriesId = series1.id).copy(deletedDate = LocalDateTime.now())