Feat: Add an ORM to save homes - #5
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds ORMLite-backed persistence and DB configuration (SQLite/MySQL), a new DatabaseManager, concurrent in-memory player homes cache with load/save lifecycle (init/shutdown), model ORM annotations and signature change for Home, removes PlayerQuit unload listener, and updates build properties for JDBC drivers. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Hommr (Plugin)
participant Manager as HomeManager
participant DB as DatabaseManager
Plugin->>Manager: onEnable -> init()
Manager->>DB: init(): open connection, ensure table
DB-->>Manager: return all homes
Manager->>Manager: populate in-memory cache
Plugin->>Manager: onDisable -> shutdown()
Manager->>DB: saveAllHomes(cached homes)
DB->>DB: transaction: clear table, batch insert
DB-->>Manager: close()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @AxenoDev. * #5 (comment) The following files were modified: * `src/main/java/me/axeno/hommr/Hommr.java` * `src/main/java/me/axeno/hommr/managers/DatabaseManager.java` * `src/main/java/me/axeno/hommr/managers/HomeManager.java` * `src/main/java/me/axeno/hommr/models/Home.java` * `src/main/java/me/axeno/hommr/models/PlayerHomes.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: 6
🤖 Fix all issues with AI agents
In `@src/main/java/me/axeno/hommr/managers/DatabaseManager.java`:
- Around line 56-59: DatabaseManager currently swallows SQLException during
initialization leaving the field homeDao null and risking NPEs later; either
fail fast by re-throwing a runtime exception from the constructor/init block
(wrap the caught SQLException and throw new IllegalStateException("Failed to
initialize database", e)) so the application cannot continue with a null DAO, or
add defensive checks at the start of getAllHomes and saveAllHomes (e.g., if
(homeDao == null) throw new IllegalStateException("Database not initialized"))
to prevent usage when initialization failed — update the catch in the
DatabaseManager init and/or add the null-checks in getAllHomes and saveAllHomes
accordingly.
- Around line 38-46: In DatabaseManager where type.equals("mysql") and you build
databaseUrl/username/password via Hommr.getInstance().getConfig(), validate that
databaseUrl, username and password are non-null/non-empty before constructing
the JdbcConnectionSource; if any are missing, throw a clear
IllegalArgumentException or log an explicit error and abort initialization
(instead of calling new JdbcConnectionSource with nulls). Update the block that
sets databaseUrl/username/password and the creation of connectionSource to
perform these checks and emit a helpful message referencing the missing config
keys so the failure is actionable.
- Around line 75-90: The saveAllHomes method clears the Home table up front
(TableUtils.clearTable) and then performs inserts with homeDao.callBatchTasks,
which can leave the DB empty if interrupted; change the implementation to
perform the clear-and-insert inside a single transaction using
TransactionManager.callInTransaction (import
com.j256.ormlite.misc.TransactionManager) so the clear and all
homeDao.create(...) calls are atomic and will roll back on exception; ensure any
caught exceptions are rethrown as SQLException to preserve the current API.
In `@src/main/java/me/axeno/hommr/managers/HomeManager.java`:
- Around line 44-58: Current shutdown-only persistence risks data loss; update
HomeManager to persist changes incrementally by calling
databaseManager.saveHome(home) inside setHome(...) after updating the
PlayerHomes instance and call databaseManager.deleteHome(playerId, homeName)
inside deleteHome(...), handling SQLException with
Hommr.getInstance().getLogger().log(...). To avoid blocking server threads,
perform these DB calls asynchronously (use an ExecutorService or scheduler) or
add a periodic autosave task that calls databaseManager.saveAllHomes(new
ArrayList<>(collect homes from playerHomesCache.values())), and ensure
databaseManager is thread-safe and closed on shutdown in shutdown().
- Around line 24-42: HomeManager.init() assumes databaseManager is usable but
shutdown() leaves a closed instance, causing databaseManager.getAllHomes() to
fail; fix by ensuring shutdown() nullifies or replaces the closed
DatabaseManager: in HomeManager.shutdown() call the
DatabaseManager.close()/shutdown routine and set HomeManager.databaseManager =
null (or provide a DatabaseManager.isClosed/isConnected check), and in
HomeManager.init() re-create databaseManager when it is null or closed before
calling databaseManager.getAllHomes(); reference symbols: HomeManager.init(),
HomeManager.shutdown(), databaseManager, DatabaseManager,
databaseManager.getAllHomes().
In `@src/main/resources/config.yml`:
- Around line 14-16: The config.yml currently ships with real-looking default DB
credentials (url/username/password) which is insecure; update the username and
password fields in config.yml (the username and password keys) to non-credential
placeholders or empty strings (e.g., username: your_username and password:
your_password or username: "" and password: "") and add a short comment or
README note instructing users to populate them with real credentials before
deployment.
🧹 Nitpick comments (2)
src/main/java/me/axeno/hommr/models/Home.java (1)
23-27: Consider adding a unique constraint on (owner, name) to prevent duplicate home names per player.Currently, the database schema doesn't prevent multiple homes with the same name for the same owner. While the in-memory
PlayerHomesmap would overwrite duplicates, if data is loaded from a corrupted DB state, this could cause issues.♻️ Suggested approach using ORMLite's uniqueCombo
- `@DatabaseField`(canBeNull = false, index = true) + `@DatabaseField`(canBeNull = false, index = true, uniqueCombo = true) private UUID owner; - `@DatabaseField`(canBeNull = false) + `@DatabaseField`(canBeNull = false, uniqueCombo = true) private String name;src/main/java/me/axeno/hommr/managers/DatabaseManager.java (1)
71-73: Consider adding a method to save/update individual homes rather than bulk-only operations.Currently, there's only
saveAllHomeswhich requires clearing and re-inserting all data. For efficiency and safety, consider adding methods likesaveHome(Home)anddeleteHome(Home)for incremental updates.♻️ Suggested additional methods
public void saveHome(Home home) throws SQLException { homeDao.createOrUpdate(home); } public void deleteHome(Home home) throws SQLException { homeDao.delete(home); }
📝 Add docstrings to `feat/add-orm`
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/me/axeno/hommr/models/Home.java (1)
58-71:⚠️ Potential issue | 🟡 MinorPotential NullPointerException if location or world is null.
location.getWorld()can returnnullfor unloaded worlds, and.getName()would then throw NPE. Consider adding null checks.🛡️ Proposed defensive check
public static Home fromLocation(UUID owner, String name, Location location) { + if (location == null || location.getWorld() == null) { + throw new IllegalArgumentException("Location and its world must not be null"); + } return new Home( 0, owner,
🤖 Fix all issues with AI agents
In `@src/main/java/me/axeno/hommr/managers/HomeManager.java`:
- Around line 76-78: playerHomesCache can be null if init() hasn't run, causing
getOrCreatePlayerHomes(UUID) to NPE; ensure playerHomesCache is initialized
inline (e.g., new ConcurrentHashMap<>) or add a null-guard in
getOrCreatePlayerHomes that initializes it before use, and adjust init() to
clear() rather than reassign the map; locate the playerHomesCache field and the
getOrCreatePlayerHomes and init() methods to implement the inline initialization
or lazy-init guard.
🧹 Nitpick comments (1)
src/main/java/me/axeno/hommr/Hommr.java (1)
38-38: Consider handling initialization failure.If
HomeManager.init()fails to connect to the database, the plugin continues with an empty cache. Depending on requirements, you may want to disable the plugin on critical initialization failure.💡 Example: Fail-fast on init error
try { HomeManager.init(); } catch (Exception e) { getLogger().severe("Failed to initialize HomeManager: " + e.getMessage()); getServer().getPluginManager().disablePlugin(this); return; }This would require
HomeManager.init()to propagate exceptions rather than catching them internally.
Merged
Contributor
|
✅ Created PR with unit tests: #7 |
…oved error handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/me/axeno/hommr/managers/DatabaseManager.java`:
- Around line 63-67: The TableUtils.createTableIfNotExists call inside
DatabaseManager currently swallows SQLException which leaves homeDao initialized
but the table missing; modify the catch to rethrow the exception (or wrap it in
a RuntimeException) so the outer try/catch in the DatabaseManager constructor
can handle it and abort initialization. Locate the block around
TableUtils.createTableIfNotExists(connectionSource, Home.class) and change the
SQLException handling to throw the caught exception (or new
RuntimeException("Failed to create Home table", e)) instead of only logging;
this prevents subsequent calls like getAllHomes() or saveAllHomes() from running
against an inconsistent state.
🧹 Nitpick comments (1)
src/main/java/me/axeno/hommr/managers/DatabaseManager.java (1)
97-99: Add null guard forhomeDaoin data access methods.If
getAllHomes()is called after a failed initialization or afterclose(),homeDaocould be null, causing an NPE. Consider adding a defensive check.🛡️ Suggested fix
public List<Home> getAllHomes() throws SQLException { + if (homeDao == null) { + throw new IllegalStateException("Database not initialized"); + } return homeDao.queryForAll(); }
…ant error handling
Merged
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.
#1
Summary by CodeRabbit
New Features
Configuration
Changes