Prepared Statement mit unbekannter Anzahl von Where-Clauses

[norm]

Hallo Community,

ich entwickle zur Zeit im Rahmen eines Forschungsprojektes eine Datenbank-Anwendung, die die Verwaltung von Bild-Dateien ermöglicht.

Wir arbeiten u.A. mit Java, Tomcat, JSPs und einer Postgre Datenbank.

Wenn ein benutzer die "Erweiterte Bildersuche" benutzt, steht ja noch nicht fest, inwieweit er seine Suche eingrenzen möchte:

Z.b.

Alle Bilder =>

Select * from image

Alle Bilder vom Autor xyz =>

Select * from image WHERE autor LIKE 'xyz'

Alle Bilder vom Autor xyz und hoher Auflösung =>

Select * from image WHERE autor LIKE 'xyz' AND solution LIKE 'high'

Daraus ergibt sich eine hohe Anzahl von Kombinations-Möglichkeiten und dass die Anzahl der WHERE Klauseln erst zur Laufzeit feststeht.
Bisher haben wir dieses Problem mit String-Konkatenation gelöst. Die Where Clauses werden in einer Liste gesammelt und in einem loop konkateniert.

Leider soll diese Form der Query-Generierung sehr inperfomant sein und ist definitv anfällig für SQL-Injections.

Kennt jemand eine elegante Möglichkeit, dieses Problem zu umgehen?

Gruß
Norm

 

[tfa]

Benutze einen OR-Mapper, z.B. Hibernate.

 

[norm]

Benutze einen OR-Mapper, z.B. Hibernate.

müsste ich da nicht die komplexen finder-Methoden selber schreiben und würde dann wieder vor dem selben Problem stehen? :bahnhof:

 

[tfa]

müsste ich da nicht die komplexen finder-Methoden selber schreiben und würde dann wieder vor dem selben Problem stehen? :bahnhof:

Ne, gerade nicht. Das geht alles automatisch.
Für so eine dynamische Suche verwende ich z.B. Query-by-Example: Chapter*15.*Criteria Queries
Das ist sehr praktisch.

 

[musiKk]

Leider soll diese Form der Query-Generierung sehr inperfomant sein und ist definitv anfällig für SQL-Injections.

Mit

PreparedStatement

nicht. Die Teile, die konkateniert werden, sind ja für sich gesehen immer konstant. Das Einsetzen geht wieder normal über die setX-Methoden.

Was nicht heißen soll, dass dabei immer schöner Code entsteht... Vielleicht gibt es da auch bessere Möglichkeiten. Ich habe ich sowas auch mal für ein Query gebaut. In dem Projekt wird auch Vanilla-JDBC verwendet und für ein einziges Query habe ich da den pragmatisch Weg gewählt. Ist die Frage, ob es sich immer lohnt, die technische Grundlage komplett umzuschreiben; ORM ist auch kein goldener Hammer.

 

[norm]

Ne, gerade nicht. Das geht alles automatisch.
Für so eine dynamische Suche verwende ich z.B. Query-by-Example: Chapter*15.*Criteria Queries
Das ist sehr praktisch.

danke für den tip! :toll:

Allerdings graust es mir vor dem refactorn der gesamten persistenz-schicht und ich warte mal ab, ob es noch eine andere möglichkeit gibt

Fürs Erste werde ich mal vorschlagen, kritische Zeichen von den JSPs escapen oder ablehnen zu lassen um zumindest die SQL Injections zu vermeiden

 

[tfa]

Fürs Erste werde ich mal vorschlagen, kritische Zeichen von den JSPs escapen oder ablehnen zu lassen um zumindest die SQL Injections zu vermeiden

Du kannst auch die Prepared Statements von Hand zusammenbasteln. Das ist auf jeden Fall sicherer und der Aufwand ist ähnlich zum dynamischen SQL.

 

[musiKk]

Ohne zu sehr darauf rumhacken zu wollen - wenn ORM für Dich geeignet ist, dann nimm es unbedingt - aber wieso sollte bei erzeugten [c]PreparedStatement[/c]s die Gefahr einer SQL Injection größer sein, als bei statischen?

 

[norm]

Mit

PreparedStatement

nicht. Die Teile, die konkateniert werden, sind ja für sich gesehen immer konstant. Das Einsetzen geht wieder normal über die setX-Methoden.

Was nicht heißen soll, dass dabei immer schöner Code entsteht... Vielleicht gibt es da auch bessere Möglichkeiten. Ich habe ich sowas auch mal für ein Query gebaut. In dem Projekt wird auch Vanilla-JDBC verwendet und für ein einziges Query habe ich da den pragmatisch Weg gewählt. Ist die Frage, ob es sich immer lohnt, die technische Grundlage komplett umzuschreiben; ORM ist auch kein goldener Hammer.

Stimmt, warum nicht einfach PreparedStatements konkatenieren
Das werde ich mal probieren...
Danke auch dafür!

 

[schalentier]

Nur der Vollstaendigkeit halber: Prepared Statements sind nicht automatisch performanter als Nicht-Prepared Statements. Es gibt Faelle, wo sie sogar deutlich schlechter performen. Das liegt daran, dass beim Erstellen des Ausfuehrplans bei Prepared Statements die konkreten Werte in der Query nicht verfuegbar sind und somit keine Optimierung des Plans anhand der konkreten Query moeglich ist.

Im Kampf gegen SQL Injection sind sie natuerlich erste Wahl.

 

[tfa]

Nur der Vollstaendigkeit halber: Prepared Statements sind nicht automatisch performanter als Nicht-Prepared Statements. Es gibt Faelle, wo sie sogar deutlich schlechter performen. Das liegt daran, dass beim Erstellen des Ausfuehrplans bei Prepared Statements die konkreten Werte in der Query nicht verfuegbar sind und somit keine Optimierung des Plans anhand der konkreten Query moeglich ist.

Aber nur, wenn es wirklich mit echtem statischem SQL vergleicht. Dynamisch zusammengebaute Querys ohne Parameter-Marker, wie das hier gemacht wurde, sollten doch eigentlich immer langsamer sein.

 

[schalentier]

Mhmh... naja, was man (performancetechnisch) beim Einsatz von PrepStatements spart, ist das Parsen des SQL und halt das Erstellen des Execution Plans. Dafuer is der Plan dann fest.

Schickt man dynamisch erstellte SQLs (weis nicht so recht, was du mit statischem SQL meinst) hin, so muss diese natuerlich jedes Mal geparst werden (stimmt so nich ganz, die DB erkennt gleiche SQL normalerweise) - dafuer wird ein optimierter Plan erstellt. Besonders krass ist uns das letztens aufgefallen, beim Einsatz von limit, bzw rownum() bei gleichzeitigem Sortieren. Also sowas:

select * from (select * from table order by name) where rownum()<100

Wenn man dann die 100 am Ende durch ein ? und PrepStatement ersetzt, kann die Abfrage um Dimensionen (Minuten statt Sekunden) langsamer werden. Das bloede ist, das Hibernate immer Prepared Statements raushaut... allerdings spielt das wohl erst bei groesseren Tabellen eine Rolle (so ab 10 Mio Zeilen). Das ist halt das miese an den Datenbanken: Im kleinen geht immer alles super und sau schnell. Sobald es aber mehr Daten werden (was ja ueblich ist) kann man sich so echte Performanceprobleme einhandeln.

 

[tfa]

Statisches SQL wird vorkompiliert und gar nicht mehr interpretiert. Das geht natürlich nur in der Datenbank (z.B. in Cobol-Programmen), kann aber auch mit Java erreicht werden.

Wenn man dann die 100 am Ende durch ein ? und PrepStatement ersetzt, kann die Abfrage um Dimensionen (Minuten statt Sekunden) langsamer werden.

Bist du sicher, dass das mit Prepared Statements zu tun hat? War vielleicht irgendein Caching-Effekt.

Das bloede ist, das Hibernate immer Prepared Statements raushaut

Man kann auch direkt SQL-Querys schreiben, oder SQL-Restrictions verwenden. Wenn die 100 in "where rownum()<100" immer eine 100 ist, würde ich das hier auch tun.

 

[schalentier]

Bist du sicher, dass das mit Prepared Statements zu tun hat? War vielleicht irgendein Caching-Effekt.

100% sicher bin ich natuerlich nicht (Restrisiko gibts immer...). Aber all unsre Tests haben das bisher gezeigt. Konkret haben wir Query.setMaxResult benutzt, was im Oracle Dialekt ein rownum()<? anhaengt und alles als PrepStatement abschickt. Derzeit ueberlegen wir ernsthaft, da einen eignen Dialekt zu bauen, der genau das (uns nur das) etwas anders loest. Wenn wir da konkrete Ergebnisse haben, werd ich hier wohl nochmal bescheid geben.

Man kann auch direkt SQL-Querys schreiben, oder SQL-Restrictions verwenden. Wenn die 100 in "where rownum()<100" immer eine 100 ist, würde ich das hier auch tun.

Leider is das nicht immer 100, aber in den allermeisten Faellen. Ich meinte, wenn man das normale Criteriagedoehns von Hibernate nimmt, kommen immer PrepStatements raus. Ich hab auch noch kein Schalter gefunden, mit dem man das umstellen kann - vermutlich is das aber auch gut so.

 

[tfa]

Es müsste so funktionieren:

Criteria crit = session.createCriteria(...);
crit.add(Restrictions.sqlRestriction("rownum()<100");
List res = crit.list();

Das ist dann aber natürlich nicht mehr DB-unabhängig.

 

[schalentier]

Bei Oracle muss man das so schreiben:

Ohne "limit":

select * from table order by name

Mit limit:

select * from (select * from table order by name) where rownum()<100

Aber wir sind inzwischen sehr offtopic... sorry dafuer. Ich meld mich mit dem Thema nochmal in nem eignen Thread. Und von der DB Unabhaengigkeit haben wir uns schon laenger verabschiedet... prinzipiell is das zwar schoen, in der Praxis nur net wirklich realisierbar.

 

[norm]

Hallo Community,

ich habe jetzt begonnen die relevanten Statements durch PreparedStatements zu ersetzen.
Leider bekomme ich nun eine Exception, die ich mir nicht erklären kann:

"org.postgresql.util.PSQLException: ERROR: syntax error at or near "[" Position: 1"

Ich hänge mal den Code an, vielleicht macht sich ja jemand die Mühe mal kurz drüber zu schauen.

	/**
	 * @param Meta
	 *            Object containing User-Search Entries
	 * @return Returns Vector of MetaIMP-Objects fitting to User-Search Entries
	 * 
	 * 
	 */
	public Vector<Meta> getMetaFromDB(Meta meta) {

		StringBuffer sqlQuery = new StringBuffer("SELECT * FROM meta");
		ArrayList<String> query = new ArrayList<String>();
		ResultSet resultSet = null;
		Vector<Meta> returnVector = new Vector<Meta>();
		PreparedStatement prepStat = null;
		
		// Where Clauses are collected in an ArrayList
		if (meta.getID() != null) {
			query.add(" id = ?");
		}

		if (meta.getArtist() != null) {
			query.add("artist LIKE ?");
		}

		if (meta.getImageHeight() != null) {
			query.add("imageheight = ?");
		}

		if (meta.getImageWidth() != null) {
			query.add("imagewidth = ?" );
		}

		if (meta.getCopyright() != null) {
			query.add("copyright LIKE ? ");
		}

		if (meta.getDateTime() != null) {
			query.add("datatime = ?" );
		}

		if (meta.getOrientation() != null) {
			query.add("orientation = ?");
		}

		if (meta.getUserComment() != null) {
			query.add("usercomment LIKE ? ");
		}

		if (meta.getImageSize() != null) {
			query.add("imagesize = ?");
		}
		
		// SQL-Statement is generated from the collected Where-Clauses
		for (int i = 0; i < query.size(); i++) {
			if (i == 0)
				sqlQuery.append(" WHERE ");
			sqlQuery.append(query.get(i));
			if (i < query.size() - 1)
				sqlQuery.append(" AND ");
		}
		
		try {
			System.out.println(sqlQuery);
			prepStat = con.prepareStatement(query.toString());
			
			int paramCounter = 1;
			if (meta.getID() != null) {
				prepStat.setInt(paramCounter, meta.getID());
				paramCounter++;
			}

			if (meta.getArtist() != null) {
				prepStat.setString(paramCounter, meta.getArtist());
				paramCounter++;
			}

			if (meta.getImageHeight() != null) {
				prepStat.setInt(paramCounter, meta.getImageHeight());
				paramCounter++;
			}

			if (meta.getImageWidth() != null) {
				prepStat.setInt(paramCounter, meta.getImageWidth());
				paramCounter++;
			}

			if (meta.getCopyright() != null) {
				prepStat.setString(paramCounter, meta.getCopyright());
				paramCounter++;
			}

			if (meta.getDateTime() != null) {
				prepStat.setLong(paramCounter, meta.getDateTime());
				paramCounter++;
			}

			if (meta.getOrientation() != null) {
				prepStat.setInt(paramCounter, meta.getOrientation());
				paramCounter++;
			}

			if (meta.getUserComment() != null) {
				prepStat.setString(paramCounter, meta.getUserComment());
				paramCounter++;
			}

			if (meta.getImageSize() != null) {
				prepStat.setInt(paramCounter, meta.getImageSize());
				paramCounter++;
			}

		} catch (SQLException e) {
			e.printStackTrace();
		}
		

		// Execution of SQL-Statement
		try {
			resultSet = prepStat.executeQuery();
		} catch (SQLException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		}

		// Generate Meta-Object with Information from meta-table and add to
		// returned Vector
		returnVector = makeMetaObjectFromResultSet(resultSet);

		return returnVector;

	}

Also die sqlQuery wird meiner Meinung nach korrekt generiert und sieht für die Suche nach artist, copyright und orientation wie folgt aus:
"SELECT * FROM meta WHERE artist LIKE ? AND copyright LIKE ? AND orientation = ?"

Die Exception kommt definitv erst beim executeQuery() (Zeile 120)!

Sieht jemand den Fehler oder kann mir jemand sagen, wie man es eleganter lösen kann?

Liebe Grüße
norm

 

[maki]

Fehlende Lehrzeichen vielleicht?

Ansonsten kannst du dir die Abfrage ob es die erste Bedingung ist oder nicht sparen, wenn du anfangs ein [c]WHERE 1 = 1 [/c] einsetzt und dann nur noch Bedigungen mit AND anfügst.

 

[SlaterB]

muss es so eine komplizierte Query sein? ist der ganze if-else-Code wirklich interessant?
das kann schon sein, aber erstmal ist doch bisschen Grundlagenarbeit erforderlich

funktioniert eine Query [c]SELECT * FROM meta[/c]? jeweils normales Statement + PreparedStatement,
als nächstes anfangen, mit EINEM Parameter zu testen, zum einen
[c]SELECT * FROM meta WHERE artist LIKE '%test%'[/c] als normales Statement,
danach als PreparedStatement mit ? und was genau übergibst du als Parameter?

usw. bis zu einer Query die nicht funktioniert,
wenn du "SELECT * FROM meta WHERE artist LIKE ? AND copyright LIKE ? AND orientation = ?"
erfolgreich direkt programmieren kannst mit 3 PreparedStatement-Parametern,
es aber dann in obigen Code nicht geht, dann wirds langsam interessant

 

[norm]

muss es so eine komplizierte Query sein? ist der ganze if-else-Code wirklich interessant?
das kann schon sein, aber erstmal ist doch bisschen Grundlagenarbeit erforderlich

funktioniert eine Query [c]SELECT * FROM meta[/c]? jeweils normales Statement + PreparedStatement,
als nächstes anfangen, mit EINEM Parameter zu testen, zum einen
[c]SELECT * FROM meta WHERE artist LIKE '%test%'[/c] als normales Statement,
danach als PreparedStatement mit ? und was genau übergibst du als Parameter?

usw. bis zu einer Query die nicht funktioniert,
wenn du "SELECT * FROM meta WHERE artist LIKE ? AND copyright LIKE ? AND orientation = ?"
erfolgreich direkt programmieren kannst mit 3 PreparedStatement-Parametern,
es aber dann in obigen Code nicht geht, dann wirds langsam interessant

Um deine Frage zu beantworten:
Als Parameter übergebe ich quasi Eingaben eines Benutzers in ein Suchformular, eine "erweiterte Bildersuche".

So, und jetzt wirds jetzt interessant

public void testPrep(String param0, String param1, int param2){
		try {
			PreparedStatement prep = con.prepareStatement("SELECT * FROM meta WHERE artist LIKE ? AND copyright LIKE ? AND orientation = ? ");
			prep.setString(1, param0);
			prep.setString(2, param1);
			prep.setInt(3, param2);
			ResultSet rs = prep.executeQuery();
			
			while(rs.next()){
				
				System.out.println(rs.getInt("id"));
			}
		} catch (SQLException e) {
			e.printStackTrace();
		}
	}

mit aufruf durch

testPrep("Amish Patel", "Microsoft Corporation", 1);

läuft problemlos durch

Das ist mehr als merkwürdig :autsch: :autsch:

 

[SlaterB]

und derselbe Aufruf geht mit dem if-else-Code nicht?
also wirklich exakt dieselben drei Parameter, alle nicht benötigten if wegen fehlenden meta usw. auf if (false) umstellen?

na, ich persönlich kann leider JDBC gar nicht testen, also mehr als solche halb-schlauen Hinweise nicht liefern

 

[norm]

hier mal ein Screenshot der Debugging-Informationen des preparedStatements.
die werte in fragments sehen igrendwie komisch aus, oder? :autsch:

 

[norm]

und derselbe Aufruf geht mit dem if-else-Code nicht?
also wirklich exakt dieselben drei Parameter, alle nicht benötigten if wegen fehlenden meta usw. auf if (false) umstellen?

na, ich persönlich kann leider JDBC gar nicht testen, also mehr als solche halb-schlauen Hinweise nicht liefern

also habe das ganze sowohl im Debugger laufen lassen und jetzt auch mal alle nicht benötigten if-Anweisungen auskommentiert.
Trotzdem der selbe Fehler :/

 

[tfa]

Hast du dir auch mal die String ausgeben lassen, die du in die Parameter steckst?

 

[maki]

        for (int i = 0; i < query.size(); i++) {
            if (i == 0)
                sqlQuery.append(" WHERE ");
            sqlQuery.append(query.get(i));
            if (i < query.size() - 1)
                sqlQuery.append(" AND ");
        }

Sieht sehr verdächtig aus...

        sqlQuery.append(" WHERE 1 = 1 ");

        for (String clause : query) {
            sqlQuery.append(" AND ");
            sqlQuery.append(clause);
        }

*ungetested*

 

[SlaterB]

und jetzt auch mal alle nicht benötigten if-Anweisungen auskommentiert.
Trotzdem der selbe Fehler :/

auch wirklich mit einem Aufruf [c]testPrep("Amish Patel", "Microsoft Corporation", 1);[/c] wie von mir gewünscht?
wenn du wieder dein Standard-Meta-Objekt nimmst, dann sind da ja alle anderen Fehler möglich, eben wie genannt unbekannte Eingaben?

wenn bei [c]testPrep("Amish Patel", "Microsoft Corporation", 1);[/c] der Fehler kommt, dann wird es besser nachprüfbar, poste auch ruhig den zugehörigen modifizierten Code,
im Zweifel lieber zu viel als zu wenig

 

[norm]

oh Gott Leute, es tut mir Leid eure Zeit verschwendet zu haben...

Man sollte darauf achten, nicht zuviele Variablen mit ähnlichen Namen zu benutzen:

 System.out.println(sqlQuery);
 prepStat = con.prepareStatement(query.toString());

habe den falschen String an con.prepareStatement() übergeben...

 

[SlaterB]

tja, hat von 'denen' ja auch keiner gesehen
obwohl die Fehlermeldung mit [ dann also doch so ein guter Hinweis war..

 

[norm]

        for (int i = 0; i < query.size(); i++) {
            if (i == 0)
                sqlQuery.append(" WHERE ");
            sqlQuery.append(query.get(i));
            if (i < query.size() - 1)
                sqlQuery.append(" AND ");
        }

Sieht sehr verdächtig aus...

        sqlQuery.append(" WHERE 1 = 1 ");

        for (String clause : query) {
            sqlQuery.append(" AND ");
            sqlQuery.append(clause);
        }

*ungetested*

Wie kommst du auf WHERE 1 = 1 ?

            if (i == 0)
                sqlQuery.append(" WHERE ");

bewirkt, dass beim ersten Durchlauf der Schleife ein WHERE appended wird =>
"Select * from meta WHERE"
dann gehts weiter gut, könnte man auch ausserhalb der schleife machen...

 

[maki]

Wie kommst du auf WHERE 1 = 1 ?

Habe ich bereits auf der vorherigen Seite beschrieben, sparst dir deine if Bedingung, macht den Code kürzer & klarer, aber nicht langsamer.

Nebenbei:
StringBuffer solltest du nicht verwenden, falls überhaupt, dann nur StringBuilder.
Deine fehlenden Klammern bei den if abfragen machen es leicht Fehler einzubauen.

 

[norm]

Habe ich bereits auf der vorherigen Seite beschrieben, sparst dir deine if Bedingung, macht den Code kürzer & klarer, aber nicht langsamer.

Nebenbei:
StringBuffer solltest du nicht verwenden, falls überhaupt, dann nur StringBuilder.
Deine fehlenden Klammern bei den if abfragen machen es leicht Fehler einzubauen.

Ok, danke für die Tips, dass werde ich noch anpassen