Fix invalid handling of null DateTime parameters in Npgsql 6+ #3299
Conversation
hazzik
changed the title
|
I have thought about checking where the regression had happened only after validating. It seems it was in 5.3.x. So we need a backport of the fix, now that it is merged into 5.4. |
There was a problem hiding this comment.
It seems it was in 5.3.x
Yes, original fix was added in 5.3.13. But it’s not a regression as DateTime handling was broken anyway with Npgsql6 (see #2994). So I would make it a low priority 5.3.x fix that can wait for other regression issues.
| for (var i = 0; i < command.Parameters.Count; i++) | ||
| { | ||
| var parameter = command.Parameters[i]; | ||
| if (parameter.Value is DateTime) |
There was a problem hiding this comment.
I'm not sure I like this runtime type check for all parameters values.
It's not clear what original issue #2994 is about as no details are provided. But DateTime can represent multiple DbType types (Date, DateTime, Time). Original fix #3064 was applied only for DBType.DateTime.
So is it omission in original fix or other types are not affected and no fix is required? I wouldn't extend scope of fix unless it's really necessary.
There was a problem hiding this comment.
It's not clear what original issue #2994 is about as no details are provided.
There are breaking changes in Npgsql 6 that require a DateTime of certain Kind to match timestamp or timestamptz through a "help" of DbType.DateTime2 type. Here are some explanations: https://www.npgsql.org/doc/release-notes/6.0.html#detailed-notes and here: https://www.roji.org/postgresql-dotnet-timestamp-mapping
It is really hard to explain their motivation and the implementation. But it does not align with the type system used by NHibernate.
But DateTime can represent multiple DbType types (Date, DateTime, Time).
I don't think it applies here. However, I think, it is possible to write a more specific conversion.
There was a problem hiding this comment.
Ok, it does apply to DbType.Date. I'll adjust the fix.
Fixes #3291