Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughDatabaseManager now uses a pooled JDBC connection source (JdbcPooledConnectionSource), adds a parameterized init(dbUrl, dbUser, dbPassword) plus a no-arg init() that reads config, explicitly loads the MySQL driver, and adjusts table creation to check existence before creating. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config (FileConfiguration)
participant Plugin as Plugin
participant DBMgr as DatabaseManager
participant Driver as JDBC Driver Loader
participant Pool as JdbcPooledConnectionSource
participant DAO as ORMLite DAO / TableUtils
Config->>Plugin: plugin startup (reads config)
Plugin->>DBMgr: init() [no-arg]
DBMgr->>Config: read url/user/pass
DBMgr->>DBMgr: init(dbUrl, dbUser, dbPassword)
DBMgr->>Driver: Class.forName("com.mysql.cj.jdbc.Driver")
alt driver loads successfully
Driver-->>DBMgr: driver OK
else driver missing
Driver-->>DBMgr: ClassNotFoundException
DBMgr-->>Plugin: log severe + throw RuntimeException
end
DBMgr->>Pool: create JdbcPooledConnectionSource(dbUrl, user, pass)
Pool-->>DBMgr: pooled source available
DBMgr->>DAO: ensure table exists (TableUtils.createTableIfNotExists)
DAO-->>DBMgr: table ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…egrate menu-lib" This reverts commit e567f9f. modified: build.gradle modified: gradle.properties modified: src/main/java/me/axeno/hommr/Hommr.java modified: src/main/java/me/axeno/hommr/commands/HomeCommands.java deleted: src/main/java/me/axeno/hommr/menus/HomeListMenu.java
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/main/java/me/axeno/hommr/managers/DatabaseManager.java`:
- Line 14: The code in DatabaseManager erroneously imports and catches
java.nio.channels.ConnectionPendingException (remove the import and the catch
block that references ConnectionPendingException), because JDBC/ORMLite will not
throw that NIO exception; instead replace that catch with an appropriate
database exception such as java.sql.SQLException (or broaden to Exception only
if you need to handle any runtime errors), update the catch clause in the
method(s) that currently catch ConnectionPendingException to catch
java.sql.SQLException (and add the import for java.sql.SQLException if needed),
and ensure any logging/handling inside the new catch uses the caught
SQLException variable.
- Around line 34-37: The error message in DatabaseManager is inconsistent with
the hardcoded driver; update the catch in the static initializer (where
Class.forName("com.mysql.cj.jdbc.Driver") is called) to either attempt loading
the additional drivers you intend to support (e.g., try Class.forName for H2 and
SQLite driver class names) or, if only MySQL is supported, change the
Hommr.getInstance().getSLF4JLogger().error text to reference only the MySQL
driver; adjust the Class.forName block accordingly so the logger message matches
the actual driver-loading behavior.
- Around line 40-45: DatabaseManager currently passes raw values from
config.getString(...) into new JdbcPooledConnectionSource which causes "url was
never set" when keys are missing; before constructing connectionSource, retrieve
and validate dbUrl, dbUser, and dbPassword (the "database.connection.url",
"database.connection.username", "database.connection.password" keys) ensuring
they are non-null and non-empty, and if any are missing either throw a clear
IllegalStateException or log an error and abort initialization (don't call new
JdbcPooledConnectionSource with nulls); update the try block in DatabaseManager
to perform these checks and handle the failure path instead of creating the
connectionSource with null values.
In `@src/main/java/me/axeno/hommr/menus/HomeListMenu.java`:
- Around line 136-147: The direct call to player.teleport(location) in
HomeListMenu bypasses the HomeManager event flow; replace that direct teleport
with the same call used in HomeCommands.teleportHome(), i.e. invoke
HomeManager.teleportToHome(...) (use the same parameter set used
elsewhere—player and the Home or homeName) so the HomeTeleportEvent is fired and
cancellation is respected; then only send the success message and close the
inventory if teleportToHome indicates success (or not cancelled) per the
HomeCommands.teleportHome() behavior.
🧹 Nitpick comments (2)
src/main/java/me/axeno/hommr/menus/HomeListMenu.java (1)
53-60: Consider i18n for user-facing strings.All UI strings are hardcoded in French (e.g., "Monde:", "Créé le:", "Cliquez pour vous téléporter"). Consider externalizing these to a messages configuration file for internationalization support, consistent with how other plugins handle localization.
src/main/java/me/axeno/hommr/commands/HomeCommands.java (1)
37-53: Remove commented-out code.Dead code should be removed rather than commented out. Version control preserves history if the old implementation is ever needed. Keeping commented code increases maintenance burden and reduces readability.
♻️ Proposed fix
new HomeListMenu(player).open(); - -// msg(player, Component.text("Your Homes", NamedTextColor.GOLD, TextDecoration.BOLD)); -// -// for (String homeName : homes) { -// HomeManager.getHome(player.getUniqueId(), homeName).ifPresent(home -> { -// String worldInfo = home.getWorld(); -// player.sendMessage(Component.text() -// .append(Component.text(" • ", NamedTextColor.DARK_GRAY)) -// .append(Component.text(homeName, NamedTextColor.YELLOW)) -// .append(Component.text(" (" + worldInfo + ")", NamedTextColor.GRAY)) -// .build()); -// }); -// } -// -// player.sendMessage(Component.text() -// .append(Component.text(" Total: ", NamedTextColor.GOLD)) -// .append(Component.text(homes.size() + " home(s)", NamedTextColor.YELLOW)) -// .build()); }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@build.gradle`:
- Line 61: The project erroneously declares
testImplementation("com.h2database:h2:2.2.224") while tests actually use SQLite
(jdbc:sqlite:database.db) and a comment reads "Use H2 in-memory database for
testing"; remove the unused H2 dependency from build.gradle (delete the
testImplementation("com.h2database:h2:2.2.224") line) or alternatively update
the misleading comment to state that tests use SQLite, ensuring the build.gradle
dependency list and the comment ("Use H2 in-memory database for testing")
reflect SQLite usage.
In `@src/main/java/me/axeno/hommr/managers/DatabaseManager.java`:
- Around line 23-29: The unconditional Class.forName("com.mysql.cj.jdbc.Driver")
in DatabaseManager.init causes failures for non-MySQL backends; change it to
only attempt loading the MySQL driver when the JDBC URL indicates MySQL (e.g.,
check dbUrl.startsWith("jdbc:mysql:") or dbUrl.contains("mysql")) before calling
Class.forName, or remove the Class.forName call entirely and rely on the JDBC
driver auto-registration; update the init method accordingly so the
Class.forName call is guarded by a conditional that examines the dbUrl
parameter.
In `@src/test/java/me/axeno/hommr/managers/DatabaseManagerTest.java`:
- Around line 44-47: The test setup comment and DB URL are inconsistent and risk
polluting tests; in setUp() update the comment to reflect SQLite and change the
DatabaseManager.init call (databaseManager.init) to use an in-memory SQLite URL
(e.g., "jdbc:sqlite::memory:") so tests are isolated, or alternatively generate
a unique temp DB URL per test run if file-backed DB is required; ensure the
variable databaseManager and the init(...) invocation are updated accordingly
and the comment reflects the actual DB choice.
In `@src/test/resources/config.yml`:
- Around line 1-10: The test config file declares a MySQL JDBC URL but tests
(DatabaseManagerTest.java) call init("jdbc:sqlite:database.db", "sa", "")
directly and never exercise the no-arg config path; either remove the unused
config file, update its url/credentials to match the SQLite/H2 test DB (e.g.,
jdbc:sqlite:database.db or an H2 in-memory URL) so init() would load a matching
value, or add a unit test that calls the config-based initialization path (the
no-arg init() overload) to validate loading from the YAML; locate references to
DatabaseManagerTest.java and the init(...) overloads to implement the chosen
fix.
🧹 Nitpick comments (2)
src/main/java/me/axeno/hommr/managers/DatabaseManager.java (1)
39-41: Redundant table existence check.
TableUtils.createTableIfNotExists()already checks if the table exists internally. The explicitisTableExists()check is unnecessary.♻️ Simplified version
homeDao = DaoManager.createDao(connectionSource, Home.class); - if (!homeDao.isTableExists()) { - TableUtils.createTableIfNotExists(connectionSource, Home.class); - } + TableUtils.createTableIfNotExists(connectionSource, Home.class);src/test/java/me/axeno/hommr/managers/DatabaseManagerTest.java (1)
67-70: Consider adding tests for error scenarios.The test suite covers happy paths well, but consider adding tests for:
- The no-arg
init()that reads from config- Invalid/empty database URL throwing
IllegalStateExceptionThis would improve confidence in the error handling paths.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/java/me/axeno/hommr/managers/DatabaseManagerTest.java`:
- Around line 44-47: The test uses an in-memory SQLite URI which creates
isolated DBs per pooled connection; update the setUp method to use a file-based
DB created from the JUnit `@TempDir` instead: create a temporary File inside the
`@TempDir` and pass its path into DatabaseManager.init (use "jdbc:sqlite:" +
tempFile.getAbsolutePath()), leaving DatabaseManager and setUp as the locations
to change so pooled connections share the same file-backed database.
Comment on lines
+44
to
+47
| void setUp() throws SQLException { | ||
| // Use SQLite in-memory database for testing | ||
| databaseManager = new DatabaseManager(); | ||
| databaseManager.init("jdbc:sqlite::memory:", "", ""); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the DatabaseManager class
fd "DatabaseManager\.java" --type fRepository: AxenoDev/Hommr
Length of output: 116
🏁 Script executed:
# Check project Java version
fd "pom\.xml|build\.gradle" --type fRepository: AxenoDev/Hommr
Length of output: 70
🏁 Script executed:
# Look at the test file to understand the changes
cat -n src/test/java/me/axeno/hommr/managers/DatabaseManagerTest.javaRepository: AxenoDev/Hommr
Length of output: 5782
🏁 Script executed:
# Check DatabaseManager init method
sed -n '1,100p' src/main/java/me/axeno/hommr/managers/DatabaseManager.javaRepository: AxenoDev/Hommr
Length of output: 3918
🏁 Script executed:
# Check Java version in build.gradle
cat build.gradleRepository: AxenoDev/Hommr
Length of output: 5748
Replace in-memory SQLite with a file-based database to avoid flaky tests with connection pooling.
JdbcPooledConnectionSource creates multiple pooled connections, and SQLite's ::memory: URI creates a new isolated in-memory database per connection. Saves and queries can hit different databases, causing test failures. Use the available @TempDir to create a file-based test database instead.
Suggested fix
void setUp() throws SQLException {
// Use SQLite in-memory database for testing
databaseManager = new DatabaseManager();
- databaseManager.init("jdbc:sqlite::memory:", "", "");
+ Path dbFile = tempDir.resolve("test.db");
+ databaseManager.init("jdbc:sqlite:" + dbFile.toString(), "", "");🤖 Prompt for AI Agents
In `@src/test/java/me/axeno/hommr/managers/DatabaseManagerTest.java` around lines
44 - 47, The test uses an in-memory SQLite URI which creates isolated DBs per
pooled connection; update the setUp method to use a file-based DB created from
the JUnit `@TempDir` instead: create a temporary File inside the `@TempDir` and pass
its path into DatabaseManager.init (use "jdbc:sqlite:" +
tempFile.getAbsolutePath()), leaving DatabaseManager and setUp as the locations
to change so pooled connections share the same file-backed database.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests