IdentitySelectString implementation is inconsistent

[breglerj]

From NHibernate.Dialect.Dialect:

/// <summary> 
/// Get the select command to use to retrieve the last generated IDENTITY
/// value for a particular table 
/// </summary>
/// <param name="tableName">The table into which the insert was done </param>
/// <param name="identityColumn">The PK column. </param>
/// <param name="type">The <see cref="DbType"/> type code. </param>
/// <returns> The appropriate select command </returns>
public virtual string GetIdentitySelectString(string identityColumn, string tableName, DbType type)
{
	return IdentitySelectString;
}

From the comments I would expect the first parameter to be the table name. However, the name of the first parameter suggests it should be the column name.

From NHibernate.Persister.Entity.AbstractEntityPersister:

public string IdentitySelectString
{
	get
	{
		if (identitySelectString == null)
			identitySelectString =
				Factory.Dialect.GetIdentitySelectString(GetTableName(0), GetKeyColumns(0)[0], IdentifierType.SqlTypes(Factory)[0].DbType);
		return identitySelectString;
	}
}

Here the table name is passed as the first parameter matching the comment on NHibernate.Dialect.Dialect.GetIdentitySelectString.

From NHibernate.Persister.Collection.AbstractCollectionPersister:

public string IdentitySelectString
{
	get
	{
		if (identitySelectString == null)
		{
			identitySelectString =
				Factory.Dialect.GetIdentitySelectString(IdentifierColumnName, qualifiedTableName,	IdentifierType.SqlTypes(Factory)[0].DbType);
		}
		return identitySelectString;
	}
}

Here the column name is passed as the first parameter matching the parameter naming on NHibernate.Dialect.Dialect.GetIdentitySelectString.

[fredericDelaporte]

From the comments I would expect the first parameter to be the table name.

XML comment <param /> order is not significant. Tooling exploiting XML comments does care on parameter actual order and show the help re-ordered.

From NHibernate.Persister.Entity.AbstractEntityPersister:
...
Here the table name is passed as the first parameter

This is a latent bug indeed, since the first parameter is supposed to be a column name. This bug does not have any impact with built-in dialects currently, since none of them uses these parameters. But this causes this method to be unusable for a dialect for which column name or table name needs to be supplied in order to get the identity : either it would works for entities, or for collection, but not for both at the same time.

Thanks for the report.

[breglerj]

I came across this issue while implementing an NHibernate dialect for SAP HANA. HANA needs the table name when retrieving the last generated identity value. I'm planning on contributing the dialect once I'm done. Would that be ok with you?

[fredericDelaporte]

Since NHibernate already have three series of dialects for SAP products (ASE, ASA and SQL Anywhere, all seemingly originating from Sybase), I was wondering if a fourth one was really due, but it seems SAP HANA is actually another distinct database system.

So I understand it needs a dedicated dialect, and likely a dedicated driver too.

The way built-in dialects and drivers are included directly in NHibernate core is somewhat a legacy, from times where NuGet was not a de facto standard way of distributing software. And it has drawbacks, like drivers relying on reflection for avoiding taking a hard dependency on data providers of every database system. It also has the trouble of either being untested by the CI, or complicating further the CI by adding another database to test.

So instead of contributing the dialect/driver directly into NHibernate core, you may consider contributing them as a separated project within its own GitHub repository, yielding a dedicated NuGet package which could then take direct dependencies on the data provider. There is some work ongoing for splitting drivers (not yet dialects) out of NHibernate: #626. You can have a look at this PR to see in which direction the dialect/driver area is currently heading.

[breglerj]

Yes, HANA is a completely separate database and like you said it needs its own driver and dialect (actually two, one for the SAP HANA column store and one for the SAP HANA row store). The dialects will be more or less straight ports of the Hibernate HANA dialects.

I agree that having the drivers and dialects as separate packages is a good idea. However, for organizational reasons it is going to be extremely difficult for me to get approval to provide the HANA driver and dialects as a new GitHub project, especially because NHibernate is licensed under the LGPL. In addition, the HANA data provider isn't available via NuGet. So my proposal would be to contribute the minimum required functionality for NHibernate to work with HANA to the NHibernate core as a short-term solution. Once the drivers have been moved out of the NHibernate core I could then try to relocate the HANA driver to a separate project as well. Would that work for you?

[fredericDelaporte]

We should not use the bug report here to discuss another feature. There is the development group for this. Or create an issue about HANA dialect/driver (eventually directly a PR if you already have done the code, we do no more mandate a separated issue).

[breglerj]

Agreed. I'll be ready to submit a PR soon. Then we can discuss the PR directly.