NH-4058 - Fix Oracle (12c) Managed 32 failing tests. #666
Conversation
fredericDelaporte
force-pushed
the
NH-4058
branch
3 times, most recently
from
ddd708a to
8055a26
Compare
| return new AnotherItem {Name = tuple[0].ToString(), Description = tuple[1].ToString()}; | ||
| // Some db change the empty string to null, causing .ToString() to blow. | ||
| // https://stackoverflow.com/a/203536/1178314 | ||
| return new AnotherItem { Name = tuple[0]?.ToString(), Description = tuple[1]?.ToString() }; |
There was a problem hiding this comment.
That is an old Oracle behavior. '' => null.
src/NHibernate.Test/TestCase.cs
Outdated
| // Avoid ORA-12704 in tests, due to NHibernate defaulting String to NVarchar2 when creating tables while | ||
| // ODP.Net defaults it to Varchar2 (which does support Unicode too, depending on server) for parameters. | ||
| if (factory.ConnectionProvider?.Driver is OracleManagedDataClientDriver oracleManagedDriver) | ||
| oracleManagedDriver.UseNVarchar2ForStringParameter = true; |
There was a problem hiding this comment.
I have added that setting on the oracle managed driver. There are ore detail on the parameter comments. The culprit is what I see as a mismatch between NHibernate Oracle Dialect, matching String to NVarchar2, while ODP matches it to Varchar2 instead (which can be Unicode too, unlike with SQL Server). Oracle refuses to mix those string types for a number of operations, causing issues in some queries, because NHibernate creates columns as NVarchar2 while string parameters are transmitted as Varchar2.
Maybe we should change that in Oracle Dialect for re-aligning on ODP defaults. But that would then requires another Jira I think.
|
I have fixed only the failing one on TeamCity. On my local setup, with Oracle 11g XE, more are failing. |
| orderby p.ProductId | ||
| select p; | ||
|
|
||
| // ToList required otherwise the First call alters the paging and test something else than paging three pages. |
There was a problem hiding this comment.
I don't quite agree with this change.
There was a problem hiding this comment.
Skip(5).Take(5).First() does not make sense, it should be Skip(5).First(). Do you really think the test writer intended the previous code? To me, it looks like he wanted to take three pages, then made a somewhat failed attempt at just checking their first elements are not the same. The ToList guarantees we are actually taking those three pages, before starting the checks on them.
So previously, this test was IMO testing something else than what it was intended for. Something that bugs with Oracle by the way. But that bug is a bit corner case, the Take has no purpose anymore when calling First on it.
There was a problem hiding this comment.
Anyway, I think we shall not change the test. This is a valid linq expression, even if it does not make much sense for you. We shall either skip this test for oracle unchanged or/and create a JIRA issue for a specific case (with a minor/trivial priority).
fredericDelaporte
force-pushed
the
NH-4058
branch
from
fa43a35 to
fb0ce70
Compare
| @@ -76,10 +76,13 @@ public void DialectSQLFunctions() | |||
| rset = s.CreateQuery("select abs(round(s.Pay)) from s in class Simple").List(); | |||
| Assert.AreEqual(46f, rset[0], "abs(round(-45.8)) result was incorrect"); | |||
|
|
|||
| /* As of NH-3893, left and right functions are broken. Seemed confused with join keyword, and not | |||
There was a problem hiding this comment.
Should not comment a test. Need to exclude it with if statement on some condition
There was a problem hiding this comment.
Also, JIRA is required (or do we have it already?)
There was a problem hiding this comment.
That is NH-3893.
There are no other conditions than if (false) for avoiding the failure of this part of the test. It is broken, whatever the database. But since it was nested in an if (Dialect is Oracle8iDialect), it was failing only for Oracle.
There was a problem hiding this comment.
Ok, can we then remove this lines or move them outside of if oracle?
| /// for parameters type would be a major breaking change. | ||
| /// </para> | ||
| /// </remarks> | ||
| public bool UseNVarchar2ForStringParameter { get; set; } |
There was a problem hiding this comment.
This is the wrong assumption, I believe.
We have 4 different mappings of strings in ADO.NET:
- AnsiString - VARCHAR(n) (VARCHAR2(n) in the case of Oracle) - the single byte string collation dependent.
- AnsiStringFixedLength - CHAR(n) - fixed length string.
- String - NVARCHAR(n) (NVARCHAR2(n) in the case of Oracle) - always Unicode strings (UTF16, I think)
- StringFixedLength - NCHAR(n) - fixed length Unicode string (UTF16, I think).
We shall really fix the provider to obey this rule.
There was a problem hiding this comment.
I am convinced that NHibernate is wrong assuming Oracle Varchar2 is meant for single byte character encoding. See this Oracle page.
They have not implemented Unicode as a new string type, but simply as a new encoding, usable with any already existing string and character types.
The default recommended way of using Unicode with Oracle is to use a varchar2 with an adequate database encoding.
You can create a Unicode database that enables you to store UTF-8 encoded characters as SQL
CHARdatatypes (CHAR,VARCHAR2,CLOB, andLONG).
Using the N prefixed types are second option, notably meant for people wanting to transition incrementally to Unicode instead of globally switching to it.
If you prefer to implement Unicode support incrementally or if you need to support multilingual data only in certain columns, then you can store Unicode data in either the UTF-16 or UTF-8 encoding form in SQL
NCHARdatatypes (NCHAR,NVARCHAR2, andNCLOB). The SQLNCHARdatatypes are called Unicode datatypes because they are used only for storing Unicode data.
This effectively means Oracle has two Unicode support models. And NHibernate implements the one which looks like being the less used. If we really want to fix this mess, we need to give an option for letting the user tell which model he uses. This could be that UseNVarchar2ForStringParameter option, but for completeness, it needs to be taken into account by the dialect too.
The first recommended one, by the way, does not support anymore single byte encoding at all.
There was a problem hiding this comment.
Ok. Can we then separate the wheat from the chaff? I mean skip tests which are failing and create a bug report to be fixed later? As I'm not convinced with the solution of sneakily fixing such bugs
fredericDelaporte
force-pushed
the
NH-4058
branch
from
3c3a6e3 to
4857139
Compare
fredericDelaporte
force-pushed
the
NH-4058
branch
from
4857139 to
335e336
Compare
NH-4058 - Fix Oracle (12c) Managed 32 failing tests.