Bug #15677 TINYINT UNSIGNED values read with ResultSet.getShort() return incorrect values
Submitted: 12 Dec 2005 4:16 Modified: 10 Mar 2006 20:26
Reporter: Darryl Miles Email Updates:
Status: Closed Impact on me:
None 
Category:Connector / J Severity:S2 (Serious)
Version:3.1.11 OS:
Assigned to: Mark Matthews CPU Architecture:Any

[12 Dec 2005 4:16] Darryl Miles
Description:
Handling of TINYINT UNSIGNED columns through the ResultSet.getShort() does not appear to work to a 4.1.x server, it has been working in the same application when the driver talks to a 4.0.x server.

I have taken a look at the ResultSet#getNativeShort() code and think I can see inconsistant handling of UNSIGNED integral data types when compared to other functions handling different integer widths.  I would be happy to (with your guidance) investigate this matter further, provide patches, documentation and test cases if it were possible for a representative for Mysql AB to explain what the correct way for the driver to handle these conversions is.

There also looks to be some work for throwing exceptions when undesirable data value truncations occurs, maybe I can look at completing this work.

I would then like to build test cases (to improve future quality control) for all the possible permuatations of data and API usage to all possible versions of MySQL server (since the problem for me occurs with 4.1 but not with 4.0, it seems relevant).  Do you have a test case framework already in place?

How to repeat:
Connect JDBC to a 4.1.15 server.  With data setup like:

create table person (
 person_id BIGINT NOT NULL AUTO_INCREMENT,
 spouse_id BIGINT NULL,

 when_created DATETIME NOT NULL,
 local_priority TINYINT UNSIGNED DEFAULT '128' NOT NULL,

 status TINYINT UNSIGNED DEFAULT '0' NOT NULL,
 when_status DATETIME NOT NULL,
 when_terminal DATETIME NULL,

 status_ack TINYINT UNSIGNED DEFAULT '0' NOT NULL,
 when_status_ack DATETIME NULL,
 when_terminal_ack DATETIME NULL,
 
 PRIMARY KEY (person_id)
);

insert into person set person_id=1, when_created=NOW(),
	spouse_id=NULL, status=255, when_status=NOW(), when_terminal=NOW();

Though JDBC SELECT that row and read back and use ResultSet.getShort() to read the column value for 'status'.
[12 Dec 2005 8:03] Valeriy Kravchuk
Thank you for a problem report and a good will. Please, check with the latest 3.1.12 (http://dev.mysql.com/downloads/connector/j/3.1.html). 

You bug report looks similar to some others already known. Please, read http://bugs.mysql.com/bug.php?id=11874, for example. Isn't it a duplicate?
[12 Dec 2005 13:58] Darryl Miles
The problem is similar but not exactly the same.

The bug report you refer to is in connection with using JDBC ResultSet.getInt() API call to retrieve the TINYINT UNSIGNED value.  If you take a look in the ResultSet.java code you may find this calls internal function ResultSet#getNativeInt() which does have special handling for Field.isUnsigned() and biases the value + 256 etc...

My bug report is specifically for ResultSet.getShort() and I have looked at both 3.1.11 and 3.1.12 drivers and there is no change to the getShort() implementation that I can see that would fix this problem.

I jave just verified my problem exists by installing the 3.1.12 driver and running my test case.

Also when using the driver is using a 4.0 SQL server I was not getting this problem in my application, this is a strange factor I don't understand at all and would like to understand better.  Do your test cases verify the bugs against every major version of MySQL server ?  Maybe this is why in the other bug report you are not seeing part of the problem ?

My bug report was to offer to do a complete audit of this situation, if I can only be sure of exactly the behaviour you are looking for.  For example how would a BIGINT UNSIGNED => long be stored ?  when Java does not 64bit wide unsigned integral type, this value can be stored in other ways getBigDecimal(), maybe an exception should be throw similar to the commented out code in getNativeLong() I think this should be the case for ALL UNSIGNED SQL type to signed Java (same integer wide) conversion.
[12 Dec 2005 14:10] Darryl Miles
Just to clarify I am proposing to help you have test cases for:

TINYINT => of value 0 => getByte() => check result 0
TINYINT => of value 127 => getByte() => check result 127
TINYINT => of value -128 => getByte() => check result -128
TINYINT UNSIGNED => of value 0 => getByte() => check proposed Exception (even though no truncation occurs in this instance I propose by default an exception should occur for trying to perform an operation that _MAY_ cause truncation, then have a 3 state driver override flag to allow this case though, or only throw exception when a truncation does occur, or to silently truncate, then the drive user gets a safe default but has the option to change behaviour)
TINYINT UNSIGNED => of value 127 => getByte() => check proposed Exception
TINYINT UNSIGNED => of value 255 => getByte() => check proposed Exception

TINYINT => of value 0 => getShort() => check result 0
TINYINT => of value 127 => getShort() => check result 127
TINYINT => of value -128 => getShort() => check result -128
TINYINT UNSIGNED => of value 0 => getShort() => check result 0
TINYINT UNSIGNED => of value 127 => getShort() => check result 127
TINYINT UNSIGNED => of value 255 => getShort() => check result 255

etc... getInt(), getLong(), getBigDecimal(), getFloat(), getDouble(), etc.....
then SMALLINT, SMALLINT UNSIGNED, INT24, INT24 UNSIGNED, etc...
[12 Dec 2005 15:00] Darryl Miles
3.1.12 driver patch

Attachment: ResultSet.java.diff (text/x-patch), 1.04 KiB.

[12 Dec 2005 15:03] Mark Matthews
BIGINT UNSIGNED is mapped to java.lang.BigInteger, see http://dev.mysql.com/doc/refman/5.0/en/cj-type-conversions.html

As far as the test framework in use, we use JUnit. All Connector/J testsuites are built on top of a base class that creates connections and has some utility methods. See testsuite.regression.ResultSetRegressionTest for examples. That's where a testcase for this bug should live.
[12 Dec 2005 15:27] Darryl Miles
Thanks for the Unit Test details I will look at them shortly, does every test get run through every major version of SQL server (4.0, 4.1, 5.0, 5.1) before each release of the driver ?

What is your view with the truncation exception behaviour ?

I would like to see a 100% safe default but still give users the option to configure their exact behaviour, the 3 modes I propose are:

* Safe mode, the default, ALWAYS throw an Exception if the JDBC API call _MAY_ cause truncation on this column type.  TINYINT UNSIGNED => getByte() MAY cause truncation if the SQL value is in the range 128 or 255.  The reason for this being the default is that it will throw an error upon the first problematic JDBC API call allowing the programmer early warning to a future problem and then understand and take action, this maybe to use getShort() instread to keep the range or it maybe to disable Exceptions where no truncation occurs (this is upto the programmer).

* Only throw exception when truncation actually occurs, for example reading the value from TINYINY UNSIGNED into a Java byte where the value in the database is between 128 and 255 inclusive.  This is not default because an application maybe in operation a long time before one day it stops working and diagnosing this problem at that time costs a lot of time and money and only make MySQL look bad for not warning the programmer before.

* Never throw an exception, but silently execute some consistant behaviour (this is how I think it already works).  The cheapest (in code and runtime) is to store the value as-is and the UNSIGNED value is converted into its SIGNED 2s compliment value.  This is already done by the copying the raw bit values for the signed type.

So the only impact I am proposing by this is to identify the points in the driver where truncation might occur and call an Exception generator function, that checks the truncation handling mode and raises an exception accordingly.
[12 Dec 2005 15:49] Darryl Miles
I have taken a  look at: http://dev.mysql.com/doc/refman/5.0/en/cj-type-conversions.html

From this I understand that MySQL is advocating I _MUST_ use a Java "int" to fetch and store values for TINYINT and TINYINT UNSIGNED, or does that page serve as a recommendation ?

The reason why I ask is that if I _MUST_NOT_ use getShort() for TINYINT UNSIGNED then maybe an Exception should always get thrown if I do.  But that doesn't help anyone, it does not help developers and it does not help MySQL AB with application migration to MySQL.

My request here is to ratify exactly what diversity of SQL type and JDBC API usage is allowed for a given operation and have a fail fast behaviour so developers fix their application and not let it run into production thinking its working 100%.  The nature of this problem, can cause data corruption to occur because the developer is never warned otherwise, its possible for a computer department to wake up one morning to application failure because the UNSIGNED value crosses into the values 128 to 255.

I am using a Java Object Relation Mapping layer, there is no restriction in that over this behaviour, I can use getShort() to store TINYINT UNSIGNED range values in java.  It would server both developers and MySQL AB well to:

* Be lenient with the SQL type and JDBC API call divertisy (to the point of never failing).
* Fail fast in situations where truncation _MAY_ occur (dependand upon values)
* Allow the developer the option to override behaviour

The problem with introducing such restriction as in the HTML page is that people may find their application works with another SQL server and may not want to port their application into MySQL because of the amount of work and testing.
[12 Dec 2005 16:12] Darryl Miles
UNTESTED Test Case (sorry but cant see any quick way to configure and run the harness)

Attachment: ResultSetRegressionTest.java (application/octet-stream, text), 1.51 KiB.

[12 Dec 2005 16:14] Mark Matthews
The testsuite is run against all current major shipping versions of MySQL on check-in of code, and before release (where that test gets expanded to the latest current JVMs from Sun and BEA, and a matrix of operating systems, including Linux, Solaris and Windows).

As far as the mapping goes, the table shows what a given MySQL type will map to if you use ResultSet.getObject() (just as the similar table in the JDBC specification itself does). It is not intended to specify what methods you _must_ use to get various types.

It does appear to be a bug with server-side prepared statements that you do not get a truncation exception, and getShort() should work, so we'll fix that. Unfortunately, the JDBC specification itself gives no guidance on what to do with UNSIGNED types (since Java doesn't have them, and they're not covered in the SQL specification as far as I can tell).

As far as configuring the exception goes, we want to err on the side of being safe, as we have when using other statements with the JDBC driver. You can either disable the checks (but not the default), or run with them enabled (the default).
[12 Dec 2005 16:33] Darryl Miles
Understood.

Agreed, UNSIGNED is a very useful SQL type and the JDBC doesn't really cover it so this is why MySQL needs to just take a stance on how to handle it, document it, and have test cases to prove consistancy across the product family.

I will continue over the next week to produce a patch for new test cases for every permutatation of SQL data type and JDBC call.  I will create a new bug entry for that issue and attach the patch to track that as I would think it would be useful addition to the testsuite.

The exception situation again I am agreeing, I want 100% safe behaviour by default too, with an exception thrown at the first bad SQL type <> JDBC API call type convesion.  As I dont fully understand your comment on what should and shouldn't be happening in relation to exceptions I wont think anymore on that one and leave it in your hands having lodged my thoughts here.

If you need assistance or dont have time in the near future (~2 months) to resolve the exception issue, feel free to drop it on me with some guidance on how you'd like the matter resolved and I'd be happy to pick it up.

Many Thanks for the fast resolution towards this matter.
[12 Dec 2005 19:39] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/83
[12 Dec 2005 19:42] Bugs System
A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/84
[12 Dec 2005 20:45] Mark Matthews
Turns out it was an oversight, in that all other integral types (where UNSIGNED actually "expands" the range, compared to floating point types, where it just disallows negative values) for server-side prepared statements had code to handle the situation.

I'll commit a testcase later today that covers just this case (as well as bounds checks throwing exceptions), but feel free to contribute a larger scope test if you feel that the coverage for UNSIGNED types in testsuite.regression.ResultSetRegressionTest isn't enough.l
[15 Dec 2005 10:44] Darryl Miles
Just looking over your patch you throw a truncation exception only when a truncation actually occurs.  This would be case 2 of my 3 difference scenarios, this is not fail-fast.

An example of what I'm thinking would look like this, I'll use the example of MEDIUMINT UNSIGNED and getShort() which are both 16bit wide integral types, I would like my code to fail immediatly (even if no truncation occurs because the value is 0 to 32767) this is so that my code does not go into a production system and then suddently fail in 2 years time when the data value in the SQL crosses the 32767 threshold.  These changed would only affect:

TINYINT UNSIGNED => getByte()
MEDIUMINT UNSIGNED => getShort()
INT UNSIGNED => getInt()
BIGINT UNSIGNED => getLong()

I am then proposing the "failFastFlag" be on by default and be configurable by the user.  Then all 1 of my usage cases are covered, with the default being the safest handling.

This is the current code snippet from getShort() :

		case MysqlDefs.FIELD_TYPE_SHORT:
		case MysqlDefs.FIELD_TYPE_YEAR:
			short asShort = getNativeShort(columnIndex + 1);
			
			if (!f.isUnsigned() || asShort >= 0) {
				return asShort;
			}
		
			if (this.connection.getJdbcCompliantTruncation()) {
				int valueAsInt = asShort + 65536;
				
				throwRangeException(String.valueOf(valueAsInt),
						columnIndex + 1, Types.SMALLINT);
			}
			
			return asShort;

What I am proposing for my case 1 (fail-fast is) :

		case MysqlDefs.FIELD_TYPE_SHORT:
		case MysqlDefs.FIELD_TYPE_YEAR:
			short asShort = getNativeShort(columnIndex + 1);
			
			if (!f.isUnsigned()) {
				return asShort;
			}
		
			if (failFastFlag) {
				throwUnsafeConverstionException("MEDIUM UNSIGNED converted with getShort() is not 100% safe use getInt()");
			}

			if (asShort >= 0) {
				return asShort;
			}

			if (this.connection.getJdbcCompliantTruncation()) {
				int valueAsInt = asShort + 65536;
				
				throwRangeException(String.valueOf(valueAsInt),
						columnIndex + 1, Types.SMALLINT);
			}
			
			return asShort;
[15 Dec 2005 10:52] Darryl Miles
Opps... I should have used SMALLINT in my example, that is what I meant a 16bit wide type.
[15 Dec 2005 14:16] Darryl Miles
return asShort;

Shouldn't that be the biased value:

return (short) valueAsInt;

With the baising (+ 65536) outside the if() and in the scope of the return.  I am currently running with a patched driver here, I shall not be able to test your patches until they hit a release.
[15 Dec 2005 14:17] Mark Matthews
The issue with "failFast" being on by default is that it's not SQL standards-compliant (and thus not JDBC compliant), as a truncation condition that causes an exception is defined as "a loss
of its most significant digit", and is evaluated at the time of assignment of the numeric value, so we can't by default do this if we're only comparing possible ranges of the two datatypes.
[15 Dec 2005 14:19] Darryl Miles
Forget that comment, I'm talking crap.  The 2s compliment truncation is the correct thing to do in that situation.  My request for the failFast case still stands.

Would you like me to, test patch and provide test cases on top of the next driver release this behaviour ?
[15 Dec 2005 14:24] Darryl Miles
I dont understand.  There is no JDBC compliant standards for handling UNSIGNED SQL types.

What I am proposing does not affect SIGNED types only UNSIGNED.

So there is no JDBC compliant guidelines for what I am trying to cover.

I would like my code to break immediatly where I use:

TINYINT UNSIGNED => getByte()
SMALLINT UNSIGNED => getShort()
INT UNSIGNED => getInt()
BIGINT UNSIGNED => getLong()

When I say immedialty, I mean even if the value is 0 in the SQL server and no truncation had actually occured.  This means when I test my code I get an error, and find that my JDBC API usage is "unsafe".  I can then elect to continue unsafe "failFastFlag = false" or fix my code.

The concern is that code that is tested while values are in the lower range of each integer width can be pushed into production and then fail many days, months, years just because the SQL data value enters the higher ranges.

This is very bad design and the SQL driver should not allow it by standard and warn the developer on the problem and the fix.
[15 Dec 2005 23:10] Mark Matthews
The JDBC specification states that truncation rules are as in the SQL:1999 standard. It's not relevent whether or not the JDBC specification itself supports unsigned types, as (1) the truncation rules in the SQL spec address them, and (2) a driver vendor would want to follow the SQL specification when addressing truncation issues for consistency's sake anyway.

Therefore, the standards-compliant way to handle this case of truncation, as I understand the SQL standard (and I've had this truncation explained to me before by folks that do more SQL standards work than I can stomach), is that truncation only comes into effect if the _value_ (not the type) causes loss of precision.
[15 Dec 2005 23:25] Darryl Miles
Okay I understand your take on that now.

So would a patch be approved if it were not made the default with an option to enable it ?  Would this feature be considered useful by MySQL AB for other users ?

I'm proposing a MySQL specific exception that is not a truncation exception but a warning of unsafe usage that could cause a truncation exception in this situation.  I've no interest in float <> integer conversion, just to obvious programming error using standard integral widths.

If the JDBC spec does not cover UNSIGNED types then really the specification is talking from a vantage point that does not cover and has had no thought about this situation as it comes from the blinkered viewpoint that java itself takes on that matter.
[15 Dec 2005 23:34] Mark Matthews
I think we'd have no issues with tying the functionality in with the "usage advisor", which already warns against _costly_ type conversions, so "dangerous" type conversions would be similar. See Connection.getUseUsageAdvisor() references in the ResultSet implementation.
[10 Mar 2006 20:26] Mark Matthews
Fix will be available in 5.0.1 and 3.1.13. You can try a nightly snapshot from http://downloads.mysql.com/snapshots.php#connector-j if you want to test the fix before it's officially released. Thanks for the bug report.