Discussion: Make regex fields in PDFExtractor more reusable

[buchen]

Problem statement:

• The "sections" in PDF extractors contain a lot of strings that must match certain values. For example, the values must match "currency", "fee", or "tax" in order for the AbstractPDFExtractor#processTaxEntries and #processFeeEntries methods to work. Similarly, the values must be "withHoldingTax" or "creditableWithHoldingTax" for the #processWithHoldingTaxEntries. It would be nice to have compiler support here.
• Plus: a lot of pattern for amounts, dates, names, etc. are repeated inside the regular expressions.

@Nirus2000 writes in #2682

So richtig zufrieden bin ich innerlich aber irgendwie nicht. Eigentlich nervt mich an den Importer gesamt der Aufbau der einzelnen sections und die Vereinheitlichung (Standardisierung). Der Ansatz "Enums" dafür zu verwenden find ich eigentlich ganz cool.

    public enum RecordField
    {
        // If switch the transaction e.g. from buy to sell
        TYPE("type"), //$NON-NLS-1$

        // Security name
        NAME("name"), //$NON-NLS-1$

        // ISIN
        ISIN("isin"), //$NON-NLS-1$

        // WKN
        WKN("wkn"), //$NON-NLS-1$

        // Valuta date
        DATE("date"), //$NON-NLS-1$

        // Currency amount
        AMOUNT("amount"), //$NON-NLS-1$

        // Foreign currency amount
        fxAMOUNT("fxAmount"), //$NON-NLS-1$

        // Currency or Currency of security
        CURRENCY("currency"), //$NON-NLS-1$

        // Foreign currency
        fxCURRENCY("fxCurrency"), //$NON-NLS-1$

        // Shares
        SHARES("shares"), //$NON-NLS-1$

        // Taxes
        TAX("tax"), //$NON-NLS-1$

        // Tax refund
        TAXREFUND("taxRefund"), //$NON-NLS-1$

        // Fees
        FEE("fee"), //$NON-NLS-1$

        // Fee refund
        FEEREFUND("feeRefund"), //$NON-NLS-1$

        // Currency exchange rate
        EXCHANGERATE("exchangeRate"), //$NON-NLS-1$

        // Withholding tax
        WITHHOLDINGTAX("withholdingTax"), //$NON-NLS-1$

        // Creditable withholding tax
        creditableWITHHOLDINGTAX("creditableWithholdingTax"); //$NON-NLS-1$

        private static final Map<String, RecordField> LOOKUP = new HashMap<String, RecordField>();

        static
        {
            for (RecordField m : EnumSet.allOf(RecordField.class))
                LOOKUP.put(m.getRecordField(), m);
        }

        private String RecordField;

        RecordField(String envRecordField)
        {
            this.RecordField = envRecordField;
        }

        public String getRecordField()
        {
            return RecordField;
        }

        public String getRegEx(String pattern)
        {
            return "(?<" + RecordField + ">" + pattern + ")"; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
        }

        public static RecordField get(String RecordField)
        {
            return LOOKUP.get(RecordField);
        }
    }

Man müsste nun die .section irgendwie aufbohren, dass dies noch irgendwie "fluffig" funktioniert und geschmeidig aussieht. Im Kopf habe ich dazu folgende Idee.

.section(RecordField.NAME.getRecordField(), RecordField.ISIN.getRecordField(), RecordField.SHARES.getRecordField(), RecordField.CURRENCY.getRecordField())
.find("Gattungsbezeichnung ISIN")
.match("^" + RecordField.NAME.getRegEx(".*") + " " + RecordField.ISIN.getRegEx("[\\w]{12}") + "$")
.match("^STK " + RecordField.SHARES.getRegEx("[\\.,\\d]+") + " " + RecordField.CURRENCY.getRegEx("[\\w]{3}") + "$")
.assign((t, v) -> {
    t.setShares(asShares(v.get(RecordField.SHARES.getRecordField())));
    t.setSecurity(getOrCreateSecurity(v)); <--- noch kein Plan... :-)
})

Dadurch wir irgendwie alles lang, also fehlt mir hier noch die Idee wie man dass auch shorten kann, dass das ganze so aussieht...

.section(NAME, ISIN, SHARES, CURRENCY)
.find("Gattungsbezeichnung ISIN")
.match("^" + NAME.getRegEx(".*") + " " + ISIN.getRegEx("[\\w]{12}") + "$")
.match("^STK " + SHARES.getRegEx("[\\.,\\d]+") + " " + CURRENCY.getRegEx("[\\w]{3}") + "$")
.assign((t, v) -> {
    t.setShares(asShares(v.get(SHARES)));
    t.setSecurity(getOrCreateSecurity(v)); <--- immernoch kein Plan... :-P
})

oder noch smarter... und wie die .find()-funktion gleich auch noch "^" und "$" mit integrieren.

.section(NAME, ISIN, SHARES, CURRENCY)
.find("Gattungsbezeichnung ISIN")
.match("NAME.(".*") + " " + ISIN.("[\\w]{12}"))
.match("STK " + SHARES.("[\\.,\\d]+") + " " + CURRENCY.("[\\w]{3}") )
.assign((t, v) -> {
    t.setShares(asShares(SHARES));
    t.setSecurity(getOrCreateSecurity(v)); <--- immernoch kein Plan... :-P
})

[buchen]

Dadurch wir irgendwie alles lang, also fehlt mir hier noch die Idee wie man dass auch shorten kann, dass das ganze so aussieht...

Wenn es keine Enum ist, sondern ein Interface (oder eine Hilfsklasse), dann könnte man auf jeden Fall mit "static imports" arbeiten und damit den Source kürzen.

Ich denke wir müssen uns ein paar Fragen stellen:

• Macht es Sinn die regulären Ausdrücke durch String concatenations zu bauen?
• Wenn wir Ausdrücke extrahieren, müssen dann alle Ausdrücklich als Konstanten ausgedrückt werden - selbst wenn sie nur in einer Datei verwendet werden?
• Wie können wir die "input conditions" für Methoden wie #getOrCreateSecurity und #processFees deutlich machen?

Zu der Frage der regulären Ausdrücke:

.match("STK " + SHARES("[\\.,\\d]+") + " " + CURRENCY("[\\w]{3}") )

Ich denke schon, dass man den Code für die einfachen Fälle zu gekürzt bekommt. Ich bin aber sehr skeptisch, dass diese reguläre Ausdrücke später noch lesbar bleiben. Man kann sie nicht mehr ein eine regex Editor kopieren, ein neue Contributor kann sie nicht mehr lesen sondern muss erst die Automatismen dahinter lernen. Klar, man könnte etwas Doppelung sparen, aber bringt das so viel? Die meisten regulären Ausdrücke bleiben doch statisch - oder werden konkret wegen einer Datei geändert.

Und auch jetzt schon testet der Code ja ob alle Ausdrücke gefunden werden. Man muss ja die Liste der erwarteten Attribute (hier: currency und fee) vorher angeben und es gibt einen "sanity check" ob alle gefunden wurden.

                .section("currency", "fee").optional()
                .match("^Provision (?<currency>[\\w]{3}) (\\-)?(?<fee>[\\.,\\d]+)$")

Aber lass uns mal überlegen, welche Alternativen es gibt.

Man könnte "nur" die String Werte in der Section und beim Abruf ersetzen (die Konsistenz würde weiterhin durch den "sanity check" geprüft).

                .section($currency, $fee).optional()
                .match("^Provision (?<currency>[\\w]{3}) (\\-)?(?<fee>[\\.,\\d]+)$")
                .assign((t, v) -> {
                   long fee = asFee(v.get($fee));
                })

Dann zu der Frage welche Ausdrücke zu extrahieren sind: meine persönliche Meinung ist wir sollten nur Ausdrücke extrahieren, die auch in anderen Klassen verwendet werden (Wertpapiere, Steuern, Gebühren). Wenn ein Ausdruck nur in einer Klasse verwendet wird, finde ich es okay wenn der in der Klasse definiert und genutzt wird.

Zu der Frage der Input Conditions:

t.setSecurity(getOrCreateSecurity(v)); <--- noch kein Plan... :-)

Hier habe ich auch keinen Plan. Klar, man könnte die Map schon beim Aufruf der Methode auflösen: also eine Signatur von getOrCreateSecurity(isin,wkn,ticker,name...). Aber macht es das besser?

[eztam-]

Dadurch, dass ich erst neu hinzugestoßen bin und gerade meinen ersten PDF Extractor entwickelt habe, habe ich vielleicht noch einen etwas distanzierteren (aber natürlich auch naiveren) Blick und möchte gerne meine Eindrücke schildern.

Für mich war es am Anfang etwas schwer reinzufinden und ich musste erst ein paar Breakpoints setzten um den Mapping-Prozess zu verstehen. Ich fand es etwas verwirrend, wie das Parsing mit dem Transaction Model verwoben ist.

Ich würde darum vorschlagen, das Parsing vom Transaction Model zu trennen.

• Transaction Model Die Erzeugung des Models ist sehr konkret und kann gut abstrahiert werden. Hier könnte man eine klare Schnittstelle schaffen, z.B. durch Verwendung des Builder Patterns.
• Parsing Die verschiedenen Bank Dokumente sind jedoch sehr unterschiedlich weshalb eine weitere Abstraktion des Parsings, wie oben vorgeschlagen aus meiner Sicht eher schwierig und teilweise auch einschränkend sein könnte. Hier sollte man den individuellen Implementierungen eher Freiraum lassen.

[tquellenberg]

Ich würde darum vorschlagen, das Parsing vom Transaction Model zu trennen.

Ohne da jetzt tief drin zu stecken, finde ich das auch eine gutes Ziel. Das Parsing wird immer ein großer Zoo an viel Code bleiben der stark in Bewegung ist. Es sollte aber möglichst 'Idotensicher' sein, die geparsten Daten dann in PP einzuspeisen.

Man könnte auch darüber nachdenken, das Parsing in ein eigenes Modul auszulagern und eventuell sogar sowas wie dynamisches Groovy etc für das Parsen zu erlauben, damit dort neue Versionen veröffentlich werden können, ohne das der Kern von PP betroffen ist.