feat(api): readlist books are not always sorted by number

Closes: #1803

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) {
-            is SearchOperator.Is -> field.`in`(inner(searchCondition.operator.value))
-            is SearchOperator.IsNot -> field.notIn(inner(searchCondition.operator.value))
+        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()
           }
-        } to emptySet()
+        }
 
       is SearchCondition.Title ->
         searchCondition.operator.toCondition(Tables.BOOK_METADATA.TITLE) to

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

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

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
             }

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<RequiredJoin.ReadList>().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<RequiredJoin> = emptySet(),
-    joinOnReadList: Boolean = false,
   ): SelectOnConditionStep<Record> {
     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

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)

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)

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

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