Fix Guid.ToString()

[lillo42]

Fixes #1151

[hazzik]

Current failure on firebird is due provider not storing GUID correctly: http://tracker.firebirdsql.org/browse/DNET-509 (fixed in 6.0+)

[hazzik]

I think this has gone too far. @fredericDelaporte any thoughts on this topic? Shall we support such a case or shall we throw NotSupportedException? Shall we refine ToString support?

[hazzik]

SybaseASA9Dialect most likely need to have an implementation similar to MySQL or Oracle.

[lillo42]

@hazzik Is there any Sybase I can use to test my query?

[hazzik]

I don't know. @fredericDelaporte might have one, but I'm not sure.

[fredericDelaporte]

Is there any Sybase I can use to test my query?

You can download and test SAP SQL Anywhere 17 (formerly Sybase SQL Anywhere, formerly Sybase ASA, formerly SQL Anywhere (yes they have abandoned then retaken that name), formerly Watcom SQL, Watcom having been bought by Sybase) for free from here, no time limitation, no registration key required. (SAP has bought Sybase and is phasing out the Sybase name.)
This is just an executable, not a service. Their .Net dataprovider is able of starting it automatically on first connection attempt. That is still a shared database but it is usable as an embedded one.
But you will need the driver from #1854, it is not currently merged.

In fact there are two Sybase database systems.
There is also SAP Adaptive Server Enterprise (ASE), trial available only for 90 days by default. This one is the "historical" Sybase SQL Server database, a time co-developed with Microsoft before they parted ways.

I consider that SybaseASA9Dialect is an obsolete thing, superseded by SybaseSQLAnywhere dialects (and maybe some time later by the SapSQLAnywhere dialect from #1854).
Better work on SybaseSQLAnywhere dialects rather than on the ASA one. Likely you can still test the ASA dialect with SAP SQL Anywhere 17, but again you will need the driver from #1854.

For the ASE ones, it is quite less convenient to test since free trials of ASE are time limited.

I think this has gone too far. @fredericDelaporte any thoughts on this topic? Shall we support such a case or shall we throw NotSupportedException? Shall we refine ToString support?

I have seen the test case, and I wonder, what is the use case for converting a GUID to a string representation on the db side rather than on the application side?
In principle, why not doing that, provided that is not a complex thing to support. But apparently it is complex to support, and even to test since the behavior of databases about Guid may vary according to the underlying system endianness.
So unless I am missing something, why not just advise to not convert Guid to string in projections, but instead get them as Guid and convert them by code? It looks far more reliable.

Throwing NotSupportedException would allow to state our stance explicitly, but if done whatever the dialect, that could be a serious breaking change for those using it with databases on which it currently does not have issues. Better do it only for dialects having issues.

It seems currently fixed for all tested databases. But for some of them, the fix looks brittle to me, and likely to be sensitive to system endianness. It would not be an issue for databases locked on one operating system family, but that is not the case of Oracle or MySQL by example. So for these databases, maybe better throw a not supported exception. (Is-it possible to "unregister" a function?)

[fredericDelaporte]

So works both under Linux and Windows. But endianness is processor dependent. I guess both run under x86/amd64 systems. Anyone able of testing this with a PowerPC maybe?

[hazzik]

It should not have problems with endianness because on these 2 systems Guids are stored as .ToByteArray() and retrieved by new Guid(byte[]) on a client side. As far as I know modern .NET Fx and .NET Core do not run on big-endian platforms. (Mono does, but we have much bigger problems there)

What we, potentially, can do is to add a runtime check into a function generator.

We also need to have a test which checks a server generated Guids.

[lillo42]

@fredericDelaporte and @hazzik The firebase test is faild because database credential is not define.

FirebirdSql.Data.FirebirdClient.FbException (0x80004005): Your user name and password are not defined. Ask your database administrator to set up a Firebird login. ---> Your user name and password are not defined. Ask your database administrator to set up a Firebird login.

[hazzik]

This happens some times (rarely)

[fredericDelaporte]

The Travis and AppVeyor builds works on clean, fresh virtual machine images not having a lot of pre-installed software on it. Databases like Firebird have to be installed as part of the build process. Sometimes these installation steps fail. We just relaunch the build in such case. When not a team member, you cannot do this yourself, unless relaunching all the builds with some new commits.

(On the other hand, the Teamcity build works on one supplied virtual machine with all the software we need permanently installed on it. So it has no installation steps troubles. But sometimes a build do not leave it in a clean state and causes subsequent builds to fail, like I did recently when testing a SAP SQL Anywhere build on it.)

[fredericDelaporte]

(double post)

[fredericDelaporte]

As in written in an unanswered comment, I wonder, what is the use case for converting a GUID to a string representation on the db side rather than on the application side?

In principle, why not doing that, provided that is not a complex thing to support. But apparently it is complex to support.

So unless I am missing something, why not just advise to not convert Guid to string in projections, but instead get them as Guid and convert them by code? It looks far more reliable.

(And for where clause, supply them as Guid rather than string.)

[fredericDelaporte]

Sorry for the double post. GitHub encounters difficulties today. One hour after my first post, it was still not showing up on the page, so I have redone it.

[lillo42]

@fredericDelaporte

As in written in an unanswered comment, I wonder, what is the use case for converting a GUID to a string representation on the db side rather than on the application side?

I don't know very well NHibernate Code, but I'm studying it. Is possible to NHibernate convert GUID on application side?

In principle, why not doing that, provided that is not a complex thing to support. But apparently it is complex to support.

So unless I am missing something, why not just advise to not convert Guid to string in projections, but instead get them as Guid and convert them by code? It looks far more reliable.

Where we can put this advise?

[fredericDelaporte]

The NHibernate GuidType has some built-in logic to accommodate the various ways databases handle them. So a property mapped as a Guid should already support most cases.

Here is its read logic:

nhibernate-core/src/NHibernate/Type/GuidType.cs

Lines 21 to 34 in ba4147c

	public override object Get(DbDataReader rs, int index, ISessionImplementor session)
	{
	if (rs.GetFieldType(index) == typeof (Guid))
	{
	return rs.GetGuid(index);
	}
	if (rs.GetFieldType(index) == typeof(byte[]))
	{
	return new Guid((byte[])(rs[index]));
	}
	return new Guid(Convert.ToString(rs[index]));
	}

If some databases prove to yield some other type requiring a special handling, it could be added here.

And its write logic:

nhibernate-core/src/NHibernate/Type/GuidType.cs

Lines 47 to 52 in ba4147c

	public override void Set(DbCommand cmd, object value, int index, ISessionImplementor session)
	{
	var dp = cmd.Parameters[index];
	dp.Value = dp.DbType == DbType.Binary ? ((Guid)value).ToByteArray() : value;
	}

The write logic relies on the driver to set the appropriate DbType on the parameter before hand, see here for example. If the current handling is not enough for some databases, maybe requiring another DbType, their driver would have to be adapted to set that other DbType, then the GuidType.Set could be adjusted to handle that other DbType case.

[lillo42]

Should I use connection string of mysql with Old Guids=True; ?
When I try with Old Guids=True; I can use this select: select Id from Entity;, but when I remove it my insert stop to work.

[fredericDelaporte]

Should I use connection string of mysql with Old Guids=True; ?
When I try with Old Guids=True; I can use this select: select Id from Entity;, but when I remove it my insert stop to work.

This thing is a MySql data provider setting.

According to there:

After version 6.1.1 you should add "old guids=true" to your connection string whenever you use BINARY(16) as your storage type. Else you should use CHAR(36)

It is also mentioned in the connector documentation:

This option was introduced in Connector/NET 6.1.1. The back-end representation of a GUID type was changed from BINARY(16) to CHAR(36). This was done to allow developers to use the server function UUID() to populate a GUID table - UUID() generates a 36-character string. Developers of older applications can add Old Guids=true to the connection string to use a GUID of data type BINARY(16).

So, deciding whether you need this setting or not depends on which type you use for storing your GUIDs.

In version 8, MySQL has gained functions for converting between the two representations: uuid-to-bin and bin-to-uuid

[fredericDelaporte]

Dues to a formatting issue in the comment, I only see this question now.

Where we can put this advise?

Well, it looks to me as such a specific need that I would tend to emit it only on issues or PR asking for supporting this.

[fredericDelaporte]

Although the fix still looks a bit "overkill" for my perceived usefulness of the fixed feature, at least now it is robustly tested. So why not merging this, if another contributor with validation rights approves it.