Feature Request: Use PreparedStatement for search

org.jivesoftware.wildfire.user.DefaultUserProvider - findUsers()

The other findUsers() method is not used, but one could change also there the code. Or even better rewrite both methods so we don’'t see duplicate code.

As a diff looks very evil I just post the new method:

final String sqlescape = “x”; should be final String sqlescape = “<”; but the code tag has a problem with <

public Collection<User> findUsers(Set<String> fields, String query)
            throws UnsupportedOperationException
    {
        if (fields.isEmpty()) {
            return Collections.emptyList();
        }
        if (!getSearchFields().containsAll(fields)) {
            throw new IllegalArgumentException("Search fields " + fields + " are not valid.");
        }
        if (query == null || "".equals(query)) {
            return Collections.emptyList();
        }
        // SQL LIKE queries don''t map directly into a keyword/wildcard search like we want.
        // Therefore, we do a best approximiation by replacing ''*'' with ''%'' and then
        // surrounding the whole query with two ''%''. This will return more data than desired,
        // but is better than returning less data than desired.
        /* LG: with these replacements one can not search for ''*'' or ''?'' within the name,
         * as they are used as wildcards */
        final String sqlescape = "x";
        query = "*" + query + "*";
        query = query.replaceAll("%", sqlescape + "%"); /* one can search for ''%'' within the name */
        query = query.replaceAll("_", sqlescape + "_"); /* one can search for ''_'' within the name */
        query = query.replaceAll("\\*+", "%"); /* replace ''*'' or ''***'' with ''%'' (match any characters) */
        query = query.replaceAll("\\?+", "_"); /* replace ''?'' with ''_'' (match one character) */
        List<String> usernames = new ArrayList<String>(50);
        Connection con = null;
        PreparedStatement pstmt = null;
        ResultSet rs = null;
        try {
            con = DbConnectionManager.getConnection();
            StringBuilder sql = new StringBuilder(100);
            sql.append("SELECT username FROM jiveUser WHERE");
            int first = 0;
            if (fields.contains("Username")) {
                sql.append(" username LIKE ?");
                first++;
            }
            if (fields.contains("Name")) {
                if (first > 0) {
                    sql.append(" AND");
                }
                sql.append(" name LIKE ?");
                first++;
            }
            if (fields.contains("Email")) {
                if (first > 0) {
                    sql.append(" AND");
                }
                sql.append(" email LIKE ?");
                first++;
            }
            sql.append(" ESCAPE ''"+ sqlescape +"''");
            Log.debug("Searching: "+ sql.toString() +" . "+ query +" . "+ first);
            pstmt = con.prepareStatement(sql.toString());
            for (int i = 1; i<=first; i++)
            {
                 pstmt.setString(i,query);
            }
            rs = pstmt.executeQuery();
            while (rs.next()) {
                usernames.add(rs.getString(1));
            }
        }
        catch (SQLException e) {
            Log.error(e);
        }
        finally {
            DbConnectionManager.closeConnection(rs, pstmt, con);
        }              return new UserCollection(usernames.toArray(new String[usernames.size()]));
    }

Actually, there’‘s a handy class called CachedPreparedStatement in Wildfire Enterprise that we could pull over to the Open Source version. It lets you dynamically build up a PreparedStatement even when you don’‘t know the full structure until it’‘s finished being built. Using PreparedStatements for user searches might make the database slightly happier so I suppose it wouldn’'t be a bad change.

LG – i just checked the class into Wildfire. Do you want to create a JIRA issue about switching search to use PreparedStatements?

-Matt

Aah… you do have it!

Actually, I’‘ve been wondering why Wildfire doesn’'t use the dynamically created db statements. At least I could remember that jabberd does that in C codes.

Matt, how many dimes left in your pocket that you’'re still hiding and you plan not to reveal (yet) ?

Hi Matt,

a new class to cache three different prepared statements? Actually this method is called with only one of the three parameters = true, so it’‘s called three times if one want’'s to search for name, email and username.

I did not ask why it behaves like this as this is a complete different thing. And as this code is used in two methods in this class and also in other *UserProvider classes it should be a big job to change them all.

LG

Actually, I’‘ve been wondering why Wildfire doesn’'t use the dynamically created db statements. At least I could remember that jabberd does that in C codes.

In general, we use PreparedStatement almost everywhere for increased speed. There are just a few places like the search method where it’'s hard to use them due to not knowing exactly what the query is going to be each time it gets run. The CachedPreparedStatement class solves that problem.

Matt, how many dimes left in your pocket that you’'re still hiding and you plan not to reveal (yet) ?

Heh, we have many more, or are at least working on them.

-Matt