Fix NotNullUnique not taken into account for single column

[fredericDelaporte]

And rename it to something more explicit.

While working on #1854 test failures, it appeared to me that Dialect.SupportsNotNullUnique was not taken into account for single column unique constraints.

On a multi-columns unique constraint, a dialect yielding false for this property ignore the constraint and does not create it in the database. But on single column unique constraint, it was still created. No built-in dialect are setting this property to false currently.

But since SQL Anywhere rejects any null value in column having a unique constraint, I have tried to use that feature, and find out this shortcoming.
Note that SQL Anywhere does support null in unique indexes. But this Dialect.SupportsNotNullUnique feature, even for multi-columns constraints, does not fallback on creating an index instead. I have not changed this since it could be a breaking change if some other database with an externally implemented dialect was supporting null neither on unique constraints nor on unique indexes.

[hazzik]

Actually this property in Hibernate serves a different purpose, but we have harassed it.

In Hibernate this field means "does this dialect support combining 'not null' and 'unique' in the column definition of a create table statement", eg:

create table A (
  X int not null unique -- <-- here
)

If the code above is not supported, Hibernate would make 2 statements: create table + create unique index:

create table A (
  X int not null
);
create unique index ix_name on A (X);

But there are not dialects which declare that this is not supported.

In NHibernate we harassed this field and make it's meaning opposite to hibernate: "does this dialect support nullable columns in the unique index".

[hazzik]

I was thinking about this when fixing DB2 dialect (still WIP). We need to refine code around uniqueness and nullability.

Current solution (excluding nullable columns from the unique index) is a ugly "workaround" which leads to the dirty data: the user would expect unique index violation, but would not get one.

I think we shall the creation of the table/index fail instead of excluding columns.

[fredericDelaporte]

Maybe this is just a misunderstanding, but for me it does not exclude columns from the unique constraint (which would lead to a too narrow unique constraint), but it disable the unique constraint completely.

I do not think we should cause the creation of the table to fail, because this would harm portability from a database to another: the mapping would be valid for databases supporting null in unique, but it would be considered invalid for those not supporting it. Giving up the null constraint seems preferable to me in such case: it does still allow the application to run, maybe with a degraded feature somewhere.
(Normally unique constraint violations should be just a last line of defense. An application should check uniqueness beforehand for avoiding having to intercept and analyze database exceptions in order to provide meaningful user errors.)

Now instead of ignoring the unique constraint, an unique index could be created instead. At least SQL Anywhere does support null in unique indexes, although not supporting them in constraints. But I think this would require an additional property for also allowing to handle a dialect which would not support null anywhere.

[hazzik]

Maybe this is just a misunderstanding, but for me it does exclude columns from the unique constraint (which would lead to a too narrow unique constraint), but it disable the unique constraint completely.

It seems that you are missing a "no/t" somewhere so I struggle to understand what you're saying

[hazzik]

What I was trying to say in my original comment that you are changing a property which does mean something different. You should not change that property but create a new one with the intended logic.

[fredericDelaporte]

It seems that you are missing a "no/t" somewhere so I struggle to understand what you're saying

Fixed

What I was trying to say in my original comment that you are changing a property which does mean something different. You should not change that property but create a new one with the intended logic.

From my point of view, this PR does just that: add a new property with intended logic. The property SupportsNotNullUnique is obsoleted and replaced. Keeping it would not make sense, since it was not doing what is was meant for.

[hazzik]

it does not exclude columns from the unique constraint (which would lead to a too narrow unique constraint), but it disable the unique constraint completely.

Your pr does exactly that:

public class TestFixture
{
	public class TestDialect : Dialect.Dialect
	{
		public TestDialect()
		{
			RegisterColumnType(DbType.Int32, "int");
		}

		public override bool SupportsUnique => true;
		public override bool SupportsNullInUnique => false;
	}

	[Test]
	public void MyTest()
	{
		var table = new Table("MyTable");
		table.AddColumn(
			new Column("A")
			{
				Value = new SimpleValue(table) {TypeName = "int"},
				Unique = true, 
				IsNullable = true
			});
		table.AddColumn(
			new Column("B")
			{
				Value = new SimpleValue(table) {TypeName = "int"}, 
				Unique = true, 
				IsNullable = false
			});

		var createString = table.SqlCreateString(
			new TestDialect(),
			TestConfigurationHelper.GetDefaultConfiguration().BuildMapping(),
			null,
			null);

		Console.WriteLine(createString);
	}

}

results in:

create table MyTable (A int, B int not null unique)

[hazzik]

Also, your PR claims that MsSql*Dialect family support null columns in unique, while they do not.

[hazzik]

Please add a proper test from the comment above enabled for all dialects. It should test following:

A table with 2 columns (apart from Id) in a unique key (marked with unique=true). One is nullable, and one is not. Insert values with different combinations: (X, Y); (X, Z); (X, NULL), (NULL, Z), (X, Z), (Y, Z).

[fredericDelaporte]

What is the trouble with your example? This is what is expected. Your way of mapping unique is expected to create two separated unique constraints. Only the one not supported is ignored, not the one supported.

For creating an unique constraint spanning multiple columns, either name it (use the unique-key string instead of the unique boolean), or use a component. In such case, if one column is nullable with a dialect not supporting it, no constraint will be generated.

Also, your PR claims that MsSql*Dialect family support null columns in unique, while they do not.

They do support it. The tests Reversed...TestOrphanedWhileManaged would fail otherwise, because they do put a null in a column belonging to an unique constraint. But it succeeds with MsSQL.

[fredericDelaporte]

What MsSQL does not support is multiple null values in a unique constraint: it treats them as values equal to themselves, and so considers them as violating the unique constraint. But it does support a single null.
That is why I have put this comment on the new property:

Some databases do not accept <c>null</c> in unique constraints at all. In such case,
this property should be overriden for yielding <c>false</c>. This property is not meant for distinguishing
databases ignoring <c>null</c> when checking uniqueness (ANSI behavior) from those considering <c>null</c>
as a value and checking for its uniqueness.

[hazzik]

It seems you are correct. It's not my day today. Sorry about grumpiness.

[fredericDelaporte]

But I can/should still put more complete tests.

[fredericDelaporte]

As proposed on the development group, and since no continued grounded disagreement has been expressed there since one week after raised concerns being addressed, I intend to merge this PR next week (likely on Wednesday).

(I am issuing this notice on PR issued before starting to apply this handling of merges. I will not do it for new PR.)