Repository navigation
Fix MySQL initialization failures and constructor lifecycle safety - #361
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No identified issue prevents merging after normal checks. Confirm the hosted CI build completes. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (2 skipped: 2… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: handling MySQL initialization failures and improving constructor lifecycle safety.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 7.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
-
-
-
-
-
-
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a68cc2dfe6
ℹ️ 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".
| - name: Build and install SimpleAPI dependency | ||
| run: mvn -B -f .ci/simpleapi/SimpleAPI/pom.xml clean install |
There was a problem hiding this comment.
Installing the pinned SimpleAPI source only inside this CI job leaves the normal Maven build dependent on the unchanged public 1.0.2-SNAPSHOT, which the added comment and documentation acknowledge may lack the APIs now used by MySQL and GlobalMySQL. Consequently, a clean downstream checkout running mvn -f AdvancedCore/pom.xml package can fail compilation unless the developer first performs the undocumented-at-the-POM manual dependency install; reference a published dependency containing this API (or avoid requiring it) so source consumers continue to build normally.
AGENTS.md reference: AGENTS.md:L37-L42
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ca3d3530e
ℹ️ 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".
| } catch (SQLException | RuntimeException | Error failure) { | ||
| try { mysql.disconnect(); } | ||
| catch (RuntimeException | Error cleanupFailure) { failure.addSuppressed(cleanupFailure); } | ||
| throw new IllegalStateException("Failed to initialize global SQL database selection", failure); |
There was a problem hiding this comment.
When a JDBC driver or pool throws a RuntimeException or Error from connection acquisition, statement preparation, or execution, this catch converts it into an IllegalStateException. Before this change those unchecked failures propagated unchanged, and the pinned SimpleAPI implementation's equivalent path explicitly rethrows RuntimeException and Error after cleanup, including coverage for IllegalArgumentException and LinkageError; this public constructor therefore now has incompatible exception semantics and can disguise fatal linkage failures. After disconnecting, rethrow unchecked exceptions and errors unchanged and wrap only SQLException.
AGENTS.md reference: AGENTS.md:L37-L38
Useful? React with 👍 / 👎.
MySQL authentication rejection now aborts user/global SQL initialization without invoking subclass logging before its plugin reference is assigned. Successful construction still initializes the table, caches and UUID schema; a later setup failure closes owned pools/workers and preserves the primary exception. No SQLite fallback, schema/config migration or reward behavior change is introduced.
The accompanying SQL abstraction fix is merged in SimpleAPI #111. The POM now pins the published immutable artifact
1.0.2-20261011.161601-40. CI and normal clean Maven builds resolve the same dependency; no manual SimpleAPI source install is needed. This addresses Codex's clean-build dependency finding. Global database selection also preserves unchecked exception/error identity after cleanup, wrapping only checked SQL exceptions. Three regression tests exercise connection acquisition, statement preparation and execution with runtime, linkage and SQL failures and suppressed cleanup errors. The downloaded MySQL, AbstractSqlTable and ConnectionManager classes were verified byte-for-byte against the merged source candidate. The shaded JAR requires no separate runtime library installation.Validation for
d09f0d891:MySQLInitializationTest: 8 passed, zero failures/errors.mvn -B -o -ntp -f AdvancedCore/pom.xml clean package: 1,032 passed, zero failures/errors/skips. Dependency resolution was first exercised online through Maven using the published coordinate.git diff --checkpassed.f8269cf51cbe205f7bee0cb7d169159c7aa0663f3e2dfc3476a870477c9fb6e1(6,847,410 bytes). A subsequent local install for downstream testing succeeded without rerunning tests.d09f0d891; CodeRabbit is rate limited and is not claimed as a fresh clean review.Tests simulate authentication failure and setup cleanup with mocked JDBC/platform services; no live external MySQL or native Paper/Folia startup verification is claimed. Original authentication denial remains a server credential/permission problem. Merge this before VotingPlugin #1701; do not deploy an old dependency accidentally.
Summary by CodeRabbit
Bug Fixes
Documentation