Bug #54095 Unnecessary call in newSetTimestampInternal
Submitted: 31 May 2010 7:54 Modified: 10 Feb 2015 0:05
Reporter: Roman Bednarek Email Updates:
Status: Closed Impact on me:
Category:Connector / J Severity:S3 (Non-critical)
Version:5.1.12 OS:Any
Assigned to: Filipe Silva CPU Architecture:Any
Tags: calendar, timestamp

[31 May 2010 7:54] Roman Bednarek
 There is an unnecessary call targetCalendar.setTime in newSetTimestampInternal in PreparedStatement.java.
 It modifies user supplied Calendar object, which can have side effects.

How to repeat:
Calendar c=Calendar.getInstance();
System.out.println(c.getTime());  // <- modified inside driver

Suggested fix:
remove the code 
[1 Jun 2010 8:14] Tonci Grgin
Hi Roman and thanks for your report.

I kinda see what you're getting at but I'll need you to attach a complete, one procedure test case with all the DML/DDL inside. I'm especially eager to see your connection string and procedure definition.
[1 Jul 2010 23:00] Bugs System
No feedback was provided for this bug for over a month, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".
[2 Jul 2010 6:40] Tonci Grgin
This still needs followup from the reporter.
[8 Jul 2010 6:30] Roman Bednarek
sql to create table for testcase

Attachment: bug54095.sql (application/octet-stream, text), 107 bytes.

[8 Jul 2010 6:31] Roman Bednarek
simple testcase

Attachment: bug54095.java (text/x-java), 808 bytes.

[9 Jul 2010 6:21] Tonci Grgin
Roman, as I suspected, putting &useServerPrepStmts=true in your connection string solves the problem:
Connected to 5.1.31-log
java.vm.version         : 1.5.0_17-b04
java.vm.vendor          : Sun Microsystems Inc.
java.runtime.version    : 1.5.0_17-b04
os.name                 : Windows Server 2008
os.version              : null
sun.management.compiler : HotSpot Client Compiler
newYear=Fri Jan 01 00:00:00 CET 2010
newYear=Fri Jan 01 00:00:00 CET 2010

Time: 0,662

OK (1 test)

Now, driver should modify user supplied Calendar object in certain situations but I'm not yet convinced this is such case so "Analyzing" still.
[9 Jul 2010 6:32] Tonci Grgin
Forgot to say, with useServerPrepStmts=false (the default) I get this:
newYear=Fri Jan 01 00:00:00 CET 2010
newYear=Thu Jan 01 01:00:00 CET 1970
[9 Jul 2010 7:37] Bogdan Degtyariov
It seems the problem is in useLegacyDatetimeCode=false, removing this option or setting it to true (Default) solves the problem
[14 Jul 2010 11:04] Roman Bednarek
I did not mention it, but the code in question newSetTimestampInternal is a new code used with option useLegacyDatetimeCode=false.
[20 Jul 2010 9:10] Tonci Grgin
Roman, I am not sure there is a bug here at all... The combination of connection options led driver to change the timestamp...
[20 Jul 2010 12:45] Roman Bednarek
Main reason for reporting a bug was, that the call is unnecessary.
[20 Jul 2010 13:51] Tonci Grgin
Mark informs me that JDBC specs are silent on  whether the calendar is "captured" or not. So, to fix this, we will create an option that makes a copy (which will not be the default behavior!).
[21 Jul 2010 6:31] Roman Bednarek
Adding an option is a solution, but there were many different time handling options in the past and the new handling code with useLegacyDatetimeCode=false, could just work, without new set of options.
[21 Jul 2010 6:32] Tonci Grgin
Roman, Mark will decide on that.
[11 Oct 2012 11:22] Alexander Soklakov
I you take a look at legacy code you find that behavior was the same:

setTimestampInternal(int parameterIndex, Timestamp x, Calendar targetCalendar, TimeZone tz, boolean rollForward) -> TimeUtil.changeTimezone(...) -> jdbcCompliantZoneShift(sessionCalendar, targetCalendar, tstamp) ->
			targetCalendar.set(Calendar.YEAR, sessionCalendar.get(Calendar.YEAR));
			targetCalendar.set(Calendar.MONTH, sessionCalendar.get(Calendar.MONTH));
			targetCalendar.set(Calendar.DAY_OF_MONTH, sessionCalendar.get(Calendar.DAY_OF_MONTH));

			targetCalendar.set(Calendar.HOUR_OF_DAY, sessionCalendar.get(Calendar.HOUR_OF_DAY));                
			targetCalendar.set(Calendar.MINUTE, sessionCalendar.get(Calendar.MINUTE));
			targetCalendar.set(Calendar.SECOND, sessionCalendar.get(Calendar.SECOND));
			targetCalendar.set(Calendar.MILLISECOND, sessionCalendar.get(Calendar.MILLISECOND));                

So there is nothing new here with Calendar capturing.

But for me it looks ugly. People should be explicitly warned when some not void method changes passed parameters. And if this is not contained in spec then it should mean "not to change", not "do as you wish".

Tonci, Mark?
[10 Feb 2015 0:05] Daniel So
Added the following entry to the Connector/J 5.1.35 changelog:

"There was an unnecessary call of targetCalendar.setTime() in the newSetTimestampInternal() method of the PreparedStatement.java class, which modified the user-supplied Calendar object and caused side effects. The unncessary call has now been eliminated by the fix put in for Bug #18028319/Bug #71084."