NH-1752 - NHibernate Date type converts to NULL #698

fredericDelaporte · 2017-09-21T17:11:22Z

NH-1752 - NHibernate Date type converts to NULL

Occurs when date is lower than year 1753.

This behavior is removed. This behavior was handy when nullable types were lacking, and a test was still relying on this.

For testing the change with SqlServer, I have migrated all SqlClient driver to Sql2008Client.

fredericDelaporte · 2017-09-21T17:12:42Z

doc/reference/modules/configuration.xml

-      </property>
-    <property name="connection.driver_class">
-      NHibernate.Driver.SqlClientDriver
-    </property>


Our doc examples incite users to use an obsolete driver for SqlServer: I have purged unneeded parameters.

fredericDelaporte · 2017-09-21T17:13:37Z

src/NHibernate.Test/Async/NHSpecificTest/Logs/LogsFixture.cs

@@ -51,7 +51,7 @@ protected override string MappingsAssembly

 				await (s.GetAsync<Person>(1));//will execute some sql

-				var loggingEvent = spy.Events[0];
+				var loggingEvent = spy.GetWholeLog();


@ngbrown, it seems you have forgotten to regen Async files.

~~Not sure what you mean. I run the ShowBuildMenu option H, and no files change.~~

There's no documentation at https://github.com/maca88/AsyncGenerator, so what does it even do?

Sorry, I was getting an error in the nuget restore, and didn't see that.

#699 if you want to make a separate merge for that.

Contributing have been completed about that. But you now know about it.

That is not a big change so I would not have required a new PR. I will see if Alexander will rather merge your PR first or just mine.

hazzik · 2017-09-21T22:05:20Z

src/NHibernate/Driver/IDriver.cs

+		/// <summary>
+		/// The minimal date supported by this driver.
+		/// </summary>
+		DateTime MinDate { get; }


What about other drivers?

The datetime range for MySQL, Oracle is 1000-01-01 to 9999-12-31

For Postgres is 4713 BC to 294276 AD

I have just put .Net MinValue and let the test tell me if it was ok. So that is strange for MySql/Oracle. And for Postgres, as .Net native type does not allow BC dates, it is limited by .Net rather than by the DB, but that still means a limit at MinValue. Otherwise we could switch that property to nullable for meaning that the actual limit is below .Net own limit, but I do not think it is worth it.

According to this other doc, Oracle DATE support dates down to 4712 BC. So I think we can keep MinDate for Oracle.

About MySql, they also write:

For the DATE and DATETIME range descriptions, “supported” means that although earlier values might work, there is no guarantee.

So it just happens to work in our case, but we should not complain if it ceases too. I will set its MinDate accordingly.

hazzik · 2017-09-22T00:12:02Z

src/NHibernate.Test/TypesTest/DateTypeTest.cs

 		{
-			var basic = new DateClass { DateValue = new DateTime(1899,1,1) };
+			var basic = new DateClass { DateValue = expected.AddHours(1) };


~~Adding one hour does not make sense here as we only store date.~~ Ah, I got this test now. Stupid me.

Got screwed first too, I could have added a comment.

hazzik · 2017-09-22T00:14:02Z

src/NHibernate.Test/TypesTest/DateTypeTest.cs

@@ -55,36 +56,30 @@ public void ReadWriteNormal()
 		{
 			var expected = DateTime.Today.Date;


Do not need to call .Date on .Today as it is already a bare date

Sure I could have cleaned that, just had refactored previous code in a common method without changing the logic.

hazzik · 2017-09-22T00:16:21Z

I don't understand how the tests work for MySQL and Oracle as their datetime range starts from year one thousand.

fredericDelaporte · 2017-09-22T11:17:40Z

src/NHibernate.Test/NHSpecificTest/NH2207/SampleTest.cs

 		protected override bool AppliesTo(ISessionFactoryImplementor factory)
 		{
 			return factory.ConnectionProvider.Driver is Sql2008ClientDriver;
-


Indeed, this one is checked after schema creation. So if the schema cannot be created for the test, we have to test the driver by checking TestCase.Cfg property in the dialect AppliesTo, somewhat like here.

It is coded as if creating the factory was requiring the schema to be there.

Configure(); if (!AppliesTo(Dialect)) { Assert.Ignore(GetType() + " does not apply to " + Dialect); } CreateSchema(); try { _sessionFactory = BuildSessionFactory(); if (!AppliesTo(_sessionFactory)) { Assert.Ignore(GetType() + " does not apply with the current session-factory configuration"); } } catch { DropSchema(); throw; }

We could rewrite it:

Configure(); if (!AppliesTo(Dialect)) { Assert.Ignore(GetType() + " does not apply to " + Dialect); } _sessionFactory = BuildSessionFactory(); if (!AppliesTo(_sessionFactory)) { Assert.Ignore(GetType() + " does not apply with the current session-factory configuration"); } CreateSchema();

fredericDelaporte · 2017-09-22T11:22:03Z

src/NHibernate.Test/TypesTest/DateTypeTest.cs

+		public void ReadWriteYear750()
+		{
+			var expected = new DateTime(750, 5, 13);
+			if (Sfi.ConnectionProvider.Driver.MinDate < expected)


Shouldn't it be reversed?

Yes, you are correct

fredericDelaporte · 2017-09-25T15:33:28Z

May we merge this? It may allow me to do more clean-up in ongoing #703.

hazzik · 2017-09-25T20:17:47Z

Sure. Can you also rebase/squash it before merging?

It was inciting to put uneeded or even harmfull settings (forcing use of old sql client driver instead of 2008).

Obsolete base date parameterization and set its default value to MinDate

NH-1754 - More edge cases

fredericDelaporte commented Sep 21, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-1752 branch 3 times, most recently from 80c4fb5 to 15575b4 Compare September 21, 2017 20:52

hazzik reviewed Sep 21, 2017

View reviewed changes

hazzik reviewed Sep 22, 2017

View reviewed changes

fredericDelaporte commented Sep 22, 2017

View reviewed changes

fredericDelaporte added the t: Bug label Sep 22, 2017

hazzik previously approved these changes Sep 25, 2017

View reviewed changes

fredericDelaporte added 3 commits September 25, 2017 23:36

Purging examples configuration.

09f8724

It was inciting to put uneeded or even harmfull settings (forcing use of old sql client driver instead of 2008).

Switch from old SqlClientDriver to Sql2008ClientDriver

6166cb5

NH-1752 - Cease silently nullifying dates before 1753

c16868d

Obsolete base date parameterization and set its default value to MinDate

fredericDelaporte dismissed hazzik’s stale review via 4379046 September 25, 2017 21:36

fredericDelaporte force-pushed the NH-1752 branch from 863a13e to 4379046 Compare September 25, 2017 21:36

NH-2207 - Enable tests for Sql2008ClientDriver

d533eb0

NH-1754 - More edge cases

fredericDelaporte force-pushed the NH-1752 branch from 4379046 to d533eb0 Compare September 25, 2017 21:41

fredericDelaporte added this to the 5.0 milestone Sep 26, 2017

fredericDelaporte merged commit 2fa35fc into nhibernate:master Sep 26, 2017

fredericDelaporte deleted the NH-1752 branch September 26, 2017 08:41

fredericDelaporte added the r: Fixed label Sep 26, 2017

fredericDelaporte mentioned this pull request Sep 26, 2017

Add Sql2008ClientDriver TeamCity configuration #697

Merged

		@@ -55,36 +56,30 @@ public void ReadWriteNormal()
		{
		var expected = DateTime.Today.Date;

NH-1752 - NHibernate Date type converts to NULL #698

NH-1752 - NHibernate Date type converts to NULL #698

Uh oh!

Conversation

fredericDelaporte commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngbrown Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Sep 25, 2017

Uh oh!

hazzik commented Sep 25, 2017

Uh oh!

Uh oh!

ngbrown Sep 21, 2017 •

edited

Loading

fredericDelaporte Sep 21, 2017 •

edited

Loading

fredericDelaporte Sep 22, 2017 •

edited

Loading

fredericDelaporte Sep 22, 2017 •

edited

Loading

hazzik Sep 22, 2017 •

edited

Loading

hazzik Sep 22, 2017 •

edited

Loading