NH-4087 - Fix decimal parameter with scale above 5 #708

fredericDelaporte · 2017-10-03T20:29:22Z

NH-4087 (#1196) - Fix decimal parameter with scale above 5

~~Cease defining default precision/scale in SQL Server driver unless explicitly configured~~
~~Allow Firebird to adjust its default parameter type cast~~
Use dialect configurable default scale/precision

fredericDelaporte · 2017-10-03T20:31:12Z

src/NHibernate.Test/DriverTest/OdbcDriverFixture.cs

+namespace NHibernate.Test.DriverTest
+{
+	[TestFixture]
+	public class OdbcDriverFixture : TestCase


Copy paste of SqlClientDriverFixture which was executed for ODBC too, but with some specific test asserting ignore when it was ODBC. Now separated in its own fixture, with only non-ignored tests.

fredericDelaporte · 2017-10-03T20:32:16Z

src/NHibernate.Test/DriverTest/SqlClientDriverFixture.cs

+				Assert.That(m.StringProp, Is.EqualTo("a"), "StringProp");
+				Assert.That(m.StringClob, Is.EqualTo("a"), "StringClob");
+				Assert.That(m.BinaryBlob, Is.EqualTo(new byte[] { 1, 2, 3 }), "BinaryBlob");
+				Assert.That(m.Binary, Is.EqualTo(new byte[] { 4, 5, 6 }), "BinaryBlob");


The test was previously just saving/updating without checking values.

fredericDelaporte · 2017-10-03T20:33:01Z

src/NHibernate.Test/DriverTest/SqlClientDriverFixture.cs


 				t.Rollback();
 			}
 		}
+
+		[Test]
+		public void DefaultPrecisionScale()


Added test for new decimal precision/Scale settings.

fredericDelaporte · 2017-10-03T20:34:11Z

src/NHibernate.Test/TestDialect.cs

+		/// <summary>
+		/// Supports condition not bound to any data, like "where @p1 = @p2".
+		/// </summary>
+		public virtual bool SupportsNonDataBoundCondition => true;


Copied from #703, I will have to resolve a conflict on one of those PR depending on merge order. But I was needing this.

fredericDelaporte · 2017-10-03T20:35:59Z

src/NHibernate.Test/TypesTest/DecimalTypeFixture.cs

+		}
+
+		[Test]
+		public void HighScaleParameterSelect()


Starting with this one, new tests for checking "high" scale decimal are not truncated.

fredericDelaporte · 2017-10-03T20:38:28Z

src/NHibernate.Test/TypesTest/DecimalTypeFixture.cs

+			if (Dialect is FirebirdDialect)
+			{
+				configuration.SetProperty(Environment.ParameterDefaultDecimalPrecision, "18");
+				configuration.SetProperty(Environment.ParameterDefaultDecimalScale, "9");


Due to Firebird needing an explicit cast in some queries, injected by its driver, we must set its default scale/precision according to what the application needs. (Applications having varying needs will be stuck by this Firebird limitation, unless they use separated session factories just for those varying needs...)

fredericDelaporte · 2017-10-03T20:47:51Z

src/NHibernate/Driver/FirebirdClientDriver.cs

+						return _fbDialect.GetTypeName(sqlType);
+					return _fbDialect.GetTypeName(SqlTypeFactory.GetSqlType(sqlType.DbType, _dbDecimalPrecision, _dbDecimalScale));
+			}
+			return _fbDialect.GetCastTypeName(sqlType);


This GetCastTypeName is quite a terrible method, hopefully having few usages. It disregards the type Length/Precision/Scale and use instead Column.DefaultLength/Column.DefaultPrecision/Column.DefaultScale, which are 255/19/2.
This causes all string types to be cast to a 255 length. Decimal type are trim down to 18 precision by dialect, but anyway, since the type resolution currently compares provided length with the max for the type, which is well below 255 for decimal, it always uses the default for decimal type, which for Firebird is decimal(18,5). Otherwise we would have got a truncation at 2 digits after dot.
I will fill another Jira for that method.

fredericDelaporte · 2017-10-03T20:50:43Z

src/NHibernate/Driver/SqlServerCeDriver.cs

-				SqlClientDriver.SetVariableLengthParameterSize(dbParam, sqlType);
-		}
+				SqlClientDriver.SetVariableLengthParameterSize(dbParam, sqlType, _dbDecimalPrecision, _dbDecimalScale);
+			}


Does it really make sense to set length for SqlServerCe? I believe its sole purpose it to optimize query plan cache usage, but does SqlServerCe have a query plan cache?

fredericDelaporte · 2017-10-04T16:20:11Z

Allow Firebird to adjust its default parameter type cast

This part would be mostly obsoleted by what I am doing for NH-4088 in #709.

fredericDelaporte · 2017-10-09T09:04:00Z

Rebased, changed due to prepare_sql issues if decimal scale and precision are not always set: now SQL Server driver follows the defaults settable in dialect, instead of some hard-coded defaults. So it allows working around the issue by adjusting those defaults.

fredericDelaporte · 2017-10-09T09:59:05Z

src/NHibernate/Driver/SqlClientDriver.cs

 		}

 		// Used from SqlServerCeDriver as well
-		public static void SetVariableLengthParameterSize(DbParameter dbParam, SqlType sqlType)
+		public static void SetVariableLengthParameterSize(
+			DbParameter dbParam, SqlType sqlType, byte defaultDecimalPrecision, byte defaultDecimalScale)


If doing that in 5.1, I guess we will have to add back previous method but flagged obsolete, for avoiding a possible breaking change.

fredericDelaporte · 2017-10-16T17:34:14Z

Rebased and adjusted for having no more breaking changes.

* Use dialect configurable default scale/precision * Fixes nhibernate#1187

fredericDelaporte · 2017-11-19T18:36:29Z

Rebased, reworded.

fredericDelaporte · 2017-11-26T11:56:46Z

Anything against merging this ?

hazzik · 2017-11-26T20:20:22Z

@fredericDelaporte absolutely nothing apart I don't understand how it works.

fredericDelaporte · 2017-11-26T22:34:54Z

The trouble: currently SqlClientDriver set a hard coded scale (5) and precision (28) on decimal parameters when their parameter definition does not specify it. (They may be deduced and set by the HQL query parsing in some cases, but not in all cases.) This causes issues when the parameter value need a higher scale than 5.

With 5.0, a configurable default scale (10) and default precision (28) have been introduced, currently used when a cast is issued in HQL or Linq.

This PR uses them instead of the hard coded non configurable ones from SqlClientDriver. So now the user can control the default scale of decimal parameter, and adjust it to its needs, instead of being limited to 5.

This PR also adds tests for checking for all dialects we can now use decimal with a scale higher than 5. The Sql client driver test fixture is also split in two, because it was actually also testing the Odbc driver, which was complexifying it and causing issues for the sql client driver changes tests. Now there is an odbc driver fixture on its own, along with the Sql client driver fixture purged from Odbc specific bits.

fredericDelaporte commented Oct 3, 2017

View reviewed changes

fredericDelaporte added the t: Bug label Oct 3, 2017

fredericDelaporte mentioned this pull request Oct 3, 2017

NH-3787 - Decimal truncation in Linq ternary expression #707

Merged

fredericDelaporte force-pushed the NH-4087 branch from b15723f to 2874cc9 Compare October 4, 2017 14:39

hazzik modified the milestones: 5.0, 5.1 Oct 5, 2017

fredericDelaporte force-pushed the NH-4087 branch from 2874cc9 to 4a41c89 Compare October 9, 2017 09:01

fredericDelaporte commented Oct 9, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4087 branch from 4a41c89 to 2f1c4e0 Compare October 16, 2017 17:27

fredericDelaporte mentioned this pull request Nov 18, 2017

NH-4008 - Split database providers into own projects. #626

Open

fredericDelaporte force-pushed the NH-4087 branch 2 times, most recently from c4a4e00 to e12bb7d Compare November 19, 2017 18:28

NH-4087 - Fix decimal parameter with scale above 5

02a4f71

* Use dialect configurable default scale/precision * Fixes nhibernate#1187

fredericDelaporte force-pushed the NH-4087 branch from e12bb7d to 02a4f71 Compare November 19, 2017 18:36

fredericDelaporte added t: Fix and removed t: Bug labels Nov 19, 2017

Merge branch 'master' into NH-4087

8faa343

hazzik approved these changes Nov 26, 2017

View reviewed changes

hazzik merged commit 70413b7 into nhibernate:master Nov 26, 2017

fredericDelaporte deleted the NH-4087 branch November 26, 2017 23:03

fredericDelaporte added the r: Fixed label Nov 26, 2017

hazzik mentioned this pull request Nov 27, 2017

NH-4087 - Decimal truncation occurs after 5 digits #1196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-4087 - Fix decimal parameter with scale above 5 #708

NH-4087 - Fix decimal parameter with scale above 5 #708

fredericDelaporte commented Oct 3, 2017 •

edited

Loading

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017

fredericDelaporte Oct 3, 2017 •

edited

Loading

fredericDelaporte Oct 3, 2017 •

edited by hazzik

Loading

fredericDelaporte commented Oct 4, 2017 •

edited

Loading

fredericDelaporte commented Oct 9, 2017

fredericDelaporte Oct 9, 2017

fredericDelaporte commented Oct 16, 2017

fredericDelaporte commented Nov 19, 2017

fredericDelaporte commented Nov 26, 2017

hazzik commented Nov 26, 2017

fredericDelaporte commented Nov 26, 2017

NH-4087 - Fix decimal parameter with scale above 5 #708

NH-4087 - Fix decimal parameter with scale above 5 #708

Conversation

fredericDelaporte commented Oct 3, 2017 • edited Loading

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

fredericDelaporte Oct 3, 2017 • edited by hazzik Loading

Choose a reason for hiding this comment

fredericDelaporte commented Oct 4, 2017 • edited Loading

fredericDelaporte commented Oct 9, 2017

fredericDelaporte Oct 9, 2017

Choose a reason for hiding this comment

fredericDelaporte commented Oct 16, 2017

fredericDelaporte commented Nov 19, 2017

fredericDelaporte commented Nov 26, 2017

hazzik commented Nov 26, 2017

fredericDelaporte commented Nov 26, 2017

fredericDelaporte commented Oct 3, 2017 •

edited

Loading

fredericDelaporte Oct 3, 2017 •

edited

Loading

fredericDelaporte Oct 3, 2017 •

edited by hazzik

Loading

fredericDelaporte commented Oct 4, 2017 •

edited

Loading