NH-3078 removing a datetime to timespan conversion

[fredericDelaporte]

NH-3078, as per #631 discussion.

[fredericDelaporte]

Of course it will have to be rebased without the npgsql changes if we want to merge this.

[hazzik]

It does not work for SQLite, so it will require the fix similar to SqlServerCe

[hazzik]

TimeType will require the similar fix.

[hazzik]

This is in line with the GuidType, which uses the similar approach by replacing Guid with Binary if former is not supported/expected.

[hazzik]

This fix makes AdjustCommand of Sql2008ClientDriver obsolete (the body of the loop never gets called now)

[hazzik]

Does not work for Oracle

[fredericDelaporte]

Yes, I was expecting failure for TimeType tests, but I wanted to check that point (while not having much time to do it locally, I had to go out at that moment).

[fredericDelaporte]

For Oracle, that is not the tests I would have expected which fail. SQLite demonstrates some providers are not ready for the switch. We may override AdjustCommand for SQLite in order to put there the convert. But doing the change may cause regressions for other databases (or even database versions) we do not test.

So even if this change would be cleaner for databases like SqlServer or PostgreSql with up-to-date Npgsql, I do not think it is worth it, due to the high likeliness of regressions for some old or untested databases.

[fredericDelaporte]

Yes, I was expecting failure for TimeType tests

But there are no failures... Strange. Going to check the tests.

[fredericDelaporte]

Not based on the "good" branch, it has not used the new npgsql driver. Changing that.

[hazzik]

"A more robust solution" in my mind would be the following;

1. Add a property "RequiresTimeSpanToStoreTime" to a driver (better name is welcome).
2. Pass driver to Set/Get methods of a XxxType (We already broke the compatibility for these methods when we removed EntityMode parameter, so it should be fine).
3. Set the parameter/read columns based on the property instead of the current fragile logic.

[fredericDelaporte]

1. Add a property "RequiresTimeSpanToStoreTime" to a driver (better name is welcome).

Looking to do that in NH-4017.

2. Pass dialect to Set/Get methods of a XxxType.

You mean driver rather than dialect? Anyway the IType has instead setter and getter which all takes ISessionImplementor. So for maximum flexibility, maybe just pass that to all descendants overloads/others setter&getter currently not taking it.

(We already broke the compatibility for these methods when we removed EntityMode parameter, so it should be fine.)

Had impacted GetHashCode, not those concrete types setter/getter. But anyway, as written in xml comments on IUserType:

Alternatively, custom types could implement <see cref="Type.IType"/> directly or extend
one of the abstract classes in <c>NHibernate.Type</c>. This approach risks future changes
to incompatible classes or interfaces in the package.

So it should be fine provided we left IUserType untouched.

3. Set the parameter/read columns based on the property instead of the current fragile logic.

For set, sure. For get, it looks to me current code, which tries casting, is more reliable than using some declarative property.

[hazzik]

Closing in favor of #637. Feel free to reopen, if needed.