Address review comments

mickael-menu · mickael-menu · commit 600e5f93aedb · 2023-08-21T17:02:59.000+02:00
diff --git a/readium/opds/src/main/java/org/readium/r2/opds/OPDS1Parser.kt b/readium/opds/src/main/java/org/readium/r2/opds/OPDS1Parser.kt
@@ -50,15 +50,20 @@ public object Namespaces {
 public class OPDS1Parser {
     public companion object {
 
-        public suspend fun parseUrlString(url: String, client: HttpClient = DefaultHttpClient()): Try<ParseData, Exception> {
+        public suspend fun parseUrlString(
+            url: String,
+            client: HttpClient = DefaultHttpClient(
+                DefaultMediaTypeSniffer()
+            )
+        ): Try<ParseData, Exception> {
             return client.fetchWithDecoder(HttpRequest(url)) {
                 this.parse(it.body, URL(url))
             }
         }
 
         public suspend fun parseRequest(
             request: HttpRequest,
-            client: HttpClient = DefaultHttpClient()
+            client: HttpClient = DefaultHttpClient(DefaultMediaTypeSniffer())
         ): Try<ParseData, Exception> {
             return client.fetchWithDecoder(request) {
                 this.parse(it.body, URL(request.url))
@@ -192,7 +197,7 @@ public class OPDS1Parser {
         @Suppress("unused")
         public suspend fun retrieveOpenSearchTemplate(
             feed: Feed,
-            client: HttpClient = DefaultHttpClient()
+            client: HttpClient = DefaultHttpClient(DefaultMediaTypeSniffer())
         ): Try<String?, Exception> {
             var openSearchURL: URL? = null
             var selfMimeType: MediaType? = null
diff --git a/readium/opds/src/main/java/org/readium/r2/opds/OPDS2Parser.kt b/readium/opds/src/main/java/org/readium/r2/opds/OPDS2Parser.kt
@@ -45,15 +45,20 @@ public class OPDS2Parser {
 
         private lateinit var feed: Feed
 
-        public suspend fun parseUrlString(url: String, client: HttpClient = DefaultHttpClient()): Try<ParseData, Exception> {
+        public suspend fun parseUrlString(
+            url: String,
+            client: HttpClient = DefaultHttpClient(
+                DefaultMediaTypeSniffer()
+            )
+        ): Try<ParseData, Exception> {
             return client.fetchWithDecoder(HttpRequest(url)) {
                 this.parse(it.body, URL(url))
             }
         }
 
         public suspend fun parseRequest(
             request: HttpRequest,
-            client: HttpClient = DefaultHttpClient()
+            client: HttpClient = DefaultHttpClient(DefaultMediaTypeSniffer())
         ): Try<ParseData, Exception> {
             return client.fetchWithDecoder(request) {
                 this.parse(it.body, URL(request.url))
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/asset/Asset.kt b/readium/shared/src/main/java/org/readium/r2/shared/asset/Asset.kt
@@ -6,6 +6,7 @@
 
 package org.readium.r2.shared.asset
 
+import org.readium.r2.shared.resource.Container as SharedContainer
 import org.readium.r2.shared.resource.Resource as SharedResource
 import org.readium.r2.shared.util.mediatype.MediaType
 
@@ -58,7 +59,7 @@ public sealed class Asset {
     public class Container(
         override val mediaType: MediaType,
         exploded: Boolean,
-        public val container: org.readium.r2.shared.resource.Container
+        public val container: SharedContainer
     ) : Asset() {
 
         override val assetType: AssetType =
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/publication/services/ContentProtectionService.kt b/readium/shared/src/main/java/org/readium/r2/shared/publication/services/ContentProtectionService.kt
@@ -316,7 +316,7 @@ private sealed class RouteHandler {
             return if (!copyAllowed) {
                 FailureResource(Resource.Exception.Forbidden())
             } else {
-                StringResource("true")
+                StringResource("true", MediaType.JSON)
             }
         }
     }
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/resource/BytesResource.kt b/readium/shared/src/main/java/org/readium/r2/shared/resource/BytesResource.kt
@@ -48,15 +48,15 @@ public sealed class BaseBytesResource(
 /** Creates a Resource serving a [ByteArray]. */
 public class BytesResource(
     url: Url? = null,
-    mediaType: MediaType = MediaType.BINARY,
+    mediaType: MediaType,
     properties: Resource.Properties = Resource.Properties(),
     bytes: suspend () -> ResourceTry<ByteArray>
 ) : BaseBytesResource(source = url, mediaType = mediaType, properties = properties, bytes = bytes) {
 
     public constructor(
         bytes: ByteArray,
+        mediaType: MediaType,
         url: Url? = null,
-        mediaType: MediaType = MediaType.BINARY,
         properties: Resource.Properties = Resource.Properties()
     ) :
         this(url = url, mediaType = mediaType, properties = properties, { Try.success(bytes) })
@@ -68,7 +68,7 @@ public class BytesResource(
 /** Creates a Resource serving a [String]. */
 public class StringResource(
     url: Url? = null,
-    mediaType: MediaType = MediaType.TEXT,
+    mediaType: MediaType,
     properties: Resource.Properties = Resource.Properties(),
     string: suspend () -> ResourceTry<String>
 ) : BaseBytesResource(
@@ -80,8 +80,8 @@ public class StringResource(
 
     public constructor(
         string: String,
+        mediaType: MediaType,
         url: Url? = null,
-        mediaType: MediaType = MediaType.TEXT,
         properties: Resource.Properties = Resource.Properties()
     ) :
         this(url = url, mediaType = mediaType, properties = properties, { Try.success(string) })
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/resource/MediaTypeExt.kt b/readium/shared/src/main/java/org/readium/r2/shared/resource/MediaTypeExt.kt
@@ -18,6 +18,6 @@ public class ContainerMediaTypeSnifferContent(
     override suspend fun entries(): Set<String>? =
         container.entries()?.map { it.path }?.toSet()
 
-    override suspend fun read(path: String): ByteArray? =
-        container.get(path).read().getOrNull()
+    override suspend fun read(path: String, range: LongRange?): ByteArray? =
+        container.get(path).read(range).getOrNull()
 }
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/util/http/DefaultHttpClient.kt b/readium/shared/src/main/java/org/readium/r2/shared/util/http/DefaultHttpClient.kt
@@ -19,7 +19,6 @@ import org.readium.r2.shared.error.flatMap
 import org.readium.r2.shared.error.tryRecover
 import org.readium.r2.shared.util.http.HttpRequest.Method
 import org.readium.r2.shared.util.mediatype.BytesResourceMediaTypeSnifferContent
-import org.readium.r2.shared.util.mediatype.DefaultMediaTypeSniffer
 import org.readium.r2.shared.util.mediatype.EpubMediaTypeSniffer.sniff
 import org.readium.r2.shared.util.mediatype.MediaType
 import org.readium.r2.shared.util.mediatype.MediaTypeHints
@@ -38,7 +37,7 @@ import timber.log.Timber
  *        as the default value, while a timeout of zero as an infinite timeout.
  */
 public class DefaultHttpClient(
-    private val mediaTypeSniffer: MediaTypeSniffer = DefaultMediaTypeSniffer(),
+    private val mediaTypeSniffer: MediaTypeSniffer,
     private val userAgent: String? = null,
     private val additionalHeaders: Map<String, String> = mapOf(),
     private val connectTimeout: Duration? = null,
diff --git a/readium/shared/src/main/java/org/readium/r2/shared/util/mediatype/MediaTypeSnifferContent.kt b/readium/shared/src/main/java/org/readium/r2/shared/util/mediatype/MediaTypeSnifferContent.kt
@@ -89,15 +89,15 @@ public interface ContainerMediaTypeSnifferContent : MediaTypeSnifferContent {
     /**
      * Returns the entry data at the given [path] in this container.
      */
-    public suspend fun read(path: String): ByteArray?
+    public suspend fun read(path: String, range: LongRange? = null): ByteArray?
 }
 
 /**
  * Returns whether an entry exists in the container.
  */
 public suspend fun ContainerMediaTypeSnifferContent.contains(path: String): Boolean =
     entries()?.contains(path)
-        ?: (read(path) != null)
+        ?: (read(path, range = 0L..1L) != null)
 
 public class BytesResourceMediaTypeSnifferContent(
     bytes: suspend () -> ByteArray
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/PublicationTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/PublicationTest.kt
@@ -385,7 +385,7 @@ class PublicationTest {
             override fun get(link: Link): Resource? {
                 assertFalse(link.templated)
                 assertEquals("param1=a&param2=b", link.href.substringAfter("?"))
-                return StringResource("test passed", url = Url(link.href))
+                return StringResource("test passed", MediaType.TEXT, url = Url(link.href))
             }
         }
 
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/protection/TestContainer.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/protection/TestContainer.kt
@@ -17,7 +17,7 @@ import org.readium.r2.shared.util.mediatype.MediaType
 class TestContainer(resources: Map<String, String> = emptyMap()) : Container {
 
     private val entries: Map<String, Entry> =
-        resources.mapValues { Entry(it.key, StringResource(it.value)) }
+        resources.mapValues { Entry(it.key, StringResource(it.value, MediaType.TEXT)) }
 
     override suspend fun entries(): Set<Container.Entry> =
         entries.values.toSet()
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/publication/services/content/iterators/HtmlResourceContentIteratorTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/publication/services/content/iterators/HtmlResourceContentIteratorTest.kt
@@ -181,7 +181,7 @@ class HtmlResourceContentIteratorTest {
         totalProgressionRange: ClosedRange<Double>? = null
     ): HtmlResourceContentIterator =
         HtmlResourceContentIterator(
-            StringResource(html, Url(link.href)),
+            StringResource(html, MediaType.HTML, Url(link.href)),
             totalProgressionRange = totalProgressionRange,
             startLocator
         )
diff --git a/readium/shared/src/test/java/org/readium/r2/shared/util/mediatype/MediaTypeSnifferTest.kt b/readium/shared/src/test/java/org/readium/r2/shared/util/mediatype/MediaTypeSnifferTest.kt
@@ -514,8 +514,8 @@ class MediaTypeSnifferTest {
                 override suspend fun entries(): Set<String>? =
                     archive.entries()?.map { it.path }?.toSet()
 
-                override suspend fun read(path: String): ByteArray? =
-                    archive.get(path).read().getOrNull()
+                override suspend fun read(path: String, range: LongRange?): ByteArray? =
+                    archive.get(path).read(range).getOrNull()
             }
         )
     }
diff --git a/test-app/src/main/java/org/readium/r2/testapp/catalogs/CatalogFeedListViewModel.kt b/test-app/src/main/java/org/readium/r2/testapp/catalogs/CatalogFeedListViewModel.kt
@@ -6,6 +6,7 @@
 
 package org.readium.r2.testapp.catalogs
 
+import android.app.Application
 import androidx.lifecycle.AndroidViewModel
 import androidx.lifecycle.viewModelScope
 import java.net.URL
@@ -18,13 +19,13 @@ import org.readium.r2.shared.error.Try
 import org.readium.r2.shared.opds.ParseData
 import org.readium.r2.shared.util.http.HttpRequest
 import org.readium.r2.shared.util.http.fetchWithDecoder
-import org.readium.r2.testapp.Application
 import org.readium.r2.testapp.db.BookDatabase
 import org.readium.r2.testapp.domain.model.Catalog
 import org.readium.r2.testapp.utils.EventChannel
 
-class CatalogFeedListViewModel(private val application: Application) : AndroidViewModel(application) {
+class CatalogFeedListViewModel(application: Application) : AndroidViewModel(application) {
 
+    private val httpClient = getApplication<org.readium.r2.testapp.Application>().readium.httpClient
     private val catalogDao = BookDatabase.getDatabase(application).catalogDao()
     private val repository = CatalogRepository(catalogDao)
     val eventChannel = EventChannel(Channel<Event>(Channel.BUFFERED), viewModelScope)
@@ -55,7 +56,7 @@ class CatalogFeedListViewModel(private val application: Application) : AndroidVi
     }
 
     private suspend fun parseURL(url: URL): Try<ParseData, Exception> {
-        return application.readium.httpClient.fetchWithDecoder(HttpRequest(url.toString())) {
+        return httpClient.fetchWithDecoder(HttpRequest(url.toString())) {
             val result = it.body
             if (isJson(result)) {
                 OPDS2Parser.parse(result, url)
diff --git a/test-app/src/main/java/org/readium/r2/testapp/catalogs/CatalogViewModel.kt b/test-app/src/main/java/org/readium/r2/testapp/catalogs/CatalogViewModel.kt
@@ -6,6 +6,7 @@
 
 package org.readium.r2.testapp.catalogs
 
+import android.app.Application
 import androidx.lifecycle.AndroidViewModel
 import androidx.lifecycle.viewModelScope
 import java.io.File
@@ -23,27 +24,27 @@ import org.readium.r2.shared.publication.Publication
 import org.readium.r2.shared.publication.opds.images
 import org.readium.r2.shared.util.http.HttpRequest
 import org.readium.r2.shared.util.mediatype.MediaType
-import org.readium.r2.testapp.Application
 import org.readium.r2.testapp.domain.model.Catalog
 import org.readium.r2.testapp.utils.EventChannel
 import org.readium.r2.testapp.utils.extensions.downloadTo
 import timber.log.Timber
 
-class CatalogViewModel(private val application: Application) : AndroidViewModel(application) {
+class CatalogViewModel(application: Application) : AndroidViewModel(application) {
 
     val detailChannel = EventChannel(Channel<Event.DetailEvent>(Channel.BUFFERED), viewModelScope)
     val eventChannel = EventChannel(Channel<Event.FeedEvent>(Channel.BUFFERED), viewModelScope)
     lateinit var publication: Publication
+    private val app = getApplication<org.readium.r2.testapp.Application>()
 
     fun parseCatalog(catalog: Catalog) = viewModelScope.launch {
         var parseRequest: Try<ParseData, Exception>? = null
         catalog.href.let {
             val request = HttpRequest(it)
             try {
                 parseRequest = if (catalog.type == 1) {
-                    OPDS1Parser.parseRequest(request, application.readium.httpClient)
+                    OPDS1Parser.parseRequest(request, app.readium.httpClient)
                 } else {
-                    OPDS2Parser.parseRequest(request, application.readium.httpClient)
+                    OPDS2Parser.parseRequest(request, app.readium.httpClient)
                 }
             } catch (e: MalformedURLException) {
                 eventChannel.send(Event.FeedEvent.CatalogParseFailed)
@@ -60,18 +61,18 @@ class CatalogViewModel(private val application: Application) : AndroidViewModel(
 
     fun downloadPublication(publication: Publication) = viewModelScope.launch {
         val filename = UUID.randomUUID().toString()
-        val dest = File(application.storageDir, filename)
+        val dest = File(app.storageDir, filename)
 
         getDownloadURL(publication)
             .flatMap { url ->
                 url.downloadTo(
                     dest,
-                    httpClient = application.readium.httpClient,
-                    assetRetriever = application.readium.assetRetriever
+                    httpClient = app.readium.httpClient,
+                    assetRetriever = app.readium.assetRetriever
                 )
             }.flatMap {
                 val opdsCover = publication.images.firstOrNull()?.href
-                application.bookRepository.addLocalBook(dest, opdsCover)
+                app.bookRepository.addLocalBook(dest, opdsCover)
             }.onSuccess {
                 detailChannel.send(Event.DetailEvent.ImportPublicationSuccess)
             }.onFailure {
diff --git a/test-app/src/main/java/org/readium/r2/testapp/utils/extensions/File.kt b/test-app/src/main/java/org/readium/r2/testapp/utils/extensions/File.kt
@@ -46,31 +46,12 @@ fun File.listFilesSafely(filter: FileFilter? = null): List<File> {
 
 suspend fun URL.downloadTo(
     dest: File,
-    maxRedirections: Int = 2,
     httpClient: HttpClient,
     assetRetriever: AssetRetriever
 ): Try<Unit, Exception> {
-    if (maxRedirections == 0) {
-        return Try.Failure(Exception("Too many HTTP redirections."))
-    }
-
-    val urlString = toString()
-
-    if (BuildConfig.DEBUG) Timber.i("download url $urlString")
+    if (BuildConfig.DEBUG) Timber.i("download url $this")
     return httpClient.download(HttpRequest(toString()), dest, assetRetriever)
-        .flatMap {
-            try {
-                if (BuildConfig.DEBUG) Timber.i("response url ${it.url}")
-                if (BuildConfig.DEBUG) Timber.i("download destination ${dest.path}")
-                if (urlString == it.url) {
-                    Try.success(Unit)
-                } else {
-                    URL(it.url).downloadTo(dest, maxRedirections - 1, httpClient, assetRetriever)
-                }
-            } catch (e: Exception) {
-                Try.failure(e)
-            }
-        }
+        .map { }
 }
 
 private suspend fun HttpClient.download(

Original file line number	Diff line number	Diff line change
`@@ -316,7 +316,7 @@ private sealed class RouteHandler {`
`316`	`316`	`return if (!copyAllowed) {`
`317`	`317`	`FailureResource(Resource.Exception.Forbidden())`
`318`	`318`	`} else {`
`319`		`- StringResource("true")`
	`319`	`+ StringResource("true", MediaType.JSON)`
`320`	`320`	`}`
`321`	`321`	`}`
`322`	`322`	`}`
Original file line number	Diff line number	Diff line change
`@@ -18,6 +18,6 @@ public class ContainerMediaTypeSnifferContent(`
`18`	`18`	`override suspend fun entries(): Set<String>? =`
`19`	`19`	`container.entries()?.map { it.path }?.toSet()`
`20`	`20`
`21`		`- override suspend fun read(path: String): ByteArray? =`
`22`		`- container.get(path).read().getOrNull()`
	`21`	`+ override suspend fun read(path: String, range: LongRange?): ByteArray? =`
	`22`	`+ container.get(path).read(range).getOrNull()`
`23`	`23`	`}`
Original file line number	Diff line number	Diff line change
`@@ -385,7 +385,7 @@ class PublicationTest {`
`385`	`385`	`override fun get(link: Link): Resource? {`
`386`	`386`	`assertFalse(link.templated)`
`387`	`387`	`assertEquals("param1=a&param2=b", link.href.substringAfter("?"))`
`388`		`- return StringResource("test passed", url = Url(link.href))`
	`388`	`+ return StringResource("test passed", MediaType.TEXT, url = Url(link.href))`
`389`	`389`	`}`
`390`	`390`	`}`
`391`	`391`