NH-4062 - Oracle Unicode support dual model. #667

fredericDelaporte · 2017-08-01T13:29:55Z

NH-4062 - Properly handle Oracle Unicode support dual model

And reverting NH-3620: this was a workaround for a now obsolete bug in ODP.Net managed driver. Its test still pass without it now. It should probably have been solved as "external issue" with instruction on how to extend the driver for working around the bug till Oracle fixes it. Removing this workaround causes another test to fail: BinaryBlobTypeFixture.ReadWriteZeroLen. Without it, the blob come back as null.

Managed and un-managed drivers require the same code, but it was duplicated. Now it is refactored in a single base class.

fredericDelaporte · 2017-08-01T13:31:18Z

src/NHibernate.Test/DriverTest/OracleDataClientDriverFixture.cs

-		[Explicit]
-		public void NoBooleanParameters()
+		[Theory]
+		public void NoBooleanParameters(bool managed)


Adjusted for it to no more fail in case the driver could not be loaded.

fredericDelaporte · 2017-08-01T13:33:41Z

src/NHibernate.Test/Linq/ByMethod/GroupByTests.cs

@@ -772,7 +772,7 @@ public void GroupByComputedValueInObjectArrayWithJoinOnId()
 			Assert.AreEqual(2155, orderGroups.Sum(g => g.Count));
 		}

-		[Test(Description = "NH-3801")]
+		[Test(Description = "NH-3801, NH-4062")]


If #666 is merged first, will need to re-base then re-enabled Oracle in this test, since #666 disable it.

Edit: done.

fredericDelaporte · 2017-08-01T13:36:24Z

src/NHibernate/Dialect/Dialect.cs

+		/// <param name="settings">The configuration settings.</param>
+		public virtual void Configure(IDictionary<string, string> settings)
+		{
+		}


Added a way to configure the dialect.

fredericDelaporte · 2017-08-01T14:16:36Z

src/NHibernate/Cfg/Environment.cs

+		/// https://docs.oracle.com/database/121/ODPNT/featOraCommand.htm#i1007557
+		/// This setting applies only to Oracle dialects and ODP.Net managed or unmanaged driver.
+		/// </remarks>
+		public const string OracleUseNPrefixedTypesForUnicode = "oracle.use_n_prefixed_types_for_unicode";


New setting just for Oracle.

fredericDelaporte · 2017-08-01T14:27:22Z

src/NHibernate/Driver/OracleDataClientDriverBase.cs

+			base.Configure(settings);
+
+			// If changing the default value, keep it in sync with Oracle8iDialect.Configure.
+			UseNPrefixedTypesForUnicode = PropertiesHelper.GetBoolean(Cfg.Environment.OracleUseNPrefixedTypesForUnicode, settings, false);


Default value false avoid a possible breaking change for queries, by keeping the default typing of the ODP.Net provider.

fredericDelaporte · 2017-08-02T11:08:29Z

src/NHibernate/Dialect/Oracle8iDialect.cs

+
+			// If changing the default value, keep it in sync with OracleDataClientDriverBase.Configure.
+			UseNPrefixedTypesForUnicode = PropertiesHelper.GetBoolean(Environment.OracleUseNPrefixedTypesForUnicode, settings, false);
+			RegisterCharacterTypeMappings();


(Moving the comment on non-outdated file version.)
Possible breaking change for those using hbm2ddl: now Unicode string types will be mapped by default to non N prefixed types, while NH 4 was mapping to N prefixed one.

Doing it the other way round would be a possible breaking change for queries instead, because they would start using NVarchar2 for Unicode string parameters instead of Varchar2 (NH 4 currently maps that to Varchar2).

Other possible breaking change, character types will not be registered if the dialect is not configured, which may happen if not getting it through Dialect.GetDialect.

fredericDelaporte · 2017-09-02T10:38:00Z

Rebased with async regeneration embedded in "NH-4062 - Oracle Unicode support dual model." commit. (It does just alter one async test.)

hazzik · 2017-09-04T05:23:39Z

src/NHibernate/Driver/OracleDataClientDriverBase.cs

+		/// </exception>
+		protected OracleDataClientDriverBase(bool managed)
+			: this(
+				managed ? _managedProviderInvariantName : _unmanagedProviderInvariantName,


Can we move these back to the constructor instead?

I do not get it.

Would you like this to be removed and having manage and un-managed classes providing these values themselves? Or should the ReflectionBasedDriver be changed for allowing specifying these values without a constructor call (which forces to put that inside : this(...) or : base(...)), by example with some protected settable properties or a protected initialize method?
Or something else?

What I meant is to receive these strings from the child constructor instead of a bool argument. Eg. move this constructor down the inheritance chain:

public OracleManagedDataClientDriver : base(_managedProviderInvariantName, ...)

Also, it seems that we need to have only 2 variable strings: "Oracle.ManagedDataAccess" and "Oracle.DataAccess", all other are derivatives

Done, with only assembly name as a parameter. My previous idea was to be able to adapt to eventual weird naming changes done on Oracle side for some future version. But well, they may not do same than Sap with its Sybase drivers.

hazzik · 2017-09-06T03:28:33Z

src/NHibernate/Dialect/Oracle8iDialect.cs

+
+			var prefix = UseNPrefixedTypesForUnicode ? "N" : string.Empty;
+			RegisterColumnType(DbType.StringFixedLength, prefix + "CHAR(255)");
+			RegisterColumnType(DbType.StringFixedLength, 2000, prefix + "CHAR($l)");


Not sure why this is 2000.

Well me too... I am not sufficiently experimented with Oracle for knowing of any past reason, whether this should be overridden in newer dialects or not, ... So I have just kept the previous value.

hazzik · 2017-09-06T03:33:23Z

src/NHibernate/Driver/OracleDataClientDriverBase.cs

+			_oracleCommandBindByName = oracleCommandType.GetProperty("BindByName");
+
+			var parameterType = ReflectHelper.TypeFromAssembly(clientNamespace + ".OracleParameter", driverAssemblyName, true);
+			_oracleDbType = parameterType.GetProperty("OracleDbType");


Can we build delegates for these properties? Like in MySqlClientSqlCommandSet?

I forgot about it, you are right.

fredericDelaporte · 2017-09-06T11:57:30Z

src/NHibernate/Driver/OracleDataClientDriverBase.cs

+			_commandBindByNameSetter = DelegateHelper.BuildPropertySetter<bool>(oracleCommandType, "BindByName");
+
+			var parameterType = ReflectHelper.TypeFromAssembly(clientNamespace + ".OracleParameter", driverAssemblyName, true);
+			_parameterOracleDbTypeSetter = DelegateHelper.BuildPropertySetter<object>(parameterType, "OracleDbType");


We can not use the actual set type here for the delegate. This has required adapting DelegateHelper.

fredericDelaporte · 2017-09-06T11:59:37Z

src/NHibernate/Util/DelegateHelper.cs

+			var valueParameter = Expression.Parameter(typeof (T), "value");
+			Expression value = valueParameter;
+			// Cast value if required
+			if (!property.Type.IsAssignableFrom(typeof(T)))


Changes required for supporting setting without being able to provide the actual property type as a generic parameter.

fredericDelaporte force-pushed the NH-4062 branch 3 times, most recently from fc66069 to cdf4d5d Compare August 1, 2017 14:24

fredericDelaporte commented Aug 1, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4062 branch from cdf4d5d to 56ec0f2 Compare August 1, 2017 16:38

fredericDelaporte added the to review label Aug 1, 2017

fredericDelaporte force-pushed the NH-4062 branch from 56ec0f2 to d5e54db Compare August 2, 2017 11:06

fredericDelaporte commented Aug 2, 2017

View reviewed changes

hazzik modified the milestone: 5.0 Aug 3, 2017

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

fredericDelaporte force-pushed the NH-4062 branch 2 times, most recently from 3a0489d to 9b72bef Compare August 25, 2017 11:27

fredericDelaporte force-pushed the NH-4062 branch 2 times, most recently from 78f63cb to 006caa7 Compare September 2, 2017 10:36

hazzik reviewed Sep 4, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4062 branch from 006caa7 to bec901a Compare September 5, 2017 09:50

hazzik reviewed Sep 6, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4062 branch from bec901a to f0b8f34 Compare September 6, 2017 11:54

fredericDelaporte commented Sep 6, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-4062 branch 2 times, most recently from e5701dc to 4bf7d13 Compare September 6, 2017 23:32

hazzik previously approved these changes Sep 6, 2017

View reviewed changes

fredericDelaporte removed the to review label Sep 6, 2017

fredericDelaporte added 2 commits September 7, 2017 10:25

NH-4062 - Refactoring ODP.Net drivers for eliminating code duplication.

306f40a

NH-4062 - Oracle Unicode support dual model.

8ee4c72

fredericDelaporte dismissed hazzik’s stale review via 8ee4c72 September 7, 2017 08:26

fredericDelaporte force-pushed the NH-4062 branch from 4bf7d13 to 8ee4c72 Compare September 7, 2017 08:26

fredericDelaporte merged commit 05bac51 into nhibernate:master Sep 7, 2017

fredericDelaporte deleted the NH-4062 branch September 7, 2017 13:43

fredericDelaporte added the r: Fixed label Sep 7, 2017

NH-4062 - Oracle Unicode support dual model. #667

NH-4062 - Oracle Unicode support dual model. #667

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Aug 1, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Sep 2, 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!

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

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!

fredericDelaporte commented Aug 1, 2017 •

edited

Loading

fredericDelaporte Aug 1, 2017 •

edited

Loading

hazzik Sep 6, 2017 •

edited

Loading