Problem
This application is a “Student Registration System” using JDBC with Oracle as part of our school project. It works as it stands right now.
Questions:
- What is a better way to structure the project? Why?
- Any ‘crimes’ committed in the code?
Details:
- I’ve decided to make the interface as a web service/page instead of a swing application.
- I’m using Tomcat 7.0, jTable.org jquery based table, JSP, Java.
The current files in the project:
- DBInterface.java
- StudentInterface.java
- Students.java – Acts as the model interfacing with the database using JDBC
- StudentsAjax.jsp – Acts as the view providing data and services to the HTML client
index.html
Important parts of the files:
DBInterface
package models;
public interface DBInterface {
static final String dburl = "jdbc:oracle:thin:@";
static final String dbuser = "xxx";
static final String dbpass = "xxx";
}
StudentInterface
package models;
import java.util.List;
import org.json.JSONObject;
public interface StudentsInterface extends DBInterface {
public int createStudent(String sid, String firstName, String lastName, String status, double gpa, String email);
public List<JSONObject> retrieveStudent(String sid);
...........
}
Students
package models;
import java.sql.CallableStatement;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;
import oracle.jdbc.OracleTypes;
import oracle.jdbc.pool.OracleDataSource;
import org.json.JSONObject;
public class Students implements StudentsInterface {
/*
* Method creates a Student entry into the table.
* @Returns 1 if successfull. 0 otherwise.
* (non-Javadoc)
* @see models.StudentsInterface#createStudent(java.lang.String, java.lang.String, java.lang.String, java.lang.String, int, java.lang.String)
*/
@Override
public int createStudent(String sid, String firstName, String lastName, String status, double gpa, String email) {
int retVal = 0;
try {
// Connection to Oracle server
OracleDataSource ds = new oracle.jdbc.pool.OracleDataSource();
ds.setURL(dburl);
Connection conn = ds.getConnection(dbuser, dbpass);
//Insert
PreparedStatement insert = conn.prepareStatement("INSERT into students VALUES(?,?,?,?,?,?)");
// Input other values
insert.setString(1, sid);
insert.setString(2, firstName);
insert.setString(3, lastName);
insert.setString(4, status);
insert.setDouble(5, gpa);
insert.setString(6, email);
// execute the update
insert.executeUpdate();
// close the result set, statement, and the connection
conn.close();
retVal = 1;
} catch (SQLException ex) {
System.out.println("n*** SQLException caught ***n");
} catch (Exception e) {
System.out.println("n*** other Exception caught ***n");
}
return retVal;
}
//--------------------------------------
@Override
public List<JSONObject> retrieveStudent(String sid) {
List<JSONObject> t = new ArrayList<JSONObject>();
try {
// Connection to Oracle server
OracleDataSource ds = new oracle.jdbc.pool.OracleDataSource();
ds.setURL(dburl);
Connection conn = ds.getConnection(dbuser, dbpass);
// Query
Statement stmt = conn.createStatement();
System.out.println("n*** Executing");
String query = "SELECT * FROM students";
if(!sid.equals("ALL"))
query += " WHERE sid = '" + sid + "' ";
// Save result
ResultSet rset;
rset = stmt.executeQuery(query);
System.out.println("n*** Done");
// determine the number of columns in each row of the result set
ResultSetMetaData rsetMeta = rset.getMetaData();
int columnCount = rsetMeta.getColumnCount();
System.out.println("n*** Retrieved Student Count is
//insert result into a List
while (rset.next()) {
JSONObject e = new JSONObject();
//loop through columns
for (int i = 1; i <= columnCount; i++) {
String key = rsetMeta.getColumnName(i);
String value = rset.getString(i);
//Convert First char to upper case
key = key.toLowerCase();
key =toString(key.charAt(0)).toUpperCase()+key.substring(1);
e.put(key,value);
}
t.add(e);
}
// close the result set, statement, and the connection
rset.close();
stmt.close();
conn.close();
} catch (SQLException ex) {
System.out.println("n*** SQLException caught ***n");
} catch (Exception e) {
System.out.println("n*** other Exception caught ***n");
}
return t;
}
....
....
....
}
StudentsAjax
<%@page import="org.json.JSONObject"%>
<%@page import="java.util.ArrayList"%>
<%@page import="java.util.List"%>
<%@ page language="java" contentType="application/json; charset=ISO-8859-1"
pageEncoding="ISO-8859-1"%>
<%@ page import="models.*" %>
<% String incomingAction = (String)request.getParameter("action"); %>
<%
StudentsInterface si = new Students();
JSONObject cover = new JSONObject();
//Decide what to perform based on incoming action.
//-------------------- LIST
if(incomingAction.equals("list")) {
//Prepare JSON output
cover.put("Result","OK");
cover.put("Records", listStudents("ALL"));
}
else //-------------------- CREATE
if(incomingAction.equals("create")) {
//Retrieve result
int reply = si.createStudent(
(String)request.getParameter("Sid")
,(String)request.getParameter("Firstname")
,(String)request.getParameter("Lastname")
,(String)request.getParameter("Status")
,(String)request.getParameter("Email")
);
//Prepare JSON output
cover.put("Result","OK");
cover.put("Record", listStudents((String)request.getParameter("Sid")).get(0) );
}
else //-------------------- UPDATE
.....
.....
.....
//Output in the JSON format.
out.println(cover.toString());
%>
<%!
// -------------------------------- Functions
List<JSONObject> listStudents(String id) {
StudentsInterface si = new Students();
List<JSONObject> t;
//Retrieve result
if(id.equals("ALL")) {
t = si.showAllStudents();
}
else {
t = si.retrieveStudent(id);
}
return t;
}
Solution
- Follow layered architecture. Read about Model-View-Controller pattern. (This should eliminate most of your design issues so I won’t specify each one of them here)
- Externalize environment specific entries like dburl, dbuser and dbpassword by creating a JDBC DataSource in tomcat. You should also pool database connections. This should eliminate the need for
DBInterface. - Use Checked (like StudentNotFoundException etc.) and Unchecked exceptions. Here are the guidelines.
The first ‘crime’ is a SQL injection vulnerability. You are using a PreparedStatement when inserting, which is great. You avoided the problem there. But this is a problem:
String query = "SELECT * FROM students";
if(!sid.equals("ALL"))
query += " WHERE sid = '" + sid + "' ";
If the sid parameter comes from outside the system (as it does above), it can be anything, including a maliciously crafted string that could cause damage to the system. The simple solution is to build the query using parameter placeholders and use a PreparedStatement here as well.
String query = "SELECT * FROM students";
if(!sid.equals("ALL"))
query += " WHERE sid = ?";
The second ‘crime’ is the unsafe closing of database resources. Each resource should be closed in a finally block to ensure that it is properly released, even in the event that an exception occurs while working with the resource.
For example, given this code:
Connection conn = ds.getConnection(dbuser, dbpass);
// do some work using the connection (what if an exception occurs here?)
conn.close();
The connection will not be closed if an exception occurs between opening and closing the connection. To ensure the connection is always closed, use something like this:
Connection conn = ds.getConnection(dbuser, dbpass);
try {
// do some work using the connection
}
finally {
conn.close(); // this will be called even if an exception occurs in the try block
}
If you are using Java 7, check out the try-with-resources statement. The above code can be simplified to this:
try (Connection conn = ds.getConnection(dbuser, dbpass)) {
// do some work using the connection
} // connection will automatically be closed here
This also goes for prepared statements, result sets, and any other type of resource that might need to be closed/released.
createStudent is declared to return an int of 1 if successful, 0 otherwise. It seems like a boolean would better represent the possible outcomes. Having said that, it might be even better to throw an exception in the event of failure.
The retrieveStudent function is confusing. The name and argument imply that it will retrieve a single student by an sid, but the return type implies it will return multiple. It is not obvious that you can call it in a ‘special’ way to get all students. I would recommend splitting this into 2 separate functions:
public JSONObject retrieveStudent(String sid) { ... }
public List<JSONObject> retrieveStudents() { ... }
JSP files are generally used for presentation and very limited logic. It is more common to put request handling logic in a servlet (or controller in a MVC framework), then render the presentation using a JSP. In this case, there really is no presentation, so all of that code could be in a servlet.
You should not need to cast the request.getParameter() calls to a String. They are already declared to return a String.