DatabaseManager singleton makes testing difficult

[leesjensen]

The global DatabaseManager class makes it difficult to do testing using dependency inversion.

1. Properties are read from a file rather than providing them to the manager.
2. The singleton pattern makes it difficult to load different databases for different tests or to create and delete a database for a test.
3. It forces the same database to be used for both testing and development.
4. It forces tests to use introspection to implemented desired behavior at runtime.
5. It encourages the exposure of a dangerous endpoint (clear) in order to enable testing.

Most implementations are already passing a DataAccess object around and so there is little value in a DatabaseManager singleton. We should just initialize the DataAccess object with the desired DatabaseManager. Then a test could provide a mocked database version if desired, or set up and tear down a test database. You could get rid of the clear endpoint.

[leesjensen]

Of course, if we want to leave this as an example of bad design, we could leave this as is for instructional purposes.

[rodham]

Fixing this seems like a good idea.

[leesjensen]

@rodham, @jerodw, @mewilker, @Maillman - I did a quick test to see if this would make things better and it looks good.

TL;DR;

1. Code is much cleaner.
2. Gets rid of clear function (Jumping for joy.)
3. Gets rid of complex reflection calls in testing.
4. Allows for temporary testing databases that are deleted after tests. This keeps your dev database from getting polluted or cleared.

If we do this, here is the remaining work to do:

1. Replace starter code with new code. - Should only take a few minutes.
2. Replace instruction that references the old DatabaseManager. - Should only take an hour or so.
3. Fix references in videos if they exist? - this is the hard one and I don't know what it involves.

Except for what might be required for the videos, I could easily make this change next week after I merge the fall 2025 branch.

Here are all the details:

DBManager

Created a DBManager interface with a MySQLDBManager implementation.

public interface DBManager {
    public void createDatabase() throws DataAccessException;
    public void clearDatabase() throws DataAccessException;
    public void deleteDatabase() throws DataAccessException;
    public Connection getConnection() throws DataAccessException;
    public String dbName();
}

DBManager needs to be interface in order to do dependency injection.

The MySQLDBManager class has two constructors. One that simply reads from the properties file, and one that allows you to override the database name. This keeps things compatible with how it was being used before but allows for temporary databases that can be created and destroyed during testing.

Server impact

The server now takes a DBManager as a parameter. That is then passed to the DataAccess layer SQL implementation. This provides for the dependency injection.

public class Main {
    public static void main(String[] args) throws Exception {
          var dbManager = new MySQLDBManager();
          Server server = new Server(dbManager);
          server.run(8080);
    }
}

Test impact

The tests now all create the DBManager that they need, call clearDatabase when they want to refresh the database, and call deleteDatabase when they are done.

public class ServerFacadeTests {

    private static Server server;
    private static DBManager dbManager;
    private static ServerFacade serverFacade;

    @BeforeAll
    public static void init() throws Exception {
        dbManager = new MySQLDBManager("test_serverFacade");
        server = new Server(dbManager);
        var port = server.run(0);
        serverFacade = new ServerFacade(String.format("http://localhost:%d", port), null);
        System.out.println("Started test HTTP server on " + port);
    }

    @AfterEach
    public void clearDb() throws DataAccessException {
        dbManager.clearDatabase(); // This could probably be removed with a bit of work.
    }

    @AfterAll
    static void stopServer() throws DataAccessException {
        server.stop();
        dbManager.deleteDatabase();
    }

We should actually be able to get rid of the clear function entirely since each suite has its own temporary database and it is actually a bad idea to clear between each tests. Calling clear might be hiding some bugs in the tests, but I didn't play with this much, but I didn't remove it from the ServerFacadeTests. Everything passed except for ListGames. I guess that makes sense, but really the test should just add a couple games and make sure they get returned in the list while ignoring any extra games that are in the list from other tests.

Clear endpoint

No need for this anymore! 🥳 🥳 🥳

Removal of test reflection

Dependency injection and the removal of the singleton allowed me to clean up the databaseErrorHandlering test function that previously used Java reflection to rewrite the DatabaseManager and make it fail during runtime. This was a good change as the reflect felt really like a bad hack.

[rodham]

You're right, the problem with making this change now is the videos. There are some Phase 3 videos that make casual reference to the clear endpoint, and some Phase 4 videos that cover in detail or reference the current DatabaseManager. We made some updated videos in spring term that will be incorporated for fall semester. That will occur next week. It's probably too late to make more new videos at this point, but we could ask if you're interested in doing that. If we kept the clear endpoint even though we don't need it, and only change the DatabaseManager stuff, that would reduce the number of affected videos. The other option is to roll this into the changes for next year, which is my preference.

[leesjensen]

Waiting until next Fall is fine with me. I will archive the proposed code changes and we can look at it again next year.

[rodham]

Thank you, Lee. I know it's not ideal to have to wait.

[leesjensen]

Here are the changes that need to happen to the Chess repo (the starter code) and to a Student's repo that uses the alternative database manager.

You can see all the changes in context in this Pull request to Lee's private implementation of the Chess project.

DatabaseManager.java

• DELETE server/src/main/java/dataaccess/DatabaseManager.java

This is the only global singleton class and is replaced by DBManager. Alternatively we can rename DBManager to DatabaseManager if that doesn't cause confusion.

DBManager.java

• ADD server/src/main/java/dataaccess/DBManager.java

Interface for new database management functionality.

package dataaccess;

import java.sql.Connection;

public interface DBManager {
    public void createDatabase() throws DataAccessException;

    public void clearDatabase() throws DataAccessException;

    public void deleteDatabase() throws DataAccessException;

    public Connection getConnection() throws DataAccessException;

    public String dbName();
}

MySQLDBManager.java

• ADD server/src/main/java/dataaccess/MySQLDBManager.java

Implementation of DBManager for MySQL.

package dataaccess;

import java.sql.*;
import java.util.*;

public class MySQLDBManager implements DBManager {
    private String databaseName;
    private String dbUsername;
    private String dbPassword;
    private String connectionUrl;

    /**
     * Initialize from the properties file db.properties.
     */
    public MySQLDBManager() {
        loadPropertiesFromResources();
    }

    /**
     * Initialize from the properties file db.properties and overrides the database name. This is useful
     * for testing where you don't want to modify your actual database and need to be able to clean up
     * after the tests.
     */
    public MySQLDBManager(String databaseName) {
        loadPropertiesFromResources();
        this.databaseName = databaseName;
    }

    /**
     * Creates the database if it does not already exist.
     */
    public void createDatabase() throws DataAccessException {
        var statement = "CREATE DATABASE IF NOT EXISTS " + databaseName;
        try (var conn = DriverManager.getConnection(connectionUrl, dbUsername, dbPassword);
             var preparedStatement = conn.prepareStatement(statement)
        ) {
            preparedStatement.executeUpdate();
        } catch (SQLException e) {
            throw new DataAccessException(String.format("Failed to create database: %s", e.getMessage()));
        }
    }

    /**
     * Clears out all content found in the database tables.
     */
    public void clearDatabase() throws DataAccessException {
        try (
                var conn = getConnection();
                var readStmt = conn.createStatement();
                var resultSet = readStmt.executeQuery("SHOW TABLES");
                var writeStmt = conn.createStatement()
        ) {
            while (resultSet.next()) {

                writeStmt.executeUpdate("TRUNCATE TABLE " + resultSet.getString(1));
            }
        } catch (SQLException e) {
            throw new DataAccessException(String.format("Failed to clear database: %s", e.getMessage()));
        }
    }


    /**
     * Deletes the database.
     */
    public void deleteDatabase() throws DataAccessException {
        var statement = "DROP DATABASE " + databaseName;
        try (var conn = getConnection(); var preparedStatement = conn.prepareStatement(statement)) {
            preparedStatement.executeUpdate();
        } catch (SQLException e) {
            throw new DataAccessException(String.format("Failed to delete database: %s", e.getMessage()));
        }
    }


    /**
     * Create a connection to the database and sets the catalog based upon the
     * properties specified in db.properties. Connections to the database should
     * be short-lived, and you must close the connection when you are done with it.
     * The easiest way to do that is with a try-with-resource block.
     * <br/>
     * <code>
     * try (var conn = DatabaseManager.getConnection()) {
     * // execute SQL statements.
     * }
     * </code>
     */
    public Connection getConnection() throws DataAccessException {
        try {
            var conn = DriverManager.getConnection(connectionUrl, dbUsername, dbPassword);
            conn.setCatalog(databaseName);
            return conn;
        } catch (SQLException ex) {
            throw new DataAccessException("failed to get connection", ex);
        }
    }


    public String dbName() {
        return databaseName;
    }

    private void loadPropertiesFromResources() {
        try (var propStream = Thread.currentThread().getContextClassLoader().getResourceAsStream("db.properties")) {
            if (propStream == null) {
                throw new Exception("Unable to load db.properties");
            }
            Properties props = new Properties();
            props.load(propStream);
            loadProperties(props);
        } catch (Exception ex) {
            throw new RuntimeException("unable to process db.properties", ex);
        }
    }

    private void loadProperties(Properties props) {
        databaseName = props.getProperty("db.name");
        dbUsername = props.getProperty("db.user");
        dbPassword = props.getProperty("db.password");

        var host = props.getProperty("db.host");
        var port = Integer.parseInt(props.getProperty("db.port"));
        connectionUrl = String.format("jdbc:mysql://%s:%d", host, port);
    }

}

Server.java

• MODIFY server/src/main/java/server/Server.java

Server takes dependency inversion parameter.

public class Server {

    private Javalin javalin;

    public Server(DBManager dbManager) {
        try {
            javalin = Javalin.create(config -> config.staticFiles.add("web"));

            DataAccess dataAccess = new MySqlDataAccess(dbManager);
            var endpointManager = new EndpointManager(dataAccess);
            endpointManager.register(javalin);

StandardAPITests and WebSocketTests

• MODIFY server/src/test/java/passoff/server/StandardAPITests.java
• MODIFY server/src/test/java/passoff/server/WebSocketTests.java

These need to change to use DBManager.

DatabaseTests.java

• MODIFY server/src/test/java/passoff/server

This uses the new DBManager to dynamically inject the functionality instead of hard coding or using introspection in the tests. The DatabaseErrorHandling test deserves discussion as the old implementation did some interesting things that the new version replaces with a simple dependency inversion.

package passoff.server;

import chess.ChessGame;
import dataaccess.*;
import org.junit.jupiter.api.*;
import passoff.model.*;
import server.Server;

import java.sql.*;
import java.util.*;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Supplier;

@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
public class DatabaseTests {

    private static final TestUser TEST_USER = new TestUser("ExistingUser", "existingUserPassword", "eu@mail.com");

    private static TestServerFacade serverFacade;
    private static DBManager dbManager;

    private static Server server;


    @BeforeAll
    public static void startServer() {
        dbManager = new MySQLDBManager("test_chessPassOffDatabase");
        server = new Server(dbManager);
        var port = server.run(0);
        System.out.println("Started test HTTP server on " + port);

        serverFacade = new TestServerFacade("localhost", Integer.toString(port));
    }

    @BeforeEach
    public void setUp() throws DataAccessException {
        dbManager.clearDatabase();
    }

    @AfterAll
    static void stopServer() throws DataAccessException {
        server.stop();
        dbManager.deleteDatabase();
    }


    @Test
    @DisplayName("Persistence Test")
    @Order(1)
    public void persistenceTest() throws DataAccessException {
        int initialRowCount = getDatabaseRows();

        TestAuthResult regResult = serverFacade.register(TEST_USER);
        String auth = regResult.getAuthToken();

        //create a game
        String gameName = "Test Game";
        TestCreateResult createResult = serverFacade.createGame(new TestCreateRequest(gameName), auth);

        //join the game
        serverFacade.joinPlayer(new TestJoinRequest(ChessGame.TeamColor.WHITE, createResult.getGameID()), auth);

        Assertions.assertTrue(initialRowCount < getDatabaseRows(), "No new data added to database");

        // Test that we can read the data after a restart
        server.stop();
        startServer();

        //list games using the auth
        TestListResult listResult = serverFacade.listGames(auth);
        Assertions.assertEquals(200, serverFacade.getStatusCode(), "Server response code was not 200 OK");
        Assertions.assertEquals(1, listResult.getGames().length, "Missing game(s) in database after restart");

        TestListEntry game1 = listResult.getGames()[0];
        Assertions.assertEquals(game1.getGameID(), createResult.getGameID());
        Assertions.assertEquals(gameName, game1.getGameName(), "Game name changed after restart");
        Assertions.assertEquals(TEST_USER.getUsername(), game1.getWhiteUsername(),
                "White player username changed after restart");

        //test that we can still log in
        serverFacade.login(TEST_USER);
        Assertions.assertEquals(200, serverFacade.getStatusCode(), "Unable to login");
    }

    @Test
    @DisplayName("Bcrypt")
    @Order(2)
    public void bcrypt() {
        serverFacade.register(TEST_USER);

        executeForAllTables(this::checkTableForPassword);
    }


    @Test
    @DisplayName("Database Error Handling")
    @Order(3)
    public void databaseErrorHandling() {
        var badDbManager = new FailureDBManager(dbManager);
        var server = new Server(badDbManager);
        var port = server.run(0);
        System.out.println("Started test HTTP server on " + port);

        var serverFacade = new TestServerFacade("localhost", Integer.toString(port));

        List<Supplier<TestResult>> operations = List.of(
                () -> serverFacade.clear(),
                () -> serverFacade.register(TEST_USER),
                () -> serverFacade.login(TEST_USER),
                () -> serverFacade.logout(UUID.randomUUID().toString()),
                () -> serverFacade.createGame(new TestCreateRequest("inaccessible"), UUID.randomUUID().toString()),
                () -> serverFacade.listGames(UUID.randomUUID().toString()),
                () -> serverFacade.joinPlayer(new TestJoinRequest(ChessGame.TeamColor.WHITE, 1), UUID.randomUUID().toString())
        );

        badDbManager.failing = true;

        try {
            for (Supplier<TestResult> operation : operations) {
                TestResult result = operation.get();
                Assertions.assertEquals(500, serverFacade.getStatusCode(),
                        "Server response code was not 500 Internal Error");
                Assertions.assertNotNull(result.getMessage(), "Invalid Request didn't return an error message");
                Assertions.assertTrue(result.getMessage().toLowerCase(Locale.ROOT).contains("error"),
                        "Error message didn't contain the word \"Error\"");
            }
        } finally {
            server.stop();
        }
    }

    /*
      This class enables the simulation db failure after the server is already running (it started with
      MySQL working normally). If this happens, this should be considered an "Internal Server Error" and the response
      code for any endpoint which no longer can do what it needs to do (which for this project should be all of them)
      should be 500. The body of each of these responses should include a reasonable, relevant error message.
    */
    private static class FailureDBManager implements DBManager {
        private final DBManager dbManager;
        public boolean failing = false;

        public FailureDBManager(DBManager dbManager) {
            this.dbManager = dbManager;
        }

        public void createDatabase() throws DataAccessException {
            dbManager.createDatabase();
        }

        public void clearDatabase() throws DataAccessException {
            dbManager.clearDatabase();
        }

        public void deleteDatabase() throws DataAccessException {
            dbManager.deleteDatabase();
        }

        public Connection getConnection() throws DataAccessException {
            if (failing) {
                throw new DataAccessException(500, "trouble", null);
            }
            return dbManager.getConnection();
        }

        public String dbName() {
            return "failureDB";
        }
    }


    private int getDatabaseRows() {
        AtomicInteger rows = new AtomicInteger();
        executeForAllTables((tableName, connection) -> {
            try (var statement = connection.createStatement()) {
                var sql = "SELECT count(*) FROM " + tableName;
                try (var resultSet = statement.executeQuery(sql)) {
                    if (resultSet.next()) {
                        rows.addAndGet(resultSet.getInt(1));
                    }
                }
            }
        });

        return rows.get();
    }

    private void checkTableForPassword(String table, Connection connection) throws SQLException {
        String sql = "SELECT * FROM " + table;
        try (Statement statement = connection.createStatement(); ResultSet rs = statement.executeQuery(sql)) {
            ResultSetMetaData rsmd = rs.getMetaData();
            int columns = rsmd.getColumnCount();
            while (rs.next()) {
                for (int i = 1; i <= columns; i++) {
                    String value = rs.getString(i);
                    Assertions.assertFalse(value.contains(TEST_USER.getPassword()),
                            "Found clear text password in database");
                }
            }
        }
    }

    private void executeForAllTables(TableAction tableAction) {
        String sql = """
                    SELECT table_name
                    FROM information_schema.tables
                    WHERE table_schema = DATABASE();
                """;

        try (Connection conn = getConnection(); PreparedStatement preparedStatement = conn.prepareStatement(sql)) {
            try (var resultSet = preparedStatement.executeQuery()) {
                while (resultSet.next()) {
                    tableAction.execute(resultSet.getString(1), conn);
                }
            }
        } catch (DataAccessException | SQLException e) {
            Assertions.fail(e.getMessage(), e);
        }
    }

    private Connection getConnection() throws DataAccessException {
        return dbManager.getConnection();
    }

    @FunctionalInterface
    private interface TableAction {
        void execute(String tableName, Connection connection) throws SQLException;
    }

}

Changes for Students's implementation

This is an example of how a student's implementation would change.

Main.java

Pass DBManager to the server on main.

public class Main {
    public static void main(String[] args) {
        try {
            var dbManager = new MySQLDBManager();
            Server server = new Server(dbManager);
            server.run(8080);

            System.out.println("♕ 240 Chess Server");
        } catch (Exception ex) {
            System.out.println("Unable to start server: " + ex);
        }
    }
}

MySQLDataAccess

MySQLDataAccess uses the new DBManager.

Use the new DBManager in the test setup.

public abstract class DbTests {
    protected static DataAccess db;
    protected static DBManager connectionManager;

    @BeforeAll
    static void createDb() throws Exception {
        connectionManager = new MySQLDBManager("test_dbTests");
        db = new MySqlDataAccess(connectionManager);
    }


    @AfterAll
    static void deleteDb() throws Exception {
        connectionManager.deleteDatabase();
    }


    @BeforeEach
    public void ClearDb() throws Exception {
        connectionManager.clearDatabase();
    }

ServerFacadeTests.java

Uses the DBManager.

public class ServerFacadeTests {

    private static Server server;
    private static DBManager dbManager;
    private static ServerFacade serverFacade;

    @BeforeAll
    public static void init() throws Exception {
        dbManager = new MySQLDBManager("test_serverFacade");
        server = new Server(dbManager);
        var port = server.run(0);
        serverFacade = new ServerFacade(String.format("http://localhost:%d", port), null);
        System.out.println("Started test HTTP server on " + port);
    }

    @AfterEach
    public void clearDb() throws DataAccessException {
//        dbManager.clearDatabase();
    }

    @AfterAll
    static void stopServer() throws DataAccessException {
        server.stop();
        dbManager.deleteDatabase();
    }

[19mdavenport]

@leesjensen Out of curiosity, is this something you would have wanted me to try to change when I added some of the brittle features and/or the "bad hack"? I was the one who added most of the introspection/reflection code, but I was operating under a desire to create as few side effects as possible in the project and make something that worked well enough for what we needed at them time while changing as little as possible outside of the specific code in which I was working. Would you rather have seen a PR from we with a larger scope?

[leesjensen]

@19mdavenport - I think you did the right thing at the time. Your test achieved the goal for testing failure without significant disruption to the rest of the code or instruction.