Generate stable sql objects names #1802

fredericDelaporte · 2018-07-16T15:54:51Z

Possible breaking change:

Auto-generated constraint names will not be the same than the ones generated with previous NHibernate versions under .Net Framework. (Under .Net Core those names were anyway changing at each run.)

fredericDelaporte · 2018-07-16T15:57:44Z

src/NHibernate.Test/NHSpecificTest/NH1399/Fixture.cs

 using NHibernate.Mapping;
 using NUnit.Framework;

 namespace NHibernate.Test.NHSpecificTest.NH1399
 {
-	[TestFixture]
+	[TestFixture, Obsolete]
 	public class Fixture


This fixture is testing Table.UniqueColumnString with a non-null referencedTable parameter, case which was no more having any usage in NHibernate. So indeed these tests are obsolete since some times.

fredericDelaporte · 2018-07-16T15:59:13Z

src/NHibernate/Cfg/Configuration.cs

+							if (string.IsNullOrEmpty(fk.Name))
+							{
+								fk.Name = Constraint.GenerateName(
+									fk.GeneratedConstraintNamePrefix, table, fk.ReferencedTable, fk.Columns);


This is a heavily simplified port of the Hibernate code, which instead uses name policy generation and bothers with quoting/unquoting (like the things removed in #1703).

fredericDelaporte · 2018-07-16T16:01:29Z

src/NHibernate/Mapping/Constraint.cs

+			// Hash the generated name for avoiding collisions with user choosen names.
+			// This is not 100% reliable, as hashing may still have a chance of generating
+			// collisions.
+			var name = prefix + HashName(sb.ToString());


I quite dislike the idea of "uniqueness through hashcode", since it is fundamentally flawed: collisions may occur, even if very unlikely. But I have no better idea here.

fredericDelaporte · 2018-07-16T16:04:32Z

src/NHibernate/Mapping/Constraint.cs

+			if (name.Length > 30)
+			{
+				// This, of course, increases the collision risk.
+				name = name.Substring(0, 30);


Here I was tempted to use Dialect.MaxAliasLength. But that would require to partially undo #1703... Maybe instead should we add some setting for the max length here. After all we are not generating an alias. And a setting would be quite less heavy to resolve than the dialect.

@hazzik, any thoughts here?

fredericDelaporte · 2018-07-16T16:08:43Z

src/NHibernate/Mapping/Constraint.cs

+		/// <returns>The hased name.</returns>
+		private static string HashName(string name)
+		{
+			// Hibernate uses MD5, but with .Net this would throw on FIPS enabled machine.


This thing is from my point of view a very unfortunate design decision of the .Net Framework: if FIPS is enabled on the machine, any usage of non FIPS compliant algorithm is forbidden. But FIPS does not forbid them, it just forbids to use them for handling a security related task. And here we are not handling a security related task. But there is no way to tell it for allowing using MD5 here even on FIPS enabled machine. See this Microsoft blog (may be very long to load) for more information.

So if we want to use MD5 here without causing NHibernate to be unusable on a FIPS enabled machine, we would have to use a custom implementation, like this one.

fredericDelaporte · 2018-07-16T16:10:51Z

src/NHibernate/Mapping/Constraint.cs

+		{
+			// Hibernate uses MD5, but with .Net this would throw on FIPS enabled machine.
+			// As a consequence generated names will be quite longer.
+			using (var hasher = SHA256.Create())


This one should pass on a FIPS enabled machine, SHA256.Create being documented as choosing a FIPS compliant one if needed. At first I was willing to use HashAlgorithm.Create instead (which default to SHA-1, a bit shorter), but it throws PlatformNotSupported under Core.

fredericDelaporte · 2018-07-16T16:12:41Z

src/NHibernate/Mapping/Constraint.cs

+				// Converting to base 32 for shortening the name.
+				// Hibernate uses base 35, but we do not have a native implementation
+				// in .Net, and base 32 is easier to implement.
+				return ToBase32String(hash);


Hibernate creates a BigInteger from the byte array (.Net can do that too) then asks its base 35 representation: .Net does not have this natively, and implementing it was a bit heavy in my opinion.

hazzik · 2018-07-16T21:59:06Z

I think using a cryptographic hash for this trivial task is an overkill. I was researching using lightweight non-cryptographic hashes to perform the operation, like MurmurHash2 and xxHash. xx claims to be the fastest. They both generate 32 bit integer as a result (they have 64 bit versions, but I think this is an overkill again) and provide 10/10 results on SMHasher. I think it should be enough for us.

Both algorithms are open-sourced with compatible licenses

fredericDelaporte · 2018-07-17T09:56:09Z

For avoiding adding a new dependency, we could take xxHash version from Seok-Ju Yun by taking just the part of the source we need. The speed claim is about C version, so it may not be faster than MurMurHash2 unsafe or hackish versions from David Landmann (which we could also incorporate as source code).

But for the usage it will have, mapping compilation and only on some foreign keys and unique constraints, I do not think it will make much differences.

Fix nhibernate#1769

hazzik · 2018-07-17T12:02:43Z

I benchmarked the implementations:

BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i7-2670QM CPU 2.20GHz (Sandy Bridge), 1 CPU, 8 logical and 4 physical cores
Frequency=2143568 Hz, Resolution=466.5119 ns, Timer=TSC
.NET Core SDK=2.1.302
  [Host] : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT
  Clr    : .NET Framework 4.7 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3131.0
  Core   : .NET Core 2.1.2 (CoreCLR 4.6.26628.05, CoreFX 4.6.26629.01), 64bit RyuJIT

Method	Job	Runtime	N	Mean	Error	StdDev	Rank
_MurmurHash2Simple	Clr	Clr	1000	1,941.1 ns	16.336 ns	15.281 ns	7
_MurmurHash2InlineBitConverter	Clr	Clr	1000	1,249.7 ns	7.251 ns	6.782 ns	4
_MurmurHash2Unsafe	Clr	Clr	1000	387.5 ns	5.415 ns	4.800 ns	2
_xxHash	Clr	Clr	1000	2,791.2 ns	17.844 ns	16.691 ns	8
_xxHash2	Clr	Clr	1000	2,820.5 ns	25.035 ns	19.546 ns	9
_MurmurHash2Simple	Core	Core	1000	803.6 ns	2.764 ns	2.586 ns	3
_MurmurHash2InlineBitConverter	Core	Core	1000	1,249.1 ns	15.606 ns	12.184 ns	4
_MurmurHash2Unsafe	Core	Core	1000	383.0 ns	1.749 ns	1.636 ns	1
_xxHash	Core	Core	1000	1,693.8 ns	16.848 ns	14.069 ns	6
_xxHash2	Core	Core	1000	1,680.4 ns	6.565 ns	6.141 ns	5
_MurmurHash2Simple	Clr	Clr	10000	19,309.7 ns	183.592 ns	143.336 ns	14
_MurmurHash2InlineBitConverter	Clr	Clr	10000	12,535.8 ns	57.996 ns	48.429 ns	12
_MurmurHash2Unsafe	Clr	Clr	10000	3,748.7 ns	25.359 ns	21.176 ns	10
_xxHash	Clr	Clr	10000	28,138.2 ns	161.633 ns	134.971 ns	15
_xxHash2	Clr	Clr	10000	28,212.8 ns	61.340 ns	57.378 ns	15
_MurmurHash2Simple	Core	Core	10000	7,969.3 ns	28.366 ns	25.146 ns	11
_MurmurHash2InlineBitConverter	Core	Core	10000	12,541.1 ns	65.146 ns	57.750 ns	12
_MurmurHash2Unsafe	Core	Core	10000	3,754.9 ns	15.327 ns	14.337 ns	10
_xxHash	Core	Core	10000	16,782.3 ns	113.966 ns	106.604 ns	13
_xxHash2	Core	Core	10000	16,825.2 ns	42.787 ns	40.023 ns	13

hazzik · 2018-07-17T12:18:43Z

Let's stick with MurMurHash2Simple

Remove cryptographic hash

hazzik · 2018-07-17T22:27:55Z

src/NHibernate/Mapping/Constraint.cs

+		{
+			// Use a concatenation that guarantees uniqueness, even if identical names
+			// exist between all table and column identifiers.
+			var sb = new StringBuilder("table`" + table.Name + "`");


Please use StringBuilder.Append instead of concatenation of the string. Eg:

var sb = new StringBuilder("table`").Append(table.Name).Append("`");

hazzik · 2018-07-17T22:28:02Z

src/NHibernate/Mapping/Constraint.cs

+			// exist between all table and column identifiers.
+			var sb = new StringBuilder("table`" + table.Name + "`");
+			if (referencedTable != null)
+				sb.Append("references`" + referencedTable.Name + "`");


Same as above

hazzik · 2018-07-17T22:33:25Z

src/NHibernate/Mapping/Constraint.cs

+			public static readonly ColumnComparator Instance = new ColumnComparator();
+
+			public int Compare(Column col1, Column col2) {
+				return StringComparer.Ordinal.Compare(col1?.Name, col2?.Name);


This should compare on Column.CanonicalName. The Name of a Column class is case-sensitive when quoted and case-insensitive when not quoted.

No, it depends on database. SQL Server 2017 keeps respecting the collation of the database (in fact, of its metadata tables), quoted or not.
It may also be case sensitive when unquoted, not only when quoted. And this varies from databases to databases.

Quoted identifier being case sensitive is an Oracle feature. So I think it is better taking the name "as is", relying on the user to map its objects in a consistent way.

I'm not talking about databases here, but about rules of NHibernate itself. Column has overridden Equals and GetHashCode operators which follow this rule. So we need to be consistent.

hazzik · 2018-07-17T22:34:38Z

src/NHibernate/Mapping/Constraint.cs

+			foreach (var column in alphabeticalColumns)
+			{
+				var columnName = column == null ? "" : column.Name;
+				sb.Append("column`").Append(columnName).Append("`");


Again, here should be CanonicalName prob.

Use StringBuilder fully

hazzik · 2018-07-18T00:48:44Z

src/NHibernate/Mapping/Constraint.cs

+			// bindings are given.
+			var alphabeticalColumns = new List<Column>(columns);
+			alphabeticalColumns.Sort(ColumnComparator.Instance);
+			foreach (var column in alphabeticalColumns)


Why not just use foreach (var column in columns.OrderBy(c=>c.Name))?

Because I have ported the Java code without thinking about it.

Simplify code and use canonical name

bahusoid · 2018-07-18T06:07:25Z

Maybe we should include in generated key some truncated real parts like table and/or column names to minimize collisions risks on large databases and make generated keys more "user friendly"
Something like "FK_TABLEX_3B355A0C"

hazzik · 2018-07-18T07:18:48Z

User has an ability to specify a name, so I would not bother

fredericDelaporte added t: Fix possible breaking change labels Jul 16, 2018

fredericDelaporte added this to the 5.2 milestone Jul 16, 2018

fredericDelaporte force-pushed the sqlObjectsStableNames branch 2 times, most recently from 579dcb0 to 464cb10 Compare July 16, 2018 16:14

fredericDelaporte commented Jul 16, 2018

View reviewed changes

fredericDelaporte mentioned this pull request Jul 17, 2018

Cease using cryptographic hashes nhibernate/NHibernate-Caches#52

Merged

Generate stable sql objects names

9241c59

Fix nhibernate#1769

fixup! Generate stable sql objects names

3323db7

Remove cryptographic hash

fredericDelaporte force-pushed the sqlObjectsStableNames branch from 464cb10 to 3323db7 Compare July 17, 2018 12:39

fredericDelaporte changed the title ~~WIP - Generate stable sql objects names~~ Generate stable sql objects names Jul 17, 2018

hazzik reviewed Jul 17, 2018

View reviewed changes

fixup! Generate stable sql objects names

e77d02f

Use StringBuilder fully

hazzik reviewed Jul 18, 2018

View reviewed changes

fixup! Generate stable sql objects names

9aa4b76

Simplify code and use canonical name

hazzik approved these changes Jul 18, 2018

View reviewed changes

fredericDelaporte merged commit 2f8f317 into nhibernate:master Jul 18, 2018

fredericDelaporte deleted the sqlObjectsStableNames branch July 18, 2018 13:21

fredericDelaporte added the r: Fixed label Jul 18, 2018

Generate stable sql objects names #1802

Generate stable sql objects names #1802

Uh oh!

Conversation

fredericDelaporte commented Jul 16, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Jul 16, 2018 • 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 Jul 16, 2018 • 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!

hazzik commented Jul 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Jul 17, 2018

Uh oh!

hazzik commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hazzik commented Jul 17, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid commented Jul 18, 2018

Uh oh!

hazzik commented Jul 18, 2018

Uh oh!

Uh oh!

fredericDelaporte commented Jul 16, 2018 •

edited

Loading

fredericDelaporte Jul 16, 2018 •

edited

Loading

fredericDelaporte Jul 16, 2018 •

edited

Loading

hazzik commented Jul 16, 2018 •

edited

Loading

hazzik commented Jul 17, 2018 •

edited

Loading