Issue Details (XML | Word | Printable)

Key: NUCRDBMS-214
Type: New Feature New Feature
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Unassigned
Reporter: Michael Brown
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
DataNucleus Store RDBMS

Capability to persist all date types as string, in a manor that is consistent and honours timezone settings

Created: 29/Jun/09 02:28 PM   Updated: 11/Sep/09 08:41 AM   Resolved: 18/Jul/09 03:03 PM
Component/s: ORM
Affects Version/s: 1.1.4
Fix Version/s: 1.1.5, 2.0.0.m1

File Attachments: 1. Text File datetests.patch (50 kB)
2. Text File datetests2.patch (23 kB)
3. Text File dn-patch.patch (6 kB)


Datastore: MySQL
Severity: Development


 Description  « Hide
We need the ability to persist all Date types as a string in a way that consistently honors the timezone settings of the host and database, is portable across regions, and has consistent sort and search semantics.

I have made changes, and produced a patch to add this capability. And if accepted, I will produce a set of test cases also.

In addition, I have seen that these is a prior related issue NUCCORE-201, which uses a string converter. I am concerned that the Date toString method used in this approach will not produce correct sort semantics.

I am open to discussion on this issue, as I am as always not 100% familiar with DN code. I have attempted to discuss via the forums, without success. Hence going ahead and attempting to make the changes myself.

Sort Order: Ascending order - Click to sort in descending order
Michael Brown added a comment - 29/Jun/09 02:29 PM
Unfortunate that I run windows, otherwise a patch would be so much easier to create.. Anyway, I have tried using WinMerge to create a unified patch. Please advise if this is not acceptable.

Andy Jefferson added a comment - 30/Jun/09 12:16 PM
"Patch" applied (manually, since it didn't apply). Thx. This however is of no use to other datastores and that is the reason for having the generic String converter. Consequently would make sense to updates to that where appropriate.

At the end of the day, unit tests are what define behaviour and the code in core and the datastore-specific plugins should provide that handling. So need to wait til the tests define the required behaviour then comment can be made.

Andy Jefferson added a comment - 07/Jul/09 08:38 AM
Testcase?

Michael Brown added a comment - 09/Jul/09 11:38 AM
I am unfortunately delayed by some other urgent issues at work :(... Test case will be forthcoming.

Andy Jefferson added a comment - 18/Jul/09 03:03 PM
Closing since this project has to move forward and we have to have releases. I expect the testcases to arrive when you have time; attach them here

Michael Brown added a comment - 24/Aug/09 01:03 PM
Please find attached a patch providing the following:

 - Slight addition to plugin.xml allowing Date as VARCHAR in addition to the existing CHAR.
 - New DateHolder, SqlDateHolder and SqlTimeHolder sample classes for tests.
 - java.util.Date, java.sql.Date and java.sql.Time persist as VARCHAR tests.
 - And additionally, I noticed that there wasn't a generic Date as Date test, so I added that also.

I changed my development environment to Ubuntu from Windows, and have started working from the trunk out of SVN, updating daily.
The patch was generated by 'svn diff' at the root of the repository after an update, build and test.

I'll watch this issue just in case there are any problems.

Michael Brown added a comment - 25/Aug/09 10:43 AM
Embarrassingly I must confess that there is an error in that patch.

It's just one line, so easier just to mention it.

datanucleus/platform/store.rdbms/trunk/plugin.xml line 499
The VARCHAR mapping for java.util.Date references CharRDBMSMapping, but it should be VarCharRDBMSMapping.

Andy Jefferson added a comment - 26/Aug/09 10:49 AM
Don't understand the tests location. You have them under test.jdo.general, yet also test.jdo.datastore. Are they general or are they specific to datastore id ? Normally they would either go in "test.jdo.general" or under ("test.jdo.datastore" AND "test.jdo.application").

Andy Jefferson added a comment - 26/Aug/09 10:52 AM
Also in your sample you have "strings". Why is a sample for Date persisting Collection<String> ?

Andy Jefferson added a comment - 26/Aug/09 11:21 AM
Checked in change to store.rdbms. Thx.
Also added test.samples, but ditching that "strings" field and adding a value2 to each; so you can have value being persisted as the main type, and value2 as VARCHAR for example. That way you can check multiple things with one sample.
Also added package.jdo and test for "test.jdo.general". Suggest that you review these and look at merging your tests that were under "test.jdo.datastore" (datastore identity) into that, using the "value2" field for storage

Michael Brown added a comment - 26/Aug/09 01:36 PM
As noted, the test was derived from the existing URI test and URIHolder. I believe that the reasons for having a collection of string as they where there still stand. The strings themselves are misleading. They are simply a type that is unlikely to cause issue, in order that the tested type can be used in a join, in a similar vein to the use of the tested type as a key field. As a pattern I believe that it is good, and hoped that you would accept it again.

With regard to two types of test. As noted before, I added general date tests for default persistence, because I noticed that there where none. These tests are not datastore dependent.

I also added tests specifically for the persistence of date as varchar, which is datastore dependent. However, I am thinking that I should perhaps have placed them in the test.jdo.rdbms.datastore.

I will update from svn and review the changes you have applied.

Andy Jefferson added a comment - 26/Aug/09 01:50 PM
Ok, I vaguely remember you adding contribing some patch a long time ago about some reference to a class with UUID as PK, so that was the origin of the Collection<String> then was it ? Maybe better to do Collection<DateHolder> or Collection<UUIDHolder>, and also add comment around it so any unsuspecting person knows this is what it is for ;-)

Regarding "test.jdo.datastore", this is not "datastore specific", it is "datastore identity", just like "test.jdo.application" is "application identity". Unless a test is using something totally specific to RDBMS (e.g SQL, or some specific JDBC type) then the test is not RDBMS specific. e.g if storing a timezone in a String based datastore field then that applies to any datastore (I can persist a Date as a String into XML, or ODF if I want)

Michael Brown added a comment - 27/Aug/09 04:03 PM
I've had a look, and understand what you mean about the test projects.
Also I've tried to be clearer about the purpose of the tests in comments.
Tests have been merged.

I'll leave out the collections, as the existing UUID test does I believe cover the case in question. If a problem does arise I could of course amend the tests.

I've had to make one change to the mappings for key fields, changing them to varchar. The date as string time zone tests do rely on being able to find the object, but some databases react differently to time zones, so having a default mapping for date may cause them to fail where they shouldn't. This doesn't effect the default date mapping tests, as they rely on the value field more than the key field.

The date tests pass currently. We use a variety of databases here, and the inconsistencies between them cause us pain. So it is very nice to have varchar support with a single consistent behavior across databases. I hope others will benefit too.

Andy Jefferson added a comment - 28/Aug/09 02:36 PM
Applied tests for SVN trunk. Thx