Improved handling of files with errors (#16)

* Enabled Detekt for project * Extract method * Ignored doubled childs and qualifiers * Commented sample_50.xmp and also avoid errors on files with comments. * Added another test file that triggers a problem * Fixed possible NullPointerException in XMPNormalizer * Ignore empty about='' * Failed test should point to problematic sample file * Better info on failed XML read * Skip unit tests for problematic files on iOS + reformatting * Removed comments from sample files as this was the problem. * Bumped version
Ashampoo · Dec 12, 2023 · f027478 · f027478
1 parent 2815304
commit f027478
Show file tree

Hide file tree

Showing 25 changed files with 1,580 additions and 1,433 deletions.
diff --git a/.idea/detekt.xml b/.idea/detekt.xml
diff --git a/README.md b/README.md
@@ -15,7 +15,7 @@ It's part of [Ashampoo Photos](https://ashampoo.com/photos).
 ## Installation
 
 ```
-implementation("com.ashampoo:xmpcore:0.2.3")
+implementation("com.ashampoo:xmpcore:0.2.4")
 ```
 
 ## How to use

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/DomParser.kt b/src/commonMain/kotlin/com/ashampoo/xmp/DomParser.kt
@@ -36,7 +36,7 @@ object DomParser {
             return writer.target
 
         } catch (ex: Exception) {
-            throw XMPException("Error reading the XML-file", XMPError.BADSTREAM, ex)
+            throw XMPException("Error reading the XML file: ${ex.message}", XMPError.BADSTREAM, ex)
         }
     }
 }
diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPIterator.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPIterator.kt
@@ -10,11 +10,11 @@ package com.ashampoo.xmp
 
 import com.ashampoo.xmp.XMPNodeUtils.findNode
 import com.ashampoo.xmp.XMPNodeUtils.findSchemaNode
-import com.ashampoo.xmp.xpath.XMPPath
-import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
 import com.ashampoo.xmp.options.IteratorOptions
 import com.ashampoo.xmp.options.PropertyOptions
 import com.ashampoo.xmp.properties.XMPPropertyInfo
+import com.ashampoo.xmp.xpath.XMPPath
+import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
 
 /**
  * Interface for the `XMPMeta` iteration services.

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPMeta.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPMeta.kt
@@ -8,14 +8,6 @@
 // =================================================================================================
 package com.ashampoo.xmp
 
-import com.ashampoo.xmp.XMPPathFactory.composeArrayItemPath
-import com.ashampoo.xmp.XMPPathFactory.composeQualifierPath
-import com.ashampoo.xmp.XMPPathFactory.composeStructFieldPath
-import com.ashampoo.xmp.XMPUtils.convertToBoolean
-import com.ashampoo.xmp.XMPUtils.convertToDouble
-import com.ashampoo.xmp.XMPUtils.convertToInteger
-import com.ashampoo.xmp.XMPUtils.convertToLong
-import com.ashampoo.xmp.XMPUtils.decodeBase64
 import com.ashampoo.xmp.Utils.normalizeLangValue
 import com.ashampoo.xmp.XMPNodeUtils.appendLangItem
 import com.ashampoo.xmp.XMPNodeUtils.chooseLocalizedText
@@ -24,12 +16,20 @@ import com.ashampoo.xmp.XMPNodeUtils.findNode
 import com.ashampoo.xmp.XMPNodeUtils.setNodeValue
 import com.ashampoo.xmp.XMPNodeUtils.verifySetOptions
 import com.ashampoo.xmp.XMPNormalizer.normalize
-import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
+import com.ashampoo.xmp.XMPPathFactory.composeArrayItemPath
+import com.ashampoo.xmp.XMPPathFactory.composeQualifierPath
+import com.ashampoo.xmp.XMPPathFactory.composeStructFieldPath
+import com.ashampoo.xmp.XMPUtils.convertToBoolean
+import com.ashampoo.xmp.XMPUtils.convertToDouble
+import com.ashampoo.xmp.XMPUtils.convertToInteger
+import com.ashampoo.xmp.XMPUtils.convertToLong
+import com.ashampoo.xmp.XMPUtils.decodeBase64
 import com.ashampoo.xmp.options.IteratorOptions
 import com.ashampoo.xmp.options.ParseOptions
 import com.ashampoo.xmp.options.PropertyOptions
 import com.ashampoo.xmp.properties.XMPProperty
 import com.ashampoo.xmp.properties.XMPPropertyInfo
+import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
 
 /**
  * This class represents the set of XMP metadata as a DOM representation. It has methods to read and

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPMetaParser.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPMetaParser.kt
@@ -10,6 +10,7 @@ package com.ashampoo.xmp
 
 import com.ashampoo.xmp.XMPNormalizer.normalize
 import com.ashampoo.xmp.options.ParseOptions
+import nl.adaptivity.xmlutil.dom.Comment
 import nl.adaptivity.xmlutil.dom.Element
 import nl.adaptivity.xmlutil.dom.Node
 import nl.adaptivity.xmlutil.dom.ProcessingInstruction
@@ -117,6 +118,11 @@ internal object XMPMetaParser {
                 if (result != null)
                     result[2] = child.getData()
 
+            } else if (child is Comment) {
+
+                /* We ignore comments. */
+                continue
+
             } else if (child !is Text && child !is ProcessingInstruction) {
 
                 val childElement = child as Element

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPNode.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPNode.kt
@@ -64,7 +64,9 @@ class XMPNode(
 
     fun addChild(node: XMPNode) {
 
-        assertChildNotExisting(node.name!!)
+        /* Ignore doubled childs. */
+        if (childExists(node.name!!))
+            return
 
         node.parent = this
 
@@ -73,7 +75,9 @@ class XMPNode(
 
     fun addChild(index: Int, node: XMPNode) {
 
-        assertChildNotExisting(node.name!!)
+        /* Ignore doubled childs. */
+        if (childExists(node.name!!))
+            return
 
         node.parent = this
 
@@ -146,7 +150,9 @@ class XMPNode(
 
     fun addQualifier(qualNode: XMPNode) {
 
-        assertQualifierNotExisting(qualNode.name!!)
+        /* Ignore doubled qualifiers. */
+        if (qualifierExists(qualNode.name!!))
+            return
 
         qualNode.parent = this
         qualNode.options.setQualifier(true)
@@ -305,21 +311,9 @@ class XMPNode(
         return qualifier!!
     }
 
-    /**
-     * Checks that a node name is not existing on the same level, except for array items.
-     */
-    private fun assertChildNotExisting(childName: String) {
-
-        if (XMPConst.ARRAY_ITEM_NAME != childName && findChildByName(childName) != null)
-            throw XMPException("Duplicate property or field node '$childName'", XMPError.BADXMP)
-    }
+    private fun childExists(childName: String): Boolean =
+        XMPConst.ARRAY_ITEM_NAME != childName && findChildByName(childName) != null
 
-    /**
-     * Checks that a qualifier name is not existing on the same level.
-     */
-    private fun assertQualifierNotExisting(qualifierName: String) {
-
-        if (XMPConst.ARRAY_ITEM_NAME != qualifierName && findQualifierByName(qualifierName) != null)
-            throw XMPException("Duplicate '$qualifierName' qualifier", XMPError.BADXMP)
-    }
+    private fun qualifierExists(qualifierName: String): Boolean =
+        XMPConst.ARRAY_ITEM_NAME != qualifierName && findQualifierByName(qualifierName) != null
 }
diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPNodeUtils.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPNodeUtils.kt
@@ -8,14 +8,14 @@
 // =================================================================================================
 package com.ashampoo.xmp
 
-import com.ashampoo.xmp.XMPUtils.encodeBase64
 import com.ashampoo.xmp.Utils.normalizeLangValue
 import com.ashampoo.xmp.Utils.replaceControlCharsWithSpace
 import com.ashampoo.xmp.Utils.splitNameAndValue
-import com.ashampoo.xmp.xpath.XMPPath
-import com.ashampoo.xmp.xpath.XMPPathSegment
+import com.ashampoo.xmp.XMPUtils.encodeBase64
 import com.ashampoo.xmp.options.AliasOptions
 import com.ashampoo.xmp.options.PropertyOptions
+import com.ashampoo.xmp.xpath.XMPPath
+import com.ashampoo.xmp.xpath.XMPPathSegment
 
 /**
  * Utilities for `XMPNode`.

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPNormalizer.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPNormalizer.kt
@@ -9,9 +9,9 @@
 package com.ashampoo.xmp
 
 import com.ashampoo.xmp.Utils.checkUUIDFormat
-import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
 import com.ashampoo.xmp.options.ParseOptions
 import com.ashampoo.xmp.options.PropertyOptions
+import com.ashampoo.xmp.xpath.XMPPathParser.expandXPath
 
 internal object XMPNormalizer {
 
@@ -230,20 +230,16 @@ internal object XMPNormalizer {
 
         val strictAliasing = options.getStrictAliasing()
 
-        val schemaIt: Iterator<XMPNode> = tree.iterateChildren()
-
-        while (schemaIt.hasNext()) {
+        val schemas = tree.iterateChildren().asSequence().toList()
 
-            val currSchema = schemaIt.next()
+        for (currSchema in schemas) {
 
             if (!currSchema.hasAliases)
                 continue
 
-            val propertyIt = currSchema.iterateChildrenMutable()
+            val properties = currSchema.iterateChildrenMutable().asSequence().toList()
 
-            while (propertyIt.hasNext()) {
-
-                val currProp = propertyIt.next()
+            for (currProp in properties) {
 
                 if (!currProp.isAlias)
                     continue
@@ -282,7 +278,8 @@ internal object XMPNormalizer {
                             baseSchema.addChild(currProp)
 
                             // remove the alias property
-                            propertyIt.remove()
+
+                            currSchema.removeChild(currProp)
 
                         } else {
 
@@ -296,7 +293,9 @@ internal object XMPNormalizer {
 
                             baseSchema.addChild(baseNode)
 
-                            transplantArrayItemAlias(propertyIt, currProp, baseNode)
+                            transplantArrayItemAlias(currProp, baseNode) {
+                                currSchema.removeChild(currProp)
+                            }
                         }
 
                     } else if (info.getAliasForm().isSimple()) {
@@ -307,7 +306,7 @@ internal object XMPNormalizer {
                         if (strictAliasing)
                             compareAliasedSubtrees(currProp, baseNode, true)
 
-                        propertyIt.remove()
+                        currSchema.removeChild(currProp)
 
                     } else {
 
@@ -330,14 +329,16 @@ internal object XMPNormalizer {
 
                         if (itemNode == null) {
 
-                            transplantArrayItemAlias(propertyIt, currProp, baseNode)
+                            transplantArrayItemAlias(currProp, baseNode) {
+                                currSchema.removeChild(currProp)
+                            }
 
                         } else {
 
                             if (strictAliasing)
                                 compareAliasedSubtrees(currProp, itemNode, true)
 
-                            propertyIt.remove()
+                            currSchema.removeChild(currProp)
                         }
                     }
                 }
@@ -350,15 +351,15 @@ internal object XMPNormalizer {
     /**
      * Moves an alias node of array form to another schema into an array
      *
-     * @param propertyIt the property iterator of the old schema (used to delete the property)
+     * @param removeChildFromTree lambda used to delete the property of the schema
      * @param childNode  the node to be moved
      * @param baseArray  the base array for the array item
      *
      */
     private fun transplantArrayItemAlias(
-        propertyIt: MutableIterator<XMPNode>,
         childNode: XMPNode,
-        baseArray: XMPNode
+        baseArray: XMPNode,
+        removeChildFromTree: () -> Unit
     ) {
 
         if (baseArray.options.isArrayAltText()) {
@@ -372,7 +373,7 @@ internal object XMPNormalizer {
             childNode.addQualifier(langQual)
         }
 
-        propertyIt.remove()
+        removeChildFromTree()
 
         childNode.name = XMPConst.ARRAY_ITEM_NAME
 

diff --git a/src/commonMain/kotlin/com/ashampoo/xmp/XMPRDFParser.kt b/src/commonMain/kotlin/com/ashampoo/xmp/XMPRDFParser.kt
@@ -653,9 +653,7 @@ internal object XMPRDFParser {
     /**
      * 7.2.21 emptyPropertyElt
      * start-element ( URI == propertyElementURIs,
-     * attributes == set (
-     * idAttr?, ( resourceAttr | nodeIdAttr )?, propertyAttr* ) )
-     * end-element()
+     * attributes == set (idAttr?, ( resourceAttr | nodeIdAttr )?, propertyAttr* ) ) end-element()
      *
      * <ns:Prop1></ns:Prop1>
      * <ns:Prop2 rdf:resource="http: *www.adobe.com/"></ns:Prop2>
@@ -701,7 +699,7 @@ internal object XMPRDFParser {
                 "Nested content not allowed with rdf:resource or property attributes", XMPError.BADRDF
             )
 
-        // First figure out what XMP this maps to and remember the XML node for a simple value.
+        /* First figure out what XMP this maps to and remember the XML node for a simple value. */
         for (index in 0 until xmlNode.attributes.length) {
 
             val attribute = xmlNode.attributes.item(index) as Attr
@@ -713,9 +711,15 @@ internal object XMPRDFParser {
 
             when (attrTerm) {
 
-                RDFTERM_ID -> {
-                    /* Do nothing. */
-                }
+                /*
+                 * Do nothing.
+                 */
+                RDFTERM_ID -> continue
+
+                /*
+                 * sample_52.xmp has an <rdf:li rdf:about=''/> we want to skip.
+                 */
+                RDFTERM_ABOUT -> continue
 
                 RDFTERM_RESOURCE -> {
 
@@ -769,8 +773,12 @@ internal object XMPRDFParser {
                     }
                 }
 
+                /* Fail on unknown elements. */
                 else ->
-                    throw XMPException("Unrecognized attribute of empty property element", XMPError.BADRDF)
+                    throw XMPException(
+                        "Unrecognized attribute of empty property element: $attrTerm",
+                        XMPError.BADRDF
+                    )
             }
         }
 
@@ -816,9 +824,13 @@ internal object XMPRDFParser {
 
             when (attrTerm) {
 
-                RDFTERM_ID, RDFTERM_NODE_ID -> {
-                    /* Do nothing. */
-                }
+                /* Do nothing with IDs. */
+                RDFTERM_ID, RDFTERM_NODE_ID -> continue
+
+                /*
+                 * sample_52.xmp has an <rdf:li rdf:about=''/> we want to skip.
+                 */
+                RDFTERM_ABOUT -> continue
 
                 RDFTERM_RESOURCE ->
                     addQualifierNode(childNode, "rdf:resource", attribute.value)
@@ -834,7 +846,7 @@ internal object XMPRDFParser {
                 }
 
                 else -> throw XMPException(
-                    "Unrecognized attribute of empty property element",
+                    "Unrecognized attribute of empty property element: $attrTerm",
                     XMPError.BADRDF
                 )
             }