Skip to content

Commit

Permalink
fix(api): multiple tag or author filters could generate duplicate boo…
Browse files Browse the repository at this point in the history 
…k results

Closes: #1052
  • Loading branch information
@gotson
gotson committed Jan 27, 2023
1 parent 273b7d2 commit 88aa7ad
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 27 deletions.
38 changes: 11 additions & 27 deletions komga/src/main/kotlin/org/gotson/komga/infrastructure/jooq/BookDtoDao.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class BookDtoDao(
override fun findAll(search: BookSearchWithReadProgress, userId: String, pageable: Pageable, restrictions: ContentRestrictions): Page<BookDto> {
val conditions = search.toCondition().and(restrictions.toCondition(dsl))

return findAll(conditions, userId, pageable, search.toJoinConditions(), null, search.searchTerm)
return findAll(conditions, userId, pageable, false, null, search.searchTerm)
}

override fun findAllByReadListId(
Expand All @@ -98,14 +98,14 @@ class BookDtoDao(
): Page<BookDto> {
val conditions = rlb.READLIST_ID.eq(readListId).and(search.toCondition())

return findAll(conditions, userId, pageable, search.toJoinConditions().copy(selectReadListNumber = true), filterOnLibraryIds, search.searchTerm)
return findAll(conditions, userId, pageable, true, filterOnLibraryIds, search.searchTerm)
}

private fun findAll(
conditions: Condition,
userId: String,
pageable: Pageable,
joinConditions: JoinConditions = JoinConditions(),
selectReadListNumber: Boolean = false,
filterOnLibraryIds: Collection<String>?,
searchTerm: String?,
): Page<BookDto> {
Expand Down Expand Up @@ -143,9 +143,7 @@ class BookDtoDao(
.leftJoin(r).on(b.ID.eq(r.BOOK_ID)).and(readProgressCondition(userId))
.leftJoin(sd).on(b.SERIES_ID.eq(sd.SERIES_ID))
.apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } }
.apply { if (joinConditions.tag) leftJoin(bt).on(b.ID.eq(bt.BOOK_ID)) }
.apply { if (joinConditions.selectReadListNumber) leftJoin(rlb).on(b.ID.eq(rlb.BOOK_ID)) }
.apply { if (joinConditions.author) leftJoin(a).on(b.ID.eq(a.BOOK_ID)) }
.apply { if (selectReadListNumber) leftJoin(rlb).on(b.ID.eq(rlb.BOOK_ID)) }
.where(conditions)
.and(searchCondition)
.groupBy(b.ID),
Expand All @@ -154,7 +152,7 @@ class BookDtoDao(
// adjust temp table if we are paging by search results
if (pagingBySearch) dsl.insertTempStrings(batchSize, bookIdsPaged)

val dtos = selectBase(userId, joinConditions)
val dtos = selectBase(userId, selectReadListNumber)
.where(conditions)
.and(searchCondition)
.apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } }
Expand Down Expand Up @@ -299,7 +297,7 @@ class BookDtoDao(
.apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } }
.fetchOne(0, Int::class.java)

return selectBase(userId, JoinConditions(selectReadListNumber = true))
return selectBase(userId, true)
.where(rlb.READLIST_ID.eq(readListId))
.apply { filterOnLibraryIds?.let { and(b.LIBRARY_ID.`in`(it)) } }
.orderBy(rlb.NUMBER.let { if (next) it.asc() else it.desc() })
Expand All @@ -309,22 +307,20 @@ class BookDtoDao(
.firstOrNull()
}

private fun selectBase(userId: String, joinConditions: JoinConditions = JoinConditions()) =
private fun selectBase(userId: String, selectReadListNumber: Boolean = false) =
dsl.select(
*b.fields(),
*m.fields(),
*d.fields(),
*r.fields(),
sd.TITLE,
).apply { if (joinConditions.selectReadListNumber) select(rlb.NUMBER) }
).apply { if (selectReadListNumber) select(rlb.NUMBER) }
.from(b)
.leftJoin(m).on(b.ID.eq(m.BOOK_ID))
.leftJoin(d).on(b.ID.eq(d.BOOK_ID))
.leftJoin(r).on(b.ID.eq(r.BOOK_ID)).and(readProgressCondition(userId))
.leftJoin(sd).on(b.SERIES_ID.eq(sd.SERIES_ID))
.apply { if (joinConditions.tag) leftJoin(bt).on(b.ID.eq(bt.BOOK_ID)) }
.apply { if (joinConditions.selectReadListNumber) leftJoin(rlb).on(b.ID.eq(rlb.BOOK_ID)) }
.apply { if (joinConditions.author) leftJoin(a).on(b.ID.eq(a.BOOK_ID)) }
.apply { if (selectReadListNumber) leftJoin(rlb).on(b.ID.eq(rlb.BOOK_ID)) }

private fun ResultQuery<Record>.fetchAndMap(): MutableList<BookDto> {
val records = fetch()
Expand Down Expand Up @@ -370,7 +366,7 @@ class BookDtoDao(
if (deleted == true) c = c.and(b.DELETED_DATE.isNotNull)
if (deleted == false) c = c.and(b.DELETED_DATE.isNull)
if (releasedAfter != null) c = c.and(d.RELEASE_DATE.gt(releasedAfter))
if (!tags.isNullOrEmpty()) c = c.and(bt.TAG.collate(SqliteUdfDataSource.collationUnicode3).`in`(tags))
if (!tags.isNullOrEmpty()) c = c.and(b.ID.`in`(dsl.select(bt.BOOK_ID).from(bt).where(bt.TAG.collate(SqliteUdfDataSource.collationUnicode3).`in`(tags))))

if (readStatus != null) {
val cr = readStatus.map {
Expand All @@ -387,26 +383,14 @@ class BookDtoDao(
if (!authors.isNullOrEmpty()) {
var ca = noCondition()
authors.forEach {
ca = ca.or(a.NAME.equalIgnoreCase(it.name).and(a.ROLE.equalIgnoreCase(it.role)))
ca = ca.or(b.ID.`in`(dsl.select(a.BOOK_ID).from(a).where(a.NAME.equalIgnoreCase(it.name).and(a.ROLE.equalIgnoreCase(it.role)))))
}
c = c.and(ca)
}

return c
}

private fun BookSearchWithReadProgress.toJoinConditions() =
JoinConditions(
tag = !tags.isNullOrEmpty(),
author = !authors.isNullOrEmpty(),
)

private data class JoinConditions(
val selectReadListNumber: Boolean = false,
val tag: Boolean = false,
val author: Boolean = false,
)

private fun BookRecord.toDto(media: MediaDto, metadata: BookMetadataDto, readProgress: ReadProgressDto?, seriesTitle: String) =
BookDto(
id = id,
Expand Down
69 changes: 69 additions & 0 deletions komga/src/test/kotlin/org/gotson/komga/infrastructure/jooq/BookDtoDaoTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,75 @@ class BookDtoDaoTest(
books.elementAt(1).let { readProgressRepository.save(ReadProgress(it.id, user.id, 5, true)) }
}

@Nested
inner class Criteria {
@Test
fun `given books when searching by multiple tags then results are matched and not duplicated`() {
// given
val book1 = makeBook("Éric le rouge", seriesId = series.id, libraryId = library.id)
val book2 = makeBook("Éric le bleu", seriesId = series.id, libraryId = library.id)
seriesLifecycle.addBooks(
series,
listOf(
book1,
book2
),
)

bookMetadataRepository.findById(book1.id).let {
bookMetadataRepository.update(it.copy(tags = setOf("tag1", "tag2")))
}
bookMetadataRepository.findById(book2.id).let {
bookMetadataRepository.update(it.copy(tags = setOf("tag1", "tag2")))
}

// when
val page = bookDtoDao.findAll(
BookSearchWithReadProgress(tags = setOf("tag1", "tag2")),
user.id,
Pageable.unpaged(),
)

// then
assertThat(page.totalElements).isEqualTo(2)
assertThat(page.content).hasSize(2)
assertThat(page.content.map { it.metadata.title }).containsExactly("Éric le rouge", "Éric le bleu")
}

@Test
fun `given books when searching by multiple authors then results are matched and not duplicated`() {
// given
val book1 = makeBook("Éric le rouge", seriesId = series.id, libraryId = library.id)
val book2 = makeBook("Éric le bleu", seriesId = series.id, libraryId = library.id)
seriesLifecycle.addBooks(
series,
listOf(
book1,
book2
),
)

bookMetadataRepository.findById(book1.id).let {
bookMetadataRepository.update(it.copy(authors = listOf(Author("Mark", "writer"), Author("Jim", "inker"))))
}
bookMetadataRepository.findById(book2.id).let {
bookMetadataRepository.update(it.copy(authors = listOf(Author("Mark", "writer"), Author("Jim", "inker"))))
}

// when
val page = bookDtoDao.findAll(
BookSearchWithReadProgress(authors = listOf(Author("Mark", "writer"), Author("Jim", "inker"))),
user.id,
Pageable.unpaged(),
)

// then
assertThat(page.totalElements).isEqualTo(2)
assertThat(page.content).hasSize(2)
assertThat(page.content.map { it.metadata.title }).containsExactly("Éric le rouge", "Éric le bleu")
}
}

@Nested
inner class ReadProgress {
@Test
Expand Down

0 comments on commit 88aa7ad

Please sign in to comment.