New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect commit_date storing in SQL DB #325

Closed
floresek opened this Issue Jan 28, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@floresek

floresek commented Jan 28, 2016

I noticed that 31-12-2015 (13f476b) you changed storing of jv_commit.commit_date from local timestamp to ?

I guess that your intention was to store commit time in UTC time zone but:

// class org.javers.repository.sql.repositories.CommitMetadataRepository

    private Timestamp toTimestamp(LocalDateTime commitMetadata) {
        return new Timestamp(commitMetadata.toDate(TimeZone.getTimeZone("UTC")));
    }

gives the time which would be in defult time zone (local) if passed (commitMetadata) time will be set in UTC time zone.
F.e. in Europe/Belgrade local time 18:00 is converted to 'UTC' 19:00 but should be to: 17:00

Proper solution should be to create adequate DB colum type (f.e. TIMESTAMP WITH TIME ZONE in Oracle)
and set column value in different way (not resulting to JDBC setTimestamp(idx, ts) as currently is).

JDBC specification suggests using setTimestamp(idx, ts, calendar) - (ts local; calendar in UTC time zone)
but in Oracle it causes saving default time zone instead of UTC (with offsetting the time valid for UTC).

Correct result can be achieved by passing string - date converted to UTC f.e.:

def c = Calendar.getInstance(TimeZone.getTimeZone('UTC'))
def sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss z")
sdf.setTimeZone(TimeZone.getTimeZone('UTC'))
def stmt = conc.prepareStatement('INSERT INTO TEST (ts_with_tz) VALUES(?)')
def ts = new Timestamp(new LocalDateTime().toDate().getTime())
stmt.setString(1, sdf.format(ts))

If you want to to store UTC offsetted time in TIMESTAMP column you can use f.e.:

    private Timestamp toTimestamp(LocalDateTime commitMetadata) {
        return new Timestamp(DateTimeZone.getDefault().convertLocalToUTC(commitMetadata.toDate().getTime(), true))
    }

By the way - doing such a change you should warn in release notes that conversion of existing jv_commit.commit_date data is required!
(and provide update scripts)

Cheers

@bartoszwalacik bartoszwalacik added the bug label Jan 28, 2016

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 28, 2016

ouch, you are right. Think we dont want to deep dive into Oracle calendars ...
We will change SqlRepository to store commitDate as is (so as local datetime)

bartoszwalacik added a commit that referenced this issue Jan 29, 2016

#325
fixed commitDate serialization in SqlRepository
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 29, 2016

take a look at my commit

@floresek

This comment has been minimized.

floresek commented Jan 29, 2016

ok - but I think that storing commit date in UTC time zone should be applied in javers - especially that you recently added .from(date) .to(date) filters to repository (which will not work property for daylight saving change periods).

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jan 29, 2016

Well, I dont think that storing commitDate in UTC is a good idea. We should strive to use local time.
It's much easier to understand and manipulate your data using raw SQL when time is not shifted to UTC.

For now I dont know what to do with daylight saving cornercases, we should investigate it more...

@floresek

This comment has been minimized.

floresek commented Jan 29, 2016

ok - could you release new version with UTC bug fix, please?

bartoszwalacik added a commit that referenced this issue Jan 30, 2016

#325
removed redundant test

bartoszwalacik added a commit that referenced this issue Jan 31, 2016

#325
javadoc

bartoszwalacik added a commit that referenced this issue Jan 31, 2016

#325
fixed commitDate serialization in SqlRepository

bartoszwalacik added a commit that referenced this issue Jan 31, 2016

#325
removed redundant test

bartoszwalacik added a commit that referenced this issue Jan 31, 2016

#325
javadoc

@bartoszwalacik bartoszwalacik added the fixed label Feb 2, 2016

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 2, 2016

fix released in JaVers 1.4.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment