NH-4017 - Handle Time parameter conversion for newer Npgsql

[fredericDelaporte]

NH-4017 - Handle Time parameter conversion for newer Npgsql

The change appears to be supported by older version of Npgsql, so I am coding it for all instead of checking the Npgsql version and doing it only for v3 and above.

[hazzik]

LGTM

[hazzik]

It all looks good, however, I think we need more robust solution to be introduced.

[fredericDelaporte]

More robust about the version check or about the conversion? If the version check does not suit you much, I may still split that into two drivers, choose the most up to date as default and then let the user configure the older one if he needs it.

[hazzik]

More robust about the version check or about the conversion?

More robust about setting these parameters. I think we need to move the parameter setup earlier in the pipeline (not sure where as I did not check the code).

[fredericDelaporte]

Maybe not easy to do. In the case of failing tests for Npgsql 3 without that changes, earliest in the pipeline would be in the type classes, such as TimeAsTimeSpanType. Those types classes have currently no knowledge of target database and driver. They do not have any references to the session factory, and adding one would be a major change, causing those classes to no more be usable as singleton, as they are currently used.

So we will have to check if that could make sens to that at some intermediate stage between types and driver.

But in the Npgsql case, having it quite late in the pipeline does not look to me bad: Npgsql does no more accept DateTime as time value and fails with them, while other databases may still want DateTime. Converting it that late allows to catch all cases, even user own supplied parameters, allowing user to stay database agnostic. Otherwise he would have to adapt his parameter values according to the target db.

The other case is SQL Server 2008 and above: I do not know if this case still accepts DateTime as time or does throw. If it is the former, doing it that late is then less legit.

[hazzik]

I'm just thinking out loud, feel free to ignore:)

---

MySQL, Sql Server ODBC, and Sybase Advantage Database fail with the same results: they want TimeSpan instead of DateTime for DbType.Time

[fredericDelaporte]

No trouble, having concern about how to do thing reasonably better is always welcome. ("Reasonably" because we say in french "better is enemy of good", meaning wanting to do better while already having something good may lead to bad indeed.) So if we agree it is good enough as is currently, well, I will merge. And maybe do you want to add some task in Jira for redesigning this.

[hazzik]

Can you quick try the solution proposed in NH-3078?

[fredericDelaporte]

I have seen it. I expect it to cause other databases to fail. This solution remove the TimeSpan conversion to a DateTime done by all time types by default. I think this conversion has been coded because most other db likely require a DateTime instead of a TimeSpan.

And that will "fix" the Sybase issue only for parameters set by NHibernate from its type classes, not for others.

[hazzik]

I've checked the solution from NH-3078 with following drivers, and it works:

• Sql2008ClientDriver
• SqlClientDriver
• SqlServerCeDriver
• OdbcDriver
• [EDIT] FirebirdClientDriver

I believe, that it will work for MySql and PostgresSql as well (Maybe it will work for everything).

[EDIT]Does not work

• SQLite20Driver

PS: It seems that there are no way to set the parameters not through XxxType

[hazzik]

It should also work for OracleManagedDataClientDriver.
[EDIT] - it does not.

Please provide the test case demonstrating the failure outside of TimeType and TimeAsTimeSpanType

[hazzik]

Please provide a test case demonstrating the failure outside of TimeType and TimeAsTimeSpanType

[fredericDelaporte]

Please provide a test case demonstrating the failure outside of TimeType and TimeAsTimeSpanType

Ok, not doable unless diving deep in internals thanks to GetSessionImplementation(), so that would be a bit nitpicking.

[hazzik]

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