Relax rules when parsing a RWPM (#412)

Author: mickael-menu

readium/lcp/src/main/java/org/readium/r2/lcp/license/model/components/Link.kt

@@ -35,12 +35,14 @@ public data class Link(
             json: JSONObject,
             mediaTypeRetriever: MediaTypeRetriever = MediaTypeRetriever()
         ): Link {
-            val hrefString = json.optNullableString("href")
+            val href = json.optNullableString("href")
+                ?.let {
+                    Href(
+                        href = it,
+                        templated = json.optBoolean("templated", false)
+                    )
+                }
                 ?: throw LcpException.Parsing.Link
-            val href = Href(
-                href = hrefString,
-                templated = json.optBoolean("templated", false)
-            ) ?: throw LcpException.Parsing.Link
 
             return Link(
                 href = href,

readium/shared/src/main/java/org/readium/r2/shared/publication/Href.kt

@@ -21,20 +21,23 @@ import timber.log.Timber
 @Parcelize
 public class Href private constructor(private val href: Url) : Parcelable {
 
+    /**
+     * Creates an [Href] from a valid URL.
+     */
+    public constructor(href: SharedUrl) : this(StaticUrl(href))
+
     public companion object {
-        public operator fun invoke(href: SharedUrl): Href =
-            Href(StaticUrl(href))
-
-        public operator fun invoke(href: String, templated: Boolean = false): Href? {
-            val url =
-                if (templated) {
-                    TemplatedUrl(href)
-                } else {
-                    SharedUrl(href)?.let { StaticUrl(it) }
-                }
-
-            return url?.let { Href(it) }
-        }
+        /**
+         * Creates an [Href] from a valid URL or URL template (RFC 6570).
+         *
+         * @param templated Indicates whether [href] is a URL template.
+         */
+        public operator fun invoke(href: String, templated: Boolean = false): Href? =
+            if (templated) {
+                Href(TemplatedUrl(href))
+            } else {
+                SharedUrl(href)?.let { Href(StaticUrl(it)) }
+            }
     }
 
     /**

readium/shared/src/main/java/org/readium/r2/shared/publication/Link.kt

@@ -165,21 +165,10 @@ public data class Link(
             mediaTypeRetriever: MediaTypeRetriever = MediaTypeRetriever(),
             warnings: WarningLogger? = null
         ): Link? {
-            val hrefString = json?.optNullableString("href")
-            if (hrefString == null) {
-                warnings?.log(Link::class.java, "[href] is required", json)
-                return null
-            }
-            val href = Href(
-                href = hrefString,
-                templated = json.optBoolean("templated", false)
-            ) ?: run {
-                warnings?.log(Link::class.java, "[href] is not a valid URL or URL template", json)
-                return null
-            }
+            json ?: return null
 
             return Link(
-                href = href,
+                href = parseHref(json, warnings) ?: return null,
                 mediaType = json.optNullableString("type")
                     ?.let { mediaTypeRetriever.retrieve(it) },
                 title = json.optNullableString("title"),
@@ -201,6 +190,41 @@ public data class Link(
             )
         }
 
+        private fun parseHref(
+            json: JSONObject,
+            warnings: WarningLogger? = null
+        ): Href? {
+            val hrefString = json.optNullableString("href")
+            if (hrefString == null) {
+                warnings?.log(Link::class.java, "[href] is required", json)
+                return null
+            }
+
+            val templated = json.optBoolean("templated", false)
+            val href = if (templated) {
+                Href(hrefString, templated = true)
+            } else {
+                // We support existing publications with incorrect HREFs (not valid percent-encoded
+                // URIs). We try to parse them first as valid, but fall back on a percent-decoded
+                // path if it fails.
+                val url = Url(hrefString) ?: run {
+                    warnings?.log(
+                        Link::class.java,
+                        "[href] is not a valid percent-encoded URL",
+                        json
+                    )
+                    Url.fromDecodedPath(hrefString)
+                }
+                url?.let { Href(it) }
+            }
+
+            if (href == null) {
+                warnings?.log(Link::class.java, "[href] is not a valid URL or URL template", json)
+            }
+
+            return href
+        }
+
         /**
          * Creates a list of [Link] from its RWPM JSON representation.
          *

readium/shared/src/main/java/org/readium/r2/shared/publication/Manifest.kt

@@ -16,6 +16,7 @@ import org.readium.r2.shared.extensions.optStringsFromArrayOrSingle
 import org.readium.r2.shared.extensions.putIfNotEmpty
 import org.readium.r2.shared.toJSON
 import org.readium.r2.shared.util.Url
+import org.readium.r2.shared.util.logging.ConsoleWarningLogger
 import org.readium.r2.shared.util.logging.WarningLogger
 import org.readium.r2.shared.util.logging.log
 import org.readium.r2.shared.util.mediatype.MediaType
@@ -154,7 +155,7 @@ public data class Manifest(
         public fun fromJSON(
             json: JSONObject?,
             mediaTypeRetriever: MediaTypeRetriever = MediaTypeRetriever(),
-            warnings: WarningLogger? = null
+            warnings: WarningLogger? = ConsoleWarningLogger()
         ): Manifest? {
             json ?: return null
 

readium/shared/src/main/java/org/readium/r2/shared/util/logging/WarningLogger.kt

@@ -109,7 +109,7 @@ public data class JsonWarning(
 
     override val tag: String = "json"
 
-    override val message: String get() = "${javaClass.name} ${modelClass.name}: $reason"
+    override val message: String get() = "${javaClass.simpleName} ${modelClass.name}: $reason"
 }
 
 /**

readium/streamer/src/main/java/org/readium/r2/streamer/ParserAssetFactory.kt

@@ -12,7 +12,6 @@ import org.readium.r2.shared.extensions.addPrefix
 import org.readium.r2.shared.publication.Manifest
 import org.readium.r2.shared.publication.Publication
 import org.readium.r2.shared.util.AbsoluteUrl
-import org.readium.r2.shared.util.MessageError
 import org.readium.r2.shared.util.ThrowableError
 import org.readium.r2.shared.util.Try
 import org.readium.r2.shared.util.Url
@@ -28,6 +27,7 @@ import org.readium.r2.shared.util.resource.Resource
 import org.readium.r2.shared.util.resource.ResourceContainer
 import org.readium.r2.shared.util.resource.RoutingContainer
 import org.readium.r2.streamer.parser.PublicationParser
+import timber.log.Timber
 
 internal class ParserAssetFactory(
     private val httpClient: HttpClient,
@@ -36,18 +36,18 @@ internal class ParserAssetFactory(
 ) {
 
     suspend fun createParserAsset(
-        asset: org.readium.r2.shared.util.asset.Asset
+        asset: Asset
     ): Try<PublicationParser.Asset, Publication.OpenError> {
         return when (asset) {
-            is org.readium.r2.shared.util.asset.Asset.Container ->
+            is Asset.Container ->
                 createParserAssetForContainer(asset)
-            is org.readium.r2.shared.util.asset.Asset.Resource ->
+            is Asset.Resource ->
                 createParserAssetForResource(asset)
         }
     }
 
     private fun createParserAssetForContainer(
-        asset: org.readium.r2.shared.util.asset.Asset.Container
+        asset: Asset.Container
     ): Try<PublicationParser.Asset, Publication.OpenError> =
         Try.success(
             PublicationParser.Asset(
@@ -57,7 +57,7 @@ internal class ParserAssetFactory(
         )
 
     private suspend fun createParserAssetForResource(
-        asset: org.readium.r2.shared.util.asset.Asset.Resource
+        asset: Asset.Resource
     ): Try<PublicationParser.Asset, Publication.OpenError> =
         if (asset.mediaType.isRwpm) {
             createParserAssetForManifest(asset)
@@ -66,34 +66,28 @@ internal class ParserAssetFactory(
         }
 
     private suspend fun createParserAssetForManifest(
-        asset: org.readium.r2.shared.util.asset.Asset.Resource
+        asset: Asset.Resource
     ): Try<PublicationParser.Asset, Publication.OpenError> {
         val manifest = asset.resource.readAsRwpm()
             .mapFailure { Publication.OpenError.InvalidAsset(ThrowableError(it)) }
             .getOrElse { return Try.failure(it) }
 
-        val baseUrl =
-            manifest.linkWithRel("self")?.href?.resolve()
-                ?: return Try.failure(
-                    Publication.OpenError.InvalidAsset(
-                        MessageError("No self link in the manifest.")
-                    )
-                )
-
-        if (baseUrl !is AbsoluteUrl) {
-            return Try.failure(
-                Publication.OpenError.InvalidAsset(
-                    MessageError("Self link is not absolute.")
+        val baseUrl = manifest.linkWithRel("self")?.href?.resolve()
+        if (baseUrl == null) {
+            Timber.w("No self link found in the manifest at ${asset.resource.source}")
+        } else {
+            if (baseUrl !is AbsoluteUrl) {
+                return Try.failure(
+                    Publication.OpenError.InvalidAsset("Self link is not absolute.")
                 )
-            )
-        }
-
-        if (!baseUrl.isHttp) {
-            return Try.failure(
-                Publication.OpenError.UnsupportedAsset(
-                    "Self link doesn't use the HTTP(S) scheme."
+            }
+            if (!baseUrl.isHttp) {
+                return Try.failure(
+                    Publication.OpenError.UnsupportedAsset(
+                        "Self link doesn't use the HTTP(S) scheme."
+                    )
                 )
-            )
+            }
         }
 
         val container =
@@ -114,7 +108,7 @@ internal class ParserAssetFactory(
     }
 
     private fun createParserAssetForContent(
-        asset: org.readium.r2.shared.util.asset.Asset.Resource
+        asset: Asset.Resource
     ): Try<PublicationParser.Asset, Publication.OpenError> {
         // Historically, the reading order of a standalone file contained a single link with the
         // HREF "/$assetName". This was fragile if the asset named changed, or was different on

test-app/src/main/java/org/readium/r2/testapp/MainActivity.kt

@@ -16,6 +16,8 @@ import androidx.navigation.ui.setupActionBarWithNavController
 import androidx.navigation.ui.setupWithNavController
 import com.google.android.material.bottomnavigation.BottomNavigationView
 import com.google.android.material.snackbar.Snackbar
+import org.readium.r2.testapp.utils.extensions.readium.toDebugDescription
+import timber.log.Timber
 
 class MainActivity : AppCompatActivity() {
 
@@ -59,7 +61,8 @@ class MainActivity : AppCompatActivity() {
                     getString(R.string.import_publication_success)
 
                 is MainViewModel.Event.ImportPublicationError -> {
-                    event.errorMessage
+                    Timber.e(event.error.toDebugDescription(this))
+                    event.error.getUserMessage(this)
                 }
             }
         Snackbar.make(

test-app/src/main/java/org/readium/r2/testapp/MainViewModel.kt

@@ -16,6 +16,7 @@ import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.flow.receiveAsFlow
 import kotlinx.coroutines.launch
 import org.readium.r2.testapp.domain.Bookshelf
+import org.readium.r2.testapp.domain.ImportError
 import org.readium.r2.testapp.utils.EventChannel
 
 class MainViewModel(
@@ -41,8 +42,7 @@ class MainViewModel(
     private fun sendImportFeedback(event: Bookshelf.Event) {
         when (event) {
             is Bookshelf.Event.ImportPublicationError -> {
-                val errorMessage = event.error.getUserMessage(app)
-                channel.send(Event.ImportPublicationError(errorMessage))
+                channel.send(Event.ImportPublicationError(event.error))
             }
             Bookshelf.Event.ImportPublicationSuccess -> {
                 channel.send(Event.ImportPublicationSuccess)
@@ -56,7 +56,7 @@ class MainViewModel(
             Event()
 
         class ImportPublicationError(
-            val errorMessage: String
+            val error: ImportError
         ) : Event()
     }
 }

test-app/src/main/java/org/readium/r2/testapp/data/BookRepository.kt

@@ -21,7 +21,7 @@ import org.readium.r2.testapp.data.db.BooksDao
 import org.readium.r2.testapp.data.model.Book
 import org.readium.r2.testapp.data.model.Bookmark
 import org.readium.r2.testapp.data.model.Highlight
-import org.readium.r2.testapp.utils.extensions.authorName
+import org.readium.r2.testapp.utils.extensions.readium.authorName
 
 class BookRepository(
     private val booksDao: BooksDao

test-app/src/main/java/org/readium/r2/testapp/outline/BookmarksFragment.kt

@@ -28,7 +28,7 @@ import org.readium.r2.testapp.data.model.Bookmark
 import org.readium.r2.testapp.databinding.FragmentListviewBinding
 import org.readium.r2.testapp.databinding.ItemRecycleBookmarkBinding
 import org.readium.r2.testapp.reader.ReaderViewModel
-import org.readium.r2.testapp.utils.extensions.outlineTitle
+import org.readium.r2.testapp.utils.extensions.readium.outlineTitle
 import org.readium.r2.testapp.utils.viewLifecycle
 
 class BookmarksFragment : Fragment() {