MySQL | PreparedStatements >> Query Methode fehlerhaft?

  • Moin moin an den aktiven Teil der Community!


    Seit mehreren Tagen quäle ich mich mit einem Problem, was wahrscheinlich ganz leicht gelöst werden kann, wenn man nur wüsste, wo man den Fehler versteckt hat.


    Bisher habe alles mit einfachen Statements gemacht, was MySQL und Datenbanken angeht. Aufgrund der Sicherheit, so wurde es mir erklärt, eignen sich PreparedStatements besser. Demnach bin ich nun dabei meine neuen Projekte ab jetzt mit PreparedStatements auszustatten, wenn es um MySQL/Datenbanken geht.


    Problembeschreibung: Ein (neuer) Spieler betritt den Server. Wie bei jedem Join wird geprüft, ob der Spieler schon in der Datenbank vorhanden ist oder nicht. Sollte der Spieler noch nicht in der Datenbank sein, wird logischerweise für ihn ein Eintrag erstellt. Nun ist es allerdings so, dass ich folgende Fehlermeldung erhalte, wenn ein neuer Spieler den Server betritt:


    Wenn der Spieler bereits in der Datenbank existiert, erhalte ich die gleiche Fehlermeldung, nur mit einer neuen Zusatzmeldung:


    Dass er mir den doppelten Eintrag melden möchte, ist klar. Das bedeutet aber theoretisch gleichzeitig auch, dass die Abfrage, ob der Spieler bereits in der Datenbank existiert, irgendwie fehlerhaft sein muss, weil es ja sonst nicht zu der Fehlermeldung mit dem doppelten Eintrag kommen würde.

    Natürlich interessiert euch der dazugehörige Code auch brennend, welchen ihr im folgenden nachsehen könnt:


    Ich persönlich kann mir nur vorstellen, dass da irgendein Fehler bei der Query-Methode sein kann. Die Update-Methode funktioniert einwandfrei.


    Ich freue mich über eure Hilfe!


    Mit freundlichen Grüßen
    Romian

  • Roomiiaan

    Hat das Label Ungelöst hinzugefügt
  • Hi erstmal,


    also zunächst steht die Lösung ja eigentlich schon in der Fehlermeldung

    Zitat

    java.sql.SQLException: Operation not allowed after ResultSet closed

    Du kannst leider nicht einfach ein ResultSetObjekt zurückgeben zumindest nicht wenn die Methode so aufgebaut ist wie hier :D

    A ResultSet object is automatically closed when the Statement object that generated it is closed, re-executed, or used to retrieve the next result from a sequence of multiple results.

    Du müsstest entweder das Query Ergebnis direkt verarbeiten ODER in irgendein anderes Objekt speichern. Hier bietet sich eine Tabelle ganz gut an


    Solltest du dich dazu entscheiden es umzuwandeln sollte dir das hier helfen:

    Code
    1. ResultSet rs = null;
    2. HashBasedTable<Integer, String, Object> table = HashBasedTable.create();
    3. while (rs.next()) {
    4. ResultSetMetaData metaData = rs.getMetaData();
    5. for (int i = 1; i < metaData.getColumnCount(); i++) {
    6. String column = metaData.getColumnName(i);
    7. table.put(i, column, rs.getObject(column));
    8. }
    9. }

    Am Ende solltest du dann eine Repräsentation des Query Ergebnisses haben.

    Die Spalten kannst du über den Spalten Namen abfragen und die Zeilen über die Nummer 1 - X

    bzw mit einem Loop durchgehen ist ebenfalls ein Kinderspiel :D


    Weiter zu der 2 ten Fehlermeldung, das ist kein Zusatz das ist ein ganz anderes Problem.


    Das Problem ist das du versuchst einen neuen Eintrag zu erstellen obwohl die eindeutige ID bereits vergeben ist, hier ist es die UUID.

    Damit dieses Problem nicht mehr vorkommt mach einfach folgendes. Verändere dieses Statement:


    INSERT INTO `system_players`(UUID, USERNAME, IP) VALUES (?, ?, ?)


    zu diesem:


    INSERT INTO `system_players`(UUID, USERNAME, IP) VALUES (?, ?, ?) ON DUPLICATE KEY UPDATE USERNAME=?, IP=?


    Das macht folgendes sollte der PRIMARY KEY wie in deinem Fall schon vorhanden sein führt SQL stattdessen ein UPDATE aus. Ist die kurze Variante von prüfen ob der Key schon vorhanden ist und dann Einfügen bzw Updaten :D


    Allerdings musst du dann den die Parameter für USERNAME und IP wiederholt übergeben.

    Es gibt eine Möglichkeit Parameter zu benennen aber das wäre zu ausschweifend deswegen hier einfach ein Link falls es dich interessiert.

    https://stackoverflow.com/ques…/named-parameters-in-jdbc

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Guten Morgen zM4xi !


    Vielen Dank für deine Antwort!


    Ich denke, dass es in dem Fall nicht nötig ist die Ergebnisse zwischenzuspeichern. Du sagtest, dass ich die Query auch einfach direkt verarbeiten kann. Da stehe ich, um ehrlich zu sein, gerade ein wenig auf dem Schlauch und wüsste nicht, wie ich dort ansetzen soll.


    Mit freundlichen Grüßen

    Romian

  • Ja direkte Verarbeitung wäre wenn du das Statement nicht über deine Utility Klasse ausführst sondern direkt da wo du die Daten brauchst und dann direkt aus dem ResultSetausliest.


    Aber da Utility Klassen ja eigentlich genau dazu da sind, wäre die andere Variante wohl besser. Und ich würde das nicht als Zwischenspeichern bezeichnen.

    Wenn dein Schiff sinkt fährst du ja auch nicht noch bis zum nächsten Hafen, sondern rufst ein anderes Schiff und lädst deine Ware um! (Metapher)


    Da es nun mal die unveränderliche Regel gibt, dass das ResultSet gelöscht wird nachdem das Statement geschlossen wird, muss ich halt meine Daten in ein anderes Objekt verfrachten wenn ich diese weiter verwenden will.

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Kann es sein, dass das nur bei PreparedStatements der Fall ist? Bei normalen Statements funktioniert folgende Methode z.B. einwandfrei, wenn ich diese genau an der gleichen Stelle abrufe.



    Kann es sein, dass das nur bei PreparedStatements der Fall ist? Bei normalen Statements funktioniert folgende Methode z.B. einwandfrei, wenn ich diese genau an der gleichen Stelle abrufe.


    Anzumerken sei allerdings, dass die Query-Methode dann so aussieht:

  • Nein, was du hier machst ist die direkte Verarbeitung der Daten.

    Du gibts hier schließlich nicht das ResultSet zurück bzw. verwendest es nicht nachdem das Statement geschlossen wurde, sondern prüfst ob die Spalte UUID nicht leer ist

    und gibst das Ergebnis aus dieser Prüfung zurück.


    Grundsätzlich sollte bei einer Query-Abfrage immer ein PreparedStatement verwendet. Vertraue niemals User Input!

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Das hier, diese Methode ist das Problem. In so einem try-with-resources wie du ihn hier verwendest kannst du nicht einfach jede Klasse verwenden,

    sondern nur welche die das interface AutoCloseable implementieren.

    Und das gilt auch für ein PreparedStatement bzw dessen Superinterface Statement.


    Was diese besondere Art von try-catch Block macht ist, es schließt die "Resource" automatisch am Ende. bedeutet jede nachfolgende Verwendung der "Resource" Instanz wird ein Operation not allowed werfen, weil diese nun mal geschlossen ist.

    Kurz gesagt diese Objekte sind nicht dazu gedacht in irgendeinem anderen "Scope" verwendet zu werden, als in dem es Initialisiert wurde.


    Jedoch selbst wenn du nicht ein try-with-resources nutzen würdest, sondern ein try-finally

    wäre das das selbe Problem. Am Ende der Leine wartet immer die Tatsache das das Objekt geschlossen wurde.


    Wenn du jedoch zum Beispiel es einfach nicht schließen würdest hättest du ein ganz anderes Problem würde das Statement offen bleiben mag das JDBC noch viel weniger.

    By default, only one ResultSet object per Statement object can be open at the same time. Therefore, if the reading of one ResultSet object is interleaved with the reading of another, each must have been generated by different Statement objects. All execution methods in the Statement interface implicitly close a statment's current ResultSet object if an open one exists.

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Hm... Ich habe auf jeden Fall noch einiges nachzuholen, was Datenbanken angeht... #WirdZeitFürEinNeuesBuch :D (Der Link wurde da automatisch hinzugefügt, lol)


    Was hältst du von folgender Methode?

  • Naja Methode ist Ok aber gibt halt immer noch ein geschlossenes ResultSet zurück.


    Dachtest du die Leute hier würden das jedesmal manuell einfügen, wenn es den Begriff im Lexikon gibt xD :D

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Okay, dann bin ich mit meinen Latein nun wirklich endgültig am Ende. Ich würde tatsächlich eigentlich ungern auf die normalen Statements zugreifen, da sie natürlich im Bezug auf SQL Injections ziemlich anfällig sind...


    Dachtest du die Leute hier würden das jedesmal manuell einfügen, wenn es den Begriff im Lexikon gibt xD :D

    Naja... Ich nehme jetzt einfach mal als Ausrede, dass ich hier neu im Forum bin und nicht wusste, dass das Lexikon sowas automatisch hinzufügt usw. ;)

  • Hallo zusammen!


    Ich habe jetzt mal wieder alles so umgecodet, dass ich wieder mit ganz normalen Statements arbeite... Und jetzt funktioniert halt auch alles wieder einwandfrei. Es ist tatsächlich relativ komisch...

    Gibt es eventuell eine Möglichkeit/Methode, mit welcher ich SQL-Injections verhindern kann?


    Mit freundlichen Grüßen

    Romian

  • Naja also du kannst halt einfach nach gewissen Keywörtern im Input suchen ";", "UNION", "--" etc. Das sind so die bekanntesten Keywords die auf eine Injection hinweisen denke ich.

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Ich hab mal bisschen gegoogelt und alle sagen verwende einfach PreparedStatments. Es gibt in Java keinen Explicit String oder sowas und deswegen wird eine SQL Injection immer irgendwie möglich sein.

    If you are homeless ... just buy a house, duh!

    and if you wanna have a plugin matching your conditions ... just code it yourself!

  • Eine SQL Injection ist in deinem Fall ohne hin eher schwierig. Du darfst nicht vergessen, dass der Spieler auch die Möglichkeit haben muss, eine String irgendwo einzugeben. Also wenn du jetzt nicht grade irgendwas total abgefahrenes mit StringInputs via ChatBox implementiert hast, sondern der Spieler nur hin und wieder Geld von einem Konto abbucht, kann dir da quasi nichts passieren.
    Du musst halt bei dem Validieren des Userinputs ein bisschen mehr aufpassen.

    Aber auch mein Ratschlag: Verwende einfach PreparedStatements. Die sind darüber hinaus, wenn man es richtig anstellt, nicht nur sicherer, sondern auch performanter (hier dazu ein kleiner Beitrag https://stackoverflow.com/ques…dstatement-multiple-times). Dein Code wird dadurch sicherer und Nachhaltiger.

    Das einzige, was du an deinem Code oben ändern musst, ist halt dein PreparedStatement in der "query" Methode nicht zu schließen, sondern nur das ResultSet nach der Auswertung.

  • Jagut. Wollte ich ja auch zufällig. Nur irgendwie klappt dann nichts mehr ^^

    Das liegt aber nicht an den PreparedStatements, sondern an dem unterschiedlichen Code. Der entscheidende Unterschied ist schon benannt: in dem Code für die query-Methode mit "normalen" Statements schließst du - im Gegensatz zu dem mit PreparedStmts - das Statement nicht per try-with-resources.


    Die Lösung dafür ist aber nicht, einfach das Statement nicht zu schließen, wie Rincewind empfiehlt

    Das einzige, was du an deinem Code oben ändern musst, ist halt dein PreparedStatement in der "query" Methode nicht zu schließen, sondern nur das ResultSet nach der Auswertung.

    es ist nämlich mindestens good practice alles zu schließen, was geht, um Memory Leaks zu vermeiden. Ebenso ist der Workaround von zM4xi für das Problem mit den HashBasedTables auch nicht empfehlenswert, weil unnötigerweise jedes mal eine Kopie von den angefragten Daten angefertigt wird.

    Meine Lösung wäre es, einzig und allein eine Utility-Methode prepareStatement(String query, Object... params) einzuführen, die einfach nur das PreparedStatement bereitstellt (und dieses logischerweise auch nicht schließt) und dann kann man eine Abfrage wie folgt machen:

    Code
    1. try (PreparedStatement stmt = db.prepareStatement(...);
    2. ResultSet rs = stmt.executeQuery()) {
    3. // ...
    4. } catch (Exception e) {
    5. // ..
    6. }

    MfG


    00110101 00110001 00110101 00111000 00110100 00110110 00110011 00110001 00110101 00111001 00110101 00110111 00110100 00110110 00110011 00110000 00110110 00110001 00110101 00110111 00110100 01000100 00110011 01000100 8o

    Einmal editiert, zuletzt von Aquaatic ()

  • Die Lösung dafür ist aber nicht, einfach das Statement nicht zu schließen, wie Rincewind empfiehlt

    es ist nämlich mindestens good practice alles zu schließen, was geht, um Memory Leaks zu vermeiden.

    Das habe ich wohl nicht ganz eindeutig kommuniziert, weil ich geschrieben habe, es sei das einzige, was er ändern müsste. Meine Lösung wäre nach wie vor das PrepraredStatement mehrmals zu benutzen.

  • Soo, Halleluja, wenn irgendwer sonst das Problem mal haben sollte:

    Um das Szenario einmal zusammen zufassen:
    - Eine Util-Klasse soll den ganzen Kram mit den Statements regeln (gerne, weil es deutlich weniger Programmieraufwand erfordert, auch unter der Prämisse, dass die Statements immer neu angelegt werden, und nicht mehrmals benutzt werden, so wie von mir vorgeschlagen).
    - Bei einem Update kein Problem: Die Methode bekommt alle Parameter, Statement wird erzeugt, ausgeführt, geschlossen.
    - Bei Selects gibt es das Problem mit dem Freigeben ( Aquaatic hat das angesprochen): Die Methode bekommen eventuelle Suchparameter, Statement wird erzeugt, ausgeführt, ResultSet wird returned und das Statement gammelt auch wenn das ResultSet geschlossen wird noch in der ConnectionPipeline.

    ABER: Es gibt eine Methode Statement#closeOnCompletion(), die bei den Selects alle Probleme löst. Sie setzt eine Flag in einem Statement, wodurch das Statement automatisch geschlossen wird, wenn alle aus ihr erzeugten ResultSets geschlossen werden. Das heißt in der Nutzung von der Util-Klasse muss nur noch auf das Schließen des ResultSets geachtet werden.

    Ich glaube nach wie vor nicht, dass das Software-Architektonisch die beste Lösung ist, aber allen Anfängern würde ich das auf jeden Fall empfehlen.

    ~ Rincewind