NH-4052 - Collect schema validation exceptions #663

DMDTT · 2017-07-14T08:42:33Z

NH-4052 - Collect schema validation exceptions

add list of exceptions to HibernateException
create HibernateException on validation fail and add to the exception list
return list of exceptions to throw one common HibernateException to be able to evaluate it on application side

fredericDelaporte

Please fix the tests. One probably needs to be updated, the other seems to point a bug in your changes.

Your commit message has to be more descriptive. Including the Jira ticket is not enough, you should tell about what is it.

NH-4052 - aggregate schema validation exception

Would be better.

But overall this changes does not look to me as truly a valid AggregateException pattern. Here, the listed exceptions will not have been thrown and will not have any stack-trace. They should not be handled as exception, but as a simple list of string error messages. There is no value in listing exceptions having not been thrown and only carrying their message as significant data. Better just consolidate all their messages in one string.

fredericDelaporte · 2017-07-15T20:12:15Z

src/NHibernate/Mapping/Table.cs

-					throw new HibernateException(string.Format("Missing column: {0} in {1}", column.Name,
-															   dialect.Qualify(tableInfo.Catalog, tableInfo.Schema, tableInfo.Name)));
+					exceptions.Add(new HibernateException(string.Format("Missing column: {0} in {1}", column.Name,
+															   dialect.Qualify(tableInfo.Catalog, tableInfo.Schema, tableInfo.Name))));


Here lies a bug due to the change. A throw stops code execution, protecting in this case the code from reaching a null ref condition. You should make sure that everywhere you have replaced a throw by just an add in a list, the code following that will support the state for which an exception was supposed to be thrown or will be appropriately bypassed.

fredericDelaporte · 2017-07-15T20:19:44Z

src/NHibernate/HibernateException.cs

@@ -12,6 +13,7 @@ namespace NHibernate
 	[Serializable]
 	public class HibernateException : Exception
 	{
+		public List<HibernateException> Exceptions { get; }


Better follow usual conventions, like the one of the standard AggregateException: name this InnerExceptions, and expose this as a ReadOnlyCollection<Exception>.

fredericDelaporte · 2017-07-15T20:21:26Z

src/NHibernate/HibernateException.cs

@@ -12,6 +13,7 @@ namespace NHibernate
 	[Serializable]
 	public class HibernateException : Exception


I am not for adding this to the hibernate exception base class. If we add that, it should be on a specialized class, like adding class HibernateAggregateException : HibernateException.

DMDTT · 2017-07-17T08:08:44Z

@fredericDelaporte Thanks for your review.
Keeping the validation errors as a readonlycollection seems a more apropriate way then keeping one string.

I updated the code

fredericDelaporte

This new design overall seems good for me.

Some undue changes needs to be reverted.

Modified asserts should be modernized.

A new test have to be added in SchemaValidateFixture for testing the new feature. It should ascertain that in case of many errors, they are all listed.

For the new class, better use the less specialized interface for its constructor arguments.

Please add xml comments on new public classes (SchemaValidationException).

fredericDelaporte · 2017-07-17T10:20:21Z

src/NHibernate.Test/App.config

@@ -7,7 +7,7 @@
 	</configSections>

 	<connectionStrings>
-		<add name="TestConnectionString" connectionString="Server=localhost\sqlexpress;Database=nhibernate;Integrated Security=SSPI" />
+		<add name="TestConnectionString" connectionString="Server=localhost;Database=nhibernate;Integrated Security=SSPI" />


Please revert. You should not need to change that. Use ShowBuildMenu and setup your test configuration with it.

fredericDelaporte · 2017-07-17T10:23:45Z

src/NHibernate/HibernateException.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;


Change now unneeded, please revert too.

fredericDelaporte · 2017-07-17T10:24:58Z

src/NHibernate.TestDatabaseSetup/App.config

@@ -9,7 +9,7 @@
    <session-factory name="NHibernate.TestDatabaseSetup">
 			<property name="connection.driver_class">NHibernate.Driver.SqlClientDriver</property>
 			<property name="connection.connection_string">
-        Server=.\SQLExpress;initial catalog=master;Integrated Security=SSPI
+        Server=localhost;initial catalog=master;Integrated Security=SSPI


Please revert too.

src/NHibernate/SchemaValidationException.cs

fredericDelaporte · 2017-07-17T10:34:41Z

src/NHibernate.Test/NHSpecificTest/NH1274ExportExclude/NH1274ExportExcludeFixture.cs

@@ -7,6 +7,7 @@
 using NHibernate.Criterion;
 using NHibernate.Engine;
 using NHibernate.Tool.hbm2ddl;
+using NHibernate.Util;


We wish to remove NHibernate collection custom extensions, so please do not add more usages of them.

fredericDelaporte · 2017-07-17T10:51:26Z

src/NHibernate.Test/NHSpecificTest/NH1274ExportExclude/NH1274ExportExcludeFixture.cs

 			{
-				Assert.IsTrue(he.Message.Contains("Home_Validate"));
+				Assert.IsTrue(e.Message.Contains("Schema validation failed: see list of validation errors"));
+				Assert.IsTrue(e.ValidationErrors.Contains("Missing table: Home_Validate"));


Better use Fluent syntax: Assert.That. NUnit does no more add features to the others, and may obsolete them.
By the way, just having "expected true but was false" as an error message while there is more than one test on true is not practical : better at least add an explicit error message.

Here the whole thing could be simplified too:

Assert.That(() => validator.Validate(), Throws.TypeOf<SchemaValidationException>() .And.Property("ValidationErrors").Contain("Missing table: Home_Validate"));

(Checking the type and the error list without checking the exception message seems enough to me, the type is already sufficiently specialized.)

fredericDelaporte · 2017-07-17T10:52:23Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

 			{
-				Assert.That(e.Message, Does.StartWith("Missing column: Name"));
+				Assert.That(e.Message, Does.StartWith("Schema validation failed: see list of validation errors"));
+				Assert.That(e.ValidationErrors[0], Does.StartWith("Missing column: Name"));


Better but could be simplified as previous assert.

hazzik · 2017-09-06T03:08:48Z

Mentioned issues resolved. Squashed, rebased and Async tests regenerated.

Changes implemented

hazzik · 2017-09-06T03:12:03Z

Ok, I've forgot to add a new test for the feature.

fredericDelaporte

I may finish that if you are not already on it.

fredericDelaporte · 2017-09-06T10:23:08Z

src/NHibernate.Test/Async/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

+				() => validatorV2.ValidateAsync(),
+				Throws.TypeOf<SchemaValidationException>()
+				      .And.Message.EqualTo("Schema validation failed: see list of validation errors")
+				      .And.Property("ValidationErrors").Contains("Missing column: Name in nhibernate.dbo.Version"));


As showcased by failing tests, this column naming is specific to SQL Server and causes the test to fail for other db.

hazzik · 2017-09-06T10:28:48Z

I'm not doing it right now. So yes, if you wish

+ add SchemaValidationException + keep list of validation messages and throw new SchemaValidationException + adjust/ fix unittests

fredericDelaporte

Rebased by the way, test added.

fredericDelaporte · 2017-09-06T16:45:02Z

src/AsyncGenerator.yml

+    - conversion: Copy
+      hasAttributeName: OneTimeSetUpAttribute
+    - conversion: Copy
+      hasAttributeName: OneTimeTearDownAttribute


It was converting them to async otherwise, causing them to be invalid. The test for this feature does not inherit a base test class, and was not cleaning up correctly (leaving schema). So I have added setup and teardown logic in it.

fredericDelaporte · 2017-09-06T16:45:41Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/3_Version.hbm.xml

+	<class name="Version">
+		<id name="Id">
+			<generator class="NHibernate.Id.TableHiLoGenerator">
+				<param name="table">id_table</param>


Altered table name for generator.

fredericDelaporte · 2017-09-06T16:47:19Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/3_Version.hbm.xml

+		</id>
+		<property name="Description"/>
+		<property name="Name"/>
+		<property name="Title"/>


One more missing column.

fredericDelaporte · 2017-09-06T16:48:15Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

+		public void SetUp()
+		{
+			_export1 = new SchemaExport(_configuration1);
+			_export1.Create(true, true);


It is a wrapper more explicit than Execute(true, true, false).

fredericDelaporte · 2017-09-06T16:48:32Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

+		[TearDown]
+		public void TearDown()
+		{
+			_export1.Drop(true, true);


Was not dropping previously.

fredericDelaporte · 2017-09-06T16:49:45Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

-			var validator = new Tool.hbm2ddl.SchemaValidator(cfg);
-			validator.Validate();
+			var export = new SchemaExport(cfg);
+			export.Create(true, true);


For the Turkish case, I am not sure if creating the schema under the Turkish culture is required or not, so I have left it in place.

fredericDelaporte · 2017-09-06T16:50:44Z

src/NHibernate.Test/Tools/hbm2ddl/SchemaValidator/SchemaValidateFixture.cs

+			var error = Assert.Throws<SchemaValidationException>(() => validator.Validate());
+			Assert.That(error,
+				Has.Message.EqualTo("Schema validation failed: see list of validation errors")
+					.And.Property("ValidationErrors").Some.Contains("Missing column: Name in ").IgnoreCase.And.Contains("Version").IgnoreCase);


I do not know another way for testing multiple values.
Following code is:

Assert.That(error, Has.Property("ValidationErrors").Some.Contains("Missing column: Title in ").IgnoreCase.And.Contains("Version").IgnoreCase); Assert.That(error, Has.Property("ValidationErrors").Some.Contains("Missing sequence or table: ").IgnoreCase.And.Contains("id_table").IgnoreCase);

DMDTT · 2017-09-07T07:32:05Z

@fredericDelaporte @hazzik
Thanks for your work guys,
I didn't manage it to work on it in the last month, sry for that.

But nethertheless the work of yours is great. Thanks again.

* NH-4052 (nhibernate#663) has introduced a new exception class, but has forgotten to provide its serializable implementation.

- NH-4052 (#663) has introduced a new exception class but has forgotten to provide its serializable implementation.

fredericDelaporte previously requested changes Jul 15, 2017

View reviewed changes

fredericDelaporte reviewed Jul 17, 2017

View reviewed changes

fredericDelaporte added the needs work label Jul 18, 2017

fredericDelaporte changed the title ~~NH-4052~~ NH-4052 - Collect schema validation exceptions Jul 18, 2017

hazzik modified the milestone: 5.1 Aug 3, 2017

hazzik added the t: New Feature label Aug 4, 2017

hazzik removed the needs work label Sep 6, 2017

hazzik requested a review from fredericDelaporte September 6, 2017 03:10

fredericDelaporte reviewed Sep 6, 2017

View reviewed changes

NH-4052 - Aggregate schema validation exception

f1cb4c9

+ add SchemaValidationException + keep list of validation messages and throw new SchemaValidationException + adjust/ fix unittests

fredericDelaporte reviewed Sep 6, 2017

View reviewed changes

hazzik approved these changes Sep 6, 2017

View reviewed changes

hazzik modified the milestones: 5.1, 5.0 Sep 6, 2017

fredericDelaporte merged commit 8d0f102 into nhibernate:master Sep 6, 2017

fredericDelaporte added the r: Fixed label Sep 7, 2017

fredericDelaporte mentioned this pull request Nov 26, 2017

NH-4052 - Add missing serializable implementation #1456

Merged

hazzik pushed a commit that referenced this pull request Nov 27, 2017

NH-4052 - Adding missing serializable implementation (#1456)

7dc47e0

- NH-4052 (#663) has introduced a new exception class but has forgotten to provide its serializable implementation.

		@@ -12,6 +13,7 @@ namespace NHibernate
		[Serializable]
		public class HibernateException : Exception

		@@ -1,4 +1,5 @@
		using System;
		using System.Collections.Generic;

NH-4052 - Collect schema validation exceptions #663

NH-4052 - Collect schema validation exceptions #663

Uh oh!

Conversation

DMDTT commented Jul 14, 2017 • edited by fredericDelaporte Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

DMDTT commented Jul 17, 2017

Uh oh!

fredericDelaporte left a comment

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!

Choose a reason for hiding this comment

Uh oh!

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!

hazzik commented Sep 6, 2017

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fredericDelaporte left a comment

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!

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 6, 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 6, 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!

DMDTT commented Sep 7, 2017

Uh oh!

Uh oh!

DMDTT commented Jul 14, 2017 •

edited by fredericDelaporte

Loading

fredericDelaporte left a comment •

edited

Loading

hazzik commented Sep 6, 2017 •

edited

Loading

hazzik commented Sep 6, 2017 via email •

edited

Loading

fredericDelaporte Sep 6, 2017 •

edited

Loading

fredericDelaporte Sep 6, 2017 •

edited

Loading