GH-1147 Make UserManager use UserRepository #1147
Conversation
CitralFlo
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the UserManager to use a repository pattern for data persistence, which is a great improvement. I've identified a few issues, including a critical bug that causes double database writes and a race condition during initial data loading. Additionally, there are opportunities to improve code clarity and maintainability by simplifying some logic and improving API design. The new integration tests are a good addition, but could be improved by removing console output.
eternalcore-core/src/main/java/com/eternalcode/core/user/UserManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/user/UserManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/user/UserManager.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/user/database/UserRepository.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/user/database/UserRepository.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/user/database/UserRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/test/java/com/eternalcode/core/user/BatchTest.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/test/java/com/eternalcode/core/user/UserBatchFetchTest.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/test/java/com/eternalcode/core/user/UserBatchFetchTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public CompletableFuture<Collection<User>> fetchUsersBatch(int batchSize) { | ||
| return CompletableFuture.supplyAsync(() -> { |
There was a problem hiding this comment.
I think you should supply a custom executor, since you are already using one in all AbstractRepositoryOrmLite methods
…-manager-database
Jakubk15
changed the title
| + "database type " | ||
| + "to be used."}) | ||
| public DatabaseDriverType databaseType = DatabaseDriverType.SQLITE; | ||
| public DatabaseDriverType databaseType = DatabaseDriverType.MYSQL; |
There was a problem hiding this comment.
hm czemu tak? jak w testach coś nie gra to settuj fielda jest public
| SQLITE(SQLITE_DRIVER, SQLITE_JDBC_URL); | ||
| SQLITE(SQLITE_DRIVER, SQLITE_JDBC_URL), | ||
|
|
||
| H2_TEST(H2_DRIVER, "jdbc:h2:mem:test;DB_CLOSE_DELAY=-1;MODE=MYSQL"); |
There was a problem hiding this comment.
raczej bym unikał dodawania kawałków kodu które są tylko dla testów, jaki tutaj był problem? może da się to lepiej rozwiązać?
|
|
||
| @EventHandler | ||
| public void onJoin(PlayerJoinEvent event) { | ||
| this.userManager.fetchUser(event.getPlayer().getUniqueId()); |
4 participants
Methods do not change so #1136 can be merged :>