0

I have finally completed my application (Eclipse, GWT, Java, MySQL, Tomcat) and it has been uploaded onto a server (I have someone else uploading the application onto a server). However, there seems to be an issue with the server installation and my code is not sending back any errors.

For instance: when a new account is created the following message is displayed "Your account has been created. Please contact a leader to associate youth members to it." however the database is not updated. It seems that I am not catching an exception correctly.

My code is:

Client side call:

AsyncCallback<User> callback = new CreationHandler<User>();
rpc.createUser(textBoxAccount.getText(), textBoxPassword.getText(), null, null, null, callback);

Server side:

public User createUser(String userName, String pass, String level, String pack, java.sql.Date archived) {
    User user = null; // necessary unless you do something in the exception handler
    ResultSet result = null;
    PreparedStatement ps = null;
    String pw_hash = BCrypt.hashpw(pass, BCrypt.gensalt());
    try {
      ps = conn.prepareStatement(
          "INSERT INTO at_accounts (acc_email_address, acc_password, acc_enabled) " +
                  "VALUES (?, ?, ?)");
      ps.setString(1, userName);
      ps.setString(2, pw_hash);
      ps.setString(3, "1");
      ps.executeUpdate();
    }
    catch (SQLException e) {
      //do stuff on fail
        System.out.println("SQLException createUser 1.");
        e.printStackTrace();
        user = null;
    }
    finally {
        if (result != null) {
            try {
                result.close();
            }
            catch (SQLException e) {
                System.out.println("SQLException createUser 2.");
                e.printStackTrace();
            }
        }
        if (ps != null) {
            try {
                ps.close();
            }   
            catch (SQLException e) {
                System.out.println("SQLException createUser 3.");
                e.printStackTrace();
            }
        }
    }
    return user;
}

Client side:

class CreationHandler<T> implements AsyncCallback<User> {
    //Create the account.
    public void onFailure(Throwable ex) {
        Window.alert("RPC call failed - CreationHandler - Notify Administrator.");  
    }
    public void onSuccess(User result) {
        Window.alert("Your account has been created. Please contact a leader to associate youth members to it.");

    }
}

Any help would be greatly appreciated.

Regards,

Glyn

Hi JonK,

Is this what you mean please?

public User createUser(String userName, String pass, String level, String pack, java.sql.Date archived) {
    User user = null; // necessary unless you do something in the exception handler
    ResultSet result = null;
    PreparedStatement ps = null;
    String pw_hash = BCrypt.hashpw(pass, BCrypt.gensalt());
    try {
      ps = conn.prepareStatement(
          "INSERT INTO at_accounts (acc_email_address, acc_password, acc_enabled) " +
                  "VALUES (?, ?, ?)");
      ps.setString(1, userName);
      ps.setString(2, pw_hash);
      ps.setString(3, "1");
      ps.executeUpdate();
    }
    catch (SQLException e) {
      //do stuff on fail
        try {
            conn.rollback();
        } catch (SQLException e1) {
            // TODO Auto-generated catch block
            e1.printStackTrace();
        }
        System.out.println("SQLException createUser 1.");
        e.printStackTrace();
        user = null;
    }
    finally {
        if (result != null) {
            try {
                result.close();
            }
            catch (SQLException e) {
                try {
                    conn.rollback();
                } catch (SQLException e1) {
                    // TODO Auto-generated catch block
                    e1.printStackTrace();
                }
                System.out.println("SQLException createUser 2.");
                e.printStackTrace();
            }
        }
        if (ps != null) {
            try {
                ps.close();
            }   
            catch (SQLException e) {
                try {
                    conn.rollback();
                } catch (SQLException e1) {
                    // TODO Auto-generated catch block
                    e1.printStackTrace();
                }
                System.out.println("SQLException createUser 3.");
                e.printStackTrace();
            }
        }
    }

    try {
        conn.commit();
    } catch (SQLException e) {
        try {
            conn.rollback();
        } catch (SQLException e1) {
            // TODO Auto-generated catch block
            e1.printStackTrace();
        }
        System.out.println("SQLException createUser 4 - commit error.");
        e.printStackTrace();
    }
    return user;
}

This is the updated code with the suggested error handling:

package org.AwardTracker.server;

import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.sql.Connection;
import java.sql.Date;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.ArrayList;
import java.util.List;

import com.google.gwt.user.server.rpc.RemoteServiceServlet;

import org.AwardTracker.client.BCrypt;

import org.AwardTracker.client.Account;
import org.AwardTracker.client.AccountAndCubs;
import org.AwardTracker.client.AccountCubAssociation;
import org.AwardTracker.client.AwardAward;
import org.AwardTracker.client.AwardDescription;
import org.AwardTracker.client.AwardStockDtls;
import org.AwardTracker.client.DBConnection;
import org.AwardTracker.client.SectionDetails;
import org.AwardTracker.client.Stock;
import org.AwardTracker.client.User;
import org.AwardTracker.client.ViewData;
import org.AwardTracker.client.YMATask;
import org.AwardTracker.client.YMAwards;
import org.AwardTracker.client.YMandAward;
import org.AwardTracker.client.YMAwardDetails;
import org.AwardTracker.client.YouthMember;
import org.AwardTracker.client.YouthMemberAwards;
import org.AwardTracker.client.YthMmbrSectDtls;

import org.AwardTracker.server.Base64Encode2;

public class MySQLConnection extends RemoteServiceServlet implements DBConnection {
//TODO
//  •Use JNDI to bind the data source.
//  •Close the connection as soon as its done in finally block.
//  •Manage the connection in single class for whole application.
//  •Initialise the data source at application start up single time.
//  •Store the database configuration outside the JAVA code somewhere in properties file or web.xml.
//  •Create an abstract class for AsyncCallback that will handle all the failures happened while performing any RPC calls.
//  •Extend this abstract class for all RPC AsyncCallback but now you have to just provide implementation of onSuccess() only.
//  •Don't handle any exception in service implementation just throw it to client or if handled then re-throw some meaning full exception back to client.
//  •Add throws in all the methods for all the RemoteService interfaces whenever needed.

private static final long serialVersionUID = 1L;
private Connection conn = null;
private String url = "jdbc:mysql://localhost/awardtracker";
private String user = "awtrack";
private String pass = "************";

public MySQLConnection() {
    try {
        Class.forName("com.mysql.jdbc.Driver");
        conn = DriverManager.getConnection(url, user, pass);
    } catch (Exception e) {
        //NEVER catch exceptions like this

        System.out.println("Error connecting to database - not good eh");
        e.printStackTrace();
    }
}

//Store and retrieve data used by Views within the application
//This allows us to securely pass parameters between Views.
private ViewData viewData = null;

public ViewData setViewData(String accountId, String accountLevel,
        String ymId, String awId, String adGroup) {

    viewData = new ViewData();

    viewData.setaccountId(accountId);
    viewData.setaccountLevel(accountLevel);
    viewData.setymId(ymId);
    viewData.setawId(awId);
    viewData.setadGroup(adGroup);

    return viewData;
}

public ViewData getViewData() {
    return viewData;
}


public User authenticateUser(String accID, String userName, String pass, String level, String pack, Integer enabled, java.sql.Date archived) {

    User user = null; // necessary unless you do something in the exception handler
    ResultSet result = null;
    PreparedStatement ps = null;
    String stored_hash = null;

    try {
        ps = conn.prepareStatement(
          "SELECT * " +
          "FROM at_accounts " +
          "WHERE acc_email_address = ?");

        ps.setString(1, userName);

        result = ps.executeQuery();

        while (result.next()) {
            user = new User(result.getString(1), result.getString(2), result.getString(3), result.getString(4), result.getString(5), result.getInt(6), result.getDate(7));
            stored_hash = result.getString(3);
        }
    }
    catch (SQLException e) {
        try {
            conn.rollback();
        } 
        catch (SQLException e2) {
            System.out.println("Error rolling back transaction for authenticateUser.");
            e2.printStackTrace();
        }
        System.out.println("SQLException in authenticateUser.");
        e.printStackTrace();
    }

    if (stored_hash != null) {
        if (BCrypt.checkpw(pass, stored_hash))  {
        } else {
            user = null;
        }
    }else{
        user = null;
    }

    return user;
}

//Disable or enable Account
public User disableUser(String user, Integer enabled) {
    PreparedStatement ps = null;

    try {
      ps = conn.prepareStatement(
              "UPDATE at_accounts " +
              "SET acc_enabled=? " +
              "WHERE acc_email_address=?");

      ps.setInt(1, enabled);
      ps.setString(2, user);
      ps.executeUpdate();
      conn.commit();
    }
    catch (SQLException e) {
        try {
            conn.rollback();
        } 
        catch (SQLException e2) {
            System.out.println("Error rolling back transaction for createUser.");
            e2.printStackTrace();
        }
        System.out.println("SQLException in createUser.");
        e.printStackTrace();
    }

    return null;
}

public User duplicateUser(String userName, String pass, String level, String pack, java.sql.Date archived) {
    User user = null; // necessary unless you do something in the exception handler
    ResultSet result = null;
    PreparedStatement ps = null;
    try {
      ps = conn.prepareStatement(
          "SELECT * " +
          "FROM at_accounts " +
          "WHERE acc_email_address = ?");

      ps.setString(1, userName);

      result = ps.executeQuery();
      while (result.next()) {
         user = new User(null, result.getString(2), null, null, null, null, null);
      }
    }
    catch (SQLException e) {
        try {
            conn.rollback();
        } 
        catch (SQLException e2) {
            System.out.println("Error rolling back transaction for duplicateUser.");
            e2.printStackTrace();
        }
        System.out.println("SQLException in duplicateUser.");
        e.printStackTrace();
    }
    return user;
}

public User createUser(String userName, String pass, String level, String pack, java.sql.Date archived) {
    PreparedStatement ps = null;
    String pw_hash = BCrypt.hashpw(pass, BCrypt.gensalt());
    try {
      ps = conn.prepareStatement(
          "INSERT INTO at_accounts (acc_email_address, acc_password, acc_enabled) " +
                  "VALUES (?, ?, ?)");
      ps.setString(1, userName);
      ps.setString(2, pw_hash);
      ps.setString(3, "1");
      ps.executeUpdate();
      conn.commit();
    }
    catch (SQLException e) {
        try {
            conn.rollback();
        } 
        catch (SQLException e2) {
            System.out.println("Error rolling back transaction for createUser.");
            e2.printStackTrace();
        }
        System.out.println("SQLException in createUser.");
        e.printStackTrace();
    }

    return null;
}
Glyn
  • 1,933
  • 5
  • 37
  • 60
  • Is three any Exception logged? – Braj May 11 '14 at 04:55
  • Hi Braj, I have asked this question of the person setting up the server and have not heard back yet. I have to be patient as he is doing the set up in his own time as a favour to our Group. Regards, Glyn. – Glyn May 11 '14 at 22:51
  • I will take this one step at a time. I have now posted the change with the new error handling. Please check to ensure this is what you mean. Next I will move error handling to an abstract class (need to pass parameters to it so I know which class is returning the error), look at log4j and move the DB login to XML. Regards, Glyn. – Glyn May 11 '14 at 23:15

2 Answers2

1

You aren't committing the transaction to the database. In order for the changes made by ps.executeUpdate(); to become permanent, you will need to call conn.commit(); after the update.

Similarly, in your catch block you should call conn.rollback(); so as to avoid the possibility of dud data being inserted into the database.

I can't see the declaration for conn, so I assume it's a member variable whatever class createUser belongs to. You might want to consider changing the Connection to be a local in the method, so that you don't forget to close it once it's no longer needed (which should be once you've committed).

Lastly, if you're using Java 7+, you can take advantage of try-with-resources to handle the closing of your PreparedStatement, ResultSet and Connection for you (although you don't appear to be using the ResultSet for anything, so consider removing it from the method).


Here are two examples of what I meant (one for Java 6 and below, and one for Java 7 and later utilising try-with-resources:

Java 6-

public void createUser(String userName, String pass) {
    PreparedStatement ps = null;
    Connection conn = null;
    String pw_hash = BCrypt.hashpw(pass, BCrypt.gensalt());
    try {
        // Acquire a Connection here rather than using a member variable
        // NOTE: See Braj's answer for a better way of doing this
        // using his ConnectionUtil class.
        conn = DriverManager.getConnection(
                    "jdbc:mysql://localhost/awardtracker", "awtrack", 
                    "**************");

        ps = conn.prepareStatement(
                "INSERT INTO at_accounts (acc_email_address, acc_password,"
                  + " acc_enabled) "
              + "VALUES (?, ?, ?)");
        ps.setString(1, userName);
        ps.setString(2, pw_hash);
        ps.setString(3, "1");
        ps.executeUpdate();
        conn.commit();
    } catch (SQLException e) {
        try {
            conn.rollback();
        } catch (SQLException e2) {
            System.out.println("Error rolling back transaction.");
            e2.printStackTrace();
        }
        System.out.println("SQLException createUser 1.");
        e.printStackTrace();
    } finally {
        if (ps != null) {
            try {
                ps.close();
            } catch (SQLException e) {
                System.out.println("SQLException createUser 3.");
                e.printStackTrace();
            }
        }

        if (conn != null) {
            try {
                conn.close();
            } catch (SQLException e) {
                System.out.println("Error closing Connection.");
                e.printStackTrace();
            }
        }
    }
}

Java 7+

private static final String INSERT_STATEMENT =
        "INSERT INTO at_accounts (acc_email_address, acc_password, "
      + "acc_enabled) VALUES (?, ?, ?)";

public void createUser(String userName, String pass) {
    String pw_hash = BCrypt.hashpw(pass, BCrypt.gensalt());

    // NOTE: See Braj's answer for a better way of getting Connections.
    try (Connection conn = DriverManager.getConnection(
            "jdbc:mysql://localhost/awardtracker", "awtrack",
            "**************");
         PreparedStatement ps = conn.prepareStatement(INSERT_STATEMENT);) {

        try {
            ps.setString(1, userName);
            ps.setString(2, pw_hash);
            ps.setString(3, "1");
            ps.executeUpdate();
            conn.commit();
        } catch (SQLException e) {
            try {
                conn.rollback();
            } catch (SQLException e2) {
                System.out.println("Error rolling back transaction.");
                e2.printStackTrace();
            }
            System.out.println("SQLException createUser 1.");
            e.printStackTrace();
        }
    } catch (SQLException e) {
        System.out.println("Error connecting to DB.");
        e.printStackTrace();
    }
}

In both examples I have removed unused method parameters (why have them there if you're doing nothing with them?) and have changed the return type to void. I did this because in its current form your method will always return null (you initialise your User object to null, then do nothing with it to change its value, then return it at the end).

You should also consider using a logging framework such as log4j to handle your exception logging rather than relying on printStackTrace(). See Why is exception.printStackTrace() considered bad practice? for more information on why printStackTrace() isn't recommended.

Community
  • 1
  • 1
JonK
  • 2,097
  • 2
  • 25
  • 36
  • Hi JoinK, Thank you. I have entered my revised code above. Is this correct /what you meant please? Regards, Glyn. – Glyn May 11 '14 at 06:14
  • Hi JonK, I have updated my code above with your error handling. If this is OK then I will move all error handling into a single class as suggested by Braj. I will then look at using log4j and moving the connections to XML. Regards, Glyn. – Glyn May 11 '14 at 23:21
  • Hi JonK, When testing this the user is successfully created however an error is generated you cannot commit when auto commit is yes. So I have removed the "conn.commit();" line. Is this the correct thing to do or should I turn auto commit off (where is this)? Regards, Glyn. – Glyn May 11 '14 at 23:39
  • Glyn - you should probably turn off auto-commit yes. [This article](http://dev.mysql.com/doc/refman/5.0/en/server-system-variables.html#sysvar_autocommit) shows you how to turn it off. – JonK May 11 '14 at 23:48
  • OK, thanks for your help. I take it that the rest of what I am doing is correct. Regards, Glyn. – Glyn May 12 '14 at 03:20
  • It looks a bit better now (though I would still make `conn` a local variable instead of a member variable, tidy up the unused method parameters and change the return type to `void`). Also, if you're using Java 7+ you no longer need to explicitly call `Class.forName(String)` to load the driver, so you might be able to get rid of that too. – JonK May 12 '14 at 08:16
  • Hi JonK, I also very much appreciate your efforts and assistance. I will work through this and implement as I can. Regards, Glyn. – Glyn May 12 '14 at 21:36
  • Hi JonK, "Baby Steps". I now understand the void and have successfully made the change for createUser method. I will now implement this in the other methods that do not return a value. Regards, Glyn. – Glyn May 12 '14 at 21:48
1

Points to rememeber:

  • Use JNDI to bind the data source.
  • Close the connection as soon as its done in finally block.
  • Manage the connection in single class for whole application.
  • Initialize the data source at application start up single time.
  • Store the database configuration outside the JAVA code somewhere in properties file or web.xml.

I have already shared a sample code for ConnectionUtil class that's sole purpose is to manage the connection in single class using JNDI lookup and it can log that how many connections are opened for what time in the application?

Please have a look at below posts:


GWT - how to catch an exception correctly?

  • Create an abstract class for AsyncCallback that will handle all the failures happened while performing any RPC calls.
  • Extend this abstract class for all RPC AsyncCallback but now you have to just provide implementation of onSuccess() only.
  • Don't handle any exception in service implantation just throw it to client or if handled then re-throw some meaning full exception back to client.
  • Add throws in all the methods for all the RemoteService interfaces whenever needed.

Sample code:

// single class to handle all the AsyncCallback failure
public abstract class MyAsyncCallback<T> implements AsyncCallback<T> {

    @Override
    public void onFailure(Throwable caught) {
        // all the failure are catched here
        // prompt user if needed
        // on failure message goes to here
        // send the failure message back to server for logging
    }

}

// do it for all the RPC AsyncCallback
public class CreationHandler<T> extends MyAsyncCallback<T> {
    //Create the account.
    public void onSuccess(T result) {
        // on success message goes to here
    }
}

// use in this way
AsyncCallback<User> callback = new CreationHandler<User>();
Community
  • 1
  • 1
Braj
  • 46,415
  • 5
  • 60
  • 76
  • Hi Braj, Thank you for this. It will take me some time to digest this and work it out. Regards, Glyn. – Glyn May 11 '14 at 06:16
  • @Glyn I have updated my post to handler all the RPC exception in GWT in single class. – Braj May 11 '14 at 07:02
  • Hi Braj, Thanks for this. I have updated my code to include the correct error handling from JonK. Next I will move the error handling to your single class. Regards, Glyn. – Glyn May 11 '14 at 23:17
  • Hi Braj, I went to include your code above on the server side then it struck me that this is the same code I am using on the client side. I think I totally miss understood what you were saying. So I think I am doing the right thing on the client side in capturing messages sent back from the server side and displaying error messages when appropriate. All I need to do is get my server side code correct. Is that right? Regards, Glyn. – Glyn May 11 '14 at 23:53
  • Suppose you are making 100 RPC calls with different type of AsyncCallbacks then you have to override `onFailure()` method for all the calls but by extending single class `MyAsyncCallback` that I provided in my post will responsible for `onFailure()` method, you don'tn need to implement `onFailure()` for all 100 RPC calls of different type of AsyncCallbacks. I hope you got it now. I summarize it again **single `onFailure()` method for all the RPC calls for whole application.** – Braj May 12 '14 at 07:20
  • Hi Braj, I very much appreciate all your help and I apologise if I am a little slow. I will persist in understanding and implementing this. Regards, Glyn. – Glyn May 12 '14 at 21:34
  • My pleasure. Happy leaning. – Braj May 12 '14 at 21:36