Merge minor corrections from PR #88 review

r2-shared/build.gradle

@@ -53,6 +53,7 @@ dependencies {
 
     testImplementation "org.jetbrains.kotlin:kotlin-test-junit:$kotlin_version"
     testImplementation 'junit:junit:4.12'
+    testImplementation 'org.assertj:assertj-core:3.14.0'
     testImplementation 'org.mockito:mockito-core:2.19.0'
     testImplementation 'xmlpull:xmlpull:1.1.3.1'
     testImplementation 'net.sf.kxml:kxml2:2.3.0'

r2-shared/src/main/java/org/readium/r2/shared/URLHelper.kt

@@ -11,13 +11,14 @@ package org.readium.r2.shared
 
 import android.net.Uri
 import java.net.URI
+import java.net.URLDecoder
 
 fun getAbsolute(href: String, base: String): String {
     return try {
         val baseURI = URI.create(base)
         val relative = baseURI.resolve(href)
         relative.toString()
-    }catch (e:IllegalArgumentException){
+    } catch (e:IllegalArgumentException){
         val hrefUri = Uri.parse(href)
         if (hrefUri.isAbsolute){
             href
@@ -27,26 +28,24 @@ fun getAbsolute(href: String, base: String): String {
     }
 }
 
-
-internal fun normalize(base: String, in_href: String?) : String {
-    if (in_href == null || in_href.isEmpty()) {
-        return ""
-    }
-    val hrefComponents = in_href.split( "/").filter { it.isNotEmpty() }
-    val baseComponents = base.split( "/").filter { it.isNotEmpty() }
-    baseComponents.dropLast(1)
-
-    val replacementsNumber = hrefComponents.filter { it == ".." }.count()
-    var normalizedComponents = hrefComponents.filter { it != ".." }
-    for (e in 0 until replacementsNumber) {
-        baseComponents.dropLast(1)
-    }
-    normalizedComponents = baseComponents + normalizedComponents
-    var normalizedString = ""
-    for (component in normalizedComponents) {
-        normalizedString += "/$component"
+fun normalize(base: String, href: String?): String {
+    val resolved = if (href.isNullOrEmpty()) ""
+    else try { // href is returned by resolve if it is absolute
+        val absoluteUri = URI.create(base).resolve(href)
+        val absoluteString = absoluteUri.toString() // this is a percent-decoded
+        val addSlash = absoluteUri.scheme == null && !absoluteString.startsWith("/")
+        (if (addSlash) "/" else "") + absoluteString
+    } catch (e: IllegalArgumentException){ // one of the URIs is ill-formed
+        val hrefUri = Uri.parse(href) // Android Uri is more forgiving
+        // Let's try to return something
+        if (hrefUri.isAbsolute) {
+            href
+        } else if (base.startsWith("/")) {
+            base + href
+        } else
+            "/" + base + href
     }
-    return normalizedString
+    return URLDecoder.decode(resolved, "UTF-8")
 }
 
 

r2-shared/src/main/java/org/readium/r2/shared/parser/xml/XmlParser.kt

@@ -1,8 +1,8 @@
 /*
  * Module: r2-shared-kotlin
- * Developers: Aferdita Muriqi, Clément Baumann
+ * Developers: Quentin Gliosca
  *
- * Copyright (c) 2018. Readium Foundation. All rights reserved.
+ * Copyright (c) 2020. Readium Foundation. All rights reserved.
  * Use of this source code is governed by a BSD-style license which is detailed in the
  * LICENSE file present in the project repository where this source code is maintained.
  */
@@ -14,16 +14,28 @@ import org.xmlpull.v1.XmlPullParserException
 import org.xmlpull.v1.XmlPullParserFactory
 import java.io.IOException
 import java.io.InputStream
+import java.util.Locale
 import java.util.Stack
 import javax.xml.XMLConstants
 
-class XmlParser (val isNamespaceAware: Boolean = true, val isCaseSensitive: Boolean = true) {
+/** XML Parser with support for namespaces, mixed content and lang inheritance
+ *
+ * [isNamespaceAware] behaves as defined in XmlPullParser specification.
+ * If [isCaseSensitive] is false, attribute and tag names are lowercased during the parsing
+ */
+class XmlParser(val isNamespaceAware: Boolean = true, val isCaseSensitive: Boolean = true) {
+
+    val parser: XmlPullParser = XmlPullParserFactory.newInstance().let {
+        it.isNamespaceAware = isNamespaceAware
+        it.newPullParser()
+    }
+
     @Throws(XmlPullParserException::class, IOException::class)
-    fun parse(stream: InputStream) : ElementNode {
-        val parser = buildParser(isNamespaceAware)
+    fun parse(stream: InputStream): ElementNode {
         parser.setInput(stream, null) // let the parser try to determine input encoding
 
         val stack = Stack<Triple<MutableList<Node>, AttributeMap, String>>()
+        // stack contains children, attributes, and lang
         stack.push(Triple(mutableListOf(), mutableMapOf(), ""))
         var text = ""
 
@@ -32,16 +44,17 @@ class XmlParser (val isNamespaceAware: Boolean = true, val isCaseSensitive: Bool
                 XmlPullParser.START_TAG -> {
                     maybeAddText(text, stack.peek().first)
                     text = ""
-                    val attributes =  buildAttributeMap(parser)
-                    val langAttr = if (isNamespaceAware) attributes[XMLConstants.XML_NS_URI]?.get("lang")
-                    else attributes[""]?.get("xml:lang")
+                    val attributes = buildAttributeMap(parser)
+                    val langAttr =
+                        if (isNamespaceAware) attributes[XMLConstants.XML_NS_URI]?.get("lang")
+                        else attributes[""]?.get("xml:lang")
                     stack.push(Triple(mutableListOf(), attributes, langAttr ?: stack.peek().third))
                 }
                 XmlPullParser.END_TAG -> {
                     val (children, attributes, lang) = stack.pop()
                     maybeAddText(text, children)
                     text = ""
-                    val element = buildElement(parser, attributes, children, lang)
+                    val element = buildElement(attributes, children, lang)
                     stack.peek().first.add(element)
                 }
                 XmlPullParser.TEXT, XmlPullParser.ENTITY_REF -> {
@@ -62,81 +75,80 @@ class XmlParser (val isNamespaceAware: Boolean = true, val isCaseSensitive: Bool
         }
     }
 
-    private fun buildParser(isNamespaceAware: Boolean) : XmlPullParser {
-        val factory = XmlPullParserFactory.newInstance()
-        factory.isNamespaceAware = isNamespaceAware
-        return factory.newPullParser()
-    }
-
     private fun maybeAddText(text: String, children: MutableList<Node>) {
         if (text.isNotEmpty()) {
-           children.add(TextNode(text))
+            children.add(TextNode(text))
         }
     }
 
-    private fun buildElement(parser: XmlPullParser, attributes:AttributeMap, children: MutableList<Node>, lang: String) : ElementNode {
+    private fun buildElement(attributes: AttributeMap, children: MutableList<Node>, lang: String): ElementNode {
         val rawName = parser.name
-        val name = if (isCaseSensitive) rawName else rawName.toLowerCase()
-        val node = ElementNode(name, parser.namespace,  lang, attributes, children)
-        return node
+        val name = if (isCaseSensitive) rawName else rawName.toLowerCase(Locale.getDefault())
+        return ElementNode(name, parser.namespace, lang, attributes, children)
     }
 
-    private fun buildAttribute(parser: XmlPullParser, index: Int) : Attribute {
+    private fun buildAttribute(index: Int): Attribute {
         with(parser) {
             val rawName = getAttributeName(index)
-            val name = if (isCaseSensitive) rawName else rawName.toLowerCase()
-            return Attribute(name,
-                    getAttributeNamespace(index),
-                    getAttributeValue(index))
+            val name = if (isCaseSensitive) rawName else rawName.toLowerCase(Locale.getDefault())
+            return Attribute(name, getAttributeNamespace(index), getAttributeValue(index))
         }
     }
 
-    private fun buildAttributeMap(parser: XmlPullParser) : AttributeMap {
-        val attributes = (0 until parser.attributeCount).map { buildAttribute(parser, it) }
+    private fun buildAttributeMap(parser: XmlPullParser): AttributeMap {
+        val attributes = (0 until parser.attributeCount).map { buildAttribute(it) }
         val namespaces = attributes.map(Attribute::namespace).distinct()
-        return namespaces.associateWith { ns -> attributes
-                .filter{ it.namespace == ns }.associate { Pair(it.name, it.value) } }
+        return namespaces.associateWith { ns ->
+            attributes.filter { it.namespace == ns }.associate { Pair(it.name, it.value) }
+        }
     }
 }
 
-open class Node
+data class Attribute(val name: String, val namespace: String, val value: String)
 
-data class TextNode(val text: String) : Node()
+typealias AttributeMap = Map<String, Map<String, String>>
 
-data class Attribute(val name: String, val namespace: String, val value:String)
+sealed class Node
 
-typealias AttributeMap = Map<String,Map<String,String>>
+/** Container for text in the XML tree */
+data class TextNode(val text: String) : Node()
 
+/** Represents a node with children in the XML tree */
 data class ElementNode(
-        val name: String,
-        val namespace: String = "",
-        val lang: String = "",
-        val attributes: AttributeMap = mapOf(),
-        val children: List<Node> = listOf()) : Node() {
-
-    // Text of the first child, if it is a TextNode, otherwise null
+    val name: String,
+    val namespace: String = "",
+    val lang: String = "",
+    val attributes: AttributeMap = emptyMap(),
+    val children: List<Node> = listOf()
+) : Node() {
+
+    /** Text of the first child if it is a [TextNode], or null otherwise */
     val text: String?
         get() = (children.firstOrNull() as? TextNode)?.text
 
-    // Id with fallback to XML namespace
+    /** Return the [id] attribute as specified in [getAttr] with fallback to XML namespace */
     val id: String?
         get() = getAttr("id") ?: getAttrNs("id", XMLConstants.XML_NS_URI)
 
-    // Get attribute in the same namespace as this ElementNode or in no namespace
+    /** Return the value of an attribute picked in the same namespace as this [ElementNode],
+     * fallback to no namespace and at last to null. */
     fun getAttr(name: String) = getAttrNs(name, namespace) ?: getAttrNs(name, "")
 
-    // Get attribute in a specific namespace
+    /** Return the value of an attribute picked in a specific namespace or null if it does not exist */
     fun getAttrNs(name: String, namespace: String) = attributes[namespace]?.get(name)
 
-    // Get all ElementNode children
+    /** Return a list of all ElementNode children */
     fun getAll() = children.filterIsInstance<ElementNode>()
 
-    // Get ElementNode children with specific name and namespace
-    fun get(name: String, namespace: String) = getAll().filter { it.name == name && it.namespace == namespace}
+    /** Return a list of [ElementNode] children with the given name and namespace */
+    fun get(name: String, namespace: String) =
+        getAll().filter { it.name == name && it.namespace == namespace }
 
+    /** Return the first [ElementNode] child with the given name and namespace, or null if there is none */
     fun getFirst(name: String, namespace: String) = get(name, namespace).firstOrNull()
 
-    fun collect(name: String, namespace: String) : List<ElementNode> {
+    /** Recursively collect all descendent [ElementNode] with the given name and namespace into a list */
+    fun collect(name: String, namespace: String): List<ElementNode> {
         val founded: MutableList<ElementNode> = mutableListOf()
         for (c in getAll()) {
             if (c.name == name && c.namespace == namespace) founded.add(c)
@@ -145,10 +157,11 @@ data class ElementNode(
         return founded
     }
 
-    fun collectText() : String {
+    /** Recursively collect and concatenate all descendent [TextNode] in depth-first order */
+    fun collectText(): String {
         val text = StringBuilder()
         for (c in children) {
-            when(c) {
+            when (c) {
                 is TextNode -> text.append(c.text)
                 is ElementNode -> text.append(c.collectText())
             }

r2-shared/src/test/java/org/readium/r2/shared/URLHelperTest.kt

@@ -0,0 +1,44 @@
+/*
+ * Module: r2-streamer-kotlin
+ * Developers: Quentin Gliosca
+ *
+ * Copyright (c) 2018. Readium Foundation. All rights reserved.
+ * Use of this source code is governed by a BSD-style license which is detailed in the
+ * LICENSE file present in the project repository where this source code is maintained.
+ */
+
+package org.readium.r2.shared
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+
+class NormalizeTest {
+    @Test
+    fun `Anchors are accepted as href`() {
+        assertThat(normalize("OEBPS/xhtml/nav.xhtml", "#toc")).isEqualTo("/OEBPS/xhtml/nav.xhtml#toc")
+    }
+
+    @Test
+    fun `Directories are accepted as base`() {
+        assertThat(normalize("OEBPS/xhtml/", "nav.xhtml")).isEqualTo("/OEBPS/xhtml/nav.xhtml")
+    }
+
+    @Test
+    fun `href is returned unchanged if it is an absolute path`() {
+        assertThat(normalize("OEBPS/content.opf", "/OEBPS/xhtml/index.xhtml")).isEqualTo("/OEBPS/xhtml/index.xhtml")
+    }
+
+    @Test
+    fun `href is returned unchanged if it is an absolute URI`() {
+        assertThat(normalize("OEBPS/content.opf", "http://example.org/index.xhtml"))
+    }
+
+    @Test
+    fun `Result is percent-decoded`() {
+        val base = "OEBPS/xhtml/%E4%B8%8A%E6%B5%B7%2B%E4%B8%AD%E5%9C%8B/base.xhtml"
+        val href = "%E4%B8%8A%E6%B5%B7%2B%E4%B8%AD%E5%9C%8B.xhtml"
+        assertThat(normalize(base, href)).isEqualTo("/OEBPS/xhtml/上海+中國/上海+中國.xhtml")
+        assertThat(normalize("OEBPS/xhtml%20files/nav.xhtml", "chapter1.xhtml")).isEqualTo("/OEBPS/xhtml files/chapter1.xhtml")
+
+    }
+}