Correct MaxAliasLength for various dialects #1415

fredericDelaporte · 2017-11-01T19:15:18Z

hazzik · 2017-11-01T20:03:31Z

src/NHibernate/Dialect/SQLiteDialect.cs

@@ -395,6 +395,10 @@ public override bool SupportsForeignKeyConstraintInAlterTable
 		/// </remarks>
 		public override bool SupportsConcurrentWritingConnections => false;

+		// Said to be unlimited. http://sqlite.1065341.n5.nabble.com/Max-limits-on-the-following-td37859.html
+		/// <inheritdoc />
+		public override int MaxAliasLength => 100;


I think it needs to be aligned with others. Say 128.

hazzik · 2017-11-01T20:04:37Z

src/NHibernate/Dialect/IngresDialect.cs

+		// Ingres 10.2 supports 256 bytes (so worst unicode case would mean 64 characters), but I am unable to find
+		// the limit for older versions, excepted many various sites mention a 32 length limit. Being conservative.
+		/// <inheritdoc />
+		public override int MaxAliasLength => 32;


I think we need not specify if we’re not sure.

hazzik · 2017-11-01T20:05:16Z

src/NHibernate/Dialect/Dialect.cs

+		/// <summary>
+		/// The maximal length a SQL alias can have.
+		/// </summary>
+		public virtual int MaxAliasLength => 10;


Take the minimum here - 18

.

hazzik · 2017-11-01T22:27:16Z

src/NHibernate/Dialect/Dialect.cs

-			get { return 10; }
-		}
+		/// <summary>
+		/// The maximal length a SQL alias can have.


It seems it shall be "The maximum".

fredericDelaporte · 2017-11-02T09:36:35Z

src/NHibernate/Dialect/Oracle12cDialect.cs

+
+		// 128 since 12.2. https://stackoverflow.com/a/756569/1178314
+		/// <inheritdoc />
+		public override int MaxAliasLength => 128;


OK, our own CI is 12.1, and fails due to that, should have stayed on the safe side here.

fredericDelaporte · 2017-11-02T09:42:45Z

src/NHibernate/Dialect/FirebirdDialect.cs

+		// cases supporting more but considered as bugs and no more tolerated in v3.
+		// It seems it may be extended to 63 for Firebird v4.
+		/// <inheritdoc />
+		public override int MaxAliasLength => 30;


It seems we still have a bug. A test has generated the alias requestcompletionstatus3_8691_0_ for a query, that is 32 characters long.

It's because the identifier has 2 "unique" parts for some reasons.

It's in "Column.cs", line 173.

Or not. I'm confused.

I am going to have a look. I am currently adjusting lengths and sources for those lengths.

It is from SelectFragment.cs suffix field. Which is a "collectionSuffix" which is a table number + _.

Run one by one, the tests succeed. It seems there is some coupling between tests.

The coupling is very likely coming from the thread static tableCounter in Table class. It seems it endlessly grows as we build/discard ~~session factories~~ configuration.
~~Either this field should be reset when starting to build a session factory (if it has no more usages after build) or~~ it should be bound to the ~~session factory~~ configuration instead of the thread on which it was built. (Moved to #1417.)

The SelectFragment suffix is an additional suffix which is meant (at least in the cases I have checked) to differentiate columns of the same table in case it is auto-joined. It does not detect auto-join but simply always add the suffix according to query joins count.

It then means the Column.GetAlias should account for that potential additional suffix by not exhausting the full maximum length. At least 3 characters should be left in my opinion, for supporting more than 9 joins (up to 99) and the additional _ of the suffix. (Moved to #1418.)

If we want want to support more than 99 joins (up to 999), we should keep 4 characters left, but I am not used to have such monstrous queries with NHibernate.

fredericDelaporte · 2017-11-02T15:29:42Z

Added fix for #1418. It could be put as a separated PR, but the trouble it fixes is revealed only in this current PR, and is related. I may still drop that commit and put it in another PR if you prefer.

fredericDelaporte · 2017-11-03T10:58:56Z

#879 commits squashed, I have pushed the Oracle build up in its queue and intend to merge after it rather than right now, even if there is no reason for the squash to introduce a failure.

hazzik · 2017-11-03T11:23:36Z

@fredericDelaporte are you going to squash or merge commits?

fredericDelaporte · 2017-11-03T11:28:12Z

Merge, since it is two issues. What I have squashed was the three commits concerning 879.

hazzik · 2017-11-03T11:29:48Z

Then you need to reverse order of the commits.

AbstractCollectionPersister and SelectFragment could add additional suffix to an alias Fixes nhibernate#1418

Fixes nhibernate#879

hazzik · 2017-11-03T11:45:38Z

src/NHibernate/Mapping/Column.cs

@@ -115,10 +115,14 @@ public string GetQuotedName(Dialect.Dialect d)
 			return IsQuoted ? d.QuoteForColumnName(_name) : _name;
 		}

+		// Accomodate the one character suffix appended in AbstractCollectionPersister and


Typo should be "Accommodate" (double "m")

hazzik · 2017-11-03T11:49:32Z

I've changed commits order, reworded messages and fixed a typo.

fredericDelaporte · 2017-11-03T11:50:50Z

Sorry, done the same right now excepted reword of commit messages...

hazzik · 2017-11-03T11:51:39Z

@fredericDelaporte force push war is fun 😂

fredericDelaporte · 2017-11-03T11:52:07Z

Just reforce your push^^.

hazzik · 2017-11-03T11:52:35Z

Ok, done.

hazzik previously approved these changes Nov 1, 2017

View reviewed changes

hazzik reviewed Nov 1, 2017

View reviewed changes

fredericDelaporte commented Nov 2, 2017

View reviewed changes

fredericDelaporte mentioned this pull request Nov 2, 2017

Table counter for aliases should be stable #1417

Closed

hazzik previously approved these changes Nov 2, 2017

View reviewed changes

hazzik modified the milestones: 5.1, 5.0.1 Nov 2, 2017

hazzik added c: Core c: DataProviders / Dialects p: Major t: Bug labels Nov 2, 2017

fredericDelaporte dismissed hazzik’s stale review via 32fd87d November 3, 2017 10:35

fredericDelaporte force-pushed the 879 branch from 5b932b2 to 32fd87d Compare November 3, 2017 10:35

fredericDelaporte added 2 commits November 4, 2017 00:44

Allow some room for other suffixes in Column.GetAlias

52f7cdf

AbstractCollectionPersister and SelectFragment could add additional suffix to an alias Fixes nhibernate#1418

Provide correct MaxAliasLength for various dialects

0b20a3b

Fixes nhibernate#879

hazzik reviewed Nov 3, 2017

View reviewed changes

hazzik force-pushed the 879 branch from 32fd87d to 0b20a3b Compare November 3, 2017 11:46

hazzik changed the title ~~Fix #879 - Provide a correct MaxAliasLength for various dialects~~ Correct MaxAliasLength for various dialects Nov 3, 2017

hazzik previously approved these changes Nov 3, 2017

View reviewed changes

fredericDelaporte dismissed hazzik’s stale review via 3b492d5 November 3, 2017 11:49

fredericDelaporte force-pushed the 879 branch from 0b20a3b to 3b492d5 Compare November 3, 2017 11:49

hazzik force-pushed the 879 branch from 3b492d5 to 0b20a3b Compare November 3, 2017 11:51

hazzik merged commit 6bca2b0 into nhibernate:master Nov 3, 2017

fredericDelaporte deleted the 879 branch November 3, 2017 11:54

hazzik added the r: Fixed label Nov 3, 2017

fredericDelaporte mentioned this pull request Jan 5, 2018

[WIP] Intern string metadata in Session Factory #1503

Closed

Correct MaxAliasLength for various dialects #1415

Correct MaxAliasLength for various dialects #1415

Uh oh!

Conversation

fredericDelaporte commented Nov 1, 2017 • edited by hazzik 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!

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!

Choose a reason for hiding this comment

Uh oh!

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

fredericDelaporte commented Nov 2, 2017

Uh oh!

fredericDelaporte commented Nov 3, 2017

Uh oh!

hazzik commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte commented Nov 3, 2017

Uh oh!

hazzik commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hazzik commented Nov 3, 2017

Uh oh!

fredericDelaporte commented Nov 3, 2017

Uh oh!

hazzik commented Nov 3, 2017

Uh oh!

fredericDelaporte commented Nov 3, 2017

Uh oh!

hazzik commented Nov 3, 2017

Uh oh!

Uh oh!

fredericDelaporte commented Nov 1, 2017 •

edited by hazzik

Loading

fredericDelaporte Nov 2, 2017 •

edited

Loading

hazzik commented Nov 3, 2017 •

edited

Loading