Bug 54395 - JdbcInterceptor config parameter parsing errors
JdbcInterceptor config parameter parsing errors
Status: RESOLVED FIXED
Product: Tomcat Modules
Classification: Unclassified
Component: jdbc-pool
unspecified
All All
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2013-01-09 20:51 UTC by Glen Taylor
Modified: 2014-08-07 21:32 UTC (History)
0 users



Attachments
Patch for PoolProperties.java (822 bytes, patch)
2013-01-09 20:53 UTC, Glen Taylor
Details | Diff
New unit test for parser (6.91 KB, text/plain)
2013-01-09 20:54 UTC, Glen Taylor
Details
Updated unit test per comments (6.85 KB, text/plain)
2013-01-09 21:37 UTC, Glen Taylor
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Glen Taylor 2013-01-09 20:51:23 UTC
The parameter value for a JdbcInterceptor config is parsed incorrectly if there is any white-space following the closing parentheses (it includes the closing paren in the parameter value).

I will be attaching two items momentarily:
  * a patch to resolve the issue
  * a unit test that fails in TRUNK but passes post-patch.

The parser can behave unpredictably in several other boundary cases (extra white-space, spurious semicolons, empty parm lists, etc.) so I have attempted to document the current behavior in the unit test for clarity.  It may be worth considering a more uniform response to incorrect config(s), but I skipped that here to keep it simple.  I may submit an enhancement request later (with some suggestions as a patch).
Comment 1 Glen Taylor 2013-01-09 20:53:10 UTC
Created attachment 29833 [details]
Patch for PoolProperties.java
Comment 2 Glen Taylor 2013-01-09 20:54:37 UTC
Created attachment 29835 [details]
New unit test for parser
Comment 3 Christopher Schultz 2013-01-09 21:22:34 UTC
FWIW, test cases using the following idiom:

			caught = null;
			try {
				props.getJdbcInterceptorsAsArray();
			} catch (Exception e) {
				caught = e;
			}
			assertNull(caught);

can be written somewhat more simply like this:

			try {
				props.getJdbcInterceptorsAsArray();
			} catch (Exception e) {
                            assertFail("Didn't expect exception");
			}

Likewise:
			caught = null;
			try {
				props.getJdbcInterceptorsAsArray();
			} catch (Exception e) {
				caught = e;
			}
			assertNotNull(caught);

can be written like this:

			try {
				props.getJdbcInterceptorsAsArray();
                                assertFail("Expected Exception");
			} catch (Exception e) {
                                // Expected
			}
Comment 4 Glen Taylor 2013-01-09 21:37:14 UTC
Created attachment 29836 [details]
Updated unit test per comments

Well, the JUnit 3.8.1 pulled in by Maven doesn't have an "assertFail(String)" method, but I think the "fail(String)" alternative is there in spirit.  Point(s) taken, unit test attachment updated.

I had been doing more with the Exception in case 1, and didn't refactor far enough when that changed, and case 2 was copy/paste laziness (blah,blah).  Thanks for the good suggestions.
Comment 5 Konstantin Kolinko 2013-01-09 23:34:12 UTC
(In reply to comment #3)
> FWIW, test cases using the following idiom:
> (..)
> can be written somewhat more simply like this:
> 
> 			try {
> 				props.getJdbcInterceptorsAsArray();
> 			} catch (Exception e) {
>                             assertFail("Didn't expect exception");
> 			}

1. Just add "throws Exception" to the test method. JUnit notices the thrown exception and the test does not pass.

2. It is the first time I am seeing "assertFail". A typo? The method that I know about is called   fail(message) (Assert.fail(..)).
Comment 6 Christopher Schultz 2013-01-10 03:42:31 UTC
Sorry, yes, I meant "fail()".

As for "throws Exception" doesn't that change a failure into an "error" when JUnit runs?
Comment 7 Filip Hanik 2014-08-07 21:32:42 UTC
Love nothing more than receiving tests

The assertions are backwards (parameters swapped), other than that looks great.
org.junit.ComparisonFailure: 
Expected :value2 )
Actual   :value2

Thanks for the report
Fixed in r1616599