From a5bd9087df1e43be9bef5dea1039e1720f0f9541 Mon Sep 17 00:00:00 2001 From: Gauthier Roebroeck Date: Mon, 6 Apr 2020 15:19:08 +0800 Subject: [PATCH] fix(api): thumbnails not updating properly incorrect cache control could prevent updated thumbnails to show up use shallow etags for thumbnails --- .../web/EtagFilterConfiguration.kt | 21 ++++++ .../komga/interfaces/rest/BookController.kt | 18 ++---- .../interfaces/rest/BookControllerTest.kt | 60 ++++++++++++++--- .../interfaces/rest/SeriesControllerTest.kt | 64 +++++++++++++++++++ 4 files changed, 143 insertions(+), 20 deletions(-) create mode 100644 komga/src/main/kotlin/org/gotson/komga/infrastructure/web/EtagFilterConfiguration.kt diff --git a/komga/src/main/kotlin/org/gotson/komga/infrastructure/web/EtagFilterConfiguration.kt b/komga/src/main/kotlin/org/gotson/komga/infrastructure/web/EtagFilterConfiguration.kt new file mode 100644 index 000000000..f5e3fe196 --- /dev/null +++ b/komga/src/main/kotlin/org/gotson/komga/infrastructure/web/EtagFilterConfiguration.kt @@ -0,0 +1,21 @@ +package org.gotson.komga.infrastructure.web + +import org.springframework.boot.web.servlet.FilterRegistrationBean +import org.springframework.context.annotation.Bean +import org.springframework.context.annotation.Configuration +import org.springframework.web.filter.ShallowEtagHeaderFilter + + +@Configuration +class EtagFilterConfiguration { + @Bean + fun shallowEtagHeaderFilter(): FilterRegistrationBean = + FilterRegistrationBean(ShallowEtagHeaderFilter()) + .also { + it.addUrlPatterns( + "/api/*", + "/opds/*" + ) + it.setName("etagFilter") + } +} diff --git a/komga/src/main/kotlin/org/gotson/komga/interfaces/rest/BookController.kt b/komga/src/main/kotlin/org/gotson/komga/interfaces/rest/BookController.kt index dbbfcbf61..2c56a37c9 100644 --- a/komga/src/main/kotlin/org/gotson/komga/interfaces/rest/BookController.kt +++ b/komga/src/main/kotlin/org/gotson/komga/interfaces/rest/BookController.kt @@ -186,19 +186,10 @@ class BookController( @PathVariable bookId: Long ): ResponseEntity = bookRepository.findByIdOrNull(bookId)?.let { book -> - val etag = book.id.toString() - if (request.checkNotModified(etag, getBookLastModified(book))) { - return@let ResponseEntity - .status(HttpStatus.NOT_MODIFIED) - .eTag(etag) - .setNotModified(book) - .body(ByteArray(0)) - } if (!principal.user.canAccessBook(book)) throw ResponseStatusException(HttpStatus.UNAUTHORIZED) if (book.media.thumbnail != null) { ResponseEntity.ok() - .eTag(etag) - .setNotModified(book) + .setCachePrivate() .body(book.media.thumbnail) } else throw ResponseStatusException(HttpStatus.NOT_FOUND) } ?: throw ResponseStatusException(HttpStatus.NOT_FOUND) @@ -392,11 +383,14 @@ class BookController( bookRepository.save(book).toDto(includeFullUrl = true) } ?: throw ResponseStatusException(HttpStatus.NOT_FOUND) - private fun ResponseEntity.BodyBuilder.setNotModified(book: Book) = + private fun ResponseEntity.BodyBuilder.setCachePrivate() = this.cacheControl(CacheControl.maxAge(0, TimeUnit.SECONDS) .cachePrivate() .mustRevalidate() - ).lastModified(getBookLastModified(book)) + ) + + private fun ResponseEntity.BodyBuilder.setNotModified(book: Book) = + this.setCachePrivate().lastModified(getBookLastModified(book)) private fun getBookLastModified(book: Book) = book.media.lastModifiedDate!!.toInstant(ZoneOffset.UTC).toEpochMilli() diff --git a/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/BookControllerTest.kt b/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/BookControllerTest.kt index 3aacc98bc..097f72aaa 100644 --- a/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/BookControllerTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/BookControllerTest.kt @@ -27,6 +27,7 @@ import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabas import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc import org.springframework.boot.test.context.SpringBootTest import org.springframework.data.repository.findByIdOrNull +import org.springframework.http.HttpHeaders import org.springframework.http.MediaType import org.springframework.jdbc.core.JdbcTemplate import org.springframework.test.context.junit.jupiter.SpringExtension @@ -36,9 +37,8 @@ import org.springframework.test.web.servlet.get import org.springframework.test.web.servlet.patch import org.springframework.transaction.annotation.Transactional import java.time.LocalDate -import java.time.LocalDateTime -import java.time.ZoneOffset import javax.sql.DataSource +import kotlin.random.Random @ExtendWith(SpringExtension::class) @SpringBootTest @@ -327,16 +327,23 @@ class BookControllerTest( inner class HttpCache { @Test @WithMockCustomUser - fun `given request with If-Modified-Since headers when getting thumbnail then returns 304 not modified`() { + fun `given request with cache headers when getting thumbnail then returns 304 not modified`() { val series = makeSeries( name = "series", - books = listOf(makeBook("1.cbr")) + books = listOf(makeBook("1.cbr").also { + it.media.thumbnail = Random.nextBytes(100) + }) ).also { it.library = library } seriesRepository.save(series) - mockMvc.get("/api/v1/books/${series.books.first().id}/thumbnail") { + val url = "/api/v1/books/${series.books.first().id}/thumbnail" + + val response = mockMvc.get(url) + .andReturn().response + + mockMvc.get(url) { headers { - ifModifiedSince = LocalDateTime.now().toInstant(ZoneOffset.UTC).toEpochMilli() + ifNoneMatch = listOf(response.getHeader(HttpHeaders.ETAG)!!) } }.andExpect { status { isNotModified } @@ -352,9 +359,14 @@ class BookControllerTest( ).also { it.library = library } seriesRepository.save(series) - mockMvc.get("/api/v1/books/${series.books.first().id}/pages/1") { + val url = "/api/v1/books/${series.books.first().id}/pages/1" + + val lastModified = mockMvc.get(url) + .andReturn().response.getHeader(HttpHeaders.LAST_MODIFIED) + + mockMvc.get(url) { headers { - ifModifiedSince = LocalDateTime.now().toInstant(ZoneOffset.UTC).toEpochMilli() + set(HttpHeaders.IF_MODIFIED_SINCE, lastModified!!) } }.andExpect { status { isNotModified } @@ -362,6 +374,38 @@ class BookControllerTest( } } + //Not part of the above @Nested class because @Transactional fails + @Test + @WithMockCustomUser + @Transactional + fun `given request with cache headers and modified resource when getting thumbnail then returns 200 ok`() { + val book = makeBook("1.cbr").also { + it.media.thumbnail = Random.nextBytes(1) + } + val series = makeSeries( + name = "series", + books = listOf(book) + ).also { it.library = library } + seriesRepository.save(series) + + val url = "/api/v1/books/${series.books.first().id}/thumbnail" + + val response = mockMvc.get(url) + .andReturn().response + + Thread.sleep(100) + book.media.thumbnail = Random.nextBytes(1) + bookRepository.saveAndFlush(book) + + mockMvc.get(url) { + headers { + ifNoneMatch = listOf(response.getHeader(HttpHeaders.ETAG)!!) + } + }.andExpect { + status { isOk } + } + } + @Nested inner class MetadataUpdate { @Test diff --git a/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/SeriesControllerTest.kt b/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/SeriesControllerTest.kt index a6b95091c..97ff29976 100644 --- a/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/SeriesControllerTest.kt +++ b/komga/src/test/kotlin/org/gotson/komga/interfaces/rest/SeriesControllerTest.kt @@ -7,6 +7,7 @@ import org.gotson.komga.domain.model.UserRoles import org.gotson.komga.domain.model.makeBook import org.gotson.komga.domain.model.makeLibrary import org.gotson.komga.domain.model.makeSeries +import org.gotson.komga.domain.persistence.BookRepository import org.gotson.komga.domain.persistence.LibraryRepository import org.gotson.komga.domain.persistence.SeriesRepository import org.hamcrest.Matchers @@ -23,6 +24,7 @@ import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabas import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc import org.springframework.boot.test.context.SpringBootTest import org.springframework.data.repository.findByIdOrNull +import org.springframework.http.HttpHeaders import org.springframework.http.MediaType import org.springframework.jdbc.core.JdbcTemplate import org.springframework.test.context.junit.jupiter.SpringExtension @@ -32,6 +34,7 @@ import org.springframework.test.web.servlet.get import org.springframework.test.web.servlet.patch import org.springframework.transaction.annotation.Transactional import javax.sql.DataSource +import kotlin.random.Random @ExtendWith(SpringExtension::class) @SpringBootTest @@ -40,6 +43,7 @@ import javax.sql.DataSource class SeriesControllerTest( @Autowired private val seriesRepository: SeriesRepository, @Autowired private val libraryRepository: LibraryRepository, + @Autowired private val bookRepository: BookRepository, @Autowired private val mockMvc: MockMvc ) { @@ -404,4 +408,64 @@ class SeriesControllerTest( } } + @Test + @WithMockCustomUser + fun `given request with cache headers when getting series thumbnail then returns 304 not modified`() { + val book = makeBook("1.cbr").also { + it.media.thumbnail = Random.nextBytes(1) + } + val series = makeSeries( + name = "series", + books = listOf(book) + ).also { it.library = library } + seriesRepository.save(series) + + val url = "/api/v1/series/${series.id}/thumbnail" + + val response = mockMvc.get(url) + .andReturn().response + + mockMvc.get(url) { + headers { + ifNoneMatch = listOf(response.getHeader(HttpHeaders.ETAG)!!) + } + }.andExpect { + status { isNotModified } + } + } + + //Not part of the above @Nested class because @Transactional fails + @Test + @WithMockCustomUser + @Transactional + fun `given request with cache headers and modified first book when getting series thumbnail then returns 200 ok`() { + val book = makeBook("1.cbr").also { + it.media.thumbnail = Random.nextBytes(1) + } + val book2 = makeBook("2.cbr").also { + it.media.thumbnail = Random.nextBytes(1) + } + val series = makeSeries( + name = "series", + books = listOf(book, book2) + ).also { it.library = library } + seriesRepository.save(series) + + val url = "/api/v1/series/${series.id}/thumbnail" + + val response = mockMvc.get(url) + .andReturn().response + + book.metadata.numberSort = 3F + bookRepository.saveAndFlush(book) + + mockMvc.get(url) { + headers { + ifNoneMatch = listOf(response.getHeader(HttpHeaders.ETAG)!!) + } + }.andExpect { + status { isOk } + } + } + }