This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 83686b0ac5db03891ae5dd40031cb0f670cc725b Author: opencode <[email protected]> AuthorDate: Fri Oct 23 17:54:40 2026 +0200 Harvest the legacy jdbc-pool suite's scenarios into the new suite The first step of retiring the JUnit 4 suite: keep the regression memory it encodes, expressed against the real database instead of the hand-written stub driver. The legacy code itself is still in place and will go in its own commit. - AsyncBorrowTest: async borrow accounting stays consistent with the database, and an async borrow from an empty fair pool establishes the first connection instead of timing out (Bug50805, Bug50477). - InterruptedWaiterTest: a waiter interrupted while blocked on an empty pool neither takes a permit nor starves the next borrower (Bug53367). - ValidationTest: a failing validation query fails the borrow and leaks nothing, a dead idle connection is replaced without error, a dead connection is released on return, validationQueryTimeout actually cancels, a custom Validator replaces the query, the validation interval suppresses repeats, and validation commits an open transaction. - ConnectionSetupTest: initSQL reaches every physical session, a blank initSQL is not configured at all, initSQL doubles as connect validation, and an unusable configured isolation level does not block the pool (Bug54225, Bug54978, Bug50571). - PurgeTest: purge() and purgeOnReturn() start a new pool generation; sessions established before it are closed on return instead of being handed to the next borrower, and the pool keeps serving afterwards. - ProxyContractTest: a closed facade stays closed, invalid and unusable while its session works for the next borrower; closing twice returns once; each borrow has its own identity; the physical session stays reachable through the PooledConnection interface. - StatementLifecycleTest: dangling statements are closed with their connection, and a cached statement arrives at the next borrower with no queued batch, no closeOnCompletion and no inherited queryTimeout, maxRows or fetchSize. - SessionStateTest: the configured auto commit, isolation level and read-only flag apply to every borrower and are restored for the next one, verified by what an admin connection can see. - AlternateCredentialsTest: a borrow is authenticated as exactly the user it asked for (read back with CURRENT_USER), a session is never served to a borrower of another identity, wrong credentials are refused by the database, and mixed identities under concurrency always get their own user. - InterceptorLifecycleTest: an interceptor named by short name is resolved, configured from the definition string, wired into the chain of every borrow, told about pool start and stop, and cleans its statistics up after the pool closes. - JmxMonitorTest: the JMX view of a pool never hands out the password. - config package: the pure-unit cases ported as they are, with PoolPropertiesTest now also checking that the configuration dump masks the password and ShouldForceReconnectTest expanded to a parameterised case per credential combination. PoolTestSupport now pins the in-memory database's lifetime to the server with SET DB_CLOSE_DELAY -1. Without it, the pool closing its last physical session - which eviction, purge() and an alternate-credential reconnect all do - destroys the database, and the next connection silently gets an empty one, so assertions about committed data could pass against a database that was never the one the test wrote to. TESTING-PLAN.md records the scenario map, the cases dropped with reasons (a GC-triggered close cannot be gated; H2 cannot witness maxFieldSize, fetch direction, isPoolable or driver warnings) and the behaviours left unpinned as unintended-looking. 75 new tests; the module is green at 319 tests, at 66.1% instruction / 61.3% branch coverage. --- modules/jdbc-pool/TESTING-PLAN.md | 141 +++++++++-- .../config/JdbcInterceptorConfigParsingTest.java | 172 +++++++++++++ .../tomcat/jdbc/config/PoolPropertiesTest.java | 98 ++++++++ .../jdbc/config/ShouldForceReconnectTest.java | 169 +++++++++++++ .../jdbc/config/SlowQueryComparatorTest.java | 108 ++++++++ .../jdbc/functional/AlternateCredentialsTest.java | 217 +++++++++++++++++ .../tomcat/jdbc/functional/AsyncBorrowTest.java | 158 ++++++++++++ .../jdbc/functional/ConnectionSetupTest.java | 141 +++++++++++ .../jdbc/functional/InterceptorLifecycleTest.java | 136 +++++++++++ .../jdbc/functional/InterruptedWaiterTest.java | 151 ++++++++++++ .../tomcat/jdbc/functional/JmxMonitorTest.java | 38 +++ .../tomcat/jdbc/functional/PoolTestSupport.java | 13 + .../tomcat/jdbc/functional/ProxyContractTest.java | 140 +++++++++++ .../apache/tomcat/jdbc/functional/PurgeTest.java | 114 +++++++++ .../tomcat/jdbc/functional/SessionStateTest.java | 135 ++++++++++ .../jdbc/functional/StatementLifecycleTest.java | 253 +++++++++++++++++++ .../tomcat/jdbc/functional/ValidationTest.java | 271 +++++++++++++++++++++ 17 files changed, 2438 insertions(+), 17 deletions(-) diff --git a/modules/jdbc-pool/TESTING-PLAN.md b/modules/jdbc-pool/TESTING-PLAN.md index 8b88b6202e..3f4c1e5322 100644 --- a/modules/jdbc-pool/TESTING-PLAN.md +++ b/modules/jdbc-pool/TESTING-PLAN.md @@ -41,6 +41,59 @@ regressions; scenario, not code, must survive: Benchmarks, if wanted later, go to a JMH profile; they never gate CI. The old suite is preserved in git history only. +### Harvest result (done) + +Scenario → new class, as harvested. Where the legacy test asked a fake +driver what it remembered, the new test asks the database. + +| Legacy scenario | New home | +|---|---| +| `Async0IdleTestBug50477`, `Bug50805`, `TestAsyncQueue` (async borrow must not strand a waiter, accounting stays honest) | `functional/AsyncBorrowTest` (4) | +| `Bug53367`, `FairnessTest`/`StarvationTest` *interrupt* case (an interrupted waiter must not eat a permit) | `functional/InterruptedWaiterTest` (2) | +| `TestValidation`, `TestValidationQueryTimeout`, `TestTimeout` (validation query, timeout, `testOnBorrow`/`Return`/`WhileIdle`, `isValid()`, custom `Validator`) | `functional/ValidationTest` (9) | +| `TestGetConnection`, `TwoDataSources`, `Bug50571`, `Bug54225`, `Bug54978` (connect-time setup: `initSQL`, blank `initSQL`, `initSQL` as connect validation, unusable configured isolation, `dbProperties`, side-door close detected) | `functional/ConnectionSetupTest` (6) | +| `PoolPurgeTest` | `functional/PurgeTest` (2) — re-encoded to the real generational semantics: `purge()`/`purgeOnReturn()` both start a new pool generation, a pre-generation session is closed on return, `purge()` also closes the ones idle right now | +| `MultipleCloseTest`, `EqualsHashCodeTest` (facade identity path) | `functional/ProxyContractTest` (4) | +| `StatementFinalizerTest`, `TestStatementCache`, `ProxiedResultSetTest`/`ProxiedStatementTest` back-references | `functional/StatementLifecycleTest` (7) | +| `TestConnectionState` (hygiene half; the leak half stays defect #3 backlog) | `functional/SessionStateTest` (3) | +| `AlternateUsernameTest` (now a real test: H2 users, identity read back with `CURRENT_USER`) | `functional/AlternateCredentialsTest` (6) | +| `TestInterceptorShortName`, `TestSlowQueryReport` lifecycle/statistics half | `functional/InterceptorLifecycleTest` (3) | +| `JmxPasswordTest` (documented `getPassword()` redaction only) | `functional/JmxMonitorTest` (+1, 5 total) | +| `PoolPropertiesTest` (config package: `toString` bracket balance **plus** password masking) | `config/PoolPropertiesTest` (2) | +| `TestJdbcInterceptorConfigParsing` | `config/JdbcInterceptorConfigParsingTest` (4) | +| `TestSlowQueryComparator` | `config/SlowQueryComparatorTest` (5) | +| `ShouldForceReconnectTest` (truth table, expanded to a parameterised case per credential combination) | `config/ShouldForceReconnectTest` (17) | + +Deliberately dropped, with reasons: + +* `TestGCClose` — a finalizer-on-GC scenario cannot be gated (no + deterministic trigger). The hazard it gestured at (a stale proxy + driving somebody else's session) is a High catalog finding and is + pinned at the facade level by `ProxyContractTest`. +* `TestQueryTimeoutInterceptor`, `TestValidation`'s "how many times did + the driver get asked" cases — the first is already covered by + `InterceptorSemanticsTest`; the second counted stub-driver calls and + is superseded by session-count and outcome oracles. +* `TestSlowQueryReport`'s JMX-notification counting and `bugs/Bug51582` + (`SlowQueryReportJmx`) — High defect #7, §4 backlog. +* `bugs/Bug54227` (a session older than `maxAge` must be replaced) was + already covered before the harvest, by + `PoolEvictionTest#maxAgeRetiresAnOldConnectionBetweenBorrowers`. +* StatementCache hygiene cases H2 cannot observe: `maxFieldSize` + (always reports 0), `getFetchDirection` (always `FETCH_FORWARD`), + `isPoolable()` (always false — the interceptor tracks it itself) and + driver-generated warnings (H2 raises none on `executeUpdate`). The + observable half — batch, `closeOnCompletion`, `queryTimeout`, + `maxRows`, `fetchSize` — is in `StatementLifecycleTest`. +* Everything else per the disposition table above (benchmarks, DBCP + comparisons, sleep-driven timing, stub plumbing, examples). + +Two observations made *while* harvesting that are consciously **not** +pinned (they look unintended; see §6): a failed alternate-credential +borrow discards a healthy idle session (the pool shrinks to 0 sessions +instead of keeping the one it had), and `ProxyConnection`'s own +`equals`/`hashCode` below the disposable facade. + ## 2. Database choice (unchanged conclusion) Two tiers, H2 first: @@ -94,12 +147,41 @@ JUnit 5, three groups: `lockCount` property); if it is kept, it needs direct unit tests in the later phase — not in the functional baseline. +Layout: `org.apache.tomcat.jdbc.functional` (behaviour against the real +database, every class extending the fixture), +`org.apache.tomcat.jdbc.config` (the pure-unit configuration cases +ported from the old suite — no pool, no database), and, until it is +deleted, the legacy JUnit 4 suite in `org.apache.tomcat.jdbc.test` +under the vintage engine. + Infrastructure: H2-TCP fixture (per-class server + fresh DB name); state-hygiene extension (every `DataSource` closed, pool statics and server session count back to baseline after each test — this kills the `PoolCleanerTest` flake class permanently); Awaitility instead of sleeps; `-Pdocker` profile for the Postgres tier. +Fixture details worth knowing before writing a test against it +(`functional/PoolTestSupport`): + +* The database is bootstrapped by an admin connection with + `SET DB_CLOSE_DELAY -1`. Without it an in-memory database is + destroyed when the pool closes its last physical session — eviction, + `purge()` and an alternate-credential reconnect all do that — and the + next connection silently gets an *empty* database, which can make an + assertion about committed data pass against a database that was never + the one the test wrote to. Putting it in the URL is not an option: + every client re-applies URL settings and a non-admin user then fails + login with "Admin rights are required for this operation". +* Oracles are `poolSessions()` (the database's own + `INFORMATION_SCHEMA.SESSIONS` count minus the admin session), + `setSessionMarker`/`sessionMarker` (an H2 session variable, to prove + *which* physical session a borrower got) and `adminRowCount` + (committed data, read over a separate admin connection). +* Fault injection goes through the real objects: + `((javax.sql.PooledConnection) pooled).getConnection()` for the + physical session, `(JdbcInterceptor) Proxy.getInvocationHandler(pooled)` + to walk the interceptor chain. + ## 4. High-defect map (backlog for the later defect-processing phase — not implemented now) | Defect (defects-summary.md #) | Reproducer design | @@ -135,7 +217,7 @@ the working list. execution, transactions, sizing/eviction/lifecycle first; then validation variants, fairness queues, interceptor chain semantics, JMX with live traffic, XA, JNDI factories. - **Done except the validation variants**: `InterceptorSemanticsTest` + **Done**: the phase 1 classes plus `InterceptorSemanticsTest` (slow/failed query reporting, `QueryTimeoutInterceptor` cancellation, `StatementCache`, `StatementDecoratorInterceptor`), `JmxMonitorTest` (pool + per-connection MBeans with live @@ -143,21 +225,23 @@ the working list. against H2's XA resource), `NamingFactoryTest` (`DataSourceFactory`, `GenericNamingResourcesFactory`) and `FairBlockingQueueTest` (most-recently-used hand-off, blocking - retrieval, waiter fairness, documented-unsupported operations) - join the phase 1 classes: 59 functional tests, ~72 s, full module - green at 244 tests. Coverage of the module is now 64.8 % - instruction / 58.9 % branch. - Still open here: the validation variants (`testOnBorrow`, - `testOnReturn`, `testWhileIdle`; query vs `isValid()` vs a custom - `Validator`) — cheapest to write next to the defects that touch - them. `InterceptorSemanticsTest` alone costs ~60 s: real timeout + retrieval, waiter fairness, documented-unsupported operations). + The validation variants (`testOnBorrow`, `testOnReturn`, + `testWhileIdle`; query vs `isValid()` vs a custom `Validator`, + `validationQueryTimeout`) arrived with the harvest as + `ValidationTest`. + `InterceptorSemanticsTest` alone costs ~60 s: real timeout cancellation needs a real multi-second query (H2 has no `SLEEP()` and an in-process `CREATE ALIAS` is not cancel-safe), so the "< 60 s" budget is understood to exclude it. -3. **Old suite removal**: benchmarks → optional JMH profile; - harvest the remaining regression scenarios (`Bug50571`, - `Bug50805`, `Bug50477`) as spec tests; delete `driver/`, - `DefaultTestCase` DBCP plumbing, DBCP test dependency. +3. **Old suite removal**: harvest first (§1 "Harvest result" — + **done**: 75 tests in 12 new classes plus one added + `JmxMonitorTest` case, module green at 319 tests, 66.1 % + instruction / 61.3 % branch), then delete the suite, its stub + driver, the DBCP-comparison plumbing and the `tomcat-dbcp`, + JUnit 4 and vintage-engine test dependencies along with the + `PoolCleanerTest` exclude. Benchmarks, if wanted later, move to a + JMH profile; they never gate CI. 4. **Defect processing** (later): per defect, a reproducer test per §4 + the fix; Postgres/kill tier added when the first defect needs it (single-session kill, `idle_session_timeout`, @@ -168,10 +252,10 @@ the working list. ## 6. Open points -* `defects-pool/` is currently untracked. The reproducer tests cite - it by name, so the catalog should be committed alongside this - suite (suggested home: `modules/jdbc-pool/docs/defects/`). Needs a - decision (it's outside this module, in the repo root). +* `defects-pool/` is deliberately left **untracked** for now (owner's + call, and it stays out of the harvest commits). The reproducer tests + of phase 4 cite it by name, so it will have to be committed before or + with them (suggested home: `modules/jdbc-pool/docs/defects/`). * Whether defect reproducers should also assert *log output* (many findings are "failed silently"): yes where a fix prescribes a log line; use a juli capture appender in the fixture. @@ -189,3 +273,26 @@ the working list. `...,connections=PooledConnection[n]` MBean — a pool closed while connections are borrowed leaves those MBeans in the platform server. +* Found while harvesting, same status (unintended-looking, so not + pinned): + * `ProxyConnection`'s own `equals`/`hashCode`, reachable only with + `useDisposableConnectionFacade=false`, throw `NullPointerException` + because they are forwarded to the underlying connection through a + path that resolves `getXAConnection()` to `null` first. Any + `HashSet<Connection>` of pooled connections breaks. Already High + findings in `defects-pool/defects-...ProxyConnection.md` and + `...DisposableConnectionFacade.md`; the baseline therefore pins + only the identity semantics *at the facade* + (`ProxyContractTest`). + * a borrow with alternate credentials that the database refuses + leaves the pool having *closed the healthy idle session it + already had*: size and server session count drop to 0 and the + next ordinary borrow has to reconnect. The pool stays usable + (`AlternateCredentialsTest` pins that much), but the shrinkage is + not something to lock in as expected behaviour. +* Statement-hygiene cases that this database cannot witness at all, so + no amount of test code would pin them: `setMaxFieldSize` (H2 always + reports 0), `setFetchDirection` (always `FETCH_FORWARD`), + `isPoolable()` (always `false`, which is why `StatementCache` tracks + the hint itself) and driver-generated warnings (none on + `executeUpdate`). A Postgres tier could observe the first two. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/JdbcInterceptorConfigParsingTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/JdbcInterceptorConfigParsingTest.java new file mode 100644 index 0000000000..39134e97bd --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/JdbcInterceptorConfigParsingTest.java @@ -0,0 +1,172 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.PoolProperties; +import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorDefinition; +import org.apache.tomcat.jdbc.pool.PoolProperties.InterceptorProperty; +import org.apache.tomcat.jdbc.pool.TrapException; + +/** + * Configuration only, no database and no pool: how the + * {@code jdbcInterceptors} string is turned into interceptor definitions. + * The order matters (the trap interceptor is always first) and so does + * whitespace handling, because the string is what ends up in a resource + * definition. + * + * <p>Ported from the retired legacy + * {@code TestJdbcInterceptorConfigParsing} (added for bug 54395).</p> + */ +class JdbcInterceptorConfigParsingTest { + + @Test + void basicNamesAndPropertiesAreParsed() { + String interceptorConfig = "FirstInterceptor;SecondInterceptor(parm1=value1,parm2=value2)"; + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(interceptorConfig); + InterceptorDefinition[] interceptorDefs = props.getJdbcInterceptorsAsArray(); + assertNotNull(interceptorDefs); + + // 3 items because parser automatically inserts TrapException interceptor to front of list + assertEquals(3, interceptorDefs.length); + assertEquals(TrapException.class.getName(), interceptorDefs[0].getClassName()); + + assertNotNull(interceptorDefs[1]); + assertEquals("FirstInterceptor", interceptorDefs[1].getClassName()); + assertNotNull(interceptorDefs[2]); + assertEquals("SecondInterceptor", interceptorDefs[2].getClassName()); + + Map<String, InterceptorProperty> secondProps = interceptorDefs[2].getProperties(); + assertNotNull(secondProps); + assertEquals(2, secondProps.size()); + assertNotNull(secondProps.get("parm1")); + assertEquals("value1", secondProps.get("parm1").getValue()); + assertNotNull(secondProps.get("parm2")); + assertEquals("value2", secondProps.get("parm2").getValue()); + } + + @Test + void whitespaceAndNewlinesAroundNamesAndPropertiesAreIgnored() { + String interceptorConfig = "FirstInterceptor ; \n" + + "SecondInterceptor (parm1 = value1 , parm2= value2 ) ; \n\n" + + "\t org.cyb.ThirdInterceptor(parm1=value1); \n" + + "EmptyParmValInterceptor(parm1= )"; + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(interceptorConfig); + InterceptorDefinition[] interceptorDefs = props.getJdbcInterceptorsAsArray(); + assertNotNull(interceptorDefs); + + // 5 items because parser automatically inserts TrapException interceptor to front of list + assertEquals(5, interceptorDefs.length); + assertEquals(TrapException.class.getName(), interceptorDefs[0].getClassName()); + + assertNotNull(interceptorDefs[1]); + assertEquals("FirstInterceptor", interceptorDefs[1].getClassName()); + assertNotNull(interceptorDefs[2]); + assertEquals("SecondInterceptor", interceptorDefs[2].getClassName()); + assertNotNull(interceptorDefs[3]); + assertEquals("org.cyb.ThirdInterceptor", interceptorDefs[3].getClassName()); + + Map<String, InterceptorProperty> secondProps = interceptorDefs[2].getProperties(); + assertNotNull(secondProps); + assertEquals(2, secondProps.size()); + assertNotNull(secondProps.get("parm1")); + assertEquals("value1", secondProps.get("parm1").getValue()); + assertNotNull(secondProps.get("parm2")); + // Bug 54395: the value kept its leading space + assertEquals("value2", secondProps.get("parm2").getValue()); + + Map<String, InterceptorProperty> thirdProps = interceptorDefs[3].getProperties(); + assertNotNull(thirdProps); + assertEquals(1, thirdProps.size()); + assertNotNull(thirdProps.get("parm1")); + assertEquals("value1", thirdProps.get("parm1").getValue()); + + Map<String, InterceptorProperty> emptyParmValProps = interceptorDefs[4].getProperties(); + assertNotNull(emptyParmValProps); + assertEquals(1, emptyParmValProps.size()); + assertNotNull(emptyParmValProps.get("parm1")); + assertEquals("", emptyParmValProps.get("parm1").getValue()); + } + + /* + * Some of these should probably be handled more cleanly by the parser, but a few known + * exception scenarios are presented here just to document current behavior. In many cases + * failure in parsing will just be propagated to a definition that will fail later + * when instantiated. Should we be failing faster (and with more detail)? + */ + @Test + void someMalformedConfigsFailAtParsingAndSomeOnlyLater() { + String[] exceptionInducingConfigs = { + "EmptyParmsInterceptor()", + "WhitespaceParmsInterceptor( )" + }; + for (String badConfig : exceptionInducingConfigs) { + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(badConfig); + assertThrows(Exception.class, props::getJdbcInterceptorsAsArray, + "expected a failure for " + badConfig); + } + + String[] noExceptionButInvalidConfigs = { + "MalformedParmsInterceptor(= )", + "MalformedParmsInterceptor( =)", + "MalformedParmsInterceptor(", + "MalformedParmsInterceptor( ", + "MalformedParmsInterceptor)", + "MalformedParmsInterceptor) ", + "MalformedParmsInterceptor )" + }; + for (String badConfig : noExceptionButInvalidConfigs) { + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(badConfig); + props.getJdbcInterceptorsAsArray(); + } + } + + @Test + void extraSemicolonsProduceAnExtraDefinitionOnlyInSomePositions() { + // This one DOES get an extra/empty definition + PoolProperties props = new PoolProperties(); + props.setJdbcInterceptors(";EmptyLeadingSemiInterceptor"); + InterceptorDefinition[] jiDefs = props.getJdbcInterceptorsAsArray(); + assertNotNull(jiDefs); + assertEquals(3, jiDefs.length); + + // This one does NOT get an extra/empty definition (no trailing whitespace) + props = new PoolProperties(); + props.setJdbcInterceptors("EmptyTrailingSemiInterceptor;"); + jiDefs = props.getJdbcInterceptorsAsArray(); + assertNotNull(jiDefs); + assertEquals(2, jiDefs.length); + + // This one DOES get an extra/empty definition (with trailing whitespace) + props = new PoolProperties(); + props.setJdbcInterceptors("EmptyTrailingSemiInterceptor; "); + jiDefs = props.getJdbcInterceptorsAsArray(); + assertNotNull(jiDefs); + assertEquals(3, jiDefs.length); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/PoolPropertiesTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/PoolPropertiesTest.java new file mode 100644 index 0000000000..59392d1e01 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/PoolPropertiesTest.java @@ -0,0 +1,98 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.PoolProperties; + +/** + * Configuration only, no database and no pool: {@code PoolProperties} + * renders itself for logging and diagnostics. The rendering must not leak + * the password, and it must stay parseable - the legacy suite checked that + * with a bracket balance, because a truncated or unbalanced dump is what + * a broken {@code toString()} produces. + * + * <p>Ported from the retired legacy {@code PoolPropertiesTest}.</p> + */ +class PoolPropertiesTest { + + private static final String DEFAULT_USER = "username_def"; + private static final String DEFAULT_PASSWD = "password_def"; + + @Test + void toStringOutputShouldHaveBalancedBrackets() { + PoolProperties properties = new PoolProperties(); + properties.setUsername(DEFAULT_USER); + properties.setPassword(DEFAULT_PASSWD); + properties.setAlternateUsernameAllowed(true); + properties.setInitialSize(0); + properties.setRemoveAbandoned(false); + properties.setTimeBetweenEvictionRunsMillis(-1); + + String asString = properties.toString(); + + List<Character> stack = new ArrayList<>(); + for (char c : asString.toCharArray()) { + switch (c) { + case '{': + case '(': + case '[': + stack.add(Character.valueOf(c)); + break; + case '}': + assertEquals('{', stack.remove(stack.size() - 1).charValue()); + break; + case ')': + assertEquals('(', stack.remove(stack.size() - 1).charValue()); + break; + case ']': + assertEquals('[', stack.remove(stack.size() - 1).charValue()); + break; + default: + break; + } + } + assertEquals(0, stack.size(), "All brackets should have been closed"); + } + + @Test + void toStringMasksThePasswordButStillShowsTheConfiguration() { + PoolProperties properties = new PoolProperties(); + properties.setUsername(DEFAULT_USER); + properties.setPassword(DEFAULT_PASSWD); + properties.setUrl("jdbc:h2:mem:shown"); + + String asString = properties.toString(); + + assertTrue(asString.contains("password=********"), + "the password must be masked in the dump, was: " + asString); + assertFalse(asString.contains(DEFAULT_PASSWD), + "the cleartext password must not appear anywhere in the dump"); + assertTrue(asString.contains(DEFAULT_USER), + "the non-secret configuration must still be readable, was: " + asString); + assertTrue(asString.contains("jdbc:h2:mem:shown"), + "the JDBC URL must stay readable, was: " + asString); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/ShouldForceReconnectTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/ShouldForceReconnectTest.java new file mode 100644 index 0000000000..8cf1e90104 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/ShouldForceReconnectTest.java @@ -0,0 +1,169 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.config; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.stream.Stream; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import org.apache.tomcat.jdbc.pool.ConnectionPool; +import org.apache.tomcat.jdbc.pool.PoolProperties; +import org.apache.tomcat.jdbc.pool.PooledConnection; + +/** + * The credential half of the pool's decision "must this idle session be + * re-established for the borrower that asks for it?", checked in + * isolation. A pooled session remembers the identity it logged in as, a + * {@code null} in a request means the pool's default identity, and the + * operation both decides <em>and</em> records what was asked - so every + * case below starts from an explicitly configured session. + * + * <p>Ported from the retired legacy {@code ShouldForceReconnectTest}. The + * {@code maxAge} half of the same decision is covered against a real + * database by {@code PoolEvictionTest#maxAgeRetiresAnOldConnectionBetweenBorrowers}.</p> + */ +class ShouldForceReconnectTest { + + private static final String DEFAULT_USER = "username_def"; + private static final String DEFAULT_PASSWD = "password_def"; + private static final String ALT_USER = "username_alt"; + private static final String ALT_PASSWD = "password_alt"; + + private PoolProperties properties; + + @BeforeEach + void setUp() { + properties = new PoolProperties(); + properties.setUsername(DEFAULT_USER); + properties.setPassword(DEFAULT_PASSWD); + properties.setAlternateUsernameAllowed(true); + properties.setInitialSize(0); + properties.setRemoveAbandoned(false); + properties.setTimeBetweenEvictionRunsMillis(-1); + } + + private PooledConnection pooledConnection() throws Exception { + return new PooledConnection(properties, new ConnectionPool(properties)); + } + + /** Records the identity a session was last established with. */ + private static void loggedInAs(PooledConnection con, String user, String password) { + if (user == null) { + con.getAttributes().remove(PooledConnection.PROP_USER); + } else { + con.getAttributes().put(PooledConnection.PROP_USER, user); + } + if (password == null) { + con.getAttributes().remove(PooledConnection.PROP_PASSWORD); + } else { + con.getAttributes().put(PooledConnection.PROP_PASSWORD, password); + } + } + + @ParameterizedTest(name = "session {1}/{2} asked {3}/{4} forces reconnect: {5}") + @MethodSource("credentialCombinations") + void theSessionIsReEstablishedOnlyForAnotherIdentity(String sessionLabel, + String loggedInUser, String loggedInPassword, + String askedUser, String askedPassword, boolean expected) throws Exception { + PooledConnection con = pooledConnection(); + loggedInAs(con, loggedInUser, loggedInPassword); + assertEquals(expected, con.shouldForceReconnect(askedUser, askedPassword), + sessionLabel + " session asked for " + askedUser); + } + + static Stream<Arguments> credentialCombinations() { + return Stream.of( + // a session in the pool's default identity serves the default borrower + same("default", DEFAULT_USER, DEFAULT_PASSWD, null, null), + same("default", DEFAULT_USER, DEFAULT_PASSWD, DEFAULT_USER, DEFAULT_PASSWD), + same("default", DEFAULT_USER, DEFAULT_PASSWD, null, DEFAULT_PASSWD), + same("default", DEFAULT_USER, DEFAULT_PASSWD, DEFAULT_USER, null), + // ... and nobody else + forced("default", DEFAULT_USER, DEFAULT_PASSWD, ALT_USER, ALT_PASSWD), + forced("default", DEFAULT_USER, DEFAULT_PASSWD, null, ALT_PASSWD), + forced("default", DEFAULT_USER, DEFAULT_PASSWD, ALT_USER, null), + // a session in an alternate identity serves exactly that user + same("alternate", ALT_USER, ALT_PASSWD, ALT_USER, ALT_PASSWD), + forced("alternate", ALT_USER, ALT_PASSWD, null, null), + forced("alternate", ALT_USER, ALT_PASSWD, DEFAULT_USER, DEFAULT_PASSWD), + forced("alternate", ALT_USER, ALT_PASSWD, null, DEFAULT_PASSWD), + forced("alternate", ALT_USER, ALT_PASSWD, DEFAULT_USER, null), + forced("alternate", ALT_USER, ALT_PASSWD, null, ALT_PASSWD), + forced("alternate", ALT_USER, ALT_PASSWD, ALT_USER, null)); + } + + private static Arguments same(String label, String loggedInUser, String loggedInPassword, + String askedUser, String askedPassword) { + return Arguments.of(label, loggedInUser, loggedInPassword, askedUser, askedPassword, + Boolean.FALSE); + } + + private static Arguments forced(String label, String loggedInUser, String loggedInPassword, + String askedUser, String askedPassword) { + return Arguments.of(label, loggedInUser, loggedInPassword, askedUser, askedPassword, + Boolean.TRUE); + } + + @Test + void aSessionThatNeverRecordedAnIdentityIsEstablishedForTheDefaultBorrower() + throws Exception { + PooledConnection con = pooledConnection(); + // no PROP_USER/PROP_PASSWORD at all: the pool cannot know who this + // session belongs to, so it re-establishes it rather than trusting it + assertTrue(con.shouldForceReconnect(null, null)); + } + + @Test + void theDecisionRecordsTheIdentityItJustVetoed() throws Exception { + PooledConnection con = pooledConnection(); + loggedInAs(con, DEFAULT_USER, DEFAULT_PASSWD); + + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), + "another identity must be refused the pooled session"); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), + "the decision records the new identity, so the next borrower asking " + + "for the same user gets the session once it is re-established"); + + loggedInAs(con, DEFAULT_USER, DEFAULT_PASSWD); + assertFalse(con.shouldForceReconnect(null, null)); + assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + assertTrue(con.shouldForceReconnect(null, null), + "and the same holds in the other direction"); + } + + @Test + void withoutAlternateCredentialsSupportNoBorrowerEverForcesAReconnect() + throws Exception { + properties.setAlternateUsernameAllowed(false); + PooledConnection con = pooledConnection(); + loggedInAs(con, ALT_USER, ALT_PASSWD); + + assertFalse(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD), + "with alternates disallowed the credentials of the borrower are ignored, " + + "so the session can never mismatch"); + assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/SlowQueryComparatorTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/SlowQueryComparatorTest.java new file mode 100644 index 0000000000..4053633ddc --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/config/SlowQueryComparatorTest.java @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.config; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.QueryStats; +import org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.QueryStatsComparator; + +/** + * The statistics comparator the slow query report sorts its results with, + * checked in isolation. Queries that were never invoked must sort last, + * and the ordering must be a consistent total order - an inconsistent + * comparator makes {@code Collections.sort} throw, which used to take the + * reporting MBean down with it (bug 58489). + * + * <p>Ported from the retired legacy {@code TestSlowQueryComparator}.</p> + */ +class SlowQueryComparatorTest { + + private final Comparator<QueryStats> comparator = new QueryStatsComparator(); + + @Test + void sortingARealisticMixOfInvokedAndNeverInvokedQueriesSucceeds() { + long[] testData = { 0, 0, 0, 1444225382010L, 0, 1444225382011L, 0, + 1444225382012L, 0, 1444225382056L, 0, 1444225382014L, 0, + 1444225382015L, 0, 1444225382016L, 0, 0, 1444225382017L, 0, + 1444225678350L, 0, 1444225680397L, 0, 1444225382018L, + 1444225382019L, 1444225382020L, 0, 1444225382021L, 0, + 1444225382022L, 1444225382023L + }; + + List<QueryStats> stats = new ArrayList<>(); + for (int i = 0; i < testData.length; i++) { + QueryStats qs = new QueryStats(String.valueOf(i)); + qs.add(0, testData[i]); + stats.add(qs); + } + + assertDoesNotThrow(() -> Collections.sort(stats, comparator), + "sorting the collected statistics must not fail"); + } + + @Test + void aQueryComparedWithItselfIsEqual() { + QueryStats neverInvoked = new QueryStats("abc"); + assertEquals(0, comparator.compare(neverInvoked, neverInvoked)); + + QueryStats invoked = new QueryStats("abc"); + invoked.add(0, 100); + assertEquals(0, comparator.compare(invoked, invoked)); + } + + @Test + void aQueryThatWasNeverInvokedSortsLast() { + QueryStats neverInvoked = new QueryStats("abc"); + QueryStats invoked = new QueryStats("def"); + invoked.add(0, 100); + + assertEquals(1, comparator.compare(neverInvoked, invoked), + "a query without invocations must sort after an invoked one"); + assertEquals(-1, comparator.compare(invoked, neverInvoked)); + } + + @Test + void queriesInvokedAtTheSameTimeAreEqual() { + QueryStats first = new QueryStats("abc"); + QueryStats second = new QueryStats("def"); + first.add(0, 100); + second.add(0, 100); + + assertEquals(0, comparator.compare(first, second)); + assertEquals(0, comparator.compare(second, first)); + } + + @Test + void theMoreRecentlyInvokedQuerySortsLast() { + QueryStats older = new QueryStats("abc"); + QueryStats newer = new QueryStats("abc"); + older.add(0, 100); + newer.add(0, 150); + + assertEquals(-1, comparator.compare(older, newer)); + assertEquals(1, comparator.compare(newer, older)); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AlternateCredentialsTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AlternateCredentialsTest.java new file mode 100644 index 0000000000..dcecb8a3e7 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AlternateCredentialsTest.java @@ -0,0 +1,217 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.Queue; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; + +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * Expected functionality of borrowing with credentials other than the + * pool's default: the session a borrower gets must be authenticated as + * exactly the user it asked for, and a session must never be handed to a + * borrower with a different identity. The identity oracle is the + * database's own {@code CURRENT_USER}, so a pool that merely remembered + * the requested name without re-authenticating cannot pass. + * + * <p>Scenario harvested from the retired legacy + * {@code AlternateUsernameTest}, which could only ask a fake driver which + * credentials it had been handed.</p> + */ +class AlternateCredentialsTest extends PoolTestSupport { + + private static final String ALT_USER = "POOL_ALT_USER"; + private static final String ALT_PASSWORD = "alt-password"; + + private static final String DEFAULT_USER = "SA"; + + @BeforeAll + static void createAlternateUser() throws SQLException { + try (Connection admin = DriverManager.getConnection(getUrl(), "sa", ""); + Statement st = admin.createStatement()) { + st.execute("CREATE USER IF NOT EXISTS " + ALT_USER + + " PASSWORD '" + ALT_PASSWORD + "'"); + } + } + + private DataSource alternatePool(boolean allowed) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setAlternateUsernameAllowed(allowed); + ds.getPoolProperties().setMaxActive(4); + return ds; + } + + /** The user the database authenticated this connection as. */ + private static String sessionUser(Connection c) throws SQLException { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT CURRENT_USER")) { + rs.next(); + return rs.getString(1); + } + } + + @ParameterizedTest(name = "alternateUsernameAllowed={0}") + @CsvSource({ "true", "false" }) + void theSessionServesTheIdentityTheBorrowerAskedFor(boolean allowed) throws Exception { + DataSource ds = alternatePool(allowed); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + assertEquals(DEFAULT_USER, sessionUser(c), + "a plain borrow uses the pool's own credentials"); + } + + try (Connection c = ds.getConnection(ALT_USER, ALT_PASSWORD)) { + String expected = allowed ? ALT_USER : DEFAULT_USER; + assertEquals(expected, sessionUser(c), allowed + ? "a borrow with alternate credentials must be authenticated as that user" + : "with alternates disallowed the configured user must be kept"); + } + + try (Connection c = ds.getConnection()) { + assertEquals(DEFAULT_USER, sessionUser(c), + "a default borrow must never be served by the alternate user's session"); + } + } + + @Test + void aSessionIsNeverServedToABorrowerOfAnotherIdentity() throws Exception { + DataSource ds = alternatePool(true); + ds.createPool(); + + Connection first = ds.getConnection(ALT_USER, ALT_PASSWORD); + setSessionMarker(first, 12); + first.close(); + + // the same identity gets the pooled session back, untouched + Connection again = ds.getConnection(ALT_USER, ALT_PASSWORD); + try { + assertEquals(Integer.valueOf(12), sessionMarker(again), + "the alternate user's session must be pooled and reused for the same user"); + assertEquals(ALT_USER, sessionUser(again)); + } finally { + again.close(); + } + + // a different identity must not be served by that session: the pool + // re-establishes the session instead, so the switch costs a reconnect + try (Connection other = ds.getConnection()) { + assertEquals(DEFAULT_USER, sessionUser(other), + "the alternate user's session must not be handed to the default user"); + } + assertEquals(1, ds.getSize(), + "switching identity replaces the session in place, it does not add one"); + awaitSessions(1); + } + + @Test + void aWrongAlternatePasswordIsRefusedAndThePoolSurvives() throws Exception { + DataSource ds = alternatePool(true); + ds.getPoolProperties().setMaxWait(5000); + ds.createPool(); + + assertThrows(SQLException.class, () -> ds.getConnection(ALT_USER, "wrong-password"), + "the database must refuse a borrow with a wrong password"); + + try (Connection c = ds.getConnection()) { + assertEquals(DEFAULT_USER, sessionUser(c), + "a refused borrow must leave the pool serving its default user"); + } + assertEquals(0, ds.getActive(), "a refused borrow must not leak a checked out connection"); + } + + @Test + void anUnknownUserIsRefusedByTheDatabase() throws Exception { + DataSource ds = alternatePool(true); + ds.getPoolProperties().setMaxWait(5000); + ds.createPool(); + + assertThrows(SQLException.class, () -> ds.getConnection("NO_SUCH_POOL_USER", "x"), + "the database must refuse a borrow for a user that does not exist"); + assertEquals(0, ds.getActive()); + } + + @Test + void mixedIdentitiesUnderConcurrencyAlwaysGetTheirOwnUser() throws Exception { + final int threads = 8; + final int iterations = 15; + DataSource ds = alternatePool(true); + ds.getPoolProperties().setMaxActive(threads); + ds.createPool(); + + CountDownLatch start = new CountDownLatch(1); + CountDownLatch done = new CountDownLatch(threads); + Queue<String> problems = new ConcurrentLinkedQueue<>(); + AtomicInteger served = new AtomicInteger(); + for (int i = 0; i < threads; i++) { + final boolean useAlternate = i % 2 == 0; + Thread worker = new Thread(() -> { + try { + start.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS); + for (int n = 0; n < iterations; n++) { + try (Connection c = useAlternate + ? ds.getConnection(ALT_USER, ALT_PASSWORD) + : ds.getConnection()) { + String user = sessionUser(c); + String expected = useAlternate ? ALT_USER : DEFAULT_USER; + if (!expected.equals(user)) { + problems.add("expected " + expected + " but got " + user); + } + served.incrementAndGet(); + } + } + } catch (Exception e) { + problems.add(e.toString()); + } finally { + done.countDown(); + } + }, "credential-churn-" + i); + worker.setDaemon(true); + worker.start(); + } + start.countDown(); + assertTrue(done.await(2 * AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the churn threads must finish, problems: " + problems); + + assertTrue(problems.isEmpty(), "every borrower must get its own identity: " + problems); + assertEquals(threads * iterations, served.get()); + assertEquals(0, ds.getActive(), "every borrowed connection must be back"); + assertTrue(ds.getSize() <= threads, + "the pool must not exceed maxActive, size was " + ds.getSize()); + assertTrue(poolSessions() <= threads, + "the database must never see more sessions than maxActive, was " + + poolSessions()); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AsyncBorrowTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AsyncBorrowTest.java new file mode 100644 index 0000000000..20a1f89b4e --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/AsyncBorrowTest.java @@ -0,0 +1,158 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * Expected functionality of the asynchronous borrow API + * ({@code getConnectionAsync()}): the same accounting, the same + * database-side truth and the same delivery guarantees as the blocking + * API, and a clear failure when the pool is configured with a queue that + * cannot hand out futures. + * + * <p>Scenario harvested from the retired legacy classes + * {@code Bug50805} (async borrow accounting) and + * {@code Async0IdleTestBug50477} (async borrow from a fair pool that has + * no idle connections yet), neither of which ran in the old build.</p> + */ +class AsyncBorrowTest extends PoolTestSupport { + + @Test + void asyncBorrowKeepsAccountingConsistentWithTheDatabase() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(1); + ds.getPoolProperties().setMaxActive(10); + ds.createPool(); + + assertEquals(0, ds.getSize(), "an empty pool must hold no connections"); + assertEquals(0, ds.getIdle()); + assertEquals(0, ds.getActive()); + assertEquals(0, poolSessions(), "the database must not see any session yet"); + + ds.getConnection().close(); + assertEquals(1, ds.getSize(), "the pool must keep the connection it created"); + assertEquals(1, ds.getIdle()); + assertEquals(0, ds.getActive()); + awaitSessions(1); + + Future<Connection> future = ds.getConnectionAsync(); + try (Connection c = future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + assertNotNull(c, "the future must hand out a connection"); + assertEquals(1, ds.getActive(), "an async borrow must count as active"); + assertEquals(0, ds.getIdle()); + assertEquals(1, poolSessions(), + "the async borrow must reuse the pooled connection, not open a second one"); + try (ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + assertTrue(rs.next(), "the asynchronously borrowed connection must work"); + } + } + assertEquals(0, ds.getActive(), "the returned connection must stop counting as active"); + assertEquals(1, ds.getIdle()); + awaitSessions(1); + } + + @Test + void asyncBorrowFromAnEmptyFairPoolEstablishesTheFirstConnection() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(5); + ds.createPool(); + assertEquals(0, poolSessions()); + + Future<Connection> future = ds.getConnectionAsync(); + try (Connection c = future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + assertNotNull(c, "an empty fair pool must create the connection on demand"); + try (ResultSet rs = c.createStatement().executeQuery("SELECT 1")) { + assertTrue(rs.next()); + } + } + awaitSessions(1); + } + + @Test + void asyncBorrowOnAnExhaustedPoolNeedsTheFairQueue() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(false); + ds.getPoolProperties().setMaxActive(1); + ds.createPool(); + + Connection holder = ds.getConnection(); + try { + SQLException error = assertThrows(SQLException.class, ds::getConnectionAsync, + "a non-fair queue cannot hand out futures once the pool is exhausted"); + assertTrue(error.getMessage().toLowerCase().contains("fair"), + "the failure must name the missing configuration, was: " + error.getMessage()); + } finally { + holder.close(); + } + } + + @Test + void concurrentAsyncBorrowersAreAllServed() throws Exception { + final int borrowers = 6; + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(2); + ds.createPool(); + + final AtomicInteger served = new AtomicInteger(); + final AtomicInteger failed = new AtomicInteger(); + Thread[] threads = new Thread[borrowers]; + for (int i = 0; i < borrowers; i++) { + threads[i] = new Thread(() -> { + try { + Future<Connection> future = ds.getConnectionAsync(); + try (Connection c = future.get(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + c.createStatement().execute("SELECT 1"); + served.incrementAndGet(); + } + } catch (Exception e) { + failed.incrementAndGet(); + } + }, "async-borrower-" + i); + threads[i].start(); + } + for (Thread thread : threads) { + thread.join(AWAIT_TIMEOUT_MS); + } + + assertEquals(0, failed.get(), "no async borrower may fail"); + assertEquals(borrowers, served.get(), "every async borrower must be served"); + assertEquals(0, ds.getActive()); + assertEquals(2, ds.getIdle(), "the pool must keep at most maxActive connections"); + awaitSessions(2); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ConnectionSetupTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ConnectionSetupTest.java new file mode 100644 index 0000000000..39ed5dd348 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ConnectionSetupTest.java @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; + +/** + * Expected functionality of the per-connection setup the pool performs + * while a physical session is being established: the configured init SQL + * is applied to the session, unusable configuration is normalised or + * tolerated instead of wedging the pool, and the session the application + * receives is in a defined state. + * + * <p>Scenarios harvested from the retired legacy classes + * {@code Bug54225} (blank init SQL), {@code Bug54978} (init SQL used for + * connect validation) and {@code Bug50571} (unusable configured + * transaction isolation with the {@code ConnectionState} interceptor) - + * none of which ran in the old build.</p> + */ +class ConnectionSetupTest extends PoolTestSupport { + + private static final String INIT_MARKER = "SET @POOL_INIT_STATE = 'applied'"; + + private static String initMarker(Connection c) throws Exception { + try (Statement st = c.createStatement(); + ResultSet rs = st.executeQuery("SELECT @POOL_INIT_STATE")) { + rs.next(); + return rs.getString(1); + } + } + + @Test + void initSqlIsAppliedToEveryPhysicalConnection() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitSQL(INIT_MARKER); + ds.getPoolProperties().setMaxActive(2); + ds.createPool(); + + try (Connection initial = ds.getConnection()) { + assertEquals("applied", initMarker(initial), + "the init SQL must have run on the initial connection"); + } + awaitSessions(1); + + // a second physical connection gets the init SQL as well + Connection secondPhysical; + try (Connection first = ds.getConnection()) { + secondPhysical = ds.getConnection(); + assertEquals(2, poolSessions(), "two concurrent borrowers need two sessions"); + } + assertEquals("applied", initMarker(secondPhysical), + "the init SQL must run on every session the pool establishes"); + secondPhysical.close(); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = { " " }) + void aBlankInitSqlIsNotConfiguredAtAll(String initSql) throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitSQL(initSql); + assertNull(ds.getPoolProperties().getInitSQL(), + "a blank init SQL must be normalised to 'not configured'"); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + assertNull(initMarker(c), "nothing may have been executed on the session"); + c.createStatement().execute("SELECT 1"); + } + } + + @Test + void theInitSqlValidatesANewlyEstablishedConnection() throws Exception { + // Documented in PoolConfiguration#setValidationQuery: "Setting an + // initSQL will override this setting, as the init SQL will be used + // instead of the validation query" - so for a freshly established + // session the init SQL is the validation, and the (broken) + // validationQuery below is never executed. + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitSQL(INIT_MARKER); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationQuery("SELECT * FROM NO_SUCH_VALIDATION_TABLE"); + ds.getPoolProperties().setValidationInterval(0); + ds.getPoolProperties().setMaxActive(1); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + assertEquals("applied", initMarker(c), + "the configured init SQL must have validated the session"); + } + assertEquals(1, ds.getIdle()); + } + + @Test + void anUnusableTransactionIsolationSettingDoesNotBlockThePool() throws Exception { + DataSource ds = newDataSource(); + // not a JDBC isolation level at all; the pool must not strand borrowers over it + ds.getPoolProperties().setDefaultTransactionIsolation(-55); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getName()); + ds.createPool(); + + for (int i = 0; i < 2; i++) { + try (Connection c = ds.getConnection()) { + assertEquals(Connection.TRANSACTION_READ_COMMITTED, + c.getTransactionIsolation(), + "the session keeps the driver's isolation level"); + assertTrue(c.createStatement().execute("SELECT 1"), + "the connection must be usable despite the unusable setting"); + } + } + assertEquals(1, ds.getIdle(), "the pool must still pool the connection"); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java new file mode 100644 index 0000000000..21d699c320 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterceptorLifecycleTest.java @@ -0,0 +1,136 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.lang.reflect.Proxy; +import java.sql.Connection; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.JdbcInterceptor; +import org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport; + +/** + * Expected functionality of the interceptor configuration a resource + * definition supplies: an interceptor named by its short name is found, + * instantiated once with its properties applied, wired into the chain of + * every borrowed connection, told about the pool starting and stopping - + * and it cleans up after the pool is gone. + * + * <p>Scenarios harvested from the retired legacy + * {@code TestInterceptorShortName} and the statistics-cleanup case of + * {@code TestSlowQueryReport}. The built-in {@code SlowQueryReport} plays + * the role of the configured interceptor, so nothing here needs a test + * driver: the interceptor leaves evidence in the database work it sees and + * in the statistics it publishes.</p> + */ +class InterceptorLifecycleTest extends PoolTestSupport { + + private static final String REPORT_SQL = "SELECT COUNT(*) FROM SYSTEM_RANGE(1,10)"; + + private DataSource reportedPool() { + DataSource ds = newDataSource(); + // deliberately the short name, as it appears in a resource definition + ds.getPoolProperties().setJdbcInterceptors("SlowQueryReport(threshold=7,maxQueries=5)"); + return ds; + } + + /** Walks the interceptor chain of a borrowed connection. */ + private static JdbcInterceptor findInChain(Connection pooled, Class<?> type) { + JdbcInterceptor interceptor = + (JdbcInterceptor) Proxy.getInvocationHandler(pooled); + while (interceptor != null) { + if (type.isInstance(interceptor)) { + return interceptor; + } + interceptor = interceptor.getNext(); + } + return null; + } + + @Test + void aShortInterceptorNameIsResolvedAndConfigured() throws Exception { + DataSource ds = reportedPool(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + JdbcInterceptor interceptor = + findInChain(pooled, SlowQueryReport.class); + assertNotNull(interceptor, + "the interceptor named by short name must sit in the chain of a " + + "borrowed connection"); + assertInstanceOf(SlowQueryReport.class, interceptor); + assertEquals(7L, ((SlowQueryReport) interceptor).getThreshold(), + "the properties from the configuration string must be applied"); + } + } + + @Test + void theInterceptorSeesTheStatementsOfEveryBorrower() throws Exception { + DataSource ds = reportedPool(); + ds.createPool(); + String poolName = ds.getPool().getName(); + + for (int i = 0; i < 2; i++) { + try (Connection pooled = ds.getConnection(); Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery(REPORT_SQL)) { + assertTrue(rs.next()); + } + } + + Map<String, SlowQueryReport.QueryStats> stats = + SlowQueryReport.getPoolStats(poolName); + assertNotNull(stats, "a running pool must have its report registered"); + SlowQueryReport.QueryStats query = stats.get(REPORT_SQL); + assertNotNull(query, "both executions must be reported, saw " + stats.keySet()); + assertEquals(2, query.getNrOfInvocations(), + "the report must see the work of every borrower"); + } + + @Test + void thePoolLifecycleIsReportedToTheInterceptor() throws Exception { + DataSource ds = reportedPool(); + String poolName = ds.getPoolProperties().getPoolName(); + assertNull(SlowQueryReport.getPoolStats(poolName), + "nothing may be published before the pool starts"); + + ds.createPool(); + assertNotNull(SlowQueryReport.getPoolStats(poolName), + "poolStarted must have reached the interceptor"); + + try (Connection pooled = ds.getConnection()) { + pooled.createStatement().execute(REPORT_SQL); + } + assertFalse(SlowQueryReport.getPoolStats(poolName).isEmpty(), + "the report must hold data while the pool runs"); + + ds.close(); + assertNull(SlowQueryReport.getPoolStats(poolName), + "poolClosed must have made the interceptor give its statistics up"); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterruptedWaiterTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterruptedWaiterTest.java new file mode 100644 index 0000000000..cea1eb7577 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/InterruptedWaiterTest.java @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * A borrower that is interrupted while waiting for a connection must + * leave the pool exactly as it was: the waiters it registered have to be + * removed, otherwise the pool believes connections are still owed and + * strands the later borrowers. Scenario harvested from the retired + * legacy class {@code Bug53367}, which never ran in the old build. + * + * <p>Asserted with pool accounting <em>and</em> the database's own + * session count, for both queue implementations.</p> + */ +class InterruptedWaiterTest extends PoolTestSupport { + + private static final int HOLDERS = 3; + + @ParameterizedTest(name = "fairQueue={0}") + @ValueSource(booleans = { true, false }) + void interruptedWaitersLeaveThePoolUsable(boolean fairQueue) throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setFairQueue(fairQueue); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(HOLDERS); + ds.getPoolProperties().setMaxIdle(HOLDERS); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxWait(5000); + ds.getPoolProperties().setRollbackOnReturn(true); + ds.createPool(); + + CountDownLatch holdersBorrowed = new CountDownLatch(HOLDERS); + CountDownLatch releaseHolders = new CountDownLatch(1); + CountDownLatch holdersReturned = new CountDownLatch(HOLDERS); + for (int i = 0; i < HOLDERS; i++) { + Thread holder = new Thread(() -> { + try { + Connection c = ds.getConnection(); + holdersBorrowed.countDown(); + if (releaseHolders.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + c.close(); + holdersReturned.countDown(); + } + } catch (Exception e) { + // the assertions below report the broken state + } + }, "connection-holder"); + holder.setDaemon(true); + holder.start(); + } + assertTrue(holdersBorrowed.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the holders must reach maxActive"); + assertEquals(0, ds.getIdle()); + assertEquals(HOLDERS, ds.getActive()); + assertEquals(HOLDERS, ds.getSize()); + awaitSessions(HOLDERS); + + // Now the pool is exhausted: every further borrower is a waiter. + AtomicInteger interruptedWithConnection = new AtomicInteger(); + AtomicInteger stillWaiting = new AtomicInteger(); + Thread[] waiters = new Thread[HOLDERS]; + for (int i = 0; i < HOLDERS; i++) { + waiters[i] = new Thread(() -> { + stillWaiting.incrementAndGet(); + try (Connection c = ds.getConnection()) { + interruptedWithConnection.incrementAndGet(); + } catch (Exception expected) { + // interrupted or timed out while waiting - as intended + } + }, "interrupted-waiter"); + waiters[i].setDaemon(true); + waiters[i].start(); + } + sleep(500); // give the waiters time to register with the queue + for (Thread waiter : waiters) { + waiter.interrupt(); + } + for (Thread waiter : waiters) { + waiter.join(AWAIT_TIMEOUT_MS); + } + + assertEquals(0, interruptedWithConnection.get(), + "no connection was available, so no waiter may hold one"); + assertEquals(HOLDERS, stillWaiting.get()); + assertEquals(0, ds.getIdle(), + "interrupted waiters must not leave connections checked out"); + assertEquals(HOLDERS, ds.getActive(), + "interrupted waiters must not change the active count"); + assertEquals(HOLDERS, ds.getSize()); + assertEquals(HOLDERS, poolSessions(), + "interrupted waiters must not leave physical connections behind"); + + releaseHolders.countDown(); + assertTrue(holdersReturned.await(AWAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS), + "the holders must return their connections"); + assertEquals(HOLDERS, ds.getIdle(), + "the returned connections must be reusable again"); + assertEquals(0, ds.getActive()); + + // and the pool must serve a fresh wave of borrowers + AtomicInteger served = new AtomicInteger(); + AtomicInteger failed = new AtomicInteger(); + Thread[] borrowers = new Thread[HOLDERS]; + for (int i = 0; i < HOLDERS; i++) { + borrowers[i] = new Thread(() -> { + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + served.incrementAndGet(); + } catch (Exception e) { + failed.incrementAndGet(); + } + }, "later-borrower"); + borrowers[i].setDaemon(true); + borrowers[i].start(); + } + for (Thread borrower : borrowers) { + borrower.join(AWAIT_TIMEOUT_MS); + } + assertEquals(0, failed.get(), + "a pool that only ever had interrupted waiters must still serve them"); + assertEquals(HOLDERS, served.get()); + awaitSessions(HOLDERS); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/JmxMonitorTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/JmxMonitorTest.java index 58220cdf00..bb32f100ae 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/JmxMonitorTest.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/JmxMonitorTest.java @@ -24,8 +24,10 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.lang.management.ManagementFactory; import java.sql.Connection; import java.util.HashSet; +import java.util.Properties; import java.util.Set; +import javax.management.JMX; import javax.management.MBeanServer; import javax.management.MalformedObjectNameException; import javax.management.ObjectName; @@ -33,6 +35,7 @@ import javax.management.ObjectName; import org.junit.jupiter.api.Test; import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.jmx.ConnectionPoolMBean; /** * Expected JMX monitoring functionality. A container registers the @@ -166,6 +169,41 @@ class JmxMonitorTest extends PoolTestSupport { } } + @Test + void theJmxViewNeverExposesThePassword() throws Exception { + final String secret = "s3cr3t-pool-password"; + MBeanServer server = ManagementFactory.getPlatformMBeanServer(); + DataSource ds = newDataSource(); + ds.getPoolProperties().setJmxEnabled(true); + ds.createPool(); + // the pool is already connected; this only records what a real + // configuration holds + ds.getPoolProperties().setPassword(secret); + + ObjectName dsName = uniqueDataSourceName("credentials"); + ObjectName poolOname = registerAndFindPoolMBean(server, ds, dsName); + try { + assertNotNull(poolOname, "an enabled pool must publish its MBean"); + ConnectionPoolMBean mbean = + JMX.newMBeanProxy(server, poolOname, ConnectionPoolMBean.class); + + assertEquals(secret, ds.getPoolProperties().getPassword(), + "the pool itself must still hold the configured password"); + assertFalse(String.valueOf(mbean.getPassword()).contains(secret), + "the MBean must not hand out the password"); + + Properties dbProperties = mbean.getDbProperties(); + assertNotNull(dbProperties); + for (String name : dbProperties.stringPropertyNames()) { + assertFalse(secret.equals(dbProperties.getProperty(name)), + "the JMX view of the driver properties must not carry the" + + " password, property " + name); + } + } finally { + ds.postDeregister(); + } + } + @Test void monitoredConnectionsStayFullyFunctional() throws Exception { MBeanServer server = ManagementFactory.getPlatformMBeanServer(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java index f0d47a5d80..5e1968b612 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PoolTestSupport.java @@ -70,6 +70,19 @@ public abstract class PoolTestSupport { server = Server.createTcpServer("-tcpPort", "0", "-ifNotExists").start(); url = "jdbc:h2:tcp://localhost:" + server.getPort() + "/mem:pooltest_" + UUID.randomUUID(); + // Create the database and pin its lifetime to the server. Without + // DB_CLOSE_DELAY an in-memory database is destroyed as soon as the + // pool closes its last physical connection - which eviction, + // purge() and an alternate-credential reconnect all do - and the + // next connection would silently get an empty database, letting + // assertions about committed data pass against a database that is + // not the one the test wrote to. The setting is applied here rather + // than in the URL because it needs admin rights, and every client + // connection would have to reapply it. + try (Connection admin = DriverManager.getConnection(url, "sa", ""); + Statement st = admin.createStatement()) { + st.execute("SET DB_CLOSE_DELAY -1"); + } } @AfterAll diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ProxyContractTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ProxyContractTest.java new file mode 100644 index 0000000000..685b6bceda --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ProxyContractTest.java @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.SQLException; + +import javax.sql.PooledConnection; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * Expected contract of the {@link java.sql.Connection} object the pool + * hands out: it is a disposable facade over a pooled connection, so + * closing it is permanent even though the physical session goes on + * serving other borrowers, and the underlying session stays reachable + * for the callers that need it. + * + * <p>Scenario harvested from the retired legacy classes + * {@code MultipleCloseTest} and {@code EqualsHashCodeTest}. Note that + * the pool's own {@code equals}/{@code hashCode} handling <em>below</em> + * the facade (with {@code useDisposableConnectionFacade=false}) is not + * covered here on purpose - see TESTING-PLAN.md.</p> + */ +class ProxyContractTest extends PoolTestSupport { + + @Test + void aClosedFacadeStaysClosedAfterTheSessionIsReborrowed() throws Exception { + DataSource ds = newDataSource(); // the disposable facade is on by default + ds.createPool(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 31); + assertFalse(first.isClosed()); + + first.close(); + assertTrue(first.isClosed(), "a returned facade must report itself closed"); + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(31), sessionMarker(second), + "the next borrower must get the very same physical session back"); + assertFalse(second.isClosed(), "the new facade must be usable"); + + assertTrue(first.isClosed(), + "a closed facade must stay closed while its session works for somebody else"); + assertFalse(first.isValid(1), + "a closed facade must report itself invalid, not its session's state"); + assertThrows(SQLException.class, first::createStatement, + "a disposed facade must refuse to hand out work for the session"); + } + } + + @Test + void closingTheSameFacadeTwiceIsHarmless() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + pooled.close(); + pooled.close(); + assertTrue(pooled.isClosed()); + + // exactly one return took place, so the session is idle once + assertEquals(1, ds.getIdle()); + assertEquals(1, poolSessions()); + } + + @Test + void everyBorrowGetsItsOwnFacadeIdentity() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection first = ds.getConnection(); + assertTrue(first.equals(first), "a facade must be equal to itself"); + int firstHash = first.hashCode(); + assertEquals(firstHash, first.hashCode(), "hashCode must be stable"); + + first.close(); + assertTrue(first.equals(first), + "closing must not change the identity of a facade"); + assertEquals(firstHash, first.hashCode(), + "closing must not change the hash code of a facade"); + + try (Connection second = ds.getConnection()) { + assertFalse(first.equals(second), + "two borrows are two facades even over one physical session"); + assertFalse(second.equals(first)); + assertNotEquals(firstHash, second.hashCode(), + "two facades must not share an identity hash"); + assertEquals(second.hashCode(), second.hashCode()); + } + } + + @Test + void thePhysicalSessionStaysReachableThroughThePooledConnectionInterface() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + assertInstanceOf(PooledConnection.class, pooled, + "the facade must also expose the pooled connection interface"); + Connection physical = ((PooledConnection) pooled).getConnection(); + assertNotNull(physical, "the underlying session must be reachable"); + assertTrue(physical.getClass().getName().startsWith("org.h2."), + "the underlying session must be the driver's own object, was " + + physical.getClass().getName()); + assertEquals(getUrl(), physical.getMetaData().getURL(), + "the reachable session must be the one the pool opened"); + + assertTrue(pooled.isWrapperFor(Connection.class)); + assertFalse(pooled.isWrapperFor(javax.sql.XAConnection.class), + "a plain pool must not claim to be an XA wrapper"); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PurgeTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PurgeTest.java new file mode 100644 index 0000000000..2c5b40bfd1 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/PurgeTest.java @@ -0,0 +1,114 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; + +/** + * Expected functionality of {@code purge()} and {@code purgeOnReturn()}: + * a pool can be told that the sessions it currently holds are stale, and + * from that moment on they are physically closed instead of handed to the + * next borrower - while the borrowers that already hold one are not + * disturbed. Each pooled connection remembers the pool generation it was + * created in; both operations start a new generation, and a connection + * from an older one is closed when it comes back. {@code purge()} also + * does it to the connections that are idle right now, + * {@code purgeOnReturn()} waits for them to be returned. Scenario + * harvested from the retired legacy {@code PoolPurgeTest}, asserted here + * with the database's session count. + */ +class PurgeTest extends PoolTestSupport { + + @Test + void purgeClosesIdleSessionsAndDiscardsTheNextReturn() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(3); + ds.getPoolProperties().setMaxActive(3); + ds.createPool(); + awaitSessions(3); + + Connection inUse = ds.getConnection(); + for (int i = 0; i < 2; i++) { + ds.getConnection().close(); + } + assertEquals(3, ds.getSize()); + assertEquals(2, ds.getIdle()); + + ds.purge(); + + assertEquals(0, ds.getIdle(), "purge must drop every idle connection"); + assertEquals(1, ds.getSize(), "only the borrowed connection may survive a purge"); + awaitSessions(1); + + // the borrower is untouched by the purge... + inUse.createStatement().execute("SELECT 1"); + // ...but its session is stale, so returning it closes it + inUse.close(); + assertEquals(0, ds.getIdle(), "a session older than the purge must not be repooled"); + assertEquals(0, ds.getSize()); + awaitSessions(0); + } + + @Test + void purgeOnReturnDiscardsTheSessionsEstablishedBeforeIt() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(2); + ds.getPoolProperties().setMaxActive(2); + ds.createPool(); + awaitSessions(2); + + Connection held = ds.getConnection(); + assertEquals(1, ds.getIdle()); + + ds.purgeOnReturn(); + + assertEquals(1, ds.getIdle(), + "purgeOnReturn must not touch idle connections, unlike purge()"); + assertEquals(2, ds.getSize()); + assertEquals(2, poolSessions()); + + // both pooled sessions predate the purge, so the return of the one + // in use closes it instead of repooling it + held.close(); + assertEquals(1, ds.getIdle(), "the untouched idle session stays available"); + assertEquals(1, ds.getSize()); + awaitSessions(1); + + // the other pre-purge session is discarded on its own return as well + Connection stale = ds.getConnection(); + stale.close(); + assertEquals(0, ds.getSize(), + "every session established before the purge must end up closed"); + awaitSessions(0); + + // a session established after the purge belongs to the new + // generation and is pooled as usual + try (Connection fresh = ds.getConnection()) { + assertTrue(fresh.createStatement().execute("SELECT 1")); + } + assertEquals(1, ds.getSize(), "a session established after the purge must be pooled"); + assertEquals(1, ds.getIdle()); + awaitSessions(1); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/SessionStateTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/SessionStateTest.java new file mode 100644 index 0000000000..20c9d6e022 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/SessionStateTest.java @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.Statement; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.interceptor.ConnectionState; + +/** + * Expected functionality of session state across borrowers: a borrower + * may change the session it was given, but the next one must find the + * session in the state the pool was configured for - a session must never + * carry one borrower's auto commit, isolation or read-only setting into + * another borrower's work. + * + * <p>Scenario harvested from the retired legacy + * {@code TestConnectionState}, with the database itself deciding whether + * the state really was restored: a write that stays invisible to an admin + * connection proves the physical session is in manual commit mode.</p> + */ +class SessionStateTest extends PoolTestSupport { + + private DataSource configuredPool() { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(ConnectionState.class.getSimpleName()); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + ds.getPoolProperties() + .setDefaultTransactionIsolation(Connection.TRANSACTION_SERIALIZABLE); + ds.getPoolProperties().setDefaultReadOnly(Boolean.FALSE); + return ds; + } + + @Test + void aBorrowerSeesTheConfiguredDefaults() throws Exception { + DataSource ds = configuredPool(); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + assertFalse(c.getAutoCommit(), "the configured auto commit must apply"); + assertEquals(Connection.TRANSACTION_SERIALIZABLE, c.getTransactionIsolation(), + "the configured isolation level must apply"); + assertFalse(c.isReadOnly(), "the configured read-only state must apply"); + } + } + + @Test + void theDefaultsAreRestoredForTheNextBorrower() throws Exception { + DataSource ds = configuredPool(); + ds.createPool(); + + try (Connection first = ds.getConnection()) { + setSessionMarker(first, 21); + first.setAutoCommit(true); + first.setTransactionIsolation(Connection.TRANSACTION_READ_COMMITTED); + first.setReadOnly(true); + assertTrue(first.getAutoCommit()); + assertTrue(first.isReadOnly()); + } + + try (Connection second = ds.getConnection()) { + assertEquals(Integer.valueOf(21), sessionMarker(second), + "the same physical session must have been reused"); + assertFalse(second.getAutoCommit(), + "auto commit must be back at the pool default for the next borrower"); + assertEquals(Connection.TRANSACTION_SERIALIZABLE, + second.getTransactionIsolation(), + "the isolation level must be back at the pool default"); + assertFalse(second.isReadOnly(), + "the read-only flag must be back at the pool default"); + + // database-side proof that the physical session really is not in + // auto commit any more: the row stays invisible until committed + String table = uniqueTable(); + try (Connection admin = adminConnection(); + Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + try (Statement st = second.createStatement()) { + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } + assertEquals(0, adminRowCount(table), + "an uncommitted write must not be visible on a session in manual commit mode"); + second.rollback(); + } + assertEquals(1, poolSessions(), "the session must have survived the state changes"); + } + + @Test + void theReadOnlyFlagDoesNotOutliveTheBorrower() throws Exception { + DataSource ds = configuredPool(); + String table = uniqueTable(); + ds.createPool(); + try (Connection admin = adminConnection(); Statement st = admin.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + + try (Connection reader = ds.getConnection()) { + reader.setReadOnly(true); + assertTrue(reader.isReadOnly(), + "the borrower's own change must be visible to it"); + } + + try (Connection writer = ds.getConnection()) { + assertFalse(writer.isReadOnly(), + "the read-only flag must not leak to the next borrower"); + assertEquals(1, writer.createStatement() + .executeUpdate("INSERT INTO " + table + " VALUES (2)"), + "the next borrower must be able to write again"); + writer.commit(); + } + assertEquals(1, adminRowCount(table)); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementLifecycleTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementLifecycleTest.java new file mode 100644 index 0000000000..73fd0532ee --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/StatementLifecycleTest.java @@ -0,0 +1,253 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.function.Consumer; +import java.util.stream.Stream; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.interceptor.StatementCache; +import org.apache.tomcat.jdbc.pool.interceptor.StatementDecoratorInterceptor; +import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; + +/** + * Expected functionality of the statement decorators: statements an + * application forgot about are closed when its connection goes back, and + * a statement that the pool recycles (cached) arrives at the next + * borrower with nothing left over from the previous one - no queued work, + * no settings it never asked for. + * + * <p>Scenarios harvested from the retired legacy classes + * {@code StatementFinalizerTest} and {@code TestStatementCache}. Where the + * legacy versions had to ask a fake driver what it remembered, the batch + * case here is decided by the database: the rows a recycled statement + * actually inserted.</p> + */ +class StatementLifecycleTest extends PoolTestSupport { + + private static final String INSERT_SQL = "INSERT INTO %s VALUES (?)"; + + private DataSource pooledWithInterceptor(String interceptorWithProperties) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.getPoolProperties().setJdbcInterceptors(interceptorWithProperties); + return ds; + } + + private DataSource cachingPool() { + return pooledWithInterceptor(StatementCache.class.getName() + + "(prepared=true,callable=false,max=10)"); + } + + @Test + void closingTheConnectionClosesStatementsTheApplicationLeftOpen() throws Exception { + DataSource ds = pooledWithInterceptor(StatementFinalizer.class.getName()); + ds.createPool(); + + Connection pooled = ds.getConnection(); + Statement single = pooled.createStatement(); + assertFalse(single.isClosed()); + + Statement[] many = new Statement[50]; + for (int i = 0; i < many.length; i++) { + many[i] = pooled.createStatement(); + assertFalse(many[i].isClosed(), "statement " + i + " must be open"); + } + + pooled.close(); + + assertTrue(single.isClosed(), + "a statement the application never closed must be closed with its connection"); + for (Statement st : many) { + assertTrue(st.isClosed(), "every dangling statement must be finalised"); + } + // finalising the statements must not have cost the pool its session + assertEquals(1, ds.getSize()); + assertEquals(1, poolSessions()); + } + + @Test + void anAbandonedBatchItemIsNotExecutedByTheNextBorrower() throws Exception { + DataSource ds = cachingPool(); + String table = uniqueTable(); + ds.createPool(); + try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + } + + // first caller queues a row and abandons the unit of work without + // ever executing it; the statement is closed and recycled + try (Connection abandoning = ds.getConnection()) { + try (PreparedStatement ps = + abandoning.prepareStatement(String.format(INSERT_SQL, table))) { + ps.setInt(1, 101); + ps.addBatch(); + } + } + + int[] counts; + try (Connection next = ds.getConnection()) { + try (PreparedStatement ps = + next.prepareStatement(String.format(INSERT_SQL, table))) { + ps.setInt(1, 202); + ps.addBatch(); + counts = ps.executeBatch(); + } + } + + assertArrayEquals(new int[] { 1 }, counts, + "the recycled statement must carry exactly the batch it was given"); + assertEquals(1, adminRowCount(table), + "only the second caller's row may reach the database"); + } + + @Test + void aStatementToldToCloseOnCompletionIsNotRecycled() throws Exception { + DataSource ds = cachingPool(); + ds.createPool(); + + try (Connection first = ds.getConnection()) { + try (PreparedStatement ps = first.prepareStatement("SELECT 1")) { + // there is no JDBC call that undoes closeOnCompletion(), so + // the only correct outcome is that the cache never takes it + ps.closeOnCompletion(); + } + } + + try (Connection second = ds.getConnection()) { + try (PreparedStatement ps = second.prepareStatement("SELECT 1")) { + assertFalse(ps.isCloseOnCompletion(), + "the next borrower must not inherit closeOnCompletion"); + try (ResultSet rs = ps.executeQuery()) { + assertTrue(rs.next()); + } + } + } + } + + /** + * A property the first caller set and never reset must not survive the + * recycle into the statement cache. Only the properties a real driver + * reports back are checked; H2 silently accepts and forgets + * {@code maxFieldSize} and always reports {@code FETCH_FORWARD}, so a + * leak of those cannot be observed here. + */ + @ParameterizedTest(name = "{0}") + @MethodSource("leakableProperties") + void statementPropertiesDoNotLeakAcrossTheCacheRecycle(String label, + Consumer<PreparedStatement> mutate, Reader reader, int leaked, + int afterReset) throws Exception { + DataSource ds = cachingPool(); + ds.createPool(); + + try (Connection first = ds.getConnection()) { + try (PreparedStatement ps = first.prepareStatement("SELECT 1")) { + mutate.accept(ps); + } + } + + try (Connection second = ds.getConnection()) { + try (PreparedStatement ps = second.prepareStatement("SELECT 1")) { + int observed = reader.read(ps); + assertNotEquals(leaked, observed, + label + " must not be inherited by the next borrower"); + assertEquals(afterReset, observed, label + " must be back at the driver default"); + } + } + } + + static Stream<Arguments> leakableProperties() { + // H2 maps a fetch size of 0 onto its own default of 100, which is + // what the reset leaves behind; the other two have a real zero. + return Stream.of( + Arguments.of("queryTimeout", + (Consumer<PreparedStatement>) ps -> setQuietly(ps, 37), + (Reader) Statement::getQueryTimeout, 37, 0), + Arguments.of("maxRows", + (Consumer<PreparedStatement>) ps -> setMaxRowsQuietly(ps, 999), + (Reader) Statement::getMaxRows, 999, 0), + Arguments.of("fetchSize", + (Consumer<PreparedStatement>) ps -> setFetchSizeQuietly(ps, 777), + (Reader) Statement::getFetchSize, 777, 100)); + } + + /** Reads one of the statement's mutable properties. */ + interface Reader { + int read(PreparedStatement ps) throws SQLException; + } + + private static void setQuietly(PreparedStatement ps, int timeout) { + try { + ps.setQueryTimeout(timeout); + } catch (SQLException e) { + throw new IllegalStateException(e); + } + } + + private static void setMaxRowsQuietly(PreparedStatement ps, int rows) { + try { + ps.setMaxRows(rows); + } catch (SQLException e) { + throw new IllegalStateException(e); + } + } + + private static void setFetchSizeQuietly(PreparedStatement ps, int size) { + try { + ps.setFetchSize(size); + } catch (SQLException e) { + throw new IllegalStateException(e); + } + } + + @Test + void resultSetsAndStatementsPointBackAtThePoolProxies() throws Exception { + DataSource ds = pooledWithInterceptor( + StatementDecoratorInterceptor.class.getSimpleName()); + ds.createPool(); + + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + assertTrue(rs.next()); + assertSame(st, rs.getStatement(), + "a result set must report the pool's statement, not the driver's"); + assertFalse(rs.getStatement().getClass().getName().startsWith("org.h2."), + "the reported statement must not be the raw driver statement"); + assertSame(pooled, rs.getStatement().getConnection(), + "a statement must report the connection it was created from"); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ValidationTest.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ValidationTest.java new file mode 100644 index 0000000000..73bae957ae --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/functional/ValidationTest.java @@ -0,0 +1,271 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.jdbc.functional; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.pool.DataSource; +import org.apache.tomcat.jdbc.pool.PooledConnection; +import org.apache.tomcat.jdbc.pool.Validator; + +/** + * Expected validation functionality of the pool: which checks run when, + * what happens when a check fails, and what the check does to the + * physical session. The oracles are the database's own session count and + * session identity probe, plus the committed data, so a validation that + * silently skipped its work cannot pass. + * + * <p>Scenarios harvested from the retired legacy classes + * {@code TestValidation} and {@code TestValidationQueryTimeout}; the + * {@code jdbc-pool} contract that a validation which fails on an idle + * connection replaces the connection instead of reporting an error is + * asserted explicitly, because it is easy to mistake for a bug.</p> + */ +class ValidationTest extends PoolTestSupport { + + /** Heavy pure-SQL aggregate; H2 honours statement timeouts on it. */ + private static final String SLOW_SQL = + "SELECT COUNT(*) FROM SYSTEM_RANGE(1,400000000) r1, SYSTEM_RANGE(1,3)"; + + private static final String BROKEN_SQL = "SELECT * FROM NO_SUCH_VALIDATION_TABLE"; + + /** Validates with a query and never skips a check. */ + private DataSource validatingDataSource(String validationQuery) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setValidationQuery(validationQuery); + ds.getPoolProperties().setValidationInterval(0); + return ds; + } + + /** The physical connection behind a pooled one, for fault injection. */ + private static Connection physical(Connection pooled) throws SQLException { + return ((javax.sql.PooledConnection) pooled).getConnection(); + } + + @Test + void aFailingValidationQueryFailsTheBorrowAndLeaksNothing() throws Exception { + DataSource ds = validatingDataSource(BROKEN_SQL); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxWait(2000); + ds.createPool(); + assertEquals(1, poolSessions(), "the initial connection exists before the borrow"); + + assertThrows(SQLException.class, ds::getConnection, + "an idle connection that fails validation must not be handed out"); + assertEquals(0, poolSessions(), + "the rejected connection must be closed on the database"); + assertEquals(0, ds.getSize(), "the pool must not keep a connection it rejected"); + } + + @Test + void aConnectionCreatedDuringTheBorrowIsNotValidatedOnBorrow() throws Exception { + // Documented asymmetry: testOnBorrow guards idle connections. A + // connection created to satisfy this very borrow is only subject to + // the on-connect validation (testOnConnect / initSQL), which is off + // here - so a broken validation query goes unnoticed on that path. + DataSource ds = validatingDataSource(BROKEN_SQL); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMaxActive(2); + ds.createPool(); + + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + assertEquals(1, poolSessions()); + } + assertEquals(1, ds.getIdle(), + "the newly created connection is pooled without borrow validation"); + } + + @Test + void aFailingValidationQueryAtConnectStopsThePoolFromStarting() throws Exception { + DataSource ds = validatingDataSource(BROKEN_SQL); + ds.getPoolProperties().setTestOnConnect(true); + ds.getPoolProperties().setInitialSize(1); + + assertThrows(SQLException.class, ds::createPool, + "testOnConnect must fail fast when the initial connection does not validate"); + assertEquals(0, poolSessions(), "no session may survive the failed startup"); + } + + @Test + void aDeadIdleConnectionIsReplacedOnBorrowWithoutError() throws Exception { + DataSource ds = validatingDataSource("SELECT 1"); + ds.getPoolProperties().setTestOnBorrow(true); + ds.createPool(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 11); + Connection physical = physical(first); + first.close(); + physical.close(); // kill the session the pool believes it owns + awaitSessions(0); + + try (Connection second = ds.getConnection()) { + assertNull(sessionMarker(second), + "the borrow must not get the dead session back, but a fresh one"); + try (java.sql.ResultSet rs = second.createStatement().executeQuery("SELECT 1")) { + assertTrue(rs.next(), "the replacement connection must work"); + } + } + assertEquals(1, ds.getSize(), "the dead connection must be replaced, not lost"); + awaitSessions(1); + } + + @Test + void aDeadConnectionIsReleasedOnReturn() throws Exception { + DataSource ds = validatingDataSource("SELECT 1"); + ds.getPoolProperties().setTestOnReturn(true); + ds.createPool(); + + Connection pooled = ds.getConnection(); + physical(pooled).close(); // dies while the application holds it + pooled.close(); // the return validation must notice + + assertEquals(0, ds.getSize(), + "a connection that failed the return validation must leave the pool"); + assertEquals(0, ds.getIdle()); + awaitSessions(0); + + try (Connection fresh = ds.getConnection()) { + fresh.createStatement().execute("SELECT 1"); + } + assertEquals(1, ds.getSize(), "the pool must recover with a new connection"); + } + + @Test + void theValidationQueryTimeoutCancelsAnUnfinishedValidation() throws Exception { + DataSource ds = validatingDataSource(SLOW_SQL); + ds.getPoolProperties().setTestOnConnect(true); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setValidationQueryTimeout(1); + ds.getPoolProperties().setMaxWait(5000); + ds.createPool(); + + long start = System.currentTimeMillis(); + assertThrows(SQLException.class, ds::getConnection, + "a validation query that outlives its timeout must fail the borrow"); + long elapsed = System.currentTimeMillis() - start; + assertTrue(elapsed >= 900 && elapsed < 10000, + "the validation must be cancelled at the configured timeout, took " + + elapsed + " ms"); + awaitSessions(0); + } + + @Test + void aCustomValidatorReplacesTheQueryBasedValidation() throws Exception { + CountingValidator validator = new CountingValidator(); + DataSource ds = validatingDataSource(BROKEN_SQL); // would fail if used + ds.getPoolProperties().setValidator(validator); + ds.getPoolProperties().setTestOnBorrow(true); + ds.createPool(); + + for (int i = 0; i < 2; i++) { + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + } + } + assertEquals(2, validator.actions.size(), + "the validator must be consulted once per borrow"); + for (Integer action : validator.actions) { + assertEquals(Integer.valueOf(PooledConnection.VALIDATE_BORROW), action, + "a borrow validation must report the borrow action to the validator"); + } + + validator.accept = false; + assertThrows(SQLException.class, ds::getConnection, + "a rejecting validator must keep the connection out of the application"); + awaitSessions(0); + + validator.accept = true; + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + } + assertEquals(1, ds.getSize(), "the pool must serve again once validation passes"); + } + + @Test + void theValidationIntervalSuppressesRepeatedChecks() throws Exception { + CountingValidator validator = new CountingValidator(); + DataSource ds = validatingDataSource("SELECT 1"); + ds.getPoolProperties().setValidator(validator); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationInterval(600000); + ds.createPool(); + + for (int i = 0; i < 3; i++) { + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + } + } + // The interval is measured from the last successful validation, + // which for a fresh physical connection is its creation. + assertEquals(0, validator.actions.size(), + "within the validation interval no borrow may validate"); + + ds.getPoolProperties().setValidationInterval(0); + try (Connection c = ds.getConnection()) { + c.createStatement().execute("SELECT 1"); + } + assertEquals(1, validator.actions.size(), + "with the interval switched off the borrow must validate"); + } + + @Test + void validationCommitsTheTransactionOpenOnTheSession() throws Exception { + // Documented behaviour: PooledConnection.validate() finishes with a + // silent commit, so that a validated connection never carries an + // open transaction into the next borrow. Legacy TestValidation + // pinned the same contract ("no transaction must be running"). + DataSource ds = validatingDataSource("SELECT 1"); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.FALSE); + ds.getPoolProperties().setTestOnReturn(true); + String table = uniqueTable(); + + try (Connection c = ds.getConnection(); Statement st = c.createStatement()) { + st.execute("CREATE TABLE " + table + " (v INT)"); + st.executeUpdate("INSERT INTO " + table + " VALUES (1)"); + } // returned without an explicit commit, autoCommit is off + + assertEquals(1, adminRowCount(table), + "the validation on return must leave no open transaction behind"); + } + + /** Records every validation the pool asks it to perform. */ + private static final class CountingValidator implements Validator { + private final List<Integer> actions = new ArrayList<>(); + private volatile boolean accept = true; + + @Override + public boolean validate(Connection connection, int validateAction) { + actions.add(Integer.valueOf(validateAction)); + return accept; + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
