Fix MySQL 8.0.20+ VALUES() deprecation & param count mismatch#1
Open
NanoBotAgent wants to merge 81 commits into
Open
Fix MySQL 8.0.20+ VALUES() deprecation & param count mismatch#1NanoBotAgent wants to merge 81 commits into
NanoBotAgent wants to merge 81 commits into
Conversation
…ent to Paper AsyncChatEvent
…, economy logic, database, edge cases
…roperties, comprehensive test suite
…ead of 2), causing ClassCastException on Paper 26.1.2
…format, changed to '26.1'
…ate step; add || true on wait to handle exit 143
…omadSMP pattern; fix server.properties heredoc indent
… zip-closed exceptions during async task cleanup
…can - was false-positive matching Error.*aurel
… Paper to build 61
…ent, not response value
…ubcommands, /pay, /market, /web, /stocks, /ah, /orders console rejection
…pilation error)
…nd fix param count mismatch (APPLEPIE6969#21)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoFix MySQL 8.0.20+ compatibility, parameter mismatch, and add comprehensive CI/CD testing
WalkthroughsDescription• **Fixed MySQL 8.0.20+ VALUES() deprecation**: Replaced all deprecated VALUES(col) syntax with modern AS new alias in 4 upsert locations (deposit(), setBalance(), loadBalance(), updatePlayerMetadata()) in EconomyManager.java • **Fixed PreparedStatement parameter count mismatch**: Added isMySQL() conditional checks to handle different parameter counts between MySQL (3-2 params) and SQLite (4-3 params) in upsert operations • **Enhanced auction display names**: Added getItemDisplayName() helper method that prefers custom display names over raw material type names in 4 auction message locations • **Improved error handling**: Added comprehensive exception handling with try-catch blocks and null-safety checks across EconomyCommand.java and AuctionCommand.java • **Added database type detection**: Introduced isMySQL() public method in DatabaseManager.java for conditional SQL syntax handling • **Removed synchronization**: Removed synchronized modifiers from deposit() and withdraw() methods for better concurrency • **Added async scheduling helper**: Introduced scheduleAsyncWrite() helper to intelligently schedule async tasks • **Migrated deprecated API**: Replaced deprecated AsyncPlayerChatEvent with AsyncChatEvent from Paper API in ChatPromptManager.java • **Comprehensive CI/CD pipeline**: Added GitHub Actions workflow with 4 jobs (build, smoke-test, ingame-test, mysql-test) including 25 RCON-based command tests and MySQL 8.0 compatibility validation • **Version and dependency updates**: Updated to v1.4.3, upgraded Java toolchain to 25, Paper API to 26.1.2, and Gradle to 9.3.0 • **Documentation updates**: Updated README, patch notes, and configuration files with v1.4.3 release information and formatting improvements Diagramflowchart LR
A["MySQL 8.0.20+ Deprecation"] --> B["Replace VALUES() with AS new alias"]
C["Parameter Count Mismatch"] --> D["Add isMySQL() conditional checks"]
E["Auction Display Names"] --> F["Add getItemDisplayName() helper"]
G["Error Handling"] --> H["Add try-catch and null-safety checks"]
I["CI/CD Infrastructure"] --> J["GitHub Actions with 4 test jobs"]
B --> K["EconomyManager.java"]
D --> K
F --> L["AuctionManager.java"]
H --> M["Command classes"]
J --> N["Automated Testing"]
File Changes1. src/main/java/com/aureleconomy/commands/EconomyCommand.java
|
Code Review by Qodo
1. Config nesting broken
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
7
to
+33
| database: | ||
| # sqlite or mysql | ||
| type: sqlite | ||
| # sqlite or mysql | ||
| type: sqlite | ||
|
|
||
| # SQLite | ||
| file: "database.db" | ||
| # SQLite | ||
| file: "database.db" | ||
|
|
||
| # MySQL (only if type is mysql) | ||
| mysql: | ||
| host: "localhost" | ||
| port: 3306 | ||
| database: "aurelium" | ||
| username: "root" | ||
| password: "password" | ||
| # MySQL (only if type is mysql) | ||
| mysql: | ||
| host: "localhost" | ||
| port: 3306 | ||
| database: "aurelium" | ||
| username: "root" | ||
| password: "password" | ||
|
|
||
| # --- Economy --- | ||
| economy: | ||
| default-currency: "Aurels" | ||
|
|
||
| currencies: | ||
| Aurels: | ||
| symbol: "₳" | ||
| starting-balance: 100.0 | ||
| # Add more currencies here: | ||
| # Dollars: | ||
| # symbol: "$" | ||
| # starting-balance: 0.0 | ||
|
|
||
| # -1 = no cap | ||
| max-balance: -1 | ||
| min-pay-amount: 0.01 | ||
|
|
||
| # Messages (MiniMessage format) | ||
| # Placeholders: %amount%, %symbol%, %player%, %currency% | ||
| prefix: "<gold>[Aurelium] <gray>" | ||
| balance: "Balance (%currency%): %amount%%symbol%" | ||
| balance-other: "Balance of %player% (%currency%): %amount%%symbol%" | ||
| paid: "You paid <green>%amount%%symbol%</green> to <yellow>%player%</yellow>." | ||
| received: "You received <green>%amount%%symbol%</green> from <yellow>%player%</yellow>." | ||
| insufficient-funds: "You don't have enough %currency%!" | ||
| admin-give: "Gave <green>%amount%%symbol%</green> (%currency%) to <yellow>%player%</yellow>." | ||
| admin-take: "Took <red>%amount%%symbol%</red> (%currency%) from <yellow>%player%</yellow>." | ||
| admin-set: "Set <yellow>%player%</yellow>'s balance to <green>%amount%%symbol%</green> (%currency%)." | ||
| default-currency: "Aurels" | ||
|
|
||
| currencies: | ||
| Aurels: | ||
| symbol: "₳" | ||
| starting-balance: 100.0 | ||
| # Add more currencies here: | ||
| # Dollars: | ||
| # symbol: "$" | ||
| # starting-balance: 0.0 |
There was a problem hiding this comment.
1. Config nesting broken 🐞 Bug ≡ Correctness
src/main/resources/config.yml indentation no longer nests keys under sections like database.mysql and economy.currencies, so lookups like database.mysql.host and economy.currencies will return null/defaults and can crash command handlers expecting those sections.
Agent Prompt
### Issue description
`src/main/resources/config.yml` lost required YAML nesting indentation under sections like `database.mysql` and `economy.currencies`. This causes runtime config lookups (e.g., `database.mysql.host`, `economy.currencies`) to return null/default values, breaking DB connection settings and currency validation.
### Issue Context
Code expects nested sections:
- MySQL config is read via `database.mysql.host/port/database/username/password`.
- Currency lists are read via `economy.currencies` as a `ConfigurationSection`.
### Fix Focus Areas
- src/main/resources/config.yml[7-126]
### What to change
- Indent `host/port/database/username/password` under `database.mysql:`.
- Indent currencies under `economy.currencies:` (e.g., `Aurels:` under `currencies:` and `symbol/starting-balance` under `Aurels:`).
- Repeat for other nested sections (`market.price-recovery`, `market.alerts`, `web.local`, `web.cloud`, etc.) so the YAML hierarchy matches the dotted paths used in code.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
145
to
151
| return true; | ||
| } | ||
| currency = args[3]; | ||
| if (!plugin.getConfig().getConfigurationSection("economy.currencies").contains(currency)) { | ||
| if (plugin.getConfig().getConfigurationSection("economy.currencies") != null && plugin.getConfig().getConfigurationSection("economy.currencies").contains(currency)) { | ||
| player.sendMessage(Component.text("Invalid currency: " + currency, NamedTextColor.RED)); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
2. Currency check inverted 🐞 Bug ≡ Correctness
AuctionCommand rejects valid currencies because it sends "Invalid currency" when the currency key is present in economy.currencies for the 4-arg /ah sell|bin path.
Agent Prompt
### Issue description
`AuctionCommand` rejects currencies that actually exist in `economy.currencies` because the validation condition is inverted.
### Issue Context
This affects `/ah sell <price> <duration> <currency>` and `/ah bin <price> <duration> <currency>`.
### Fix Focus Areas
- src/main/java/com/aureleconomy/commands/AuctionCommand.java[140-152]
### What to change
- Change the validation to:
- if section is null -> treat as invalid (or default currency)
- else if `!section.contains(currency)` -> invalid
- Also avoid calling `getConfigurationSection("economy.currencies")` twice; store it in a local variable for clarity.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+98
to
+110
| public void withdraw(OfflinePlayer player, BigDecimal amount, String currency) { | ||
| if (amount.compareTo(BigDecimal.ZERO) <= 0) | ||
| return; | ||
|
|
||
| UUID uuid = player.getUniqueId(); | ||
| BigDecimal normalizedAmount = amount.setScale(SCALE, ROUNDING_MODE); | ||
|
|
||
| BigDecimal currentBalance = getBalanceFromCache(uuid, currency); | ||
| if (currentBalance.compareTo(normalizedAmount) < 0) { | ||
| plugin.getComponentLogger().warn("Insufficient funds for withdraw: " + player.getName() + | ||
| " tried to withdraw " + normalizedAmount + " but only has " + currentBalance); | ||
| return; | ||
| } |
There was a problem hiding this comment.
3. Withdraw ignores db balance 🐞 Bug ≡ Correctness
EconomyManager.withdraw() uses a cache-only balance (returns ZERO when missing), so valid withdrawals can be rejected after cache invalidation and in flows that do not pre-load balance (e.g., /eco take).
Agent Prompt
### Issue description
`EconomyManager.withdraw()` currently checks funds using cache-only state, which can be empty (especially after `invalidateCache()`), causing false "insufficient funds" and skipped withdrawals.
### Issue Context
- `getBalanceFromCache()` returns ZERO when the currency is not cached.
- Some callers (notably `/eco take`) call `withdraw()` directly without first calling `has()`.
### Fix Focus Areas
- src/main/java/com/aureleconomy/economy/EconomyManager.java[98-137]
- src/main/java/com/aureleconomy/commands/EconomyCommand.java[282-302]
### What to change (choose one consistent approach)
1) **Safer default:** In `withdraw()`, use `getBalance(player, currency)` (cache + DB fallback) for the funds check instead of `getBalanceFromCache()`.
2) Alternatively, in `/eco take`, call `withdrawIfSufficient(...)` (it already performs an atomic DB update and returns boolean), and only send success if it returns true.
Also consider reintroducing synchronization/atomic cache updates if concurrent withdraw/deposit calls are possible, since the current cache update is a read-modify-write without locking.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
Fixes
1. MySQL
VALUES(col)deprecated since 8.0.20 (APPLEPIE6969#21)Replaced all
VALUES(col)references with modernAS newalias syntax across 4 upsert locations inEconomyManager.java:deposit()ON DUPLICATE KEY UPDATE balance = balance + VALUES(balance)AS new ON DUPLICATE KEY UPDATE balance = balance + new.balancesetBalance()ON DUPLICATE KEY UPDATE balance = VALUES(balance)AS new ON DUPLICATE KEY UPDATE balance = new.balanceloadBalance()ON DUPLICATE KEY UPDATE balance = VALUES(balance)AS new ON DUPLICATE KEY UPDATE balance = new.balanceupdatePlayerMetadata()ON DUPLICATE KEY UPDATE name = VALUES(name)AS new ON DUPLICATE KEY UPDATE name = new.name2. PreparedStatement param count mismatch
MySQL upserts use 3 params (or 2 for players), SQLite needs 4 (or 3) for the
ON CONFLICT ... SET col = ?pattern. The extraps.setBigDecimal(4, ...)/ps.setString(3, ...)was always executed unconditionally — now wrapped withisMySQL()guards so MySQL only sets the params it actually uses.3. Auction display name (APPLEPIE6969#22)
getItemDisplayName()helper added — prefers custom display name over raw material type in all 4 auction message locations.CI Results