Skip to content

Fix invalid cast exception for DateTimeOffset type #2821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fix invalid cast exception for DateTimeOffset type #2821

wants to merge 10 commits into from

Conversation

amul047
Copy link

@amul047 amul047 commented Jun 11, 2021 •
edited by bahusoid
Loading

Fixes #2820

@amul047
Copy link
Author

amul047 commented Jun 11, 2021

I can't add a linked issue, but this PR relates to #2820

@bahusoid
Copy link
Member

It seems we have tests to cover DateTimeOffset. See DateTimeOffsetQueryFixture. Why tests work without this changes? Can you add a failing test case there.

@bahusoid
Copy link
Member

Ah... It is specific to column type timestamptz (which is not what is created by NHIbernate by default)

@amul047
Copy link
Author

amul047 commented Jun 11, 2021

Ah... It is specific to column type timestamptz (which is not what is created by NHIbernate by default)

Are you saying that if we had RegisterColumnType(DbType.DateTimeOffset, "timestamptz"); over at

nhibernate-core/src/NHibernate/Dialect/PostgreSQLDialect.cs

Line 139 in 47992e7

protected virtual void RegisterDateTimeTypeMappings()
, then we won't have this issue at all?

@amul047
Copy link
Author

amul047 commented Jun 11, 2021

Ah... It is specific to column type timestamptz (which is not what is created by NHIbernate by default)

Are you saying that if we had RegisterColumnType(DbType.DateTimeOffset, "timestamptz"); over at

nhibernate-core/src/NHibernate/Dialect/PostgreSQLDialect.cs

Line 139 in 47992e7

protected virtual void RegisterDateTimeTypeMappings()

, then we won't have this issue at all?

It looks like we do this for another dialect

nhibernate-core/src/NHibernate/Dialect/SybaseSQLAnywhere12Dialect.cs

Line 74 in 47992e7

RegisterColumnType(DbType.DateTimeOffset, "TIMESTAMP WITH TIME ZONE");

timestamptz is just short for TIMESTAMP WITH TIME ZONE in PostgresQL

@amul047
Copy link
Author

amul047 commented Jun 11, 2021

Also, it appears that DateTimeOffsetFixture tests are being ignored for PostgresQL - https://teamcity.jetbrains.com/viewLog.html?buildId=3511808&buildTypeId=bt875&tab=testsInfo .

@hazzik
Copy link
Member

hazzik commented Jun 11, 2021

Need tests.

@gliljas
Copy link
Member

gliljas commented Jun 11, 2021

AFAIK, no RDBMS other than SQL Server can actually handle a DateTimeOffset with any credibility. Some, like Postgres, can certainly store it, but the value is converted to UTC and the offset is thrown away.

Sky walker added 4 commits June 11, 2021 22:42
Register column type DateTimeOffset
4c407f8 
- this should also run the existing DateTimeOffsetFixture tests
- also undoing my change before, so we can replicate the issue first
Revert "Fix invalid cast exception for date time off set type"
7dad5d7 
This reverts commit 6f948f6.
Revert "Revert "Fix invalid cast exception for date time off set type""
f27417e 
This reverts commit 7dad5d7.
Use "timestamp with time zone" instead of "timestamptz" (only valid 9…
df40b30 
….1+)
@fredericDelaporte
Copy link
Member

fredericDelaporte commented Jun 11, 2021 •
edited
Loading

See my comments in #2820. Gunnar is right. And I think we should not fake a DateTimeOffset support when it is not actually fully supported. So this PR would have to be rejected.

From Npgsql documentation:

A common mistake is for users to think that the PostgreSQL timestamp with timezone type stores the timezone in the database. This is not the case: only the timestamp is stored. There is no single PostgreSQL type that stores both a date/time and a timezone, similar to .NET DateTimeOffset.

@bahusoid
Copy link
Member

Original fix contains only 6f948f6. And I see no harm in this change. I agree we shouldn't add timestampz registrations for DateTimeOffset for PostreSQL if it's not supported properly. But why should we restrict custom mapping? It's user responsibility for data correctness in this case. I think original fix is good move - requesting exact type is better than typing after retrieving object. Dialect might handle it more effectively.

@gliljas
Copy link
Member

gliljas commented Jun 12, 2021

Yes, that change makes sense. Perhaps even on a broader scale, not only for DateTimeOffset.

@hazzik hazzik changed the title Fix invalid cast exception for date time off set type Fix invalid cast exception for DateTimeOffset type Jun 12, 2021
@bahusoid
Copy link
Member

And... It fails the same on PostgreSQL (Npgsql Version="4.1.9"):

System.InvalidCastException : Unable to cast object of type 'System.DateTime' to type 'System.DateTimeOffset'.
[00:08:09]    at NHibernate.Loader.Loader.DoList(ISessionImplementor session, QueryParameters queryParameters, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1981
[00:08:09]    at NHibernate.Loader.Loader.ListIgnoreQueryCache(ISessionImplementor session, QueryParameters queryParameters) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1837
[00:08:09]    at NHibernate.Hql.Ast.ANTLR.QueryTranslatorImpl.List(ISessionImplementor session, QueryParameters queryParameters) in C:\projects\nhibernate-core\src\NHibernate\Hql\Ast\ANTLR\QueryTranslatorImpl.cs:line 119
[00:08:09]    at NHibernate.Engine.Query.HQLQueryPlan.PerformList(QueryParameters queryParameters, ISessionImplementor session, IList results) in C:\projects\nhibernate-core\src\NHibernate\Engine\Query\HQLQueryPlan.cs:line 116
[00:08:09]    at NHibernate.Impl.SessionImpl.List(IQueryExpression queryExpression, QueryParameters queryParameters, IList results, Object filterConnection) in C:\projects\nhibernate-core\src\NHibernate\Impl\SessionImpl.cs:line 559
[00:08:09]    at NHibernate.Impl.SessionImpl.List(IQueryExpression queryExpression, QueryParameters queryParameters, IList results) in C:\projects\nhibernate-core\src\NHibernate\Impl\SessionImpl.cs:line 524
[00:08:09]    at NHibernate.Impl.AbstractSessionImpl.List[T](IQueryExpression query, QueryParameters parameters) in C:\projects\nhibernate-core\src\NHibernate\Impl\AbstractSessionImpl.cs:line 183
[00:08:09]    at NHibernate.Impl.AbstractQueryImpl2.List[T]() in C:\projects\nhibernate-core\src\NHibernate\Impl\AbstractQueryImpl2.cs:line 108
[00:08:09]    at NHibernate.Linq.DefaultQueryProvider.ExecuteList[TResult](Expression expression) in C:\projects\nhibernate-core\src\NHibernate\Linq\DefaultQueryProvider.cs:line 114
[00:08:09]    at NHibernate.Linq.NhQueryable`1.System.Collections.Generic.IEnumerable<T>.GetEnumerator() in C:\projects\nhibernate-core\src\NHibernate\Linq\NhQueryable.cs:line 65
[00:08:09]    at System.Collections.Generic.List`1.AddEnumerable(IEnumerable`1 enumerable)
[00:08:09]    at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
[00:08:09]    at NHibernate.Test.NHSpecificTest.GH2820.PostgresTimestampzFixture.CanRetrieveEntityWithTimeStampColumn() in C:\projects\nhibernate-core\src\NHibernate.Test\NHSpecificTest\GH2820\FixtureByCode.cs:line 59
[00:08:09] --FormatException
[00:08:09]    at NHibernate.Type.DateTimeOffsetType.Get(DbDataReader rs, Int32 index, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Type\DateTimeOffSetType.cs:line 72
[00:08:09]    at NHibernate.Type.NullableType.NullSafeGet(DbDataReader rs, String name, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Type\NullableType.cs:line 235
[00:08:09]    at NHibernate.Persister.Entity.AbstractEntityPersister.Hydrate(DbDataReader rs, Object id, Object obj, String[][] suffixedPropertyColumns, ISet`1 fetchedLazyProperties, Boolean allProperties, Int32[] indexes, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Persister\Entity\AbstractEntityPersister.cs:line 2854
[00:08:09]    at NHibernate.Persister.Entity.LoadableExtensions.Hydrate(ILoadable loadable, DbDataReader rs, Object id, Object obj, String[][] suffixedPropertyColumns, ISet`1 fetchedLazyProperties, Boolean allProperties, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Persister\Entity\ILoadable.cs:line 101
[00:08:09]    at NHibernate.Loader.Loader.LoadFromResultSet(DbDataReader rs, Int32 i, Object obj, ILoadable persister, EntityKey key, LockMode lockMode, ILoadable rootPersister, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1301
[00:08:09]    at NHibernate.Loader.Loader.InstanceNotYetLoaded(DbDataReader dr, Int32 i, ILoadable persister, EntityKey key, LockMode lockMode, EntityKey optionalObjectKey, Object optionalObject, IList hydratedObjects, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1164
[00:08:09]    at NHibernate.Loader.Loader.GetRow(DbDataReader rs, ILoadable[] persisters, EntityKey[] keys, Object optionalObject, EntityKey optionalObjectKey, LockMode[] lockModes, IList hydratedObjects, ISessionImplementor session, Boolean mustLoadMissingEntity, Action`2 cacheBatchingHandler) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1041
[00:08:09]    at NHibernate.Loader.Loader.GetRowFromResultSet(DbDataReader resultSet, ISessionImplementor session, QueryParameters queryParameters, LockMode[] lockModeArray, EntityKey optionalObjectKey, IList hydratedObjects, EntityKey[] keys, Boolean returnProxies, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder, Action`2 cacheBatchingHandler) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 398
[00:08:09]    at NHibernate.Loader.Loader.DoQuery(ISessionImplementor session, QueryParameters queryParameters, Boolean returnProxies, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 558
[00:08:09]    at NHibernate.Loader.Loader.DoQueryAndInitializeNonLazyCollections(ISessionImplementor session, QueryParameters queryParameters, Boolean returnProxies, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 303
[00:08:09]    at NHibernate.Loader.Loader.DoList(ISessionImplementor session, QueryParameters queryParameters, IResultTransformer forcedResultTransformer, QueryCacheResultBuilder queryCacheResultBuilder) in C:\projects\nhibernate-core\src\NHibernate\Loader\Loader.cs:line 1972
[00:08:09] --InvalidCastException
[00:08:09]    at System.Data.Common.DbDataReader.GetFieldValue[T](Int32 ordinal)
[00:08:09]    at NHibernate.Type.DateTimeOffsetType.Get(DbDataReader rs, Int32 index, ISessionImplementor session) in C:\projects\nhibernate-core\src\NHibernate\Type\DateTimeOffSetType.cs:line 68

@bahusoid
Copy link
Member

Maybe because of our NHybridDataReader...

@bahusoid
Copy link
Member

So it seems fix requires specific type checks like:

var value = rs.GetValue(index);
if (value is DateTime dateTime)
	return (DateTimeOffset) dateTime;

So yeah maybe it's not worth it as you guys already said... @amul047 If you really need broken DateTimeOffset you can create custom type with this logic and register it globally with TypeFactory.RegisterType. Something like we have in our tests for DateTime:

nhibernate-core/src/NHibernate.Test/TypesTest/ChangeDefaultTypeFixture.cs

Line 30 in 322de5e

TypeFactory.RegisterType(typeof(DateTime), _testDefaultDateTimeType, TypeFactory.EmptyAliases);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@amul047 @bahusoid @hazzik @gliljas @fredericDelaporte