refactor: try-with-resources for remaining Getter.java JDBC sites (#855)#856
Merged
Conversation
Wrap all remaining PreparedStatement/CallableStatement/ResultSet sites in Getter.java with try-with-resources so JDBC resources are deterministically closed on every path. SQL strings, parameter bindings, and control flow are unchanged (verified: SQL literals byte-identical, 58 param-binding calls before and after). Notable cases: - getJsonScore: statement is conditionally assigned (totalScoreboard vs classScoreboard), so it cannot go in the Connection TWR header; registered as a closeable resource via `try (CallableStatement x = callstmnt; ResultSet rs = callstmnt.executeQuery())`. - isModuleOpen: converted statement + ResultSet to nested TWR, removed the now-redundant manual rs.close(). Completes the JDBC resource-leak cleanup for the dbProcs layer alongside Setter.java (#846 / PR #852). Addresses the recurring code-scanning "Potential database resource leak" flags on #816. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors remaining JDBC access sites in Getter.java to use try-with-resources, aligning the getter layer with recent connection-pooling and Setter.java cleanup work.
Changes:
- Wraps remaining
PreparedStatement/CallableStatement/ResultSetusages in try-with-resources. - Handles special conditional statement creation in
getJsonScore. - Removes manual
ResultSetclosing in favor of deterministic resource management.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Author
|
Validated via |
7 tasks
SeanDuggan
added a commit
that referenced
this pull request
May 30, 2026
…816) * feat: add connection pooling to fix db connection exhaustion (#536) Replace per-request DriverManager connections with HikariCP connection pooling for MySQL/MariaDB and singleton MongoClient for MongoDB. This prevents automated tools (e.g. ZAP spider/fuzzer) from exhausting all database connections and causing multi-hour outages. - Add ConnectionPool using HikariCP with configurable pool size, timeouts - Refactor Database.java to delegate all connections through the pool - Refactor MongoDatabase.java to use singleton pattern with internal pooling - Add DatabaseLifecycleListener for clean startup/shutdown of pools - Add connection pool tests and MongoDB singleton tests - Add database configuration, testing, and development workflow docs - Add example properties files for MySQL and MongoDB configuration Made-with: Cursor * fix: resolve CI failures and convert tests to JUnit 5 - Add missing imports (Arrays, FileInputStream) in MongoDatabase.java lost during rebase conflict resolution - Wrap ConnectionPool init failure as SQLException to maintain API compatibility with callers that catch SQLException - Convert ConnectionPoolTest and DatabaseLifecycleListenerTest from JUnit 4 to JUnit 5 so surefire discovers and runs them - Add mongo_challenge_test.properties test resource for MongoDatabaseTest - Apply Google Java Format to all modified files Made-with: Cursor * ci: update lint-java to googlejavaformat-action v4 The pinned v3.6.0 action uses Node 12 (deprecated, scheduled for removal Sep 2026) and may download an incompatible google-java-format version for the current ubuntu-latest JDK. Update to v4 (pinned to c1134eb) which uses Node 20 and auto-downloads the latest compatible GJF release. Also update checkout to v4. Made-with: Cursor * chore: trigger CI re-run Made-with: Cursor * fix: allowMultiQueries boolean and ConnectionPoolIT error handling - Change allowMultiQueries=yes to allowMultiQueries=true in Database.java (HikariCP requires strict boolean values, not yes/no) - Catch RuntimeException in ConnectionPoolIT.testChallengeConnection (HikariCP throws PoolInitializationException which extends RuntimeException) Made-with: Cursor * fix: JDBC driver resolution and Setup host/port validation ConnectionPool: read DriverType from properties with URL-based fallback for mariadb/mysql drivers. Fixes Tomcat classloader issue where JDBC 4 auto-registration doesn't work for webapp-scoped drivers. Setup: fix XOR logic for db host/port validation — the original condition rejected valid input when both fields were provided. Extract validation into testable validateHostPort() method. Add SetupTest with 7 tests covering all host/port combinations. Made-with: Cursor * fix: reduce challenge pool sizing to minimize idle connections Challenge pools use per-schema credentials by design (isolation between challenges), but don't need the same sizing as the core pool. Set challenge pools to max 3 / min idle 0 / 2min idle timeout so they hold zero connections when unused. * style: format ConnectionPool.java with Google Java Format * test: add proper integration tests for challenge connection pools Replace the silent-pass testChallengeConnection with tests that verify: - Challenge connections are valid and usable - Same credentials reuse the same pool - Different schemas create separate pools - Challenge pools use correct sizing (max 3, min idle 0, 2min timeout) - Max connections are enforced at pool limit - Shutdown clears all challenge pools - Concurrent challenge connections succeed and share one pool Also adds getChallengePoolCount() and getChallengePool() accessors to ConnectionPool for test observability. * feat: add connection leak detection with 60s threshold Configures HikariCP's leakDetectionThreshold to log a warning when a connection is held for longer than 60 seconds without being closed. Helps identify connection leaks that could lead to pool exhaustion. * docs: add troubleshooting for stale Docker DB image cache Documents the MariaDB init failure caused by Docker caching an image before the DELIMITER conversion script runs. Includes the fix (--no-cache rebuild + volume cleanup) in both AGENTS.md and docs/database-configuration.md. * docs: add first-time setup instructions for Security Shepherd Documents the setup.jsp flow required after docker compose up, including how to get the auth token and the correct database hostname (container name, not localhost). * docs: fix setup instructions with correct parameter names The setup servlet uses 'dbauth' (not 'authToken') and requires MongoDB params (mhost, mport) even when not using mongo challenges. Documents the correct curl command and all parameter names for both agents and human contributors. * test: add load test simulating normal + aggressive users Python load test that boots the stack, configures the platform, registers 20 users, and simulates 17 normal users browsing alongside 3 aggressive users doing rapid automated scanning. Monitors DB connections and app responsiveness, reports pass/fail based on connection bounds and response times. * fix: return core DB connections to pool from Getter (stacked on #816) (#817) * fix: return core DB connections to pool from Getter methods - Use try-with-resources for all Getter paths that used manual closeConnection - Keep legacy ResultSet APIs (getClassInfo all classes, getPlayersByClass, getAdmins) unchanged - Add ConnectionPool.getCoreActiveConnections for tests - Add GetterCorePoolLeakIT to assert bounded pool usage under repeated authUser * style: format Getter.java with Google Java Format * fix: cache isInstalled(), tune pool for stability under load Setup.isInstalled() was called on every HTTP request via SetupFilter, each time reading database.properties from disk and borrowing a core pool connection just to check non-null. Under concurrent load this exhausted the pool and cascaded into a full app lockup. Cache with volatile Boolean + double-checked locking so the check runs once, then returns constant-time on all subsequent requests. resetInstalledCache() called after setup completes so the first post-setup request re-evaluates. Warm the cache at startup from DatabaseLifecycleListener.contextInitialized(). Pool tuning: - maxPoolSize 10 → 20 (supports realistic classroom concurrency) - connectionTimeout 30s → 5s (fail fast under overload instead of blocking Tomcat threads for 30s each) - minIdle 2 → 5 (reduce cold-start latency) Known limitation: authUser holds a DB connection during Argon2 password verification (~100-200ms). This limits throughput under high concurrency. Follow-up will release connection before hashing. Load test updated with --target and --concurrency flags for targeted per-class/per-method testing instead of broad soak only. * fix: only cache isInstalled()=true, not transient false If the DB is temporarily unreachable during the first isInstalled() call, caching false permanently locks the app into "not installed" state for the JVM lifetime. Only cache the true (terminal) state; leave installedCached=null on failure so subsequent requests retry. * chore: update pool docs, harden SetupIT, remove stale TODO Address Copilot review feedback on PR #817: - Update database.properties.example and docs/database-configuration.md to reflect new pool defaults (maxPoolSize=20, minIdle=5, connectionTimeout=5000) - Add assumeTrue guard in SetupIT cache tests so they skip instead of false-passing when the database is unavailable - Remove stale TODO in Getter.authUser (try-with-resources answers it) * fix: release DB connection before Argon2 verification in authUser (#819) (#821) * fix: release DB connection before Argon2 verification in authUser Split authUser into three phases so the connection is only held during actual DB work, not during CPU-bound Argon2 hashing: 1. Fetch user record into local variables (~1-5ms DB hold) 2. Verify password with Argon2 (no connection held, ~100-200ms) 3. Re-acquire connection briefly for userBadLoginReset if needed Previously the entire method ran inside a single try-with-resources block, holding a pool connection for ~200ms per login attempt. Under 20 concurrent logins this exhausted the pool and blocked unrelated endpoints (challenge lookups, scoreboards, module status). Auth decision flow and side effects are identical to previous implementation; only connection scope changes. No schema changes or stored procedure modifications required. Closes #819 Made-with: Cursor * fix: null-safe loginType check, remove unused variable - Use "login".equals(loginType) to prevent NPE if loginType is null - Remove unused userFound variable left over from refactor Made-with: Cursor * fix: check suspension and login type before Argon2 verification Move loginType and suspendedUntil checks before the expensive Argon2 hash so suspended and SSO users are rejected cheaply. Also adds a null guard on suspendedUntil to prevent NPE when the column is NULL. * test: add integration tests for authUser fail-fast checks Add GetterAuthIT with JUnit 5 tests verifying that: - suspended users are rejected before Argon2 verification - SSO users cannot authenticate via password login These cover the fail-fast checks moved before Argon2 in the previous commit. * ci: add concurrency group to deduplicate workflow runs (#827) When pushing to a feature branch with an open PR, both push and pull_request triggers fire. The concurrency group ensures only one run proceeds per PR/branch, cancelling the duplicate. * Remove Trello link from README (#822) Removed Trello link from the README. * fix: migrate all integration tests to JUnit 5 and fix revealed failures (#826) * fix: migrate all integration tests from JUnit 4 to JUnit 5 (#825) 37 IT files were using JUnit 4 annotations (org.junit.Test, @before, @BeforeClass, etc.) but the project only has junit-jupiter-engine — no junit-vintage-engine. The JUnit Platform silently skipped them all, reporting 0 tests run with no warning. Migrated all IT files to JUnit 5: updated imports, annotations, and converted @test(expected=...) to assertThrows(). Closes #825 * style: apply google-java-format to SetterIT.java * fix: resolve 8 integration test failures revealed by JUnit 5 migration - CountdownHandler.saveCountdowns: fix copy-paste bug where startTime was saved as lockTime, corrupting DB state - CountdownHandler: add forceReload() so cached static state can be invalidated when tests reset the database - TestProperties.executeSql: call forceReload() after DB reset to prevent stale CountdownHandler state across test classes - EnableModuleBlock: fix error message casing ("received" → "Received") - XxeChallenge1IT: use XxeChallenge1OldWebService (the XML-based challenge endpoint) instead of XxeLesson (which blocks challenge file content with its "Wrong File" check) * fix: add missing module-open check to XxeChallenge1OldWebService XxeChallenge1OldWebService was processing requests even when the module was closed, unlike its sibling XxeChallenge1 which properly checks Getter.isModuleOpen(). This caused XxeChallenge1IT testLevelWhenUnsafeLevelsAreDisabled to fail. * fix: add missing JUnit 5 imports and fix assertTrue argument order in SetupIT assertEquals and assumeTrue were used but not imported after the merge conflict resolution dropped them. Also fix assertTrue(String, boolean) to assertTrue(boolean, String) for JUnit 5 compatibility. * fix: return core DB connections to pool from Setter (stacked on #816) (#834) * test: fix three ITs missed by #832's TestProperties API split PR #832 split TestProperties.executeSql(Logger) into ensureSchemaReady(Logger) + reseedTestData(). The dev->dev#536 merge migrated most ITs to the new pattern but missed three files that fail to compile against the new signature. - src/it/java/dbProcs/GetterAuthIT.java - src/it/java/dbProcs/GetterCorePoolLeakIT.java - src/it/java/servlets/SetupIT.java (two @test methods) Same one-line swap applied to each — replace the executeSql(log) call with ensureSchemaReady(log) + reseedTestData(). No behavior change; restores the branch to a compilable state ahead of the Setter leak fix. * fix: return core DB connections to pool from Setter methods Mirrors the Getter leak fix in #817 for Setter.java. Pre-pooling each request opened a fresh DB connection, so leaks were invisible; with the HikariCP pool (#816), every leaky path bleeds the core pool and surfaces as 5s connection-acquire timeouts. - Convert all 41 Database.getCoreConnection / getChallengeConnection call sites in Setter.java to try-with-resources. Removes every manual Database.closeConnection — the Connection is AutoCloseable, so it returns to the pool on every code path including thrown exceptions. - Apply the auth-hold-time fix (#819) to three methods that ran Argon2 while holding a DB connection (updatePassword, updatePasswordAdmin, userCreate). Argon2 verify/hash is CPU-bound ~100-200ms; moving the hash before connection acquisition frees pool capacity for concurrent callers. - updatePassword additionally relied on Getter.authUser inside its own connection scope, which doubled the hold time. Phase 1 (verify) now uses Getter.authUser's own connection scope; phase 2 (hash) holds none; phase 3 (UPDATE) opens a fresh connection. - The 17 'throws SQLException' setting methods (setAdminCheatStatus, setFeedbackStatus, setLockTime, etc.) declared Connection outside any try and could throw RuntimeException before manual closeConnection, leaking unconditionally. Now bounded by try-with-resources. Add SetterCorePoolLeakIT mirroring GetterCorePoolLeakIT: hammers resetBadSubmission and suspendUser 500 times each with synthetic userIds and asserts ConnectionPool.getCoreActiveConnections stays bounded. Verified: 500 reset + 500 suspend iterations leave active connections at 0. SetterIT's update-family tests (updatePassword, updatePasswordAdmin, updateUsername, updateUserRole) still pass. Out of scope: the wider leak surface in servlets, Setup, scoreboards, JSON exporters, etc. Tracked as follow-up — same try-with-resources treatment applies file by file. * style: apply Google Java Format to Setter.java Two single-line collapses where my wrapping was below the 100-col threshold. CI's lint-java step runs google-java-format with --set-exit-if-changed on **/*.java; this brings the file back to canonical format. * fix: address PR #834 Copilot review feedback - userCreateSSO de-dup loop: wrap PreparedStatement and ResultSet in try-with-resources inside the while loop. The loop can run up to 500 iterations and previously held up to 500 open statements + server-side cursors simultaneously until the outer connection closed. - userCreateSSO userCreate call: wrap CallableStatement and ResultSet in try-with-resources so they release promptly instead of waiting on the outer connection scope. - updateCsrfCounter: fix log prefix (said 'Getter.updateCsrfCounter', should be 'Setter.updateCsrfCounter'). Pre-existing typo. - SetterCorePoolLeakIT: tighten the leak assertion from active <= MAX_ALLOWED_ACTIVE (32) to active == baseline. The loop is serial — a non-leaking pool MUST return active to baseline (typically 0) after every iteration. The old threshold could mask leaks: the actual pool max is 20, so a value of 21-32 was already saturation, and Hikari would throw SQLTransientConnectionException long before active reached 32, leaving the test green while the pool was dead. Drop the MAX_ALLOWED_ACTIVE constant. * test: tighten GetterCorePoolLeakIT leak assertion (#835) Mirrors the SetterCorePoolLeakIT tightening from #834. The loop is serial, so a non-leaking pool MUST return active connection count to the pre-call baseline after every iteration. The old assertion (active <= MAX_ALLOWED_ACTIVE = 32) could mask leaks: the actual core pool max is 20, and Hikari's 5s acquire timeout throws SQLTransientConnectionException long before active reaches 32 — so the test could pass while the pool was already dead. Replace with assertEquals(baseline, active) and drop the MAX_ALLOWED_ACTIVE constant. * fix: address second round of PR #834 Copilot review feedback String validation: - setModuleLayout and setScoreboardStatus compared incoming String args with != against literals (reference equality). This worked by accident because callers happened to pass interned strings, but would throw IllegalArgumentException on valid values arriving via HTTP params, JSON deserialization, or string concatenation. Switch to .equals(). Statement lifetimes: - updateUsername (PreparedStatement), updatePasswordAdmin (CallableStatement), and userCreate (CallableStatement + ResultSet) borrowed JDBC statements outside try-with-resources. With pooled connections the practical leak risk is small (Hikari releases statements when the connection returns to the pool), but explicit release matches the convention used in Getter.authUser (#817) and the userCreateSSO fix in 173836b. Wrap each in try-with-resources. All 14 affected SetterIT tests still pass — every accepted layout/status value remains accepted, every rejected value (including case-sensitive "CTF" and "Open") still rejected. * fix: address third round of PR #834 Copilot review feedback - updatePlayerResult held a core DB connection while calling Getter.getModuleNameLocaleKey, which opens its own. Two pooled connections per call doubles pool pressure under concurrency. Move the Getter call outside the try-with-resources so the first connection returns to the pool before the second is borrowed. Same auth-hold-time pattern that #819 applied to Getter.authUser. - SetterCorePoolLeakIT Javadoc claimed the success path leaked. The success path actually returned the connection; the leak was on the exception/early-return paths that skipped Database.closeConnection. Rewrite the comment to describe the real failure mode. - updatePassword Javadoc said it returned a ResultSet but the method returns boolean. Fix the @return. - updateUsername Javadoc was copy-pasted from updatePassword: wrong parameter docs (mentioned currentPassword/newPassword which do not exist on this method) and wrong return type. Rewrite to match the actual (ApplicationRoot, userName, newUsername) -> boolean shape. * fix: harden first-time setup against pool state (stacked on #834) (#836) * fix: harden first-time setup path against pool state Three bugs reviewers identified in the setup flow, all caused by routing setup credential-test and schema-init through the connection pool that the rest of the app uses: 1. ConnectionPool.getChallengeConnection always called loadDatabaseProperties() inside computeIfAbsent. During first setup, database.properties does not exist yet, so this threw RuntimeException (uncaught by Setup.java's catch (SQLException)) and 500'd the setup screen. The challenge path already receives url/user/password as arguments and only uses the loaded properties for optional pool tuning (all with defaults). Add loadDatabasePropertiesIfPresent() that returns an empty Properties when the file is missing. 2. Setup.java's connection-test went through Database.getConnection -> ConnectionPool.getChallengeConnection, which keys pools by (url, username). If setup is re-run with the same url+user but a different password, the test reuses the stale pool, validates against the old password, and writes the new password to disk — leaving the app broken on next startup. Setup is a one-shot credential check that runs before the pool config exists; route it through DriverManager.getConnection directly and bypass the pool entirely. 3. Setup.executeSqlScript and Setup.executeUpdateScript borrowed Database.getDatabaseConnection(null, true) without closing. Pre-pooling each setup attempt opened a fresh JDBC connection that GC eventually reclaimed. Now those calls draw from the challenge pool (max 3) and never return — repeated setup/upgrade attempts exhaust it. Wrap both methods in try-with-resources. * fix: bound setup connection-test timeout to 5s Address PR #836 Copilot review: when the setup credential test moved from the pool to DriverManager.getConnection, it lost Hikari's connectionTimeout=5000ms and inherited DriverManager's default (no bound). A typo'd hostname or unreachable port at setup time would hang the servlet thread on the OS's default TCP connect timeout — 75s on Linux, 21s on macOS, sometimes much longer — making the setup page appear frozen with no error. Append connectTimeout=5000 to the test JDBC URL so the credential test fails fast at the same threshold the pool uses for production traffic. MariaDB and MySQL JDBC drivers both honor this option natively, scoped to just this connection — no JVM-wide DriverManager.setLoginTimeout side effects. * test: bootstrap DB props in DowngradeAdminsIT and DeletePlayersIT (#837) These two ITs called into Getter/Setter without first writing the test database.properties file. Pre-pool (#816) this worked because the application code read the properties file lazily on every call, and the test runtime arranged for the file to exist via other means. Post- pool, ConnectionPool.initialize() is called once per JVM on the first getConnection() and reads the props file exactly once; if it is missing, the pool is marked unusable for the entire JVM and every subsequent test in that fork throws "Connection pool not available". Symptom in CI: every test method in both classes FATAL'd with a long FileNotFoundException stack trace pointing at target/test-classes/conf/database.properties, drowning out any real leak signal in the IT log. Add the standard @BeforeAll bootstrap used by the other 42 IT files (createMysqlResource + ensureSchemaReady + reseedTestData) to both files. Keep the existing @beforeeach for per-test request/response reset. Verified locally: 8 tests pass across the two files (4 each); prior to this fix all 8 failed at pool init. * fix: return DB connections from Getter ResultSet methods (CachedRowSet) (#838) * fix: return DB connections from Getter ResultSet-returning methods Three methods returned live JDBC ResultSets while leaving their owning Connection and CallableStatement borrowed from the core pool: - Getter.getClassInfo(String) — all classes - Getter.getPlayersByClass(String, String) - Getter.getAdmins(String) The caller could not return them because Connection ownership was private to the method. Under load this slowly bled the core pool until CI hit "CorePool - Connection is not available, request timed out after 5005ms". Materialize the results into a CachedRowSet (javax.sql.rowset) inside try-with-resources on Connection / CallableStatement / ResultSet. The caller still receives a ResultSet-compatible object but DB resources are released immediately. Result sizes here are all bounded (one row per class / per admin / per player in a class) so the in-memory cost is negligible. CachedRowSet.populate() leaves the cursor after the last row, so call beforeFirst() before returning so callers that do `while (rs.next())` without a prior beforeFirst() see all rows. Two latent bugs surfaced by this change: 1. The original getPlayersByClass had `resultSet.next()` before returning — a one-row cursor advance. The only production caller (GetPlayersByClass.playersInOptionTags) does `beforeFirst()` then `while (next())` so it iterates all rows; the advance was masked. The IT (testGetPlayersByClass) didn't call beforeFirst() and so counted N-1 rows, which the test compensated for by creating 10 users and expecting 9. Production semantics is "all rows"; the test was wrong. Fix the test to expect 10/10. 2. The IT had `if (i != 9) { if (i < 9) ... else if (i > 9) ... else { fail("Then surely the number WAS 9? How did this happen") } }` — the else branch was unreachable. Removed. Verified: GetterCorePoolLeakIT, SetterCorePoolLeakIT, GetterIT#testGetPlayersByClass, testGetAdmins, testGetClassInfoString, and testGetClassInfoStringString all pass. * fix: cap CachedRowSet materialization at 10k rows Address PR #838 Copilot review feedback: CachedRowSet.populate() materializes the entire result set in memory, and the backing stored procs (classesGetData, playersByClass, playersWithoutClass, adminGetAll) have no LIMIT clause. Realistic Security Shepherd workloads are well under this bound (class rosters of tens to a few hundred), but a runaway query — accidental or malicious — could OOM the JVM. Add CallableStatement.setMaxRows(MAX_ROWSET_ROWS) where MAX_ROWSET_ROWS = 10_000 to fail fast on the pathological case while remaining well above realistic use. Proper bounded-collection conversion (return List<DTO> instead of ResultSet) is tracked in #839 as part of the broader DAO refactor scope. Verified: GetterCorePoolLeakIT, SetterCorePoolLeakIT, GetterIT#testGetPlayersByClass, testGetAdmins, testGetClassInfoString, testGetClassInfoStringString all pass. * fix: address CodeQL JDBC resource-leak flags + IT timezone fix (#842) * fix: nested try-with-resources for CodeQL-flagged JDBC sites + IT timezone fix Closes CodeQL "potential database resource leak" flags on 8 methods. Connection-level pooling fix from #816/#817/#834/#838 already solved the real-world leak. CodeQL was flagging the surrounding Statement / ResultSet declarations as not deterministically closed — with Hikari, they get closed when the Connection returns to the pool, so practical risk is low, but explicit nested try-with-resources matches the project convention and silences the flags. Methods converted: - Getter.checkPlayerResult - Getter.findPlayerById - Getter.getAllModuleInfo - Getter.getChallenges (CodeQL: lines 540, 544) - Getter.getModulesJson (CodeQL: lines 2099, 2102) - Getter.getAdminCheatStatus (CodeQL: lines 2360, 2364) - Setter.setAdminCheatStatus (CodeQL: line 1153) - Setter.setPlayerCheatStatus (CodeQL: line 1177) - Setter.setModuleLayout (CodeQL: line 1207) - Setter.setFeedbackStatus (CodeQL: line 1231) Not converted: Getter.authUserSSO. The CodeQL flags on lines 230, 241, 345, 357 in that method are not a simple rewrap — the method has two separate prepared-statement queries with reassignment of the same `prestmt` variable, plus a nested call to Setter.userCreateSSO that holds its own pool connection. Needs the same auth-hold-time phasing that #819 and #834 applied to authUser and updatePassword. Tracked in #840. Test additions (GetterIT): - testGetAdminCheatStatus exercises the converted getAdminCheatStatus with setAdminCheatStatus round-trips (true → false → restore). - testGetModulesJson exercises the converted getModulesJson and asserts the documented 2-element JSON envelope shape + per-module field population. Catches any regression in the new try-with-resources scope around the inner result-set loop. Timezone fix (pom.xml): - Three pre-existing IT failures (testSuspendUser, testSSOAuthSuspended, suspendedUserIsRejected) pass in CI but fail on non-UTC developer machines. Root cause: JDBC `getTimestamp()` interprets SQL DATETIME values in the JVM's default timezone. With DB in UTC and JVM in Europe/Dublin (UTC+1 in summer), the suspendedUntil read comes back as an epoch-ms value 1 hour earlier than the actual stored UTC instant, so `suspendedUntil.after(currentTime)` returns false and the suspension check silently passes — a real production bug for any non-UTC Tomcat deployment. Pin tests to UTC via <argLine>-Duser.timezone=UTC</argLine> on surefire and failsafe. Underlying production bug tracked in #841. Verification: - mvn test: 164/164 unit tests pass - mvn -Dit.test='GetterIT,GetterAuthIT,GetterCorePoolLeakIT,SetterIT, SetterCorePoolLeakIT' failsafe: 132 tests pass, 0 failures (vs 130 pre-change — the 2 new tests, plus the 3 previously-failing timezone-sensitive tests now green) - semgrep custom rule: all 8 CodeQL-flagged methods now clean - google-java-format: clean * fix: address Copilot review comments on PR #842 - getModulesJson: restore broad try/catch around JSON header construction so a JSONException/ResourceBundle failure is logged and returns partial output again, instead of propagating out as it did after the try-with-resources refactor (behavior-preserving fix) - testGetModulesJson: openAllModules opens only one category per call, so call it with both false and true to actually open all modules and keep the modules.length() assertion from being data-dependent - setModuleLayout: fix copy/pasted log strings that referenced player cheat settings instead of the module layout being changed * ci: pin all GitHub Actions to commit SHA and upgrade to latest releases (#844) Repo policy requires actions pinned to full-length commit SHAs. Pin every action in test.yml and release.yml, and bump each to its current stable release (notably upload-artifact v3→v7, checkout v2/v4→v6, setup-java→v5). * fix: try-with-resources for authUserSSO (#840) + Setter JDBC statements (#845) * fix: try-with-resources for authUserSSO (#840) + Setter JDBC statements Resolves the remaining CodeQL "potential database resource leak" flags on the dev#536 connection-pooling work (stacked on #816). Getter.authUserSSO (#840): - Split into three phases, each opening its own pooled Connection in try-with-resources. The connection is no longer held across the Setter.userCreateSSO call (Phase 2), reducing peak pool usage from 2 concurrent connections per call to 1 - same anti-pattern #834 fixed in Setter.updatePassword/updatePlayerResult. - All PreparedStatements and ResultSets are now in nested try-with-resources, clearing CodeQL flags at Getter.java:230/241/345/357. - Behavior preserved: existing-user, new-user, duplicate-username and suspension paths are unchanged. The known #820 result-array quirks (result[3] unset, result[5] double-assigned) are intentionally left as-is and remain tracked by #820. One intentional improvement: a SQLException while reading the Phase-1 result set now propagates instead of being silently treated as "user not found". Setter.java: - Convert 17 settings/admin methods from bare Connection try-with-resources with an inner un-closed PreparedStatement/CallableStatement to combined try-with-resources. - Structurally complex methods (reassigned statements, conditional branches, statement+ResultSet, challenge-connection sites) are deferred to a separate follow-up to keep this change reviewable. Tests: - Add GetterIT.testSSOAuthExistingUserRelogin covering the Phase 1 (found) -> skip create -> Phase 3 path. Existing SSO ITs cover the other phases. - Unit tests pass locally (164 run, 0 failures). ITs validated in CI. * fix: address review feedback + lint on #845 - Narrow Getter.authUserSSO Phase 1 SELECT to `suspendedUntil` only (Copilot): existence comes from ResultSet.next(); avoids fetching userPass and 5 other unused columns. - Fix "forefully" -> "forcefully" typo in authUserSSO (Copilot). - Wrap two long lines in testSSOAuthExistingUserRelogin to satisfy google-java-format (lint-java CI fix). * fix: try-with-resources for remaining Setter.java JDBC sites (#846) (#852) Wrap the remaining structural JDBC sites in Setter.java with try-with-resources so PreparedStatement/CallableStatement/ResultSet are deterministically closed on all paths. SQL and branching are unchanged; this only changes resource management. - Cat A (statement reassignment): openAllModules, openOnlyMobileCategories, userDelete - Cat B (statement + ResultSet): updatePlayerClass, updatePlayerClassToNull, updateUserRole - Cat C (manual .close()): setCsrfChallengeFourCsrfToken, setCsrfChallengeSevenCsrfToken - Settings: setModuleCategoryStatusOpen, setLockTime, setEndTimeStatus, setEndTime Addresses the "Potential database resource leak" code-scanning flags in Setter.java (also surfaced on #816). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> * refactor: try-with-resources for remaining Getter.java JDBC sites (#855) (#856) * refactor: try-with-resources for Getter.java JDBC sites (batch 1-2, #855) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor: try-with-resources for remaining Getter.java JDBC sites (#855) Wrap all remaining PreparedStatement/CallableStatement/ResultSet sites in Getter.java with try-with-resources so JDBC resources are deterministically closed on every path. SQL strings, parameter bindings, and control flow are unchanged (verified: SQL literals byte-identical, 58 param-binding calls before and after). Notable cases: - getJsonScore: statement is conditionally assigned (totalScoreboard vs classScoreboard), so it cannot go in the Connection TWR header; registered as a closeable resource via `try (CallableStatement x = callstmnt; ResultSet rs = callstmnt.executeQuery())`. - isModuleOpen: converted statement + ResultSet to nested TWR, removed the now-redundant manual rs.close(). Completes the JDBC resource-leak cleanup for the dbProcs layer alongside Setter.java (#846 / PR #852). Addresses the recurring code-scanning "Potential database resource leak" flags on #816. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: SeanDuggan <sean.duggan4@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Closes #855.
What
Wraps all remaining
PreparedStatement/CallableStatement/ResultSetsites insrc/main/java/dbProcs/Getter.javawith try-with-resources, so JDBC resources are deterministically closed on every path (success, early return, exception). This is theGetter.javacounterpart to theSetter.javacleanup (#846 / PR #852) and the SSO-path work (#840 / PR #845).44 methods total were in scope (per the #855 inventory); methods already using TWR or returning a
CachedRowSetwith the live query already wrapped were left untouched.Why
Ends the recurring "Potential database resource leak" code-scanning comments that kept resurfacing on #816 — the bot only flags lines in each push's diff, so partial fixes never made it stop.
Getter.javawas the last large offender. With this, thedbProcslayer is fully converted.Behavior preservation
No SQL, parameters, or control flow changed. Verified mechanically:
prepareCall/prepareStatementSQL literals are byte-identical before and after.setString/setInt/setMaxRows/…): 58 before, 58 after..close()calls and no body-levelexecuteQuery()outside a TWR block.Cases warranting review
getJsonScore— statement is conditionally assigned (totalScoreboard()vsclassScoreboard(?)), so it can't go in the Connection TWR header. Registered as a closeable resource:try (CallableStatement x = callstmnt; ResultSet rs = callstmnt.executeQuery()) { ... }.isModuleOpen— converted statement + ResultSet to nested TWR; removed the now-redundant manualrs.close().log.debug(...)(referencing only params/constants) moved just above thetryso the statement could be hoisted into the TWR header — logged in the same order as before.Testing
mvn -o compile✅ andmvn -o spotless:check✅ (under JDK 17, matching CI'slint-java).GetterIT, challenge tests) require a DB — please validate via theworkflow_dispatchBuild and Test run (dispatched on this branch).🤖 Generated with Claude Code