NH-4093 - Running Unit tests against SQLite fails on numerous (22) datetime/UTC related tests.

[nhibernate-bot]

Nathan Brown created an issue — 12th October 2017, 6:17:57:

Running the tests against SQLite locally on my machine fails 22 tests, while the same commits are passing on TeamCity.

I noticed that the tests have to do with dates and specifically UTC times. For example:

1) Failed : NHibernate.Test.TypesTest.DateTimeTypeFixture.ReadWrite(Utc)
  Unexpected value.
  Expected: 2017-10-13 03:23:31.0094937
  But was:  2017-10-12 20:23:31.0094937
at NHibernate.Test.TypesTest.AbstractDateTimeTypeFixture.ReadWrite(DateTimeKind kind) in C:\dev\OSS\nhibernate-core\src\NHibernate.Test\TypesTest\AbstractDateTimeTypeFixture.cs:line 176

My computer is in MST or GMT-7:00 timezone.

Attached are all the failures.

---

Frédéric Delaporte added a comment — 12th October 2017, 11:39:20:

SQLite default date storage format is ISO8601. There is a gotcha with it: when SQLite receives a local or unspecified kind date, it stores it without any assumption on the time zone. When it receives an UTC one, it stores it with the UTC marker. Why not, sounds rather good.
Trouble arises when reading them back: SQLite converts them all back to local time. So, for dates saved with local or unspecified kind, it keeps them as is. But for UTC dates, it converts them back to local on reading. And worst, it yields them with unspecified kind, leaving us no chance to detect the case and convert them back. (Even with a correct kind, we would not be able to handle converting them back to UTC in all cases, due to ambiguous local time, like 2017-10-29 02:30:00 in France which could be 2017-10-29 01:30:00 UTC or 2017-10-29 00:30:00 UTC, due to time shift backward occurring at 2017-10-29 01:00:00 UTC that day.)

All that terribly looks as an external issue...

The teamcity build has no troubles since its local time zone is UTC.

SQLite has several options about DateTime storage & handling. Its storage format can be changed (some do not handle any time zone indication), and its read kind can be changed (default is Unspecified, but as written above, with an automatic conversion of UTC dates to local beforehand). That further complicates the issue.

DateTimeFormat:

• ISO8601: distinguishes between UTC dates and others.
• JulianDay: no differences between UTC dates and others, but only stores milliseconds. Avoids the issue, at the cost of a loss of fractional seconds precision.
• Ticks: no differences between UTC dates and others, but said as here only for legacy, not recommended by documentation. Avoids the issue.
• UnixEpoch: no tested but likely like JulianDay but only storing seconds.
• InvariantCulture: stores the offset with local date. Even more failures due to version check, dates being read back as unspecified.
• CurrentCulture: not tested.

About Ticks, the documentation writes:

This value is not recommended and is not well supported with LINQ.
Ticks less compatible with 3rd party tools that query the database, and renders the DateTime field unreadable as text without post-processing.
Ticks is mainly present for legacy code support.

So dodging the issue (for tests with a box not using UTC as local time) by using Ticks sounds not good. It actually causes other tests to start failing.

DateTimeKind: it would be quite hackish to play with this one, since currently the NHibernate date types completely ignore read DateTime kind. Most data providers yield it as Unspecified. But indeed it would work to set it to Utc, if we change the read logic to force the datetime back to unspecified with the non local/utc NHibernate date types. (Without a change to read logic, version checks fail due to read value being specified UTC while db value is not.) This DateTimeKind only alters reading, not writing. (At least it does not cause written local datetime to be converted to UTC.) So it eliminates this conversion of UTC to local on reading which causes us issues.
All that sounds too hackish to me for doing such changes.

Another option could be to override SQLite driver for switching to Unspecified all dates parameters, in order to avoid having their offset/utc flag stored when the underlying SQLite date is able of storing it. This is still a hack for compensating SQLite behavior.

Overall I think this is a SQLite issue, NHibernate code should not try to fix it.

So what to do with those tests failing on boxes having a local time other than UTC? It it worth coding some ignore logic for them? Or just let them fail, since it does not hinder TeamCity?

---

Nathan Brown added a comment — 12th October 2017, 19:08:37:

Hmm. That's interesting... I do think that since SQLite is a supported, tested, database for NHibernate, it needs to support the edge cases like this for any included types.

Are there other databases or ADO.Net drivers that do anything like this for non-offset DateTime values?

Since storing DateTime with a UTC marker doesn't really seem like an SQL thing to do, and since all the databases are SQL, then it would probably be best to store all DateTime values as Unspecified, as that was the previous behavior. Then on reading, NHibernate will assume all DateTimes be Unspecified as well (ignore anything passed from the database).

Usually the goal for having a UTC DateTime column in a database is that the developer is acknowledging that timezone offset wasn't stored, but he promises all the values are in the UTC timezone (for reading) and intends to store them in that timezone.

---

Frédéric Delaporte added a comment — 12th October 2017, 20:04:57:

it needs to support the edge cases like this for any included types

Only if this can be done in a non hackish, reasonable way. It really looks like an inconsistency in SQLite implementation, that needs to be addressed on its side.

Are there other databases or ADO.Net drivers that do anything like this for non-offset DateTime values?

I have tested the types tests with all the others databases tested with TeamCity, and with this locally with my French offset, no issues, unlike SQLite.

Since storing DateTime with a UTC marker doesn't really seem like an SQL thing to do, and since all the databases are SQL, then it would probably be best to store all DateTime values as Unspecified, as that was the previous behavior.

I do not believe previous behavior was doing what you write. Previously, the date and time types were not much tested. I think the issue was already there but unnoticed.

Why not forcing the date kind to Unspecified when setting them for avoiding having some databases storing its offset, since the NHibernate DateTimeType type is supposed to be offset agnostic.
It fixes all DateTimeType tests.
But for LocalDateTimeType (which works anyway without this) and UtcDateTimeType (which currently fails), this is more debatable. This later type is for explicitly sending UTC datetime to the database. So sending to it unspecified datetime just for dodging what really looks like a SQLite bug sounds wrong to me.

Then on reading, NHibernate will assume all DateTimes be Unspecified as well (ignore anything passed from the database).

Already the case for UTC and local NHibernate types, just left untouched otherwise. For my test I have changed a method common to both reading and writing for forcing dates to Unspecified when the type is neither UTC nor local, so it was also forcing it back to Unspecified on reads.

I think this issue needs to be fixed on SQLite side. (It is maybe more a System.Data.SQLite issue.)

At the very worst, maybe do the change I have tested (remove the ternary in this line for always forcing kind) then add an AdjustCommand override in SQLite driver for forcing all its DateTime parameter to Unspecified even if they were Utc or Local. But that would be a crutch for dodging a SQLite issue, crutch which may cause issues if things change on SQLite side.

[ngbrown]

As for the previous behavior for DateTimeType, the default for DateTime property mappings, the DateTime was being reconstructed in a way that defaulted to setting Kind to Unspecified:

st.Parameters[index].Value = new DateTime(dateValue.Year, dateValue.Month, dateValue.Day, dateValue.Hour, dateValue.Minute, dateValue.Second);

By commonizing everything with what used to be AbstractDateTimeSpecifiedKindType, the behavior has changed for the cases in which LocalDateTimeType or UtcDateTimeType isn't specifically specified.

I think changing the ternary is a good idea to always force Kind will keep existing code from being caught up in this.

This issue has been previously reported to the system.data.sqlite issue tracker.

The suggestion of adding to the sqlite query string ;DateTimeKind=Utc breaks even more tests because everything coming back is then marked as UTC. This is actually how DateTimeOffset could be supported (always returning an offset of +0:00).

The code in the ToDateTime does variations on this:

return DateTime.SpecifyKind(DateTime.ParseExact(
    dateText, _datetimeFormats,
    DateTimeFormatInfo.InvariantInfo,
    kind == DateTimeKind.Utc ?
        DateTimeStyles.AdjustToUniversal :
        DateTimeStyles.None),
    kind);

where kind is specified from the connection string value. The signal needed for proper handling would be there if the DateTime.SpecifyKind wasn't wrapping the entire DateTime.ParseExact function.

For SQLite, changing all DateTime parameters at AdjustCommand does end up getting all tests to pass. I think because of how System.Data.Sqlite is hacking the Kind value back and forth because of the connection string, this is the best approach.

[fredericDelaporte]

Your taking behavior of what is now DateTimeNoMs. The current DateTimeType matches what was TimeStampType instead, it does no more cut fractional seconds. And TimeStampType was not reconstructing the datetime in any way, so it was preserving its Kind, as do current DateTimeType. Now this behavior of TimeStamp with kind was probably just accidental design, so I am not opposed to changing it. But it does not seems to be required anyway, hacking AdjustCommand in SQLite driver should be enough, at least if SQLite DateTimeKind setting stays unspecified.

The suggestion of adding to the sqlite query string ;DateTimeKind=Utc breaks even more tests because everything coming back is then marked as UTC. This is actually how DateTimeOffset could be supported (always returning an offset of +0:00).

DateTimeKind=Utc causes all dates, even those stored as local, to be marked as UTC when read back, without conversion, as you have seen in the code extract you show. This is wrong unless the application only stores UTC dates.

The SQLite issue is closed as "as design". I think they have not realized there was actually something wrong, even if the opener trouble was solved by switching to DateTimeKind=Utc: this thing works only if the app only ever write UTC dates, never anything else. When the kind is unspecified, it causes stored utc dates to be read back corrupted, because they have been converted to local by their current parsing algorithm. I think it should be instead:

var date = DateTime.ParseExact(
    dateText, _datetimeFormats,
    DateTimeFormatInfo.InvariantInfo,
    DateTimeStyles.AdjustToUniversal);
if (kind == DateTimeKind.Local && date.Kind == DateTimeKind.Utc)
    date = date.ToLocalTime();
return DateTime.SpecifyKind(date, kind);

It would not convert back and forth local dates because their current string representation with SQLite ISO8601 does not include an offset, so DateTimeStyles.AdjustToUniversal will not convert them (and yield them Unspecified). Utc dates would no more be converted to local when kind is Unspecified. So yes when Unspecified, this would cause read date to be potentially local or UTC. But that is precisely the meaning of DateTimeKind.Unspecified, isn't it?

For SQLite, changing all DateTime parameters at AdjustCommand does end up getting all tests to pass. I think because of how System.Data.Sqlite is hacking the Kind value back and forth because of the connection string, this is the best approach.

This will solve our test cases, but may cause issues to users storing only UTC dates combined with DateTimeKind=Utc in their connection string. Such a hack would likely have to be configurable... Yet another setting.

[ngbrown]

Instead of further guessing, I made a unit test that checks all the combinations that I could think of: https://gist.github.com/ngbrown/195e09681572371011fb2c78c5fcf04d

The best combination is:

private int _optionToStringInSqLite = 1; // Currently implemented in System.Data.SQLite

private int _optionToDateTimeInSqLite = 1; // Currently implemented in System.Data.SQLite
//private int _optionToDateTimeInSqLite = 2; // proposed by frederic
//private int _optionToDateTimeInSqLite = 3; // variation proposed by nathan - would let UTC marker back through on read if saved

//private int _optionAdjustCommand = 1; // no driver change
private int _optionAdjustCommand = 2; // all date kind to Unspecified

//private int _optionAdjustInType = 1; // current state in NHibernate
private int _optionAdjustInType = 2; // proposed by Nathan

Changing ToDateTime in System.Data.SQLite can get the failure count from 7 to 4 or saving all DateTime to SQLite as Unspecified gets down to 1 failure, it also takes setting all Kind returned from SQLite to Unknown to work in all scenarios. Otherwise, a failure happens for the NHibernate UtcDateTimeType when the connection string has ;DateTimeKind=Utc:

 Expected: 2017-10-12 11:32:42.356-07:00
  But was:  2017-10-12 11:32:42.356+00:00

   at UnitTestProject1.SQLiteDateRoundTripTests.RoundTripWithConnectionKindFromOriginalKindCompareDateTimeOffset(DateTimeKind connectionKind, DateTimeKind typeKind) in C:\dev\Learning\NHibernate-Temp\UnitTestProject1\SQLiteDateRoundTripTests.cs:line 133

as set: 2017-10-12 11:32:42, kind: Unspecified
as saved in SQLite: "2017-10-12 11:32:42.356Z"
as retreived: 2017-10-12 11:32:42, kind: Utc
as adjusted: 2017-10-12 11:32:42, kind: Utc

The unit tests show that the my pull request as currently implemented is will continue to work in all scenarios even if the ToDateTime implementation is changed in the System.Data.SQLite code.

[fredericDelaporte]

I had not seen that SQLite DateTimeKind option set to UTC was causing it to store unspecified dates as UTC. This means that by overriding our SQLite driver AdjustCommand for forcing all dates to unspecified, we will cause local date to be stored as being UTC instead in the database, without time adjustment. What a mess. This is data corruption. Even if the way we read them back allows NHibernate to have them back as local still without time adjustment, this is not satisfactory, because the stored data is not the expected one.

[fredericDelaporte]

@hazzik, it does not look to me as a regression, but as an old overlooked trouble. Testing the 4.1.x branch, there is already the UtcDateTimeTypeFixture.ReadWrite which fails when the local time zone is not UTC, while other types like DateTimeType, DateTime2Type, TimestampType were not even testing a read-write operation in that branch. Types having some read-write tests were not testing the different combination of NHibernate types with datetime kinds either.

Adding a test into that branch for timestamp shows it is failing the same than current 5.0 with SQLite and an utc date as input. And since current DateTimeType is indeed TimestampType from 4.1, there is no regression. Switching DateTimeType to DateTimeTypeNoMs, which is the 4.1 DateTimeType counterpart in 5.0, still give back the old 4.1 DateTimeType behavior.

[fredericDelaporte]

The more I look into the less I am agreed with trying to address this. It looks like we are going to add our own layer of mess above SQLite inconsistent date handling for "compensating" it at least within NHibernate, but what is in for any direct access to the actually stored data an app may do?

I am still thinking the very most we should do is to only override the SQLite driver AdjustCommand but with an option to disable this overriding.

[fredericDelaporte]

I have submitted a ticket on SQLite driver side just in case.

[fredericDelaporte]

There is a third SQLite datetime related connection string parameter I had overlooked till now: DateTimeFormatString. Setting it to a format UTC/offset agnostic such as yyyy-MM-dd HH:mm:ss.FFFFFFF, which is the format used by SQLite for ISO8601 when datetime kind is not UTC, makes all types tests green.

So now I think the right way to fix this issue is to instruct SQLite to be datetime kind agnostic by using DateTimeFormatString=yyyy-MM-dd HH:mm:ss.FFFFFFF; in its connection string. Going to PR that after having checked locally that it works with all 10 000+ tests.

[fredericDelaporte]

I have submitted a ticket on SQLite driver side just in case.

Closed, work as designed, though the closer comment suggests he is agreeing it would be better to change that, but he is unwilling to do a breaking change.

Anyway I think we have a valid work-around for this System.Data.SQLite glitch as done in #1378, with an easy recommendation to give to who would face the issue, and which is included in documentation by this PR.

[dawncold]

This is my session factory configuration, it works for me.

sessionFactory = Fluently.Configure()
                    .Database(SQLiteConfiguration.Standard.InMemory().ConnectionString(cs => cs.Is("Data Source=:memory:;Version=3;New=True;DateTimeFormatString=yyyy-MM-dd HH:mm:ss.FFFFFFF;")))
                    .Mappings(m => m.FluentMappings.AddFromAssembly(Assembly.GetAssembly(typeof(TigerBillingModule))))
                    .CurrentSessionContext<ThreadStaticSessionContext>()
                    .ExposeConfiguration(cfg => inMemDbConfiguration = cfg)
                    .BuildSessionFactory();