fix(database): Drop a closed connection kept by DefaultPrepAndExpecte… - #963
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideFix reused DefaultPrepAndExpectedTestCase instances that retain a closed connection by checking the cached JDBC connection with isClosed(), discarding it when necessary, and reacquiring through the configured tester/provider; integration tests cover both stable reuse and replacement after connection loss. Sequence diagram for replacing a closed reusable database connectionsequenceDiagram
participant TestCase as DefaultPrepAndExpectedTestCase
participant Tester as databaseTester
participant Provider as CachingConnectionProvider
participant JDBC as JDBCConnection
TestCase->>TestCase: getReusableConnection()
TestCase->>JDBC: isClosed()
alt cached connection is closed
TestCase->>Tester: getConnection()
Tester->>Provider: getConnection()
Provider-->>Tester: live replacement
Tester-->>TestCase: replacement connection
else cached connection is open
TestCase-->>TestCase: reuse cached connection
end
TestCase->>TestCase: closeReusableConnection()
Note over TestCase: With closeConnectionAfterTest=false, the open connection remains cached
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates ChangesConnection reuse handling
Estimated code review effort: 2 (Simple) | ~15 minutes Merge Risk: ⚪ Minimal · up to The change replaces explicitly closed cached test connections while preserving reuse of live connections, preventing repeated failures in subsequent database tests. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 21.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java" line_range="340" />
<code_context>
*/
package org.dbunit;
</code_context>
<issue_to_address>
**nitpick:** The new private method is documented with `@since 3.6.0`, while the changelog places this fix in the 3.5.2 release. Generated API/source documentation therefore reports the method as introduced in a release that does not contain this change.
**Suggested fix:** Change the tag to the release containing this fix, or omit `@since` for this private helper.
</issue_to_address>Sourcery assessment
Approved.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java" line_range="345" />
<code_context>
+ {
+ try
+ {
+ return connection.getConnection().isClosed();
+ } catch (final SQLException e)
+ {
</code_context>
<issue_to_address>
**issue (bug_risk):** When the database or pool invalidates the connection without the JDBC driver changing `isClosed()` to true, `isReusableConnectionClosed()` returns false and the dead cached connection is returned indefinitely. The subsequent statement fails, but the cached field is not cleared and `CachingConnectionProvider` is never consulted to validate or replace it.
**Triggers:** When a server-side disconnect is not reflected by the driver's local `isClosed()` flag.
**Suggested fix:** Either invalidate and reacquire the cached connection when a lifecycle operation fails, or narrow the documentation and test expectations to connections whose local closed flag is updated.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: src/main/java/org/dbunit/DefaultPrepAndExpectedTestCase.java:345
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| { | ||
| try | ||
| { | ||
| return connection.getConnection().isClosed(); |
There was a problem hiding this comment.
issue (bug_risk): When the database or pool invalidates the connection without the JDBC driver changing isClosed() to true, isReusableConnectionClosed() returns false and the dead cached connection is returned indefinitely. The subsequent statement fails, but the cached field is not cleared and CachingConnectionProvider is never consulted to validate or replace it.
Triggers: When a server-side disconnect is not reflected by the driver's local isClosed() flag.
Suggested fix: Either invalidate and reacquire the cached connection when a lifecycle operation fails, or narrow the documentation and test expectations to connections whose local closed flag is updated.
The 3.5.1 release shipped from this branch; development toward the 3.5.2 patch continues here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q77iHoBhzZxJCQzkC8f82f
…dTestCase DefaultPrepAndExpectedTestCase with setCloseConnectionAfterTest(false), reused across test methods (a base-class or shared static field paired with a CachingConnectionProvider), pinned the first IDatabaseConnection it acquired in a field and never re-checked it. closeReusableConnection() deliberately keeps that field set - not closing it - when closeConnectionAfterTest is false, with no check that the kept connection is still open. So once the pool or the database dropped that connection between test methods (max-lifetime/reap, a PGConnectionPoolDataSource issuing a fresh logical handle, a bounced application context, an idle-in-transaction kill), every following test on the instance failed on it: in setupData()'s CLEAN_INSERT, then again, suppressed, in cleanupData()'s tear down operation. CachingConnectionProvider's own liveness check, which exists to replace a dead connection transparently, was never reached, because the provider is not consulted again once the field is set. * getReusableConnection() discards the cached connection when closeConnectionAfterTest is false and it reports isClosed(), so the next call re-acquires from the tester and a CachingConnectionProvider behind it hands back a live replacement. isClosed() only - cheap, no round trip, and it does not break the "acquire once, no provider" contract a round-tripping isValid() would. * closeReusableConnectionSuppressing(), only ever called from a lifecycle step that has already failed, now also forgets the connection even when closeConnectionAfterTest is false - so a server-side disconnect the driver reported only by throwing (not by flipping isClosed()) still lets the next test re-acquire, rather than depending on the driver's local closed flag. * The default closeConnectionAfterTest=true path is unchanged. * Add two DatabaseTesterConnectionReuseIT cases: one proving a reused instance keeps reusing its one open connection while it stays alive, one proving it replaces the connection after it is closed between test methods instead of failing every following test on the dead one. Refs: 962 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q77iHoBhzZxJCQzkC8f82f
8b9b0ab to
b0a9ff3
Compare
Sourcery withdrew this approval because the latest commits introduced blocking findings.
1 participant
Fixes #962. Targets
releases/3.5.xfor a 3.5.2 patch release.DefaultPrepAndExpectedTestCasewithsetCloseConnectionAfterTest(false), reused across test methods (a base-class or shared static field paired with aCachingConnectionProvider), pinned the firstIDatabaseConnectionit acquired in a field and never re-checked it.closeReusableConnection()deliberately keeps that field set — not closing it — whencloseConnectionAfterTestis false, with no check that the kept connection is still open. So once the pool or the database dropped that connection between test methods (max-lifetime/reap, aPGConnectionPoolDataSourceissuing a fresh logical handle, a bounced application context, an idle-in-transaction kill), every following test on the instance failed on it: insetupData()'sCLEAN_INSERT, then again (suppressed) incleanupData()'s tear down.CachingConnectionProvider's own liveness check, which exists to replace a dead connection transparently, was never reached because the provider is not consulted again once the field is set.getReusableConnection()discards the cached connection whencloseConnectionAfterTestis false and it reportsisClosed(), so the next call re-acquires from the tester.isClosed()only — cheap, no round trip, and it doesn't break the "acquire once, no provider" contract a round-trippingisValid()would.closeReusableConnectionSuppressing()(only ever called from an already-failed lifecycle step) now also forgets the connection even whencloseConnectionAfterTestis false — so a server-side disconnect the driver reported only by throwing still lets the next test re-acquire, not just one the driver's localisClosed()flag catches.closeConnectionAfterTest=truepath unchanged.DatabaseTesterConnectionReuseITcases: stable reuse while alive, and replacement after a between-tests close.Present since 3.4.0 (
setCloseConnectionAfterTest). Also affectsmain(3.6.0), where it additionally surfaces in the row count check's baseline capture — a separate forward-port PR will follow.Summary by Sourcery
Ensure reused database test cases recover transparently when their cached connection becomes unavailable.
Bug Fixes:
Build:
Tests: