Skip to content

Commit

Permalink
fi(api): collection series are not always sorted by number
Browse files Browse the repository at this point in the history
  • Loading branch information
@gotson
gotson committed Jan 2, 2025
1 parent 274ac6a commit 39a054b
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 17 deletions.
4 changes: 4 additions & 0 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/RequiredJoin.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ sealed class RequiredJoin {
val readListId: String,
) : RequiredJoin()

data class Collection(
val collectionId: String,
) : RequiredJoin()

data object BookMetadataAggregation : RequiredJoin()

data object SeriesMetadata : RequiredJoin()
Expand Down
26 changes: 15 additions & 11 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/SeriesSearchHelper.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,18 +169,22 @@ class SeriesSearchHelper(
is SearchCondition.AgeRating -> searchCondition.operator.toCondition(Tables.SERIES_METADATA.AGE_RATING) to setOf(RequiredJoin.SeriesMetadata)

is SearchCondition.CollectionId ->
Tables.SERIES.ID.let { field ->
val inner = { collectionId: String ->
DSL
.select(Tables.COLLECTION_SERIES.SERIES_ID)
.from(Tables.COLLECTION_SERIES)
.where(Tables.COLLECTION_SERIES.COLLECTION_ID.eq(collectionId))
}
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 series by collection number
is SearchOperator.Is ->
csAlias(searchCondition.operator.value)
.COLLECTION_ID
.eq(searchCondition.operator.value) to setOf(RequiredJoin.Collection(searchCondition.operator.value))
is SearchOperator.IsNot -> {
val inner = { collectionId: String ->
DSL
.select(Tables.COLLECTION_SERIES.SERIES_ID)
.from(Tables.COLLECTION_SERIES)
.where(Tables.COLLECTION_SERIES.COLLECTION_ID.eq(collectionId))
}
Tables.SERIES.ID.notIn(inner(searchCondition.operator.value)) to emptySet()
}
} to emptySet()
}

is SearchCondition.Complete ->
Tables.SERIES_METADATA.TOTAL_BOOK_COUNT.let { field ->
Expand Down
2 changes: 2 additions & 0 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/Utils.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,5 @@ fun ObjectMapper.deserializeMediaExtension(
}

fun rlbAlias(readListId: String) = Tables.READLIST_BOOK.`as`("RLB_$readListId")

fun csAlias(collectionId: String) = Tables.COLLECTION_SERIES.`as`("CS_$collectionId")
2 changes: 2 additions & 0 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class BookDao(
}
// shouldn't be required for books
RequiredJoin.BookMetadataAggregation -> Unit
is RequiredJoin.Collection -> Unit
}
}
}.where(conditions)
Expand All @@ -171,6 +172,7 @@ class BookDao(
}
// shouldn't be required for books
RequiredJoin.BookMetadataAggregation -> Unit
is RequiredJoin.Collection -> Unit
}
}
}.where(conditions)
Expand Down
2 changes: 2 additions & 0 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/BookDtoDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ class BookDtoDao(
// Series joins - not needed
RequiredJoin.BookMetadataAggregation -> Unit
RequiredJoin.SeriesMetadata -> Unit
is RequiredJoin.Collection -> Unit
}
}
}.where(conditions)
Expand Down Expand Up @@ -424,6 +425,7 @@ class BookDtoDao(
// Series joins - not needed
RequiredJoin.BookMetadataAggregation -> Unit
RequiredJoin.SeriesMetadata -> Unit
is RequiredJoin.Collection -> Unit
}
}
}
Expand Down
6 changes: 5 additions & 1 deletion komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.gotson.komga.domain.model.Series
import org.gotson.komga.domain.persistence.SeriesRepository
import org.gotson.komga.infrastructure.jooq.RequiredJoin
import org.gotson.komga.infrastructure.jooq.SeriesSearchHelper
import org.gotson.komga.infrastructure.jooq.csAlias
import org.gotson.komga.infrastructure.jooq.insertTempStrings
import org.gotson.komga.infrastructure.jooq.selectTempStrings
import org.gotson.komga.jooq.main.Tables
Expand Down Expand Up @@ -34,7 +35,6 @@ class SeriesDao(
private val s = Tables.SERIES
private val d = Tables.SERIES_METADATA
private val rs = Tables.READ_PROGRESS_SERIES
private val cs = Tables.COLLECTION_SERIES
private val bma = Tables.BOOK_METADATA_AGGREGATION

override fun findAll(): Collection<Series> =
Expand Down Expand Up @@ -131,6 +131,10 @@ class SeriesDao(
.apply {
joins.forEach { join ->
when (join) {
is RequiredJoin.Collection -> {
val csAlias = csAlias(join.collectionId)
leftJoin(csAlias).on(s.ID.eq(csAlias.SERIES_ID).and(csAlias.COLLECTION_ID.eq(join.collectionId)))
}
RequiredJoin.BookMetadataAggregation -> leftJoin(bma).on(s.ID.eq(bma.SERIES_ID))
RequiredJoin.SeriesMetadata -> innerJoin(d).on(s.ID.eq(d.SERIES_ID))
is RequiredJoin.ReadProgress -> leftJoin(rs).on(rs.SERIES_ID.eq(s.ID)).and(rs.USER_ID.eq(join.userId))
Expand Down
29 changes: 24 additions & 5 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesDtoDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import org.gotson.komga.domain.model.SeriesSearch
import org.gotson.komga.infrastructure.datasource.SqliteUdfDataSource
import org.gotson.komga.infrastructure.jooq.RequiredJoin
import org.gotson.komga.infrastructure.jooq.SeriesSearchHelper
import org.gotson.komga.infrastructure.jooq.csAlias
import org.gotson.komga.infrastructure.jooq.inOrNoCondition
import org.gotson.komga.infrastructure.jooq.insertTempStrings
import org.gotson.komga.infrastructure.jooq.noCase
Expand Down Expand Up @@ -156,6 +157,10 @@ class SeriesDtoDao(
.apply {
joins.forEach { join ->
when (join) {
is RequiredJoin.Collection -> {
val csAlias = csAlias(join.collectionId)
leftJoin(csAlias).on(s.ID.eq(csAlias.SERIES_ID).and(csAlias.COLLECTION_ID.eq(join.collectionId)))
}
// always joined
is RequiredJoin.ReadProgress -> Unit
RequiredJoin.SeriesMetadata -> Unit
Expand Down Expand Up @@ -190,7 +195,7 @@ class SeriesDtoDao(
joinOnCollection: Boolean = false,
): SelectOnConditionStep<Record> =
dsl
.let { if (joinOnCollection) it.selectDistinct(*groupFields) else it.select(*groupFields) }
.select(*groupFields)
.from(s)
.leftJoin(d)
.on(s.ID.eq(d.SERIES_ID))
Expand All @@ -200,9 +205,12 @@ class SeriesDtoDao(
.on(s.ID.eq(rs.SERIES_ID))
.and(readProgressConditionSeries(userId))
.apply {
if (joinOnCollection)leftJoin(cs).on(s.ID.eq(cs.SERIES_ID))
joins.forEach { join ->
when (join) {
is RequiredJoin.Collection -> {
val csAlias = csAlias(join.collectionId)
leftJoin(csAlias).on(s.ID.eq(csAlias.SERIES_ID).and(csAlias.COLLECTION_ID.eq(join.collectionId)))
}
// always joined
is RequiredJoin.ReadProgress -> Unit
RequiredJoin.SeriesMetadata -> Unit
Expand Down Expand Up @@ -239,6 +247,10 @@ class SeriesDtoDao(
.apply {
joins.forEach { join ->
when (join) {
is RequiredJoin.Collection -> {
val csAlias = csAlias(join.collectionId)
leftJoin(csAlias).on(s.ID.eq(csAlias.SERIES_ID).and(csAlias.COLLECTION_ID.eq(join.collectionId)))
}
// always joined
is RequiredJoin.ReadProgress -> Unit
RequiredJoin.SeriesMetadata -> Unit
Expand All @@ -255,10 +267,17 @@ class SeriesDtoDao(

val orderBy =
pageable.sort.mapNotNull {
if (it.property == "relevance" && !seriesIds.isNullOrEmpty())
if (it.property == "relevance" && !seriesIds.isNullOrEmpty()) {
s.ID.sortByValues(seriesIds, it.isAscending)
else
it.toSortField(sorts)
} else {
if (it.property == "collection.number") {
val collectionId = joins.filterIsInstance<RequiredJoin.Collection>().firstOrNull()?.collectionId ?: return@mapNotNull null
val f = csAlias(collectionId).NUMBER
if (it.isAscending) f.asc() else f.desc()
} else {
it.toSortField(sorts)
}
}
}

val dtos =
Expand Down
1 change: 1 addition & 0 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/main/SyncPointDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class SyncPointDao(
RequiredJoin.Media -> Unit
is RequiredJoin.ReadProgress -> Unit
RequiredJoin.BookMetadataAggregation -> Unit
is RequiredJoin.Collection -> Unit
}
}
}.join(m)
Expand Down
48 changes: 48 additions & 0 deletions komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/main/SeriesSearchTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,54 @@ class SeriesSearchTest(
}
}

@Test
fun `given some series in multiple collections when searching by collection then results are accurate`() {
val series1 = makeSeries("1", library1.id).also { seriesLifecycle.createSeries(it) }
val series2 = makeSeries("2", library1.id).also { seriesLifecycle.createSeries(it) }
val series3 = makeSeries("3", library1.id).also { seriesLifecycle.createSeries(it) }
val collection1 = SeriesCollection("col1", seriesIds = listOf(series1.id, series2.id, series3.id), ordered = true)
val collection2 = SeriesCollection("col2", seriesIds = listOf(series3.id, series2.id, series1.id), ordered = true)
collectionRepository.insert(collection1)
collectionRepository.insert(collection2)

// search by collection 1
run {
val search = SeriesSearch(SearchCondition.CollectionId(SearchOperator.Is(collection1.id)))
val found = seriesDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content
val foundDto = seriesDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content

// order not guaranteed for seriesDao
assertThat(found.map { it.name }).containsExactlyInAnyOrder("1", "2", "3")
assertThat(foundDto.map { it.name }).containsExactly("1", "2", "3")
}

// search by collection 2
run {
val search = SeriesSearch(SearchCondition.CollectionId(SearchOperator.Is(collection2.id)))
val found = seriesDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content
val foundDto = seriesDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content

// order not guaranteed for seriesDao
assertThat(found.map { it.name }).containsExactlyInAnyOrder("3", "2", "1")
assertThat(foundDto.map { it.name }).containsExactly("3", "2", "1")
}
// search by collection 1 or 2 - order is not guaranteed in that case
run {
val search =
SeriesSearch(
SearchCondition.AnyOfSeries(
SearchCondition.CollectionId(SearchOperator.Is(collection1.id)),
SearchCondition.CollectionId(SearchOperator.Is(collection2.id)),
),
)
val found = seriesDao.findAll(search.condition, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content
val foundDto = seriesDtoDao.findAll(search, SearchContext(user1), UnpagedSorted(Sort.by("collection.number"))).content

assertThat(found.map { it.name }).containsExactlyInAnyOrder("1", "2", "3")
assertThat(foundDto.map { it.name }).containsExactlyInAnyOrder("1", "2", "3")
}
}

@Test
fun `given some series when searching by deleted then results are accurate`() {
makeSeries("1", library1.id).copy(deletedDate = LocalDateTime.now()).let { series ->
Expand Down

0 comments on commit 39a054b

Please sign in to comment.