Prüfung auf mehrfach identische FileIDs

[henning-gerhardt]

Während der Umstellung des Testframeworks Junit von der Version 4 auf die Version 5 ist aufgefallen, dass ein Test im Module "Kitodo-DataFormat sich anders verhält als erwartet.

Dieser Test soll prüfen, ob die genutzten FileIDs in der internen METS/Kitodo Datei im Dokument einmalig vergeben sind. Aufgrund des Aufbau des bestehenden Tests wurde nur dann ein Fehler im Test erkannt, wenn eine Exception erzeugt wurde. Der Test lief allerdings auch dann erfolgreich durch, wenn überhaupt keine Exception erzeugt wurde. Dies liegt grundsätzlich im Aufbau des geschriebenen Tests, wobei nach der Umstellung auf Junit5 explizit auf die den nicht betrachteten Fall (keine Exception wird erzeugt) vom Test-Framework hingewiesen wird.

Die einfache Lösung wäre das Entfernen des Testes. Da die Prüfung auf Mehrfach-Nutzung der FileIDs jedoch wichtig ist und eben auch nach METS-Standard gar nicht erlaubt ist , muss eine Anpassung stattfinden. Eine mögliche Reparatur der Prüfung ist entwickelt und der Test verhält sich mit dieser Änderung sowohl mit Junit 4 als auch mit Junit 5 entsprechend korrekt:

private Map<FileType, String> createFileUseByFileCache(Mets mets) {
    HashMap<FileType, String> fileUseMap = new HashMap<>();
    FileSec fileSec = mets.getFileSec();
    Set<String> usedFileId = new HashSet<>();
    if (Objects.nonNull(fileSec)) {
        for (FileGrp fileGrp : fileSec.getFileGrp()) {
            String use = fileGrp.getUSE();
            for (FileType file : fileGrp.getFile()) {
                if (usedFileId.contains(file.getID())) {
                    throw new IllegalArgumentException(
                        "Corrupt file: file with id " + file.getID() + " is part of multiple groups"
                    );
                } else {
                    usedFileId.add(file.getID());
                    fileUseMap.put(file, use);
                }
            }
             
        }
    }
    return fileUseMap;
}

Mit der Reparatur der Prüfung ergibt sich aber eine Änderung, die in der regulären Nutzung der Anwendung nur eine (hoffentlich) geringe Auswirkung haben sollte. Jedoch ist es nicht auszuschließen, dass es in der Nutzung von Kitodo.Production zur Mehrfachnutzung von FileIDs gekommen sein kann. Wahrscheinlicher ist der Fall, dass interne METS-Dateien manuell manipuliert wurden, was zu Zeiten von 2.x von manchen Einrichtungen durchaus gemacht wurde. Wenn an der Stelle der Nutzung / Manipulation der FileIDs unsauber gearbeitet wurde und es zu Dubletten / Mehrfach-Nutzungen der gleichen FileID gekommen ist.

Wenn die Änderung der Prüfung in den Code gelangt wird dieser Fehler beim Zugriff auf die interne METS-Datei in der Anwendung angezeigt. Eine Reparatur kann vermutlich nur von außerhalb durch direkte Manipulation der METS Datei stattfinden und / oder nur durch Entfernung der Datei-Referenzierungen über einen "komplizierten" Austausch der Bild- und Bild-Derivate-Dateien geschehen (Verschiebung der Bild- und Bildderivat-Dateien, Öffnen des Vorgangs im Metadaten-Editor und Aktualisierung (Leerung) der Datei-Referenzierungen, Speichern des Vorgangs und Schließen des Metadaten-Editors, Rück-Verschieben der Bild- und Bildderivat-Dateien, Neu-Öffnen des Metadaten-Editors und Neu-Referenzierung der Dateien).

Kann dieser Weg der Reparatur der Funktionalität gegangen werden, auch wenn nicht klar benannt werden kann, wie oft bzw. wie hoch dieser Fehlerfall nun auftreten wird?

Ziel ist es, dass die Umstellung von Junit4 auf Junit5 in kurzen zeitlichen Rahmen durchgeführt wird und ggf. weitere Verbesserungen, die durch diesen aufgefallenen Kontext sichtbar geworden sind, in späteren Pull Requests in die Software selbst einfließen zu lassen. Es soll auch in dem Pull Request nur die Änderungen enthalten sein, die zwingend notwendig sind, so dass die Komplexität des Code Reviews nicht unnötig erhöht wird.

[BartChris]

Kann dieser Weg der Reparatur der Funktionalität gegangen werden, auch wenn nicht klar benannt werden kann, wie oft bzw. wie hoch dieser Fehlerfall nun auftreten wird?

Wäre die Einführung eines optionalen "strict"-Mode evtl eine Option? Dann könnten Institutionen, bei denen die Prüfung zu erheblichen Problemen führt, den Duplikate-Check über die Kitodo-Konfiguration deaktivieren.

[henning-gerhardt]

Wäre die Einführung eines optionalen "strict"-Mode evtl eine Option? Dann könnten Institutionen, bei denen die Prüfung zu erheblichen Problemen führt, die Prüfung über die Kitodo-Konfiguration deaktivieren.

Dies wäre sicher möglich, dann sollte der PR für die Junit 4 -> Junit 5 jedoch aufgesplittet werden: Im Junit 4 -> Junit 5 PR wird der fragliche Test mit Verweis auf diese Diskussion deaktiviert. Im Anschluss / Nachdem der Junit 4 -> Junit 5 PR gemerged ist, wird ein neuer PR eröffnet, der diesen Test reaktiviert und eine Konfigurationsoption auf einen strikten und nicht-strikten Test an dieser Stelle einführt. Dies verkompliziert die Logik an dieser Stelle grundsätzlich, aber es wäre grundsätzlich möglich. Nach METS Standard ist dies aber nicht zulässig. Leider wird aber nicht auf METS Validität im Lade-Kontext des internen Formats geprüft (andere potentielle Baustelle).

Rückfrage: wie soll der Konfigurationswert für die neue Konfigurationsoption (checkMetsFileId ?)standardmäßig sein? Wenn man das aktuelle Verhalten nicht ändern will, müsste es nicht-strikt sein oder?

[BartChris]

Benötigen wir einen zusätzlichen Test für die nicht-strikte Validierung? Was soll im nicht-strikten Modus getestet werden?
Ich denke auch, dass manches für nicht-strict als Default spricht. Den nicht-strikten Modus als default zu behalten würde das Vorgehen in jedem Fall erleichtern, da dann bei Nicht-Vorhandensein eines solchen Wertes in der bisher genutzten config-Datei von einer nicht-strikten Prüfung ausgegangen werden kann. Andernfalls müsste optimalerweise auf die Konfiguration des Wertes überprüft und in Kitodo der fehlende config-Wert moniert werden, das scheint mir deutlich aufwendiger.

[henning-gerhardt]

Der nicht-strikte Fall ist ja jetzt schon mehr oder weniger da: trotz doppelter FileID darf die Exception nicht getriggert werden. Jedoch müsste dazu die Konfigurationsdatei während der Test-Ausführung änderbar sein oder zumindest diese Konfigurationsoption, was aktuell nicht geht bzw. nicht vorgesehen ist. Ggf. kann man sich mit zwei Konfigurationsdateien im Modul behelfen, die dann genutzt werden bzw. während des spezifischen Tests temporär ausgetauscht werden. Damit steigt dann die Komplexität ebenfalls wieder.

In der Prüfung im Programmcode wird ja der Konfigurationswert nur einmal pro Methodenaufruf ausgelesen und nur wenn dieser den Wert "strict" enthält, wird die strikte Prüfung durchgeführt.

[solth]

Ich denke auch, dass es gut wäre, das Verhalten konfigurierbar zu machen, wenn es schwierig abzuschätzen ist, wie viele Problemfälle bei Einrichtungen tatsächlich eintreten werden. Den Test mit einem entsprechenden Kommentar auf @Ignore zu setzen und die Arbeiten in mehrere Pull Requests aufzusplitten halte ich für ein gutes Vorgehen.