GH-78 Rewrite database to ORMLite JDBC #116
Conversation
…y database classes
# Conflicts: # build.gradle.kts
Jakubk15
added
🆕 feature
|
Warning Rate limit exceeded@Jakubk15 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces a comprehensive refactoring of the database layer in the ParcelLockers plugin, transitioning from a custom database implementation to an ORM Lite (Object-Relational Mapping Lite) approach. This change involves replacing existing repository implementations with ORM Lite versions, introducing new wrapper classes for database entities, and updating the database management strategy. The modifications support multiple database types, including H2, PostgreSQL, and MySQL, with improved configuration and connection handling. Changes
Sequence DiagramsequenceDiagram
participant Plugin as ParcelLockers
participant DatabaseManager
participant ORMLite as ORM Lite Repository
participant Database
Plugin->>DatabaseManager: connect()
DatabaseManager->>Database: Establish Connection
DatabaseManager-->>Plugin: Connection Established
Plugin->>ORMLite: save(entity)
ORMLite->>Database: Insert/Update
Database-->>ORMLite: Confirmation
ORMLite-->>Plugin: Operation Complete
Plugin->>DatabaseManager: disconnect()
DatabaseManager->>Database: Close Connection
DatabaseManager-->>Plugin: Connection Closed
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/main/java/com/eternalcode/parcellockers/user/UserManager.java (1)
Line range hint
51-63: Potential Concurrency Issues increate(UUID uuid, String name)While the
createmethod works for a single-threaded approach, concurrent calls can pose these risks:
- Race Conditions: Two threads creating the same user simultaneously might slip past the
containsKeychecks.- Repository Consistency: A user might fail to be saved if an exception occurs in the repository, potentially leaving the in-memory map out of sync.
To address these concerns, consider atomic checks or synchronization around map updates, or rely on your repository if it can guarantee uniqueness at the database level.
🧹 Nitpick comments (33)
src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java (1)
22-22: Document the return value meaning
Changing the return type fromCompletableFuture<Void>toCompletableFuture<Integer>is beneficial, as it allows for tracking how many records were removed. However, please add or update documentation to clarify what the returned integer represents (e.g., the count of removed rows or an indicator of success/failure).src/main/java/com/eternalcode/parcellockers/user/repository/UserWrapper.java (4)
9-10: Consider class visibility for broader usage.
By default, this class is package-private. If future features or modules outside this package will need direct access toUserWrapper, consider using thepublicmodifier.
15-16: Specify additional column constraints for robustness.
If there is a maximum length or uniqueness requirement forusername, consider leveraging ORMLite’s@DatabaseFieldattributes (e.g.,width,unique) to ensure better data consistency and constraints.
26-28: Validate non-null argument to avoid potentialNullPointerException.
Ifuseris ever null,user.uuid()anduser.name()would raise an exception. Consider adding a null check or clarifying in the method contract thatusermust not be null.
30-32: Check consistency of exposure vs. encapsulation.
setUsername(...)is declared public while the class itself is package-private. If external consumers need to updateusername, ensure that the class is likewise accessible; otherwise, consider limiting the visibility of this setter.src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java (2)
22-28: Consider avoiding double-logging exceptions.Here, the code captures exceptions with Sentry and also prints the stack trace. This can lead to duplicate logs. Typically, either logging to Sentry with supplemental log statements or printing stack traces individually should suffice.
try { TableUtils.createTableIfNotExists(databaseManager.connectionSource(), UserWrapper.class); } catch (SQLException exception) { Sentry.captureException(exception); - exception.printStackTrace(); }
58-74: Revisit the pagination logic when checkinghasNext.Currently, the query limits results to
page.getLimit(), so you do not fetch an extra record to determine if more data exists. If you'd like to confirm there is more data, you might need to query forlimit + 1rows and then remove the last row if it exists.List<User> users = dao.queryBuilder() - .offset((long) page.getOffset()) - .limit((long) page.getLimit()) + .offset((long) page.getOffset()) + .limit((long) (page.getLimit() + 1)) .query() .stream() .map(UserWrapper::toUser) .collect(Collectors.toList()); boolean hasNext = users.size() > page.getLimit(); if (hasNext) { users.remove(users.size() - 1); }src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java (3)
17-19: Use more descriptive parameter names for clarity
In methods likesave(Class<T> type, T warp), the parameter namewarpcan be confusing. Consider renaming this parameter to something more descriptive, such asentity.
53-55: Provide a custom Executor or clarify concurrency requirements
CompletableFuture.runAsync(...)uses the common ForkJoinPool by default, which may be undesirable in some scenarios (e.g., heavy I/O operations or concurrency constraints). Consider providing a custom Executor if you need stricter control over threading or concurrency.
62-66: Avoid printing stack traces after sending them to Sentry
You are already capturing exceptions with Sentry. Printing stack traces manually might create redundant logs. Instead, consider using a logger at a suitable log level if needed for local debugging.src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java (2)
30-36: Consider removing raw exception printing
You capture exceptions with Sentry but also callex.printStackTrace(). You may want to switch to a standard logger to ensure consistent logging across the application.
38-42: Check for cache and database consistency
Whensave(Parcel parcel)completes asynchronously, the parcel is added to the cache immediately but the database operation is still pending. In highly concurrent scenarios, this might momentarily create inconsistencies. Consider documenting or synchronizing the cache update if consistency is critical.src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java (1)
118-124: Handle potential concurrency race conditions in updateCaches()
The method clears and repopulates caches asynchronously. If other threads are reading or writing at the same time, keys may vanish or reappear in unexpected ways. Add explicit synchronization or use thread-safe structures to ensure data consistency.src/main/java/com/eternalcode/parcellockers/ParcelLockers.java (5)
112-113: ORMLite log set to NullLogBackend.
Double-check if you want all logs suppressed. Debugging database operations may be harder without logs.
114-123: Instantiation and connection ofDatabaseManager.
While this works, consider instantiating directly into the field and connecting within the try block to avoid potential scoping confusion. Also consider adding more descriptive error handling or fallback logic in case of failures.
134-135:ItemStorageRepositoryOrmLitecreation.
Implementation looks straightforward. The blank line may be removed for cleanliness.
186-189: Gracefully disconnecting from the database.
Consider wrappingdisconnect()in a try-catch block to handle potential close errors or log them.
208-208: MC version support warning updated.
You might want to mention your newly supported range in documentation for clarity.src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageWrapper.java (1)
29-35: Enforce immutability of domain objects if permissible.
Currently,List<ItemStack> itemsis mutable and returned directly viatoItemStorage(). If the data should be immutable after loading from DB, consider returning an unmodifiable list.src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentWrapper.java (3)
12-13: Check table naming consistency.
Ensure that"parcel_content"aligns with naming conventions used across the project. In some teams, table names are all plural or all singular. This keeps your schema consistent if you have multiple domain objects.
15-19: Consider data growth strategy.
Similar toItemStorageWrapper, this class stores a list ofItemStackobjects in a single column. Evaluate your expected row sizes—if you need advanced queries on these items, normalizing or splitting them across multiple records may be more scalable.
29-35: Avoid passing mutable data.
When converting toParcelContent, consider returning an unmodifiable or defensive copy of the items list to prevent in-place modifications that might drift from the state saved in the database.src/main/java/com/eternalcode/parcellockers/locker/repository/LockerWrapper.java (1)
11-12: Use consistent naming for table columns.
@DatabaseTable(tableName = "lockers")is clear, but verify that other similar domain objects use either “plural” or “singular” naming for table names.src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java (1)
19-25: Improve exception logging.
In addition to capturing the exception in Sentry, consider using a logger instead ofex.printStackTrace()for consistent log formatting and better integration with logging tools.} catch (SQLException ex) { Sentry.captureException(ex); - ex.printStackTrace(); + Logger logger = LoggerFactory.getLogger(ItemStorageRepositoryOrmLite.class); + logger.error("Failed to create table", ex); }src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java (1)
28-31: Clarify asynchronous return value
When returning aCompletableFuture<Void>, it might be useful to capture and handle possible exceptions. Consider chaining a.exceptionally(...)to log or manage failures, particularly for critical database write operations.src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelWrapper.java (2)
56-58: Validate null arguments in static factory method
When mapping fromParceltoParcelWrapper, consider whether any fields can benulland handle them gracefully to prevent potentialNullPointerException.
60-62: Review single-element Set usage
ThetoParcel()method creates aSetwith only the sender. If multi-sender support is planned, consider adjusting or adding an appropriate collection. Otherwise, document this design choice for clarity.src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java (3)
17-17: Importing File for local data storage
Usingjava.io.Fileis standard, but ensure the hard-coded paths in this test file do not conflict with other test environments or CI servers.
20-20: Java Logger usage
Utilizingjava.util.logging.Loggeris acceptable. However, if your project uses SLF4J or another logging framework in production, consider aligning the test with the same logger approach for consistency.
33-33: Hard-coded data folder path
Storing data under"run/plugins/ParcelLockers"works locally but could be less flexible in CI or other environments. Consider using a parameterized or temporary folder for improved test portability.- File dataFolder = new File("run/plugins/ParcelLockers"); + File dataFolder = Files.createTempDirectory("ParcelLockersTest").toFile();src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java (3)
47-49: Expose pool size and timeouts to config
Hardcoding HikariCP parameters can limit flexibility. Consider making these properties configurable inPluginConfiguration.this.dataSource.setMaximumPoolSize(5); this.dataSource.setConnectionTimeout(5000); this.dataSource.setLeakDetectionThreshold(5000); + // e.g., this.dataSource.setMaximumPoolSize(config.settings.poolSize); + // this.dataSource.setConnectionTimeout(config.settings.connectionTimeout); + // this.dataSource.setLeakDetectionThreshold(config.settings.leakDetectionThreshold);
65-68: Potential cross-platform path issue
For H2,jdbc:h2:./plusFilemight cause path separators issues on some systems. Consider usingdataFolder.getAbsolutePath()or a platform-agnostic approach.
99-114: Concurrent access to cached DAO
Using aConcurrentHashMapis good, but creating a DAO in a race could produce redundant entries. Prevent this withcomputeIfAbsentor similar.Dao<?, ?> dao = this.cachedDao.computeIfAbsent(type, key -> { try { return DaoManager.createDao(this.connectionSource, key); } catch (SQLException exception) { throw new RuntimeException(exception); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
TODO.md(1 hunks)build.gradle.kts(3 hunks)src/main/java/com/eternalcode/parcellockers/ParcelLockers.java(7 hunks)src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepository.java(1 hunks)src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryImpl.java(0 hunks)src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentWrapper.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/AbstractDatabaseService.java(0 hunks)src/main/java/com/eternalcode/parcellockers/database/DataSourceFactory.java(0 hunks)src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/DatabaseType.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/persister/PositionPersister.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerMainGUI.java(1 hunks)src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ParcelItemStorageGUI.java(1 hunks)src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ParcelSendingGUI.java(1 hunks)src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ReceiverSelectionGui.java(1 hunks)src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepository.java(1 hunks)src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryImpl.java(0 hunks)src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageWrapper.java(1 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepository.java(1 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryImpl.java(0 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerWrapper.java(1 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java(1 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryImpl.java(0 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelWrapper.java(1 hunks)src/main/java/com/eternalcode/parcellockers/shared/Position.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/UserManager.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/UserPageResult.java(0 hunks)src/main/java/com/eternalcode/parcellockers/user/UserRepositoryImpl.java(0 hunks)src/main/java/com/eternalcode/parcellockers/user/repository/UserPageResult.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/repository/UserRepository.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/repository/UserWrapper.java(1 hunks)src/test/java/com/eternalcode/parcellockers/database/ParcelDatabaseServiceIntegrationTest.java(2 hunks)src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java(2 hunks)src/test/java/com/eternalcode/parcellockers/database/ParcelLockerIntegrationSpec.java(0 hunks)
💤 Files with no reviewable changes (9)
- src/main/java/com/eternalcode/parcellockers/user/UserPageResult.java
- src/test/java/com/eternalcode/parcellockers/database/ParcelLockerIntegrationSpec.java
- src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryImpl.java
- src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryImpl.java
- src/main/java/com/eternalcode/parcellockers/database/AbstractDatabaseService.java
- src/main/java/com/eternalcode/parcellockers/database/DataSourceFactory.java
- src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryImpl.java
- src/main/java/com/eternalcode/parcellockers/user/UserRepositoryImpl.java
- src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryImpl.java
✅ Files skipped from review due to trivial changes (7)
- TODO.md
- src/main/java/com/eternalcode/parcellockers/user/repository/UserPageResult.java
- src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ReceiverSelectionGui.java
- src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/LockerMainGUI.java
- src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ParcelItemStorageGUI.java
- src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ParcelSendingGUI.java
- src/main/java/com/eternalcode/parcellockers/user/repository/UserRepository.java
🔇 Additional comments (50)
src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java (1)
24-24: Consistent handling of remove by UUID
Likewise, returning an integer here adds consistency. Verify that any consumer code handling partial or missing records is updated to interpret the integer's value correctly.
src/main/java/com/eternalcode/parcellockers/user/repository/UserWrapper.java (1)
12-13: Verify database column type compatibility.
Storing a Java UUID in the database typically requires either a VARCHAR(36) or similar column type. Confirm that ORMLite is configured to handle the UUID field correctly.
src/main/java/com/eternalcode/parcellockers/user/UserManager.java (3)
3-4: Importing UserRepository Looks Good
Bringing in the UserRepository from com.eternalcode.parcellockers.user.repository fits well with the new ORMLite-based design. This import is necessary for integrating the database interactions within the UserManager.
Line range hint 16-26: Asynchronous getUser(UUID uuid) Implementation
Returning a CompletableFuture<Optional<User>> is a solid choice for non-blocking I/O. This allows calls to the repository to be processed asynchronously, which can yield better performance. However, please verify:
- Thread-safety: Ensure that the
usersByUUIDmap is only modified by safe operations or from a single thread, otherwise consider using concurrent structures or external synchronization. - Consistent Cache Usage: Confirm whether the cache (i.e.,
usersByUUID) remains up-to-date whenever remote changes (e.g., name updates) occur in the repository.
Line range hint 28-38: Asynchronous getUser(String name) Method
Following a similar pattern to getUser(UUID), this asynchronous retrieval from the repository is clear. Keep an eye out for:
- Case Sensitivity: If user names ever vary in casing, consider normalizing or clarifying that names are case-sensitive.
- Performance: Double-check whether name-to-user lookups are frequent. If so, ensure that the
usersByNamemap remains current to avoid stale or missing entries.
src/main/java/com/eternalcode/parcellockers/database/persister/ItemStackPersister.java (2)
38-40: Gracefully handle null inputs
Returning null from javaToSqlArg() is correct, but be mindful of any further processing in the call chain where a null might cause NullPointerException. Verify that all callers handle null results properly.
72-77: Consistent exception handling
Capturing exceptions with Sentry and throwing ParcelLockersException is a good approach. Ensure that any upstream callers handle or log the exception in a user-friendly manner.
src/test/java/com/eternalcode/parcellockers/database/ParcelDatabaseServiceIntegrationTest.java (1)
36-39: Confirm test container initialization and resource cleanup
The new approach loads configuration from disk and connects to the MySQL test container. Ensure that resource cleanup is properly handled and that leftover files in run/plugins/ParcelLockers do not interfere with subsequent tests.
src/main/java/com/eternalcode/parcellockers/ParcelLockers.java (17)
5-6: Imports for scheduler functionality look good.
The newly introduced scheduler imports should help streamline asynchronous tasks. Ensure you properly handle concurrency and shutdown worker threads when the plugin is disabled.
12-12: Import for ParcelContentRepositoryOrmLite.
This aligns with the migration to the new ORMLite-based repository. Looks consistent with the PR objectives.
13-13: Import for DatabaseManager.
Matches the new refactored approach for database handling. No issues here.
18-18: Import for ItemStorageRepositoryOrmLite.
Consistent with the switch to ORMLite. Good to have a unified repository approach.
23-23: Import for LockerRepositoryOrmLite.
Migrating to ORM-based repository helps with cleaner database interactions.
31-31: Import for ParcelRepositoryOrmLite.
Complements the other repository migrations. No concerns here.
36-37: Imports for UserRepository and UserRepositoryOrmLite.
Indicates a shift to an ORM-based user repository. Looks good.
39-40: Imports for ORMLite logger classes.
Setting a null log backend might suppress important ORMLite logs. Consider using a minimal logger instead, unless silence is your intent.
60-60: Import for SQLException.
Necessary for robust error handling with the new database logic.
78-79: Field databaseManager introduced.
The field name clearly expresses its role. This refactor centralizes DB management.
96-96: Initializing the Scheduler implementation.
Please confirm that this scheduler instance is utilized later in the code. If unused, consider removing or storing it for task scheduling.
107-107: Disabling Sentry (options.setEnabled(false)).
Revisit whether disabling Sentry here conflicts with config.settings.enableSentry. This might override user configuration.
131-131: LockerRepositoryOrmLite usage.
Immediately updating caches is good for performance, but verify that it does not introduce heavy load on large datasets. Lazy-loading strategies or partial caching might be beneficial if the dataset grows large.
136-136: ParcelRepositoryOrmLite creation.
Consistent with the ORMLite approach. No issues.
140-140: UserRepositoryOrmLite assignment.
Smooth integration with UserManager. Looks well-structured.
143-143: ParcelContentRepositoryOrmLite usage.
Good alignment with the new ORM architecture.
212-212: Conditional check for version 17 or 21.
The OR condition might exclude versions between 18–20. If you intend multi-version support, verify that your condition is correct.
src/main/java/com/eternalcode/parcellockers/database/DatabaseType.java (1)
5-8: Newly added database types.
Supporting H2, POSTGRESQL, and MARIADB expands compatibility. Note that MariaDB can often be treated similarly to MySQL, so ensure your code handles their small differences if any.
src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepository.java (1)
15-15: Changed remove method return type to CompletableFuture<Integer>.
Returning the number of removed items (instead of void) makes the API more expressive. Great improvement.
src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepository.java (1)
13-13: Updated remove to return an integer.
This adds clarity about how many records were removed, aligning with other repositories.
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepository.java (3)
23-23: remove(UUID) now returns CompletableFuture<Integer>.
Returning the count is good for validation or logging.
25-25: remove(Locker) similarly updated.
Consistent approach for remove. This helps unify repository expectations.
29-29: Renamed findLocker(uuid) to getFromCache(uuid).
The name emphasizes cache usage, which is more descriptive. Ensure there's a fallback if the cache is stale or empty.
src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageWrapper.java (2)
18-19: Verify the persister for a list of ItemStacks.
Storing a collection of complex objects such as ItemStack in a single table column can be fragile—especially when item data can be large or frequently updated. Ensure that ItemStackPersister thoroughly handles edge cases (e.g., corrupted data, partial updates).
12-13: Consider explicit access modifier.
Currently, the class is package-private. If other packages need to utilize ItemStorageWrapper, consider marking the class public, or providing factory methods/services within the same package to manage visibility.
Would you like me to search for references to ItemStorageWrapper outside this package to confirm whether it should be public?
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerWrapper.java (2)
14-21: Check data constraints for fields.
The description field currently has no length or nullability constraints. If it can be very large or null, ensure the schema and code handle that properly.
32-38: Ensure domain-object integrity.
Locker is reconstructed directly from LockerWrapper. Confirm that no additional data validations are needed (e.g., checking for null uuid or invalid position).
[approve]
src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java (2)
28-30: Clarify the handling of DAO in save method.
The completion stage discards the DAO reference (thenApply(dao -> null)). Confirm that ignoring the DAO is intended, or if an additional operation should be performed with the DAO result.
38-39: Confirm correct row count for remove operation.
Returning an Integer row count is valuable, but ensure in tests or at call sites that removing a nonexistent entry returning 0 does not cause confusion.
src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java (2)
19-25: Consider handling table creation exceptions more gracefully
Catching and printing stack traces is helpful for debugging, but for production systems, consider more robust exception handling (e.g., re-throwing a custom runtime exception, or ensuring logs are stored with proper context).
44-45: Check for potential null in parcelContentWrapper
When mapping via Optional.ofNullable(...), ensure parcelContentWrapper itself is not null. Though the code is currently safe, consider a null check or a default value to avoid subtle runtime errors if select(...) returns null unexpectedly.
src/main/java/com/eternalcode/parcellockers/database/persister/PositionPersister.java (1)
59-67: Validate format when splitting the deserialized string
Splitting on '/' without robust validation can lead to ArrayIndexOutOfBoundsException if the stored string is malformed. Consider verifying that params.length == 4 before parsing.
src/main/java/com/eternalcode/parcellockers/shared/Position.java (1)
58-58: Ensure world is not null before checking
The updated logic checks if this.world is empty, but does not handle null. If world might be null, consider a null-safe check or fallback to an empty string before comparison.
src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java (5)
3-4: New imports for Configuration Manager and Plugin Configuration
These imports appear correct and are used consistently below. Good job ensuring the configuration and manager are introduced properly in this test.
8-8: Switching to ORM Lite repository
Importing the LockerRepositoryOrmLite is in line with the PR’s migration to ORM Lite. This is well-aligned with the new database approach.
34-34: Configuration loading
It’s good that the configuration is loaded in a straightforward manner. Ensure that you have test coverage to validate behavior when the config file is missing or malformed.
35-35: DatabaseManager instantiation
This follows the new design nicely. Make sure to verify the database manager is properly closed or disconnected at the end of the test to avoid resource leaks.
37-37: Repository assignment
Assigning the repository to the new ORM Lite class is consistent with the PR’s objectives. The usage in subsequent lines appears cohesive with the new database logic.
build.gradle.kts (4)
30-30: Updated API version
Switching to 1.21.4-R0.1-SNAPSHOT aligns with the new Minecraft version target. Ensure you’ve tested compatibility thoroughly.
60-62: Adding ORM Lite, H2, and PostgreSQL dependencies
Great job adding essential dependencies for the new database architecture. Verify that the correct versions are shaded or included in the plugin jar if your production environment needs them packaged.
133-133: Bumping the run server version
Ensure the updated test environment (1.21.4) is stable in your CI or local environment.
141-141: New jar naming convention
Renaming your output for broader version compatibility is clear and helpful for end users.
src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public Object javaToSqlArg(FieldType fieldType, Object javaObject) { | ||
| if (javaObject == null) { | ||
| return null; | ||
| } | ||
|
|
||
| Position pos = (Position) javaObject; | ||
| String worldName = "world"; | ||
|
|
||
| if (!pos.isNoneWorld()) { | ||
| worldName = pos.world(); | ||
| } | ||
|
|
||
| return worldName + "/" + pos.x() + "/" + pos.y() + "/" + pos.z(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle edge-cases in string serialization
If the world name contains '/', this logic could create ambiguous serialized strings. Consider escaping special characters or using a more robust serialization format (e.g., JSON) to avoid problems.
There was a problem hiding this comment.
Can you write example how do I save this using JSON?
There was a problem hiding this comment.
@Jakubk15 Here's a simple example of how you could switch to JSON-based serialization within a custom persister:
package com.eternalcode.parcellockers.database.persister;
import com.eternalcode.parcellockers.shared.Position;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.j256.ormlite.field.FieldType;
import com.j256.ormlite.field.SqlType;
import com.j256.ormlite.field.types.BaseDataType;
import com.j256.ormlite.support.DatabaseResults;
import io.sentry.Sentry;
import java.io.IOException;
import java.sql.SQLException;
public class PositionJsonPersister extends BaseDataType {
private static final PositionJsonPersister INSTANCE = new PositionJsonPersister();
private static final ObjectMapper MAPPER = new ObjectMapper();
private PositionJsonPersister() {
super(SqlType.LONG_STRING, new Class<?>[] { PositionJsonPersister.class });
}
public static PositionJsonPersister getSingleton() {
return INSTANCE;
}
@Override
public Object javaToSqlArg(FieldType fieldType, Object javaObject) {
if (javaObject == null) {
return null;
}
Position position = (Position) javaObject;
try {
// Convert the Position to JSON
return MAPPER.writeValueAsString(position);
} catch (JsonProcessingException e) {
// Log the error if desired
Sentry.captureException(e);
throw new RuntimeException("Failed to serialize Position to JSON", e);
}
}
@Override
public Object sqlArgToJava(FieldType fieldType, Object sqlArg, int columnPos) throws SQLException {
if (sqlArg == null) {
return null;
}
String json = (String) sqlArg;
try {
// Convert the JSON back to the Position object
return MAPPER.readValue(json, Position.class);
} catch (IOException e) {
Sentry.captureException(e);
throw new RuntimeException("Failed to deserialize Position from JSON", e);
}
}
// If you parse default strings, override parseDefaultString as needed:
// @Override
// public Object parseDefaultString(FieldType fieldType, String defaultStr) {
// return defaultStr; // or appropriate handling for defaults
// }
}Key points in this approach:
- Using Jackson: An
ObjectMapperhandles serialization (javaToSqlArg) and deserialization (sqlArgToJava). - Error handling: If an issue arises (like malformed JSON), capture the exception (e.g., with Sentry) and throw a runtime exception.
- Escaping special characters: JSON inherently handles tricky characters like
'/'for you, so you don’t need to manually escape them.
This way, if the world field (or any other field in Position) contains special characters, the JSON will correctly store and parse those values.
src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java
Outdated
Show resolved
Hide resolved
This *partially* reverts commit 953942a
…anager.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java (2)
Line range hint
38-63: Consider splitting test scenarios and adding error casesWhile the test coverage is good, consider breaking down the single test method into multiple focused test cases and adding error scenarios.
Example structure:
@Test void shouldSaveAndRetrieveLockerByUUID() { // Test UUID-based operations } @Test void shouldFindLockerByPosition() { // Test position-based lookup } @Test void shouldPaginateLockers() { // Test pagination } @Test void shouldHandleNonExistentLocker() { // Test error case UUID nonExistent = UUID.randomUUID(); Optional<Locker> result = await(parcelLockerRepository.findByUUID(nonExistent)); assertTrue(result.isEmpty()); } @Test void shouldHandleInvalidPosition() { // Test error case Position invalidPosition = new Position(-1, -1, -1, "invalid"); Optional<Locker> result = await(parcelLockerRepository.findByPosition(invalidPosition)); assertTrue(result.isEmpty()); }
Line range hint
29-30: Pin MySQL container version for test stabilityUsing
mysql:latestcould lead to inconsistent test behavior across different environments.- private static final MySQLContainer mySQLContainer = new MySQLContainer(DockerImageName.parse("mysql:latest")); + private static final MySQLContainer mySQLContainer = new MySQLContainer(DockerImageName.parse("mysql:8.0.28"));src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java (2)
22-26: Add documentation for the caching strategy.Consider adding class-level Javadoc to document:
- The purpose and behavior of both caches
- Cache invalidation/update strategy
- Thread-safety guarantees
+/** + * Thread-safe repository implementation for Locker entities using ORMLite. + * Maintains two caches: + * - UUID to Locker mapping for direct entity lookup + * - Position to UUID mapping for spatial queries + * + * Caches are updated atomically during save/remove operations and can be + * manually refreshed using {@link #updateCaches()}. + */ public class LockerRepositoryOrmLite extends AbstractRepositoryOrmLite implements LockerRepository {
75-92: Consider optimizing pagination strategy.The current offset-based pagination implementation might become inefficient for large datasets. Consider:
- Using cursor-based pagination (e.g., using UUID as cursor)
- Adding an index on relevant columns to improve query performance
Also, document the limit+1 pattern used to determine if there are more pages.
@Override + /** + * Fetches a page of lockers using offset-based pagination. + * Note: Internally fetches limit+1 records to determine if there are more pages. + * + * @param page The pagination parameters + * @return LockerPageResult containing the page of lockers and hasNext flag + */ public CompletableFuture<LockerPageResult> findPage(Page page) {src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java (1)
31-36: Consider a more robust exception handling strategy.When table creation fails, the application may be launched without a properly initialized database. Consider throwing a custom exception or gracefully halting initialization steps to avoid unpredictable states after a critical error.
src/test/java/com/eternalcode/parcellockers/TestScheduler.java (6)
15-16: Consider configurable thread pool size
Using a static thread pool of size 8 may be adequate for some test scenarios, but it could become a bottleneck in certain cases or consume unnecessary resources. Providing a configurable approach (via constructor or external config) can make the test environment more flexible across various testing scenarios.
24-28: Clarify semantic difference between run and runAsync
While this method usesCompletableFuture.runAsync, it effectively does the same job asrunin a background thread. Make sure your test environment (and the rest of the code) treats these methods differently if needed (e.g., determining synchronous vs. asynchronous runs in test logic).
30-34: Consider user-cancelable or configurable delays
runLateralways schedules a fixed delay. If tests need to cancel or modify the delay after scheduling, additional control might be useful. Consider adding logic or making the schedule adjustable for advanced testing scenarios.
42-46: Ensure consistent behavior for repeating tasks
The timer method correctly usesscheduleAtFixedRate(...), but keep in mind that if a task execution time exceeds the period, subsequent executions might behave differently (e.g., tasks may run back-to-back). Make sure tests accommodate or verify this behavior.
48-52: Avoid nested async calls for repeating tasks
Similar torunLaterAsync, this approach introduces an additional layer ofCompletableFuturewithin the scheduled task. If the test logic doesn't strictly require it, simplifying the asynchronous portion may be beneficial for clarity and maintainability.
59-63: Duplicate functionality across complete(...) methods
complete(...)andcompleteAsync(...)currently forward to the sameCompletableFuture.supplyAsync(...)call. If the test environment requires a distinct distinction, consider implementing different logic or removing redundancy.src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java (2)
24-30: Use more descriptive domain naming instead of "warp".
“warp” is ambiguous (e.g.,save(Class<T> type, T entity)). This minor revision improves code clarity.-protected <T> CompletableFuture<Dao.CreateOrUpdateStatus> save(Class<T> type, T warp) { +protected <T> CompletableFuture<Dao.CreateOrUpdateStatus> save(Class<T> type, T entity) { return this.action(type, dao -> dao.createOrUpdate(entity)); }
56-73: Use a logger instead ofthrowable.printStackTrace().
Because Sentry already captures the exception, printing the stack trace is often redundant. Adopt a logging framework for better control over error output.catch (Throwable throwable) { Sentry.captureException(throwable); - completableFuture.completeExceptionally(throwable); - throwable.printStackTrace(); + LoggerFactory.getLogger(AbstractRepositoryOrmLite.class).error("An error occurred", throwable); + completableFuture.completeExceptionally(throwable); }src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java (3)
20-26: Avoid creating or modifying schema within the constructor.
Creating tables in the constructor may cause issues if multiple repository instances are instantiated or if the schema needs changing at runtime. Consider placing this logic in application startup code or a dedicated migration step.
30-30: Use a more explicit completion method.
You are discarding the return value by doingthenApply(dao -> null). Consider usingthenAcceptorthenRunfor clarity, which better signals that the return value is not needed.-return this.save(ItemStorageWrapper.class, ItemStorageWrapper.from(itemStorage.owner(), itemStorage.items())) - .thenApply(dao -> null); +return this.save(ItemStorageWrapper.class, ItemStorageWrapper.from(itemStorage.owner(), itemStorage.items())) + .thenRun(() -> {});
39-40: Consider consistent naming for removal.
The method is namedremove, but it relies ondeleteById(...). You could rename it todeletefor better alignment with the parent’s naming conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
TODO.md(1 hunks)src/main/java/com/eternalcode/parcellockers/ParcelLockers.java(7 hunks)src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java(1 hunks)src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java(1 hunks)src/test/java/com/eternalcode/parcellockers/TestScheduler.java(1 hunks)src/test/java/com/eternalcode/parcellockers/database/ParcelDatabaseServiceIntegrationTest.java(2 hunks)src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- TODO.md
- src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java
- src/test/java/com/eternalcode/parcellockers/database/ParcelDatabaseServiceIntegrationTest.java
- src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java
- src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java
🔇 Additional comments (11)
src/test/java/com/eternalcode/parcellockers/database/ParcelLockerDatabaseServiceIntegrationTest.java (1)
3-5: Import changes align with ORM transition
The import modifications correctly reflect the transition from direct connection management (HikariDataSource) to a configuration-based ORM approach.
Also applies to: 9-9
src/main/java/com/eternalcode/parcellockers/ParcelLockers.java (3)
5-6: LGTM: Clean import organization and field declaration
The changes properly set up the necessary imports and fields for the ORM transition. The DatabaseManager field is appropriately declared as a class-level field to maintain the connection lifecycle.
Also applies to: 12-13, 18-18, 23-23, 31-31, 36-37, 39-40, 78-79
186-189: LGTM: Proper resource cleanup
The database disconnection is properly handled with null checks in the shutdown sequence.
131-143: Ensure consistent cache initialization across repositories
While lockerRepository.updateCaches() is called, other repositories might also need cache initialization. Consider standardizing the cache update approach across all repositories.
Let's check if other repositories have similar cache mechanisms:
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java (1)
1-131: Enhance error handling for database operations.
Consider adding comprehensive error handling for database operations:
- Define custom exceptions for different failure scenarios
- Add logging for important operations
- Consider retry mechanisms for transient failures
Let's verify the error handling patterns in the codebase:
src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java (2)
74-78: Cache is updated consistently on successful deletion.
Good job removing the parcel from the in-memory cache within the completion stage. This ensures consistency between the cache and the database.
26-27: Verify concurrent updates to the in-memory cache.
ConcurrentHashMap is thread-safe for individual operations, but consider whether combined read-write sequences require additional synchronization for correctness. For example, saving and removing rapidly may cause eventual consistency issues if other asynchronous tasks rely on stale data.
Please confirm the concurrency requirements by checking how the repository’s asynchronous calls are scheduled and accessed. Here’s a verification script for references to addToCache or removeFromCache to see if they are called in parallel contexts:
Also applies to: 105-111
✅ Verification successful
Let me gather more information about the asynchronous operations and concurrent access patterns.
Let me check one more aspect to understand the repository's base class and scheduler usage.
The concurrent cache implementation is safe and properly synchronized
The current implementation is thread-safe for the following reasons:
-
The cache operations are atomic and properly synchronized:
ConcurrentHashMapprovides thread-safe individual operations- Cache updates (
addToCache) happen before the database operation completes - Cache removals (
removeFromCache) happen only after successful database operations viathenAccept
-
The repository uses
CompletableFuturefor all operations, ensuring proper sequencing:- Save operations update the cache first, then the database
- Remove operations update the database first, then the cache only on success
- No race conditions exist between read and write operations
-
The scheduler is used for async database operations, but cache operations are properly ordered within the CompletableFuture chains
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for direct usages of addToCache/removeFromCache methods.
rg --context 3 'addToCache|removeFromCache'
Length of output: 5266
Script:
#!/bin/bash
# Check for async operations and concurrent access patterns
rg -A 5 -B 2 'CompletableFuture|async|parallel|concurrent' src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java
# Check for any scheduled tasks or background operations
rg -A 5 -B 2 '@Scheduled|scheduler|executor|thread' src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java
# Check the repository interface to understand the contract
rg --type java -A 5 -B 2 'interface.*Repository.*Parcel'
Length of output: 3299
Script:
#!/bin/bash
# Check the abstract repository implementation for any concurrent operations
rg -A 10 'class AbstractRepositoryOrmLite'
# Check how the scheduler is used
rg -A 5 'Scheduler scheduler'
# Look for any cache initialization or refresh methods
rg -A 5 'loadCache|refreshCache|initCache|clearCache'
Length of output: 12200
src/test/java/com/eternalcode/parcellockers/TestScheduler.java (3)
18-22: Handle potential exceptions in submitted tasks
When tasks are submitted to the executor (executorService.submit(runnable)), exceptions occurring within runnable are not necessarily handled or logged. For robust testing, consider capturing & reporting errors to simplify debugging.
36-40: Review asynchronous chain in scheduled tasks
runLaterAsync currently schedules an action to run after a delay, which itself is wrapped in a CompletableFuture.runAsync. Although it works, it can add an extra layer of async complexity. Consider a simpler approach if you don’t need that nested future logic.
54-58: Use direct supplyAsync for optional lazy evaluation
The complete(...) method delegates to CompletableFuture.supplyAsync(...). Verify that all test usages of this method indeed need to be executed asynchronously; occasionally, immediate evaluation might suffice for simpler test cases.
src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java (1)
19-22: Consider validating constructor parameters.
Validate that both databaseManager and scheduler are non-null to avoid potential NullPointerExceptions later.
| File dataFolder = new File("run/plugins/ParcelLockers"); | ||
| PluginConfiguration config = new ConfigurationManager(dataFolder).load(new PluginConfiguration()); | ||
| DatabaseManager databaseManager = new DatabaseManager(config, Logger.getLogger("ParcelLockers"), dataFolder); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test setup robustness
The current test setup has several areas for improvement:
- The hardcoded path "run/plugins/ParcelLockers" could cause test failures in different environments
- No visible cleanup of test resources
- Simple string-based logger might not be ideal for testing
Consider these improvements:
- File dataFolder = new File("run/plugins/ParcelLockers");
- PluginConfiguration config = new ConfigurationManager(dataFolder).load(new PluginConfiguration());
- DatabaseManager databaseManager = new DatabaseManager(config, Logger.getLogger("ParcelLockers"), dataFolder);
+ @TempDir
+ Path tempDir;
+
+ @BeforeEach
+ void setUp() {
+ dataFolder = tempDir.resolve("ParcelLockers").toFile();
+ dataFolder.mkdirs();
+ config = new ConfigurationManager(dataFolder).load(new PluginConfiguration());
+ databaseManager = new DatabaseManager(config,
+ Logger.getLogger(ParcelLockerDatabaseServiceIntegrationTest.class.getName()),
+ dataFolder);
+ }
+
+ @AfterEach
+ void tearDown() {
+ // Clean up database connections and resources
+ if (databaseManager != null) {
+ databaseManager.close();
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { | ||
| super(databaseManager, scheduler); | ||
|
|
||
| try { | ||
| TableUtils.createTableIfNotExists(databaseManager.connectionSource(), LockerWrapper.class); | ||
| } catch (SQLException ex) { | ||
| Sentry.captureException(ex); | ||
| ex.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve error handling in constructor.
Two issues need attention:
- Remove
printStackTrace()as Sentry already handles error logging - Consider failing fast if table creation fails, as this is a critical initialization step
public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) {
super(databaseManager, scheduler);
try {
TableUtils.createTableIfNotExists(databaseManager.connectionSource(), LockerWrapper.class);
} catch (SQLException ex) {
Sentry.captureException(ex);
- ex.printStackTrace();
- }
+ throw new RuntimeException("Failed to initialize locker table", ex);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { | |
| super(databaseManager, scheduler); | |
| try { | |
| TableUtils.createTableIfNotExists(databaseManager.connectionSource(), LockerWrapper.class); | |
| } catch (SQLException ex) { | |
| Sentry.captureException(ex); | |
| ex.printStackTrace(); | |
| } | |
| } | |
| public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { | |
| super(databaseManager, scheduler); | |
| try { | |
| TableUtils.createTableIfNotExists(databaseManager.connectionSource(), LockerWrapper.class); | |
| } catch (SQLException ex) { | |
| Sentry.captureException(ex); | |
| throw new RuntimeException("Failed to initialize locker table", ex); | |
| } | |
| } |
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
…LockerRepositoryOrmLite.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java (3)
58-65: Enforce position uniqueness at database levelThe comment indicates an assumption that "there is only one locker per position". This business rule should be enforced at the database level using a unique constraint.
Consider adding a unique constraint on the position column in the
LockerWrappertable to prevent duplicate positions:ALTER TABLE locker ADD CONSTRAINT unique_position UNIQUE (position);
86-103: Consider optimizing pagination for large datasetsThe current implementation has two potential performance issues:
- Offset-based pagination becomes slower as the offset increases
- Fetching n+1 records to determine if there are more results is inefficient
Consider these improvements:
- Use cursor-based pagination (using UUID or timestamp as cursor)
- Use a COUNT query to determine total records instead of n+1 pattern
Example cursor-based implementation:
-public CompletableFuture<LockerPageResult> findPage(Page page) { +public CompletableFuture<LockerPageResult> findPage(UUID lastSeenId, int limit) { return this.action(LockerWrapper.class, dao -> { List<Locker> lockers = dao.queryBuilder() - .offset((long) page.getOffset()) - .limit((long) page.getLimit() + 1) + .where().gt("uuid", lastSeenId) + .orderBy("uuid", true) + .limit((long) limit) .query() .stream().map(LockerWrapper::toLocker) .collect(Collectors.toList()); - boolean hasNext = lockers.size() > page.getLimit(); - if (hasNext) { - lockers.remove(lockers.size() - 1); - } + boolean hasNext = lockers.size() == limit; return new LockerPageResult(lockers, hasNext); }); }
105-148: Consider implementing cache invalidation strategyWhile the cache operations are thread-safe and properly implemented, there's no mechanism to handle cache invalidation when the database is modified by external sources (other services, direct database access, etc.).
Consider implementing one of these cache invalidation strategies:
- Time-based invalidation with a configurable TTL
- Event-based invalidation using database triggers or CDC (Change Data Capture)
- Periodic cache refresh using a scheduled task
Example scheduled cache refresh:
@Scheduled(fixedRate = 5, timeUnit = TimeUnit.MINUTES) public void refreshCache() { this.updateCaches(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/eternalcode/parcellockers/ParcelLockers.java(7 hunks)src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java(1 hunks)src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/eternalcode/parcellockers/ParcelLockers.java
- src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java
🔇 Additional comments (2)
src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java (2)
1-26: LGTM! Well-structured class with thread-safe caching
The class is well-organized with appropriate imports and uses ConcurrentHashMap for thread-safe caching of lockers and their positions.
27-36: Improve error handling in constructor
This issue was previously identified. The constructor should fail fast if table creation fails, and printStackTrace() should be removed as Sentry already handles error logging.
Jakubk15
changed the title
Resolves #78
Adds support for more database types as well as rewrites general database logic to use the j256/ormlite-jdbc library.
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Parcel,Locker, andItemStorageentities with ORM Lite.UserRepository.Improvements
isNoneWorldmethod to broaden criteria for position checks.Bug Fixes
Chores