Multi-parameter search using JDBC PreparedStatement

Question

I want to create multi parameter search using JDBC prepared statement so as to prevent SQL injection attack and improve performance. As I couldn't find the best way to do it on the net.

I've tried to implement on my own as follow.

In this program I want to allow the user to search an employee by either first name, last name or department id.

1. Is my implementation would prevent SQL injection?

2. Am I using prepared statement correctly? I have some doubt on this line:

   PreparedStatement stat = conn.prepareStatement(sql.toString());
   

Let's say two users search by using the same parameters, and thus it would result the same SQL string. According to my implementation, would the database have to prepare that SQL twice or just once?

public class EmpDAO {
 public static List<Employee> findByCriteria(Employee e)
 throws SQLException, IOException {
 try (Connection conn = getConnection()) {
 StringBuilder sql = new StringBuilder("SELECT * FROM Employee ");
 //collect user supplied parameters
 Map<String, String> params = new HashMap<String, String>();
 if (e.getFname() != null && e.getFname().length() != 0) {
 params.put(EmpDAO.FNAME, e.getFname());
 }
 if (e.getLname() != null && e.getLname().length() != 0) {
 params.put(EmpDAO.LNAME, e.getLname());
 }
 if (e.getDepid() > 0) {
 params.put(EmpDAO.DEPT_ID, new Integer(e.getDepid()).toString());
 }
 //construct prepared statement based on the parameters
 Set<String> colSet = params.keySet();
 if (colSet != null && !colSet.isEmpty()) {
 StringBuilder whereClause = new StringBuilder(" WHERE");
 String andOp = "";
 for (String colName : colSet) { 
 whereClause.append(andOp);
 whereClause.append(" ");
 whereClause.append(colName);
 whereClause.append("=? ");
 andOp = " AND ";
 }
 sql.append(whereClause);
 }
 PreparedStatement stat = conn.prepareStatement(sql.toString());
 int paramPos = 1;
 for (String colName : colSet) {
 if (colName.equals(EmpDAO.FNAME)) {
 stat.setString(paramPos, params.get(colName));
 }
 if (colName.equals(EmpDAO.LNAME)) {
 stat.setString(paramPos, params.get(colName));
 }
 if (colName.equals(EmpDAO.DEPT_ID)) {
 stat.setInt(paramPos, Integer.parseInt(params.get(colName)));
 }
 paramPos++;
 }
 List<Employee> emp1 = new ArrayList<>();
 try (ResultSet result = stat.executeQuery()) {
 while (result.next()) {
 Employee emp = new Employee();
 emp.setDepid(result.getInt("DEPT_ID"));
 emp.setEmpid(result.getInt("EMP_ID"));
 emp.setFname(result.getString("FNAME"));
 emp.setJobid(result.getInt("JOB_ID"));
 emp.setLname(result.getString("LNAME"));
 emp.setMangid(result.getInt("MANAGER_EMP_ID"));
 emp.setSalary(result.getInt("SALARY"));
 emp1.add(emp);
 }
 }
 return emp1;
 }
 }
 public static Connection getConnection() throws SQLException, IOException {
 try {
 Class.forName("com.mysql.jdbc.Driver");
 } catch (ClassNotFoundException ex) {
 ex.printStackTrace();
 }
 return DriverManager.getConnection("jdbc:mysql://localhost:3306/test",
 "root", "root");
 }
 public static final String FNAME = "FNAME";
 public static final String LNAME = "FNAME";
 public static final String DEPT_ID = "FNAME";
}

Answer 1

Worst name ever: e for an Employee. Single-letter variable names are almost always dodgy, with very rare exceptions. e is perfectly acceptable in catch blocks for handling Exceptions, and often tolerable for Events.

---

Is your class design sane?

if (e.getFname() != null && e.getFname().length() != 0) {
 params.put(EmpDAO.FNAME, e.getFname());
}
if (e.getLname() != null && e.getLname().length() != 0) {
 params.put(EmpDAO.LNAME, e.getLname());
}

What kind of Employee doesn't have a first name or last name? I know some people with one name, but nobody with no name at all. If you redesign your Employee class to disallow null values for last name (at least), the checks become simpler. Also, getFname and getLname are terrible getter names. Spell them out properly, to getFirstName, getLastName.

Finally, a better way to check if a string is empty is using the .isEmpty() method instead of .length() != 0.

---

Set<String> colSet = params.keySet();
if (colSet != null && !colSet.isEmpty()) {
 StringBuilder whereClause = new StringBuilder(" WHERE");
 String andOp = "";
 for (String colName : colSet) { 
 whereClause.append(andOp);
 whereClause.append(" ");
 whereClause.append(colName);
 whereClause.append("=? ");
 andOp = " AND ";
 }
 sql.append(whereClause);
}

Here, colSet is never null, so the colSet != null condition is pointless.

It's also pointless to create a new StringBuilder for the where clause, when you can append directly to sql, the existing StringBuilder.

---

int paramPos = 1;
for (String colName : colSet) {
 if (colName.equals(EmpDAO.FNAME)) {
 stat.setString(paramPos, params.get(colName));
 }
 if (colName.equals(EmpDAO.LNAME)) {
 stat.setString(paramPos, params.get(colName));
 }

If you change the type of the params map to bMap<String, Object> then you can simplify the above to this:

for (String colName : colSet) {
 stat.setObject(paramPos, params.get(colName));
}

---

if my implementation would prevent SQL injection

Yes, easily. Your input is an instance of Employee, which normally should be already validated. And then, you're using prepared statements, so the input data will be filled into distinct fields, and can never be interpreted as running malicious sub-queries or multiple queries. Finally, the query is a SELECT, something non-destructive, so malicious field values cannot cause deletions or overwriting data.

If I am using prepared statement correctly? I have some doubt on this line

Yes.

Let's say two users search by using the same parameters, and thus it would result the same sql string. According to my implementation, would the database have to prepare that sql twice or just once?

The database should prepare only once.