⚡ Improve GemExecutor performance by removing redundant DB query#10
⚡ Improve GemExecutor performance by removing redundant DB query#10
Conversation
- Optimized `GemExecutor.addGems` to reuse `getGems` result, saving one DB query. - Added `GemExecutorTest` to verify single invocation of `getGems`. - Updated `pom.xml` with test dependencies (JUnit, Mockito) and fixed PlaceholderAPI repository. Co-authored-by: acsoto <59144459+acsoto@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3963e54e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-core</artifactId> | ||
| <version>5.2.0</version> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
The module still targets Java 8 (<java.version>1.8</java.version> in this POM), but Mockito 5.x requires Java 11+. With mockito-core/mockito-inline set to 5.2.0, any mvn test or CI that runs under Java 8 will fail to load Mockito classes before tests execute. To keep tests runnable on the project’s declared Java level, use a Mockito 4.x release (or raise the project’s Java version).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR optimizes the GemExecutor.addGems method by eliminating a redundant database query, reducing DB calls from 2 to 1 per invocation when checking if a user exists. The PR also adds test infrastructure and updates the PlaceholderAPI dependency configuration.
Changes:
- Optimized
addGemsmethod to reuse the result of the firstgetGemsquery instead of calling it again - Added JUnit and Mockito test dependencies and created
GemExecutorTestto verify the optimization - Updated PlaceholderAPI repository URL and version to ensure dependency resolution
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| modules/Gem/src/main/java/com/mcatk/gem/GemExecutor.java | Optimized addGems to reuse gems variable in null check, eliminating redundant DB query |
| modules/Gem/src/test/java/com/mcatk/gem/GemExecutorTest.java | Added unit test to verify getGems is called only once when adding gems to a new user |
| modules/Gem/pom.xml | Added test dependencies (JUnit, Mockito, Byte Buddy) and updated PlaceholderAPI repository URL and version |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <version>5.2.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-inline</artifactId> | ||
| <version>5.2.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>net.bytebuddy</groupId> | ||
| <artifactId>byte-buddy</artifactId> | ||
| <version>1.14.19</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>net.bytebuddy</groupId> | ||
| <artifactId>byte-buddy-agent</artifactId> | ||
| <version>1.14.19</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
The mockito-inline dependency (version 5.2.0) also requires Java 11 or higher, but this project targets Java 1.8. This will cause compatibility issues. Additionally, mockito-inline is typically only needed for mocking static methods and final classes. If you're using it for static field manipulation (as seen in the test), consider using PowerMock with Mockito 3.x for Java 8 compatibility, or update the project to Java 11+.
| <version>5.2.0</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-inline</artifactId> | |
| <version>5.2.0</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>net.bytebuddy</groupId> | |
| <artifactId>byte-buddy</artifactId> | |
| <version>1.14.19</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>net.bytebuddy</groupId> | |
| <artifactId>byte-buddy-agent</artifactId> | |
| <version>1.14.19</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <version>3.12.4</version> | |
| <scope>test</scope> | |
| </dependency> |
| private void setStaticField(Class<?> clazz, String fieldName, Object value) throws Exception { | ||
| Field field = clazz.getDeclaredField(fieldName); | ||
| field.setAccessible(true); | ||
| field.set(null, value); | ||
| } |
There was a problem hiding this comment.
The test uses reflection to set static fields, which is fragile and can break with different JVM configurations or security managers. Since the code uses MySQLManager.getInstance() and Gem.getPlugin() which implement singleton patterns, consider refactoring the production code to use dependency injection instead. This would make testing easier and more robust without requiring reflection hacks.
Alternatively, if reflection must be used for testing singletons, consider adding a reset method to these singleton classes specifically for testing purposes, which would be safer than direct field manipulation.
| // Simulate new user: getGems returns null | ||
| when(mySQLManagerMock.getGems(playerName)).thenReturn(null); | ||
|
|
||
| // Mock getTotal to return a value (e.g. 0) | ||
| when(mySQLManagerMock.getTotal(playerName)).thenReturn(0); | ||
|
|
||
| gemExecutor.addGems(playerName, gemsToAdd); |
There was a problem hiding this comment.
The test verifies that getGems is called only once, which is correct for the performance optimization. However, the test setup mocks getTotal to return 0, which masks a potential NullPointerException bug in the production code. When a new user is created (gems is null), getTotal will also return null, and the code on line 53 of GemExecutor.java attempts to add to this null value, which will throw a NullPointerException.
Consider adding an additional test case that mocks getTotal to return null (matching the actual behavior for new users) to expose this bug and verify it's handled correctly.
| <dependency> | ||
| <groupId>net.bytebuddy</groupId> | ||
| <artifactId>byte-buddy</artifactId> | ||
| <version>1.14.19</version> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
The PR description mentions adding Byte Buddy 1.14.19 "for Java 21 compatibility", but the pom.xml specifies Java 1.8 as the target version (line 15: <java.version>1.8</java.version>). This is misleading.
If the project is actually targeting Java 1.8, the Java 21 compatibility justification is incorrect. If the project needs Java 21 compatibility, the java.version property should be updated accordingly. Please clarify the actual Java version target and update either the description or the pom.xml configuration.
| <version>5.2.0</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-inline</artifactId> | ||
| <version>5.2.0</version> |
There was a problem hiding this comment.
Mockito 5.2.0 requires Java 11 or higher, but this project targets Java 1.8 (see line 15). This will cause compatibility issues when running tests. Consider using Mockito 3.x (e.g., 3.12.4) which is the last version series that supports Java 8, or update the project's Java version to at least Java 11.
| <version>5.2.0</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-inline</artifactId> | |
| <version>5.2.0</version> | |
| <version>3.12.4</version> | |
| <scope>test</scope> | |
| </dependency> | |
| <dependency> | |
| <groupId>org.mockito</groupId> | |
| <artifactId>mockito-inline</artifactId> | |
| <version>3.12.4</version> |
1 participant
Optimized
GemExecutor.addGemsto reuse the result of the firstgetGemsdatabase query instead of calling it again in the null check. This reduces the number of database queries by 1 peraddGemscall.Added a unit test
GemExecutorTest.javato reproduce the issue (verify 2 calls previously) and verify the fix (ensure 1 call now).Updated
pom.xmlto:https://repo.helpch.at/releases/and update version to 2.11.5 to ensure dependencies resolve correctly.PR created automatically by Jules for task 7872793457208915354 started by @acsoto