SQLiteDialect: FormatException due to string conversion of DateTime fields.

[deAtog]

The SQLiteDialect stores Date, DateTime, and Time types in TEXT fields. The conversion from DateTime to text is done using the current culture of the local computer instead of the InvariantCulture. Writing to the database in one culture and then attempting to read from it in another can result in a FormatException. When this occurs, a FormatException is thrown similar to the following:

System.FormatException: Input string '2016-02-29 00:00:00' was not in the correct format. ---> System.FormatException: String was not recognized as a valid DateTime.
at System.DateTimeParse.Parse(String s, DateTimeFormatInfo dtfi, DateTimeStyles styles)
at System.Convert.ToDateTime(String value, IFormatProvider provider)
at NHibernate.Type.AbstractDateTimeType.GetDateTime(DbDataReader rs, Int32 index, ISessionImplementor session)

Date, DateTime, and Time types when stored as text should be stored in a universal format using the InvariantCulture and parsed using the InvariantCulture to avoid such issues.

Resolving this issue is a breaking change as the format of the current culture may not match the invariant culture.

[deAtog]

I dug into this issue a little more, and here's what I found. The storing of DateTime values as text is handled internally by the System.Data.SQLite driver. When storing data, NHibernate creates a Command object with a Parameter that has a DateTime type. The System.Data.SQLite driver then converts the value to the appropriate database type based on the DateTimeFormat, DateTimeKind, and DateTimeFormatString as specified in the Connection string to the database. The SQLiteDialect indicates that Date, DateTime, and Time types should be stored in TEXT columns. Comments indicate that if a column of DATETIME is used, then the System.Data.SQLite driver will handle all the converting to and from the database type to a DateTime value based on the connection string parameters. The default DateTimeFormat used by the System.Data.SQLite driver for columns of DATETIME is a string in the ISO8601 format. This is the same format it will use when converting a DateTime value to a string for a TEXT column. However, the System.Data.SQLite driver will not attempt to convert the value of a TEXT column to a DateTime value when reading from the database and will return it as a string instead. This causes the subsequent Convert.ToDateTime call in Nhibernate to fail when the string value cannot be correctly parsed in the current locale. In my opinion, the SQLiteDialect should use the DATETIME type as understood by the System.Data.SQLite driver. This would then turn the Convert.ToDateTime call in NHibernate into a no-op call and give users complete control over how the value is stored by supplying the appropriate connection string parameters.

[fredericDelaporte]

May you have a look to #2346 and comments added in code by this PR? It seems you consider we should map datetime as DATETIME. It was previously done but it has been changed by #2346 due to issues it was causing, and because that type is unofficial and was, at least at the time, reserved for internal use of the SQLite driver.

[deAtog]

The DATETIME type used by the SQLite driver is internal in the sense that it is not an official SQLite column type and is only used by the System.Data.SQLite driver. The Entity Framework library provided in the same source repository uses the DATETIME type as implemented by the SQLite driver as do many of their tests. The problem with using the DATETIME column type is that the resulting database may be incompatible with other applications or libraries. The Microsoft.Data.SQLite driver, for instance, uses a TEXT column for DateTime values. In my opinion, this is a non-issue because a separate NHibernate dialect and driver would be needed to use the Microsoft.Data.SQLite library. Put simply, the System.Data.SQLite driver requires a non-standard SQLite dialect. Needless to say, using a DATETIME column will only fix queries from a table. DateTime values generated in SQL will still not be read correctly as they will be seen as TEXT by the System.Data.SQLite driver unless the DetectTextAffinity or DetectStringType flag is set on the Command or Connection. Setting these flags on a per connection basis would add a significant amount of overhead but an option to do so should nonetheless be provided by the NHibernate driver.

[fredericDelaporte]

Typing with the SQLite driver internal type DATETIME causes the type to have a NUMERIC affinity instead of a TEXT one, see here. This wrecks casts to the DbType.DateTime, because they then attempt to cast the string representation of the date to a number, which corrupts the value.

I have just checked the doc by downloading it there, that type is still for internal use only.

The correct fix is likely to do a culture agnostic conversion in the AbstractDateTimeType.

That said, SQLite "ISO 8601" is not actually ISO 8601: the date and time parts have to be separated by a T character, not by a space like SQLite does. That may further complicate the fix.

[deAtog]

I think you are misinterpreting what the docs are saying here. The documentation referenced above refers to only to the affinity of a column. As stated in the Type Affinity section, SQLite only has five affinities: TEXT, NUMERIC, INTEGER, REAL, and BLOB. The DateTime affinity is only used by the System.Data.SQLite library. The Affinity Name Examples section states that a DATETIME column has a NUMERIC affinity. It further states, in the Type Affinity section, that a column with NUMERIC affinity may contain values from any of the five storage classes (NULL, INTEGER, REAL, TEXT, or BLOB). Thus the fact that a DATETIME value has a NUMERIC affinity is irrelevant. When you declare a column with a type of DATETIME, SQLite will give it a NUMERIC affinity but the System.Data.SQLite library will give it a DateTime affinity. How the System.Data.SQLite driver stores a value in a column with a DateTime affinity is controlled by the connection string DateTimeKind, DateTimeFormat, and DateTimeFormatString parameters. By default the System.Data.SQLite library will store values with a DateTime affinity in a TEXT form using the ISO 8601 format as you mentioned. According to the Type Affinity section, since the ISO 8601 format cannot be converted to an INTEGER or REAL, it will be stored as TEXT. If however, the DateTimeFormat property of the connection string is set to UnixEpoch, then the System.Data.SQLite library will store an integer value which will result in the database storing the value with the INTEGER storage class, but the column will still have a NUMERIC affinity.

It should be noted, that storage class, affinity, and data type are all different despite the overlap in their names. Put simply, the DATETIME data type has a NUMERIC affinity that will use the either the NULL, INTEGER, REAL, TEXT, or BLOB storage class depending on the value being stored. The System.Data.SQLite library will however give columns that are declared as DATETIME a DateTime affinity as a means for converting to/from the user specified format.

[deAtog]

I think I've come up with a very minimal fix here. The GetDateTime method of SQLiteDataReader in the System.Data.SQLite library will attempt to convert a column value to a DateTime if it is one of the supported types, however the GetValue and this[] methods will only attempt to convert a column value to a DateTime if the DetectTextAffinity or DetectStringType flags have been set. Currently, the GetDateTime method of the AbstractDateTime type in NHibernate calls Convert.ToDateTime on the value returned by the this[] method of the associated DbDataReader. If this modified to call the GetDateTime method of the DbDataReader instead, SQLite will return a correctly interpreted DateTime value without NHibnerate needing to perform an additional cast. Admittedly, this should be better than casting the value directly as it allows the database drivers to perform the cast when needed.

The current code looks like:

protected virtual DateTime GetDateTime(DbDataReader rs, int index, ISessionImplementor session)
{
	try
	{
		return AdjustDateTime(Convert.ToDateTime(rs[index]));
	}
	catch (Exception ex)
	{
		throw new FormatException($"Input string '{rs[index]}' was not in the correct format.", ex);
	}
}

The corrected code looks like:

protected virtual DateTime GetDateTime(DbDataReader rs, int index, ISessionImplementor session)
{
	try
	{
		return AdjustDateTime(rs.GetDateTime(index));
	}
	catch (Exception ex)
	{
		throw new FormatException($"Input string '{rs[index]}' was not in the correct format.", ex);
	}
}

[fredericDelaporte]

I should have started with this: can you provide a test case showcasing the issue? We already have many tests writing and reading datetime values without issues, so we need a concrete example triggering it.

By the way, maybe the trouble could be fixed by simply instructing SQLite to use an actual ISO 8601 date format through the DateTimeFormatString connection string property, set as yyyy-MM-ddTHH:mm:ss.FFFFFFF. It is natively supported by SQLite to have it represented as an actual ISO 8601 datetime, see here. But without a failing test case, I cannot asses it.

According to the history of the source code, your fix proposal could break Oracle. See 990cd6a. That is twenty years old, so, it may not be the case. But it may be the case for some other databases. Although our CI tests many different databases, it is not testing all the supported ones. So, doing the proposed change which is less forgiving about what the reader can send to us could break one of the untested databases without us noticing it.

[deAtog]

Oof.. That's a pretty glaring oversight in their driver. A better fix would have been to create a wrapper around the Oracle driver to address the deficiencies in their implementation.

Changing the format is a non-starter here as the System.Data.SQLite driver will always convert DateTime parameters using the connection string parameters and users should be free to specify how datetime values are stored in their database.

I will see if I can put together a test case here, but I think 990cd6a should be reverted as it is wrong on so many different levels. Any type conversions should be left to the underlying driver to perform as they can ensure that the value is interpreted using the correct locale. The changes in that commit not only cause the parsing of DateTime values to fail as shown here, but will also cause the parsing of numeric values stored in text fields to fail as well if the database is opened in a different locale.

[hazzik]

This has been discussed numerous times already. Our documentation states that correct way to prevent conversion of dates to local by SQLite is to use DateTimeFormatString=… connection string option.

[fredericDelaporte]

deAtog case seems to be something else since he is not reporting a time shift but a format exception on read.

Still, setting that option could solve the issue. If it is actually solving it, but the user does not accept this way of solving it, then he could use a custom type instead, in order to handle the reading with another logic.

[hazzik]

deAtog case seems to be something else since he is not reporting a time shift but a format exception on read.

It would be the same.

[deAtog]

I wholeheartedly disagree with this approach. If an existing database stores DateTime values as the number of seconds since the Unix Epoch, a user should be able to use the appropriate connection string properties to allow the System.Data.SQLite driver to read and write those values appropriately. The calls to Convert added by 990cd6a introduced locale issues when converting values from the underlying DbDataReader. I have no doubt that locale specific tests will undoubtedly fail for decimal and DateTime values alike due to the changes introduced by that commit. If anything, NHibernate should call the type specific methods of the DbDataReader first and then fall back to using Convert if that fails. The above referenced change doesn't state if the Oracle driver threw an exception or not, so that may not work for that driver.

[deAtog]

I was able to create a test which shows this issue with SQLite. I would like to extend the test to include mapping numeric properties to text fields, but I can't seem to figure out a way to override the type used for generating the columns. If I specify type="String" on the properties, then the SchemaExporter will generate a text column, but it will also expect a string value from the database. I know this isn't officially supported, but since the introduction of calling Convert on the values returned from the DbDataReader, it is entirely plausible that someone somewhere has a database that stores numeric values in text fields.