Servlet for querying database on some high-loaded system

Question

What should the code do: Process client HTML requests, query database and return the answer in XML. Working with a high load. I need to know how can it be optimized. Is something terribly wrong with this code?

Input data: HTML-session, MAC-address (in form of GET argument).

Output data: XML (session_id, session_sign).

Servlet:

package com.packageexample.servlet;

import java.io.IOException; 

import javax.servlet.*;
import javax.servlet.annotation.WebServlet;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;

import org.apache.tomcat.jdbc.pool.DataSource;
import org.apache.tomcat.jdbc.pool.PoolProperties;

/**
 * Servlet implementation class TestServlet
 */
@WebServlet("/Servlet")
public class Servlet extends HttpServlet {
    private static final long serialVersionUID = -3954448641206344959L;
    private static final long SESSION_TIMEOUT = 360000;
    private static final String JDBC_DRIVER = "com.mysql.jdbc.Driver";
    private static final String JDBC_MYSQL_SERVER = "jdbc:mysql://192.168.0.100:3306/test?useUnicode=true&amp;characterEncoding=UTF-8";
    private static final String JDBC_MYSQL_USER = "test";
    private static final String JDBC_MYSQL_PASSWORD = "test";

    private static DataSource datasource;
    private static enum RESPONSE_STATUS {SESSION_NEW, SESSION_RESTORED, SESSION_OK, MAC_UNDEFINED, UNAUTHORIZED_ACCESS, HACK_ATTEMPT};
    /**
     * @see HttpServlet#HttpServlet()
     */
    public Servlet() {
        super();
        PoolProperties p = new PoolProperties();
        p.setUrl(JDBC_MYSQL_SERVER);
        p.setDriverClassName(JDBC_DRIVER);
        p.setUsername(JDBC_MYSQL_USER);
        p.setPassword(JDBC_MYSQL_PASSWORD);
        p.setJmxEnabled(true);
        p.setTestWhileIdle(false);
        p.setTestOnBorrow(true);
        p.setValidationQuery("SELECT 1");
        p.setTestOnReturn(false);
        p.setValidationInterval(30000);
        p.setTimeBetweenEvictionRunsMillis(30000);
        p.setMaxActive(100);
        p.setInitialSize(10);
        p.setMaxWait(10000);
        p.setRemoveAbandonedTimeout(60);
        p.setMinEvictableIdleTimeMillis(30000);
        p.setMinIdle(10);
        p.setLogAbandoned(true);
        p.setRemoveAbandoned(true);
        p.setJdbcInterceptors(
                "org.apache.tomcat.jdbc.pool.interceptor.ConnectionState;"
                        + "org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer");
        /*Using apache tomcat7 datasource for connection pooling
        /We choose to use DBCP, because we have a high load on our project*/
        datasource = new DataSource();
        datasource.setPoolProperties(p);
    }

    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        //Need to check if we've got session already
        HttpSession session = request.getSession(false);
        String session_id = null;
        //Parse GET parameter mac from client request
        String mac = request.getParameter("mac");

        if (session!=null) {session_id = session.getId();}

        //Firstly, we need to check whether we got some MAC-address provided at all 
        if (mac == null) {
            makeXml (request,response,session_id, RESPONSE_STATUS.MAC_UNDEFINED);
            return;
        }
        //Now that we need to deal with db, 
        ManageDatabase dbDo = new ManageDatabase();
        /*Check that there's a record for this MAC in database
        /If yes, return this device*/
        Device device = dbDo.getDeviceByMAC (mac, datasource);

        if (device==null) {
            makeXml (request,response,session_id, RESPONSE_STATUS.UNAUTHORIZED_ACCESS);
            return;
        }
        /*Now that we know for sure that we've got some valid mac-address provided, 
        /we need to check, if client has got some SESSION_ID*/
        if (session_id == null) { 
            session = request.getSession();
            device.setSessionId(session.getId());
            dbDo.setSessionId(device, datasource);
            makeXml (request,response,device.getSessionId(), RESPONSE_STATUS.SESSION_NEW);
            return;
        }
        else {
            //If session_id is not null, we need to check if it is valid for mac provided
            if (!(device.getSessionId().equals(session.getId()))) {
                makeXml (request,response,session_id, RESPONSE_STATUS.HACK_ATTEMPT);
                return;
            }
            //If session_id not null and it's a valid session, then check it for timeout. Restore if needed
            long sessionActiveTime = System.currentTimeMillis() - session.getLastAccessedTime();
            if (sessionActiveTime > SESSION_TIMEOUT) {
                makeXml (request,response,session_id, RESPONSE_STATUS.SESSION_RESTORED);
            }
            else{
                makeXml (request,response,session_id, RESPONSE_STATUS.SESSION_OK);
            }
        }       

    }

    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    }
    protected void makeXml (HttpServletRequest request, HttpServletResponse response, String sid, Enum<?> res) throws ServletException, IOException {
        request.setAttribute("session_id", sid);
        request.setAttribute("session_sign", res);
        RequestDispatcher view = request.getRequestDispatcher("Result.jsp");
        view.forward(request, response);
    }
}

ManageDatabase:

package com.packageexample.servlet;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

import org.apache.tomcat.jdbc.pool.DataSource;

class ManageDatabase {
    private static final String MYSQL_QUERY_CHECK_MAC = "SELECT * FROM device WHERE mac=?;";
    private static final String MYSQL_QUERY_UPDATE_SESSION_ID = "UPDATE device set session_id=? WHERE mac=?;";

    Device getDeviceByMAC(String mac, DataSource datasource) {
        ResultSet rs = null;
        Device device = null;
        try (Connection connection = datasource.getConnection();    
                PreparedStatement statement = connection.prepareStatement(MYSQL_QUERY_CHECK_MAC, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDATABLE);){

            statement.setString(1, mac);
            rs = statement.executeQuery();

            while (rs.next()) {
                device = new Device();
                device.setMac(rs.getString("mac"));
                device.setSessionId(rs.getString("session_id"));                
            }
        }
        catch (SQLException e){System.out.println(e);}
        finally { 
            if (rs!=null) 
                try {rs.close();} 
            catch (SQLException e) {System.out.println(e);}
        }
        return device;
    }


    void setSessionId(Device device, DataSource datasource) {
        try (Connection connection = datasource.getConnection();    
                PreparedStatement statement = connection.prepareStatement(MYSQL_QUERY_UPDATE_SESSION_ID, ResultSet.TYPE_SCROLL_INSENSITIVE, ResultSet.CONCUR_UPDATABLE);) {
            statement.setString(1, device.getSessionId());
            statement.setString(2, device.getMac());
            statement.executeUpdate();
        }
        catch (SQLException e){System.out.println(e);}
    }
}

Device:

package com.packageexample.servlet;

class Device {
    private String mac;
    private String sessionId;
    public String getMac() {
        return mac;
    }
    public void setMac(String mac_address) {
        this.mac = mac_address;
    }
    public String getSessionId() {
        return sessionId;
    }
    public void setSessionId(String session_id) {
        this.sessionId = session_id;
    }
    public String toString () {
        return "Device mac:" + this.mac +" ; session_id:" + this.sessionId;
    }

}

Result.jsp

<%@ page language="java" contentType="text/xml; charset=UTF-8" pageEncoding="UTF-8"%>
<result>
<session_id>
<% 
out.println(request.getAttribute("session_id"));
%>
</session_id>
<session_sign>
<% 
out.println(request.getAttribute("session_sign"));
%> 
</session_sign>
</result>

Ensure that you have indexes defined on the device table for the session_id and mac columns. — 200_success
– 200_success, Commented Jan 27, 2014 at 19:25

palacsint · Accepted Answer · 2014-01-27 18:59:48Z

Some minor random notes:

Instead of commenting, like this:
```
    //If session_id is not null, we need to check if it is valid for mac provided
    if (!(device.getSessionId().equals(session.getId()))) {
        makeXml (request,response,session_id, RESPONSE_STATUS.HACK_ATTEMPT);
        return;
    }
```
I'd use an explaining variable:
```
final boolean isValidMac = device.getSessionId().equals(session.getId());
if (!isValidMac) {
    makeXml (request, response, session_id, RESPONSE_STATUS.HACK_ATTEMPT);
    return;
}
```
This would make the comment unnecessary. References:

Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

Put the result of the expression, or parts of the expression, in a temporary variable with a name that explains the purpose.

And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.
I'd rename SESSION_TIMEOUT to SESSION_TIMEOUT_MILLIS. It would help readers, maintainers.
JDBC connection details often change. A configuration file be better for them.
session_id and mac_address do not follow the camelCase convention which the other parts of the code use.
Comments like this are unnecessary:
```
 /**
 * Servlet implementation class TestServlet
 */    
```
It says nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
Instead of raw strings I'd consider using this:
```
ConnectionState.class.getCanonicalName() + ";" + 
    StatementFinalizer.class.getCanonicalName()
```
It's not a big difference but a little bit less error-prone.

The else keyword is unnecessary here:

/*Now that we know for sure that we've got some valid mac-address provided, 
/we need to check, if client has got some SESSION_ID*/
if (session_id == null) { 
    ...
    return;
}
else {
    ... // content of the else block
}

It could be simply the following:

if (session_id == null) { 
    ...
    return;
}
... // content of the (former) else block

200_success · Accepted Answer · 2014-01-27 19:12:40Z

5

It doesn't make much sense to have a connection pool to be used by just one servlet. Usually, the connection pool is shared by the entire webapp. Furthermore, much of the setup can be accomplished by configuration rather than with code. It is certainly bad practice to hard-code connection strings, especially passwords, in your code. You wouldn't want to have to recompile the application to point to a testing database or change the password. You also wouldn't want to store the code with the password in a source code control system.

answered Jan 27, 2014 at 19:12

200_success

146k22 gold badges192 silver badges481 bronze badges

1

\$\begingroup\$ Agreed on hard-coded connection strings, we ought to know better since last century. \$\endgroup\$

konijn
– konijn

2014-01-27 20:10:00 +00:00
Commented Jan 27, 2014 at 20:10

Add a comment |

Stack Exchange Network

Servlet for querying database on some high-loaded system

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Servlet for querying database on some high-loaded system

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions