[WFLY-22003] Replace JPA initialization with JDBC CDI bean

[Riovo]

Fixes: https://redhat.atlassian.net/browse/WFLY-22003

Replaces the current JPA-based database initialization with a CDI bean using direct JDBC. This eliminates the WildFly Glow unbound datasource warning that popped up because persistence.xml referenced a CLI-defined datasource.

The CDI bean observes application startup (@observes @initialized(ApplicationScoped.class)) to initialize the H2 database schema and test users for the Elytron JDBC realm.

[luck3y]

LGTM, I added some suggestions, feel free to disagree :)

[emmartins]

Personally I dislike the usage of an EJB for this, I know this was suggested in the original JIRA but EJB is kind of legacy and considered an expensive resource, I suggest instead either:

1. In case DB initialization separated from the Servlet is prefered, to turn DatabaseInitializer into a CDI bean that observes app initialization:

@ApplicationScoped
public class DatabaseInitializer {
    ...
    public void initializeDatabase(@Observes @Initialized(ApplicationScoped.class) Object init) {
       ...
    }
    ...
}

2. Use nothing else than the existing Servlet, overriding its init() method, where you will invoke DatabaseInitializer, and changing the WebServlet annotation to load-init on startup:

@WebServlet(value = "/SecuredServlet", loadOnStartup = 1)
@ServletSecurity(@HttpConstraint(rolesAllowed = { "quickstarts" }))
public class SecuredServlet extends HttpServlet {
    ...
    @Resource(lookup = "java:jboss/datasources/ServletSecurityDS")
    private DataSource dataSource;

    @Override
    public void init() throws ServletException {
        new DatabaseInitializer(dataSource).initializeDatabase();
    }
    ...
}

Either requires no changes to the CLI script, or aditional dependencies.

PS: Ken remarks would still need to be considered.

[Riovo]

Thank you @emmartins and @luck3y for your comments. I have since implemented everything, please let me know if everything is good or if changes need to be done.

[emmartins]

@Riovo please squash all commits into one too :)

[Riovo]

@Riovo please squash all commits into one too :)

Hehe will do, thank you for you time :)

[Riovo]

@darranl

[emmartins]

LGTM @Riovo, I requested review from @kstekovi too, he was the original issue[1] reporter.

[1] https://redhat.atlassian.net/browse/JBEAP-33412

[kstekovi]

LGTM overall, one non-blocking nit: insertTestData() runs each INSERT with auto-commit on, and userExists() only gates on the USERS row. If a statement fails mid-sequence (e.g. between the USERS and ROLES inserts), a later redeploy would see quickstartUser already present and skip re-inserting the missing ROLES/USERS_ROLES rows.

Low risk given the small dataset, but conn.setAutoCommit(false) + commit at the end / rollback on SQLException would make the whole insert block atomic and avoid that edge case. Up to you — fine to merge as-is either way.

[emmartins]

LGTM overall, one non-blocking nit: insertTestData() runs each INSERT with auto-commit on, and userExists() only gates on the USERS row. If a statement fails mid-sequence (e.g. between the USERS and ROLES inserts), a later redeploy would see quickstartUser already present and skip re-inserting the missing ROLES/USERS_ROLES rows.

Low risk given the small dataset, but conn.setAutoCommit(false) + commit at the end / rollback on SQLException would make the whole insert block atomic and avoid that edge case. Up to you — fine to merge as-is either way.

@kstekovi Since the datasource uses default autocommit and the CDI Bean is not transactional, I think it will autocommit when every connection instance is closed, and if that is the case all needed to enforce your enhancement would be to create the connection at the event handler and, pass it along the 2 methods in it.

@Riovo can you please investigate and optimize this code if needed?

[Riovo]

Firstly sorry for the long wait, I was on rotation working on Lightwell. Thanks @emmartins and @kstekovi for the feedback. I've updated the code:

• A single Connection is now created in initializeDatabase() and passed to both createTables() and insertTestData(), avoiding unnecessary extra connections from the datasource.
• insertTestData() now wraps all inserts in a transaction (setAutoCommit(false) + commit/rollback) so the insert block is atomic — if any statement fails mid-sequence, everything rolls back and a redeploy will correctly re-insert all data.

[emmartins]

I think you should start by dropping the tables if those exist, this would allow you to not have to worry if user exists, which as it stands now does not guarantee test data is correct, and would also allow you to wrap all updates in a single commit... Currently table creation is is done with autocommit. My suggestion would be something like this:

    public void initializeDatabase(@Observes @Initialized(ApplicationScoped.class) Object init) {
        LOGGER.info("Initializing database schema for servlet-security...");

        try (Connection conn = dataSource.getConnection()) {
            conn.setAutoCommit(false);
            createTables(conn);
            insertTestData(conn);
            conn.commit();
            LOGGER.info("Database initialization completed successfully.");       
        } catch (SQLException e) {
            LOGGER.log(Level.SEVERE, "Failed to initialize database", e);
            conn.rollback();
            throw new RuntimeException("Database initialization failed", e);
        }
    }

    private void createTables(Connection conn) throws SQLException {
        try (Statement stmt = conn.createStatement()) {
            stmt.executeUpdate("DROP TABLE IF EXISTS USERS");
            stmt.executeUpdate(
                "CREATE TABLE USERS (" +
                "ID INT, " +
                "USERNAME VARCHAR(20), " +
                "PASSWORD VARCHAR(20))"
            );

            stmt.executeUpdate("DROP TABLE IF EXISTS ROLES");
            stmt.executeUpdate(
                "CREATE TABLE ROLES (" +
                "ID INT, " +
                "NAME VARCHAR(20))"
            );

            stmt.executeUpdate("DROP TABLE IF EXISTS USERS_ROLES");
            stmt.executeUpdate(
                "CREATE TABLE USERS_ROLES (" +
                "USER_ID INT, " +
                "ROLE_ID INT)"
            );

            LOGGER.info("Database tables created");
        }
    }


    private void insertTestData(Connection conn) throws SQLException {

            try (PreparedStatement insertStmt = conn.prepareStatement(
                "INSERT INTO USERS (ID, USERNAME, PASSWORD) VALUES (?, ?, ?)")) {

                insertStmt.setInt(1, 1);
                insertStmt.setString(2, "quickstartUser");
                insertStmt.setString(3, "quickstartPwd1!");
                insertStmt.executeUpdate();

                insertStmt.setInt(1, 2);
                insertStmt.setString(2, "guest");
                insertStmt.setString(3, "guestPwd1!");
                insertStmt.executeUpdate();
            }

            try (PreparedStatement insertStmt = conn.prepareStatement(
                "INSERT INTO ROLES (ID, NAME) VALUES (?, ?)")) {

                insertStmt.setInt(1, 1);
                insertStmt.setString(2, "quickstarts");
                insertStmt.executeUpdate();

                insertStmt.setInt(1, 2);
                insertStmt.setString(2, "guest");
                insertStmt.executeUpdate();
            }

            try (PreparedStatement insertStmt = conn.prepareStatement(
                "INSERT INTO USERS_ROLES (USER_ID, ROLE_ID) VALUES (?, ?)")) {

                insertStmt.setInt(1, 1);
                insertStmt.setInt(2, 1);
                insertStmt.executeUpdate();

                insertStmt.setInt(1, 2);
                insertStmt.setInt(2, 2);
                insertStmt.executeUpdate();
            }

            LOGGER.info("Test data inserted successfully");
    }

[emmartins]

@kstekovi IMHO this is now good to merge, can you please re-review?