Fix mets file id check including an option for use of this check (kit…

…odo#6123) * Fix strict file id check including an option for use of this check * Adjust used exception message
subhh · Jul 19, 2024 · 79e51fb · 79e51fb
1 parent 9517a72
commit 79e51fb
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 7 deletions.
diff --git a/Kitodo-DataFormat/pom.xml b/Kitodo-DataFormat/pom.xml
@@ -44,6 +44,14 @@
             <groupId>org.junit.jupiter</groupId>
             <artifactId>junit-jupiter-engine</artifactId>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-junit-jupiter</artifactId>
+        </dependency>
     </dependencies>
 
     <build>

diff --git a/Kitodo-DataFormat/src/main/java/org/kitodo/dataformat/access/MetsXmlElementAccess.java b/Kitodo-DataFormat/src/main/java/org/kitodo/dataformat/access/MetsXmlElementAccess.java
@@ -44,6 +44,7 @@
 import org.kitodo.api.dataformat.ProcessingNote;
 import org.kitodo.api.dataformat.Workpiece;
 import org.kitodo.api.dataformat.mets.MetsXmlElementAccessInterface;
+import org.kitodo.config.KitodoConfig;
 import org.kitodo.dataformat.metskitodo.DivType;
 import org.kitodo.dataformat.metskitodo.FileType;
 import org.kitodo.dataformat.metskitodo.Mets;
@@ -452,19 +453,21 @@ private StructLink createStructLink(LinkedList<Pair<String, String>> smLinkData)
     private Map<FileType, String> createFileUseByFileCache(Mets mets) {
         HashMap<FileType, String> fileUseMap = new HashMap<>();
         FileSec fileSec = mets.getFileSec();
+        Set<String> usedFileId = new HashSet<>();
+        boolean strictCheck = KitodoConfig.getConfig().getBoolean("useStrictMetsFileIdCheck", false);
         if (Objects.nonNull(fileSec)) {
             for (FileGrp fileGrp : fileSec.getFileGrp()) {
                 String use = fileGrp.getUSE();
                 for (FileType file : fileGrp.getFile()) {
-                    if (fileUseMap.containsKey(file)) {
+                    if (strictCheck && usedFileId.contains(file.getID())) {
                         throw new IllegalArgumentException(
-                            "Corrupt file: file with id " + file.getID() + " is part of multiple groups"
+                            "Corrupt file: each METS file ID has to be unique but " + file.getID() + " is used multiple times!"
                         );
                     } else {
+                        usedFileId.add(file.getID());
                         fileUseMap.put(file, use);
                     }
                 }
-
             }
         }
         return fileUseMap;

diff --git a/Kitodo-DataFormat/src/test/java/org/kitodo/dataformat/access/MetsXmlElementAccessIT.java b/Kitodo-DataFormat/src/test/java/org/kitodo/dataformat/access/MetsXmlElementAccessIT.java
@@ -11,6 +11,7 @@
 
 package org.kitodo.dataformat.access;
 
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -30,8 +31,9 @@
 import java.util.List;
 import java.util.stream.Collectors;
 
-import org.junit.jupiter.api.Disabled;
+import org.apache.commons.configuration2.PropertiesConfiguration;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.kitodo.api.MdSec;
 import org.kitodo.api.MetadataEntry;
 import org.kitodo.api.MetadataGroup;
@@ -41,7 +43,12 @@
 import org.kitodo.api.dataformat.ProcessingNote;
 import org.kitodo.api.dataformat.View;
 import org.kitodo.api.dataformat.Workpiece;
+import org.kitodo.config.KitodoConfig;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.jupiter.MockitoExtension;
 
+@ExtendWith(MockitoExtension.class)
 public class MetsXmlElementAccessIT {
 
     private static final File OUT_FILE = new File("src/test/resources/out.xml");
@@ -273,15 +280,30 @@ public void missingMetsFileForPointer() {
     }
 
     @Test
-    @Disabled("See GitHub discussion https://github.com/kitodo/kitodo-production/discussions/6087")
-    public void duplicateMetsFileDefinition() {
+    public void duplicateMetsFileDefinitionWithoutStrictFileIdCheck() throws IOException {
+        try (InputStream fileContent = new FileInputStream("src/test/resources/meta_duplicate_file.xml")) {
+            assertDoesNotThrow(
+                () -> new MetsXmlElementAccess().read(fileContent)
+            );
+        }
+    }
+
+    @Test
+    public void duplicateMetsFileDefinitionWithStrictFileIdCheck() {
+        // mock access to KitodoConfig usage
+        PropertiesConfiguration propertiesConfiguration = Mockito.mock(PropertiesConfiguration.class);
+        MockedStatic<KitodoConfig> mockedConfig = Mockito.mockStatic(KitodoConfig.class);
+        mockedConfig.when(KitodoConfig::getConfig).thenReturn(propertiesConfiguration);
+        // mock getBoolean method call like in the main class
+        Mockito.when(propertiesConfiguration.getBoolean("useStrictMetsFileIdCheck", false)).thenReturn(true);
+
         Exception exception = assertThrows(IllegalArgumentException.class,
                 () -> new MetsXmlElementAccess().read(
                         new FileInputStream("src/test/resources/meta_duplicate_file.xml")
                 )
             );
 
-        assertEquals("Corrupt file: file with id FILE_0001 is part of multiple groups", exception.getMessage());
+        assertEquals("Corrupt file: each METS file ID has to be unique but FILE_0001 is used multiple times!", exception.getMessage());
     }
 
     @Test

diff --git a/Kitodo/src/main/resources/kitodo_config.properties b/Kitodo/src/main/resources/kitodo_config.properties
@@ -419,6 +419,10 @@ issue.colours=#f94a15;#0071bc;#42ba37;#ee7e5b;#1e3946;#ca2f00;#AAAAFF;#000055;#0
 # Minimal average number of pages per process in newspaper process creation
 numberOfPages.minimum=1
 
+# Use strict mets:fileId check or not. Property is used inside the Kitodo-DataFormat module.
+# For more information see German GitHub discussion https://github.com/kitodo/kitodo-production/discussions/6087
+# On default check is disabled
+useStrictMetsFileIdCheck=false
 
 # -----------------------------------
 # Batch processing

diff --git a/pom.xml b/pom.xml
@@ -588,6 +588,12 @@ from system library in Java 11+ -->
                 <version>${mockito.version}</version>
                 <scope>test</scope>
             </dependency>
+            <dependency>
+                <groupId>org.mockito</groupId>
+                <artifactId>mockito-junit-jupiter</artifactId>
+                <version>${mockito.version}</version>
+                <scope>test</scope>
+            </dependency>
             <dependency>
                 <groupId>org.omnifaces</groupId>
                 <artifactId>omnifaces</artifactId>