search

openjverein / jverein

Open JVerein - Open Source Vereinsverwaltung
https://openjverein.github.io
GNU General Public License v3.0
42 stars 15 forks source link

Lösung zu #159 #184

Closed JohannMaierhofer closed 3 months ago

JohannMaierhofer commented 6 months ago

Inzwischen habe ich einiges dazu gelernt und das Problem der alten Implementierung verstanden.

Jetzt gibt es dann in jedem Fall nur ein Query über die Mitgliedskonten. Durch den Filter im Query sollten es aber nicht viel mehr sein als es Mitglieder gibt, eher weniger. Nur wenn man alle gewählt hat wird sehr viel gelesen und erst später auf ein Mitglied gefiltert wenn eines angegeben ist. In diesem Fall werden in Vorschlag 1 weniger Daten geholt aber dort mehr Queries gemacht. Ich weiß aber nicht was problematischer ist, die Datenmenge die geholt wird oder die Anzahl von Queries die ausgeführt werden. Ich könnte mir vorstellen, dass diese Implementierung hier noch besser ist als die in #182.

PS: Den Vergleich mit Zweck habe ich auch entfernt und die Länge der Namen auf >2 reduziert.

Ich weiß jetzt nicht wie wir weiter vorgehen. Sollte man zwei Testbuilds machen und es Lukas messen lassen was schneller ist. Oder glauben wir gleich, dass diese hier besser ist, da sie nur ein einziges Query macht?

Endgültig sollte natürlich nur einer der beiden Pull Requests übernommen werden.

dippeal commented 6 months ago

Dir fehlt wohl der mitgliedskonto.betrag im SQL select:

[ERROR][main][de.willuhn.jameica.gui.input.DialogInput$1.handleEvent] error while opening dialog java.lang.RuntimeException: java.rmi.RemoteException: error while executing sql statement: Unknown column 'mitgliedskonto.betrag' in 'having clause'; nested exception is: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Unknown column 'mitgliedskonto.betrag' in 'having clause' at de.willuhn.jameica.gui.dialogs.AbstractDialog$4.run(AbstractDialog.java:504) at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:183) at org.eclipse.swt.widgets.Display.syncExec(Display.java:5960) at de.willuhn.jameica.gui.dialogs.AbstractDialog.open(AbstractDialog.java:489) at de.willuhn.jameica.gui.input.DialogInput$1.handleEvent(DialogInput.java:81) at de.willuhn.jameica.gui.input.ButtonInput$1.widgetSelected(ButtonInput.java:118) at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:252) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89) at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1529) at org.eclipse.swt.widgets.Display.runDeferredEvents(Display.java:5065) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4517) at de.willuhn.jameica.gui.GUI.loop(GUI.java:938) at de.willuhn.jameica.gui.GUI.init(GUI.java:335) at de.willuhn.jameica.system.Application.init(Application.java:145) at de.willuhn.jameica.system.Application.newInstance(Application.java:87) at de.willuhn.jameica.Main.main(Main.java:78) Caused by: java.rmi.RemoteException: error while executing sql statement: Unknown column 'mitgliedskonto.betrag' in 'having clause'; nested exception is: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Unknown column 'mitgliedskonto.betrag' in 'having clause' at de.willuhn.datasource.db.DBServiceImpl.execute(DBServiceImpl.java:463) at de.jost_net.JVerein.gui.control.MitgliedskontoControl.getMitgliedskontoIterator(MitgliedskontoControl.java:823) at de.jost_net.JVerein.gui.control.MitgliedskontoControl.getMitgliedskontoList(MitgliedskontoControl.java:640) at de.jost_net.JVerein.gui.dialogs.MitgliedskontoAuswahlDialog.paint(MitgliedskontoAuswahlDialog.java:110) at de.willuhn.jameica.gui.dialogs.AbstractDialog$4.run(AbstractDialog.java:499) ... 16 more Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Unknown column 'mitgliedskonto.betrag' in 'having clause' at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) at com.mysql.jdbc.Util.handleNewInstance(Util.java:403) at com.mysql.jdbc.Util.getInstance(Util.java:386) at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:944) at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3933) at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3869) at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:2524) at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:2675) at com.mysql.jdbc.ConnectionImpl.execSQL(ConnectionImpl.java:2465) at com.mysql.jdbc.PreparedStatement.executeInternal(PreparedStatement.java:1915) at com.mysql.jdbc.PreparedStatement.executeQuery(PreparedStatement.java:2023) at de.willuhn.datasource.db.DBServiceImpl.execute(DBServiceImpl.java:457) ... 20 more

JohannMaierhofer commented 6 months ago

@dippeal Ich bekomme diese Ausgabe nicht. Bei mir läuft es wunderbar. Kann es an der Datenbank liegen? Ich habe eine h2db Datenbank. Der Name ist jverein.mv.db.

JohannMaierhofer commented 6 months ago

Habe den Fix für MySQL gemacht

dippeal commented 6 months ago

Eine Implementierung mit weniger requests gegen die DB dafür mit größerem Datensatz ist effizienter bei den meisten Hostern. Der Grund dafür sind Limits wie max_connections.

Macht es vielleicht Sinn die Vor-/Namen mit LIKE UPPER('TEILname%') zu suchen, damit man auch Ergebnisse bekommt wenn man nur einen Teil des Namens eingibt?

JohannMaierhofer commented 6 months ago

Ich finde den Vorschlag gut. Dann liefert die Datenbank auch nur die gefundenen Mitgliedskonten und man kann sich den Matchingalgorithmus der im ResultSetExtractor implementiert ist auch sparen. Was ist besser, die mitgliedskonten Ids im ResultSetExtractor zu sammeln und dann später zu erzeugen, wie jetzt im Code oder soll ich sie gleich im ResultSetExtractor erzeugen?

JohannMaierhofer commented 6 months ago

Warum habe ich es nicht gleich so gemacht wie in dem zweiten Tab. Jetzt passiert auch der Namens Match im SQL. So einfach geht's dann und alles mit nur einem Query. Das mit dem Teilstring habe ich jetzt nicht hier gemacht sondern in dem neuen Feature zur Angleichung des Suchen. Ich werde da die Spezial-Suche Checkbox wie im zweiten Tab auch in den ersten Tab einbauen.

JohannMaierhofer commented 6 months ago

Ich glaube den ersten Vorschlag #182 können wir verwerfen.

lukas-mertens commented 5 months ago

@JohannMaierhofer Vielen Dank für die Implementierung! Wir werden demnächst unsere JVerein-Installation auf Openjverein umstellen und es dann testen. Da wir einen recht großen Verein mit vielen Sonderfällen/komplexer eigener IT haben, sind wir noch nicht dazu gekommen. Da ich ja ein Kopfgeld für den Fix versprochen hatte: Schick mir einfach per Mail dein PayPal oder einen guten Zweck deiner Wahl, du hast dir das Kopfgeld für den Bugfix verdient und er wird unsere Arbeit im Verein bedeutend angenehmer machen ;)

JohannMaierhofer commented 5 months ago

Es gibt jetzt noch eine Sache, die mir aufgefallen ist. Die neue Implementierung über den Namensvergleich mit SQL hat den Nachteil, dass die Umlaute ä, ö etc. nicht in ae, oe etc. umgewandelt werden. Wenn also in der Buchung eine Umlautersetzung durch die Bank etc. stattgefunden hat, dann gibt es evtl. keinen Match.

Ich habe mir jetzt gedacht, dass man einen Schalter einzubauen könnte mit dem man die Ersetzung einschalten kann. Dann würde der Code aus dem vorhergehenden Commit durchlaufen. Der sollte auch besser sein als der ursprüngliche Code. Man würde das halt nur einschalten wenn man sieht, dass der Suchname eine Umlautersetzung beinhaltet.

Natürlich könnte man aber genau so gut einfach den Suchnamen editieren und die Umlautersetzung korrigieren. Das wäre wohl einfacher.

Was meint ihr?

Hallo @lukas-mertens, ich verzichte auf das auf das Kopfgeld. Vielleicht hat ja unser Team einmal ein eigenes Spendenkonto. Dann kannst du dich revanchieren.

dippeal commented 5 months ago

Es gibt jetzt noch eine Sache, die mir aufgefallen ist. Die neue Implementierung über den Namensvergleich mit SQL hat den Nachteil, dass die Umlaute ä, ö etc. nicht in ae, oe etc. umgewandelt werden. Wenn also in der Buchung eine Umlautersetzung durch die Bank etc. stattgefunden hat, dann gibt es evtl. keinen Match.

Ich habe mir jetzt gedacht, dass man einen Schalter einzubauen könnte mit dem man die Ersetzung einschalten kann. Dann würde der Code aus dem vorhergehenden Commit durchlaufen. Der sollte auch besser sein als der ursprüngliche Code. Man würde das halt nur einschalten wenn man sieht, dass der Suchname eine Umlautersetzung beinhaltet.

Natürlich könnte man aber genau so gut einfach den Suchnamen editieren und die Umlautersetzung korrigieren. Das wäre wohl einfacher.

Was meint ihr?

Hallo @lukas-mertens, ich verzichte auf das auf das Kopfgeld. Vielleicht hat ja unser Team einmal ein eigenes Spendenkonto. Dann kannst du dich revanchieren.

Einen Schalter finde ich eine unschöne Lösung.

Nachfolgendes muss für name und vorname implementiert werden und das upper muss berücksichtigt werden. SQL: REPLACE(REPLACE(REPLACE(REPLACE((mitglied.name), 'ö' 'oe'), 'ü' 'ue'), 'ä' 'ae'), 'ß' 'ss') AS name

oder die where Klausel erweitern um umgewandelte Eingaben/Suchnamen: upper(mitglied.name) like upper() or upper(mitglied.name) like upper()

Könnte mir vorstellen, dass die Java-Umwandlung etwas performanter und Fehler unanfälliger ist.

JohannMaierhofer commented 5 months ago

Das mit dem Replace hat nicht funktioniert. wenn man das im SELECT macht wird zwar für die Ausgabe ersetzt aber nicht im where. Das Replace in das where Statement einbauen hat immer zu einem Syntaxfehler geführt. Aber vielleicht habe ich es ja falsch gemacht. Ich habe die Änderungen wieder rückgängig gemacht und lasse den Vergleich im Java. Darum habe ich auch den Schalter für die Spezialsuche auskommentiert. Der Vorschlag mit LIKE UPPER('TEILname%') geht ja dann nicht weil der Namensvergleich nicht mehr im SQL passiert.

JohannMaierhofer commented 3 months ago

Nachdem der Pull Request schon 3 Monate offen ist, ist er wohl noch nicht so richtig akzeptiert. Auch nach dem Kommentar zu #233 habe ich gelernt, dass die Verwendung des DBIterator effizienter ist als das Erzeugen über ResultSet. Ich habe darum einige Änderungen gemacht abhängig von den jeweiligen Eingaben.

Es gäbe noch ein Lösung bei der man auch im letzten Fall den DBIterator nehmen könnte aber die Lösung wäre nicht sehr schön. Man könnte in der Mitgliedskonto Tabelle neben dem Sollbetrag auch den aktuellen Istbetrag speichern. Der müsste bei jeder Buchungs Zuweisung oder Ablösung angepasst werden. Das ist immer ein Risiko, dass die Werte auseinander laufen und der Istbetrag nicht mehr der Summe der zugeordneten Buchungen entspricht. Da müsste man alle möglichen Fälle abfangen z.B. auch wenn der Betrag der Buchung geändert wird.

@dippeal , @willuhn , @MSchmalzl , @NicoB77 was ist euere Meinung? Ich denke man sollte diese Implementierung in die nächste Version übernehmen. Dann könnte Lukas testen ob es sein Problem aus #159 löst. Wenn nicht kann man immer noch schauen ob man noch mehr machen kann. Zumindest hat diese Implementierung im dritten Fall oben, der ja wohl das Anwendungszenario von Lukas ist, erheblich weniger DB Queries als die aktuelle Implementierung. Siehe Beschreibung oben.

Damit wäre ich dann auch am Ende meiner Ideen zu neuen Features, ich habe euch ja auch in letzter Zeit stark mit Reviews beschäftigt. Vielen Dank noch mal. Ich denke, dass die nächste Release den Anwendern sicher gefallen wird. Wir haben ja eine ganze Menge an neuen Features und Bug Fixes implementiert.