Support HTTP headers with multiple values (#411)

CHANGELOG.md

@@ -21,7 +21,6 @@ All notable changes to this project will be documented in this file. Take a look
      1. Override loading non-linear resources with `VisualNavigator.Listener.shouldJumpToLink()`.
      2. Present a new `EpubNavigatorFragment` by providing a custom `readingOrder` with only this resource to the constructor.
 
-
 #### Streamer
 
 * The EPUB content iterator now returns `audio` and `video` elements.
@@ -46,6 +45,10 @@ All notable changes to this project will be documented in this file. Take a look
 
 ### Deprecated
 
+#### Shared
+
+* `DefaultHttClient.additionalHeaders` is deprecated. Set all the headers when creating a new `HttpRequest`, or modify outgoing requests in `DefaultHttpClient.Callback.onStartRequest()`.
+
 #### Navigator
 
 * All the navigator `Activity` are deprecated in favor of the `Fragment` variants.

readium/shared/src/main/java/org/readium/r2/shared/extensions/Map.kt

@@ -0,0 +1,30 @@
+/*
+ * Copyright 2023 Readium Foundation. All rights reserved.
+ * Use of this source code is governed by the BSD-style license
+ * available in the top-level LICENSE file of the project.
+ */
+
+package org.readium.r2.shared.extensions
+
+internal fun Map<String, List<String>>.lowerCaseKeys(): Map<String, List<String>> {
+    val normalizedMap = mutableMapOf<String, MutableList<String>>()
+    for ((k, v) in this) {
+        normalizedMap.getOrPut(
+            k.lowercase(),
+            mutableListOf()
+        ).addAll(v)
+    }
+    return normalizedMap
+}
+
+internal fun <K, V> MutableMap<K, V>.getOrPut(key: K, fallbackValue: V): V =
+    get(key) ?: run {
+        put(key, fallbackValue)
+        fallbackValue
+    }
+
+internal fun Map<String, List<String>>.joinValues(separator: CharSequence): Map<String, String> =
+    mapValues { it.value.joinToString(separator) }
+
+internal fun <K, V> Map<K, List<V>>.toMutable() =
+    mapValues { it.value.toMutableList() }.toMutableMap()

readium/shared/src/main/java/org/readium/r2/shared/util/Try.kt

@@ -38,6 +38,14 @@ public sealed class Try<out Success, out Failure> {
         override val isFailure: Boolean get() = true
         override fun getOrNull(): S? = null
         override fun failureOrNull(): F = value
+
+        @Deprecated(
+            "Renamed to value.",
+            level = DeprecationLevel.ERROR,
+            replaceWith = ReplaceWith("value")
+        )
+        public val exception: F
+            get() = value
     }
 
     /**

readium/shared/src/main/java/org/readium/r2/shared/util/downloads/foreground/ForegroundDownloadManager.kt

@@ -65,7 +65,7 @@ public class ForegroundDownloadManager(
             .download(
                 request = HttpRequest(
                     url = request.url.toString(),
-                    headers = request.headers.mapValues { it.value.joinToString(",") }
+                    headers = request.headers
                 ),
                 destination = destination,
                 onProgress = { downloaded, expected ->

readium/shared/src/main/java/org/readium/r2/shared/util/http/DefaultHttpClient.kt

@@ -15,6 +15,8 @@ import java.net.URL
 import kotlin.time.Duration
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
+import org.readium.r2.shared.extensions.joinValues
+import org.readium.r2.shared.extensions.lowerCaseKeys
 import org.readium.r2.shared.util.Try
 import org.readium.r2.shared.util.flatMap
 import org.readium.r2.shared.util.http.HttpRequest.Method
@@ -30,7 +32,6 @@ import timber.log.Timber
  *
  * @param mediaTypeRetriever Component used to sniff the media type of the HTTP response.
  * @param userAgent Custom user agent to use for requests.
- * @param additionalHeaders A dictionary of additional headers to send with requests.
  * @param connectTimeout Timeout used when establishing a connection to the resource. A null timeout
  *        is interpreted as the default value, while a timeout of zero as an infinite timeout.
  * @param readTimeout Timeout used when reading the input stream. A null timeout is interpreted
@@ -39,11 +40,31 @@ import timber.log.Timber
 public class DefaultHttpClient(
     private val mediaTypeRetriever: MediaTypeRetriever,
     private val userAgent: String? = null,
-    private val additionalHeaders: Map<String, String> = mapOf(),
     private val connectTimeout: Duration? = null,
     private val readTimeout: Duration? = null,
     public var callback: Callback = object : Callback {}
 ) : HttpClient {
+
+    @Suppress("UNUSED_PARAMETER")
+    @Deprecated(
+        "You need to provide a [mediaTypeRetriever]. If you used [additionalHeaders], pass all headers when building your request or modify it in Callback.onStartRequest instead.",
+        level = DeprecationLevel.ERROR,
+        replaceWith = ReplaceWith("DefaultHttpClient(mediaTypeRetriever = MediaTypeRetriever())")
+    )
+    public constructor(
+        userAgent: String? = null,
+        additionalHeaders: Map<String, String> = mapOf(),
+        connectTimeout: Duration? = null,
+        readTimeout: Duration? = null,
+        callback: Callback = object : Callback {}
+    ) : this(
+        mediaTypeRetriever = MediaTypeRetriever(),
+        userAgent = userAgent,
+        connectTimeout = connectTimeout,
+        readTimeout = readTimeout,
+        callback = callback
+    )
+
     public companion object {
         /**
          * [HttpRequest.extras] key for the number of redirections performed for a request.
@@ -212,15 +233,16 @@ public class DefaultHttpClient(
             return Try.failure(HttpException.CANCELLED)
         }
 
-        val location = response.valueForHeader("Location")
+        val location = response.header("Location")
             ?: return Try.failure(HttpException(kind = HttpException.Kind.MalformedResponse))
 
         val newRequest = HttpRequest(
             url = location,
             body = request.body,
             method = request.method,
             headers = buildMap {
-                response.valueForHeader("Set-Cookie")
+                response.headers("Set-Cookie")
+                    .takeUnless { it.isEmpty() }
                     ?.let { put("Cookie", it) }
             },
             extras = Bundle().apply {
@@ -253,10 +275,11 @@ public class DefaultHttpClient(
             connection.setRequestProperty("User-Agent", userAgent)
         }
 
-        for ((k, v) in this@DefaultHttpClient.additionalHeaders) {
-            connection.setRequestProperty(k, v)
-        }
-        for ((k, v) in headers) {
+        val normalizedHeaders = headers
+            .lowerCaseKeys()
+            .joinValues(",")
+
+        for ((k, v) in normalizedHeaders) {
             connection.setRequestProperty(k, v)
         }
 

readium/shared/src/main/java/org/readium/r2/shared/util/http/HttpClient.kt

@@ -59,13 +59,37 @@ public data class HttpResponse(
      * Finds the first value of the first header matching the given name.
      * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
      */
-    public fun valueForHeader(name: String): String? = httpHeaders[name]
+    @Deprecated("Use the header method instead.", level = DeprecationLevel.ERROR)
+    @Suppress("Unused_parameter")
+    public fun valueForHeader(name: String): String? {
+        throw NotImplementedError()
+    }
 
     /**
      * Finds all the values of the first header matching the given name.
      * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
      */
-    public fun valuesForHeader(name: String): List<String> = httpHeaders.getAll(name)
+    @Deprecated("Use the headers method instead.", level = DeprecationLevel.ERROR)
+    @Suppress("Unused_parameter")
+    public fun valuesForHeader(name: String): List<String> {
+        throw NotImplementedError()
+    }
+
+    /**
+     * Finds the last header matching the given name.
+     * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
+     * The returned string can contain a single value or a comma-separated list of values if
+     * the field supports it.
+     */
+    public fun header(name: String): String? = httpHeaders[name]
+
+    /**
+     * Finds all the headers matching the given name.
+     * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
+     * Each item of the returned list can contain a single value or a comma-separated list of
+     * values if the field supports it.
+     */
+    public fun headers(name: String): List<String> = httpHeaders.getAll(name)
 
     /**
      * Indicates whether this server supports byte range requests.

readium/shared/src/main/java/org/readium/r2/shared/util/http/HttpHeaders.kt

@@ -20,36 +20,24 @@ public data class HttpHeaders(val headers: Map<String, List<String>>) {
     }
 
     /**
-     * Finds the first value of the first header matching the given name.
+     * Finds the last header matching the given name.
      * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
+     * The returned string can contain a single value or a comma-separated list of values if
+     * the field supports it.
      */
-    public operator fun get(name: String): String? {
-        val n = name.lowercase()
-        return headers.firstNotNullOfOrNull { (key, value) ->
-            if (key.lowercase() == n) {
-                value.firstOrNull()
-            } else {
-                null
-            }
-        }
-    }
+    public operator fun get(name: String): String? = getAll(name)
+        .lastOrNull()
 
     /**
-     * Finds all the values of the first header matching the given name.
+     * Finds all the headers matching the given name.
      * In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
+     * Each item of the returned list can contain a single value or a comma-separated list of
+     * values if the field supports it.
      */
-    public fun getAll(name: String): List<String> {
-        val n = name.lowercase()
-        return headers
-            .mapNotNull { (key, value) ->
-                if (key.lowercase() == n) {
-                    value
-                } else {
-                    null
-                }
-            }
-            .flatten()
-    }
+    public fun getAll(name: String): List<String> = headers
+        .filter { it.key.lowercase() == name.lowercase() }
+        .values
+        .flatten()
 
     /**
      * Indicates whether this server supports byte range requests.

readium/shared/src/main/java/org/readium/r2/shared/util/http/HttpRequest.kt

@@ -1,10 +1,17 @@
+/*
+ * Copyright 2023 Readium Foundation. All rights reserved.
+ * Use of this source code is governed by the BSD-style license
+ * available in the top-level LICENSE file of the project.
+ */
+
 package org.readium.r2.shared.util.http
 
 import android.net.Uri
 import android.os.Bundle
 import java.io.Serializable
 import java.net.URLEncoder
 import kotlin.time.Duration
+import org.readium.r2.shared.extensions.toMutable
 
 /**
  * Holds the information about an HTTP request performed by an [HttpClient].
@@ -25,7 +32,7 @@ import kotlin.time.Duration
 public class HttpRequest(
     public val url: String,
     public val method: Method = Method.GET,
-    public val headers: Map<String, String> = mapOf(),
+    public val headers: Map<String, List<String>> = mapOf(),
     public val body: Body? = null,
     public val extras: Bundle = Bundle(),
     public val connectTimeout: Duration? = null,
@@ -47,7 +54,7 @@ public class HttpRequest(
     public fun buildUpon(): Builder = Builder(
         url = url,
         method = method,
-        headers = headers.toMutableMap(),
+        headers = headers.toMutable(),
         body = body,
         extras = extras,
         connectTimeout = connectTimeout,
@@ -66,7 +73,7 @@ public class HttpRequest(
     public class Builder(
         url: String,
         public var method: Method = Method.GET,
-        public var headers: MutableMap<String, String> = mutableMapOf(),
+        public var headers: MutableMap<String, MutableList<String>> = mutableMapOf(),
         public var body: Body? = null,
         public var extras: Bundle = Bundle(),
         public var connectTimeout: Duration? = null,
@@ -94,8 +101,27 @@ public class HttpRequest(
             return this
         }
 
+        /**
+         * Sets header with key [key] to [values] overriding current values, if any.
+         */
+        public fun setHeader(key: String, values: List<String>): Builder {
+            headers[key] = values.toMutableList()
+            return this
+        }
+
+        /**
+         * Sets header with [key] to [value] overriding current values, if any.
+         */
         public fun setHeader(key: String, value: String): Builder {
-            headers[key] = value
+            headers[key] = mutableListOf(value)
+            return this
+        }
+
+        /**
+         * Adds [value] to header values associated with [key].
+         */
+        public fun addHeader(key: String, value: String): Builder {
+            headers.getOrPut(key) { mutableListOf() }.add(value)
             return this
         }