NO_BACKSLASH_ESCAPES support in 3.0 and MySqlConnect

[lauxjpn]

We used to work around errors in the NO_BACKSLASH_ESCAPES handling of MySqlConnector (and the original Net/Connector btw) for parameters with strings containing single quotes, by inlining all parameters into literals/constants, before executing the MySqlCommand.

I did reimplement this behavior for 3.0 and extended it to support UPDATE and INSERT statements as well (not pushed yet).

But since this is actually working around issues in MySqlConnector itself, we should fix those errors there (resp. add propper support for NO_BACKSLASH_ESCAPES there), so there is no need for specific workarounds anymore.

I will open an issue for that in the MySqlConnect repo, provide some test code and probably even a PR to fix the SqlParser state machine for proper NO_BACKSLASH_ESCAPES handling.

/cc @bgrainger

[bgrainger]

Yes, it sounds like MySqlConnector needs support for this SQL mode. (Interestingly, there have been no requests for this until now; I assume it's not very common?)

[lauxjpn]

This seems to be even weirder than I thought. I have created a couple of tests to run against MySqlConnector and the original Connector/ODBC 8.0.

As it turns out, Connector/ODBC 8.0 handles NO_BACKSLASH_ESCAPES even worse.

It doesn't mean, that it should not be implemented, but MySqlConnector would need to detect, if the NO_BACKSLASH_ESCAPES mode has been set or not.

[lauxjpn]

It is an even less common case for EF Core, because it is mostly irrelevant as long as the user is not executing raw SQL queries over the same connection.

It has been reference with #670 and #667 in this repo.

There is a StackOverflow post regarding the general issue, but it is not entirely correct in what it states.

@bgrainger The big question is, if you would be okay with either tracking/detecting NO_BACKSLASH_ESCAPES or adding an opt-in setting (as we have here with DisableBackslashEscaping) in MySqlConnector.
If that is fine with you, I can do the work on tweaking the state machine to also handle NO_BACKSLASH_ESCAPES mode, if you like.

Implementation:

To track the mode, the incomming queries would need to be parsed to some extend. Probably looking for sql_mode = and if so, querying the sql_mode and saving it after executing the original query.
So the cost would be an additional roundtrip when sql_mode appears in a query.

To detect the mode, it would have to be queries before every query execution. So two round trips instead of one for every query.
That is probably unacceptable for a high performance provider like MySqlConnector.

For the opt-in setting, there just needs to be added a property of some sort.

[bgrainger]

It is an even less common case for EF Core, because it is mostly irrelevant as long as the user is not executing raw SQL queries over the same connection.

Doesn't it affect any parameterised query? By default, MySqlConnector will escape ' and \ in a string parameter as \' and \\. With NO_BACKSLASH_ESCAPES, those strings will be understood incorrectly on the server (possibly leading to silent data corruption?).

The big question is, if you would be okay with either tracking/detecting NO_BACKSLASH_ESCAPES or adding an opt-in Setting (as we have here with DisableBackslashEscaping) in MySqlConnector.

Although detecting the SQL mode automatically would be the most convenient for users, I'd prefer an opt-in setting (in the connection string). To detect the SQL mode at runtime, we'd have to issue a query to the database server every time a connection is opened. That would be a performance penalty for all users (for what appears to be a very infrequently used SQL mode).

Is having a connection string setting (that Pomelo EFCore can set if DisableBackslashEscaping is true) a workable solution from your end?

[lauxjpn]

A connection string setting works for us.

Doesn't it affect any parameterised query? By default, MySqlConnector will escape ' and \ in a string parameter as ' and \. With NO_BACKSLASH_ESCAPES, those strings will be understood incorrectly on the server (possibly leading to silent data corruption?).

Yes, I meant that the whole mode should not really be needed, if there is no raw sql being executed by the Pomelo.MySql user.

Without NO_BACKSLASH_ESCAPES, the parameter @p0 with the string value It's okay, would be replaced by MySqlConnector with the string literal 'It\'s okay', which would than be saved in the database as It's okay.
So that works as expected.

With NO_BACKSLASH_ESCAPES, the parameter @p0 with the string value It's okay, should be replaced by MySqlConnector with the string literal 'It''s okay', which would then be saved in the database as It's okay.
But at the moment, it is being replaced by MySqlConnector with something mangled like 'It\\'s okay\', because the state machine gets confused, which leads to a syntax error.

The thing is that, as long as the user is not executing sql statements on his own, he doesn't need to care how they are being translated, as long as they are being saved and retrieved correctly. So as far as I can see, NO_BACKSLASH_ESCAPES mainly makes sense for someone using Pomelo.MySql, when raw sql statements are being mixed with DbContext calls on the same connection and he does not want the escape functionality for his own statements for some reason.

---

Anyway, I think NO_BACKSLASH_ESCAPES is way to prominently promoted on our main page here at the moment. This should really be buried deep in the wiki instead. (Done with f890ea0 and [wiki]:c7c47ed.)

[lauxjpn]

This feature is now fully supported by MySqlConnector.
I'll prepare the (very little) code changes, update the tests and merge, as soon as there is a MySqlConnector release out, that includes the support.

@bgrainger I will also do a quick test today, if the improved datatype handling (casting to similar types before returning to the caller) is sufficient now, so we can drop MySqlConverterRelationalCommandBuilder, or if there are any conversions left, that need to be addressed.

This would conclude all the currently needed changes for the 3.0.0 release from MySqlConnector.

[bgrainger]

MySqlConnector 0.58.0 is now available on NuGet.

[lauxjpn]

I updated all dependencies to .NET Core 3.0 and MySqlConnector to 0.58.0.

There are 10 more failing tests with MySqlConverterRelationalCommandBuilder disabled than without.
I will do a quick diff and then post, which conversions are the missing ones.

[lauxjpn]

The only missing conversion is hit, when the query is returning a float, but Pomelo.MySql is expecting a double.

This can happen, because of the workaround that is used for CAST(1 as float) for MySQL versions less than 8.0.17 (the current one), where proper double and float conversion was added.

The workaround is able to convert a value to a double, but not to a float.

Pomelo.EntityFrameworkCore.MySql/src/EFCore.MySql/Query/Internal/MySqlQuerySqlGenerator.cs

Lines 214 to 247 in ed38395

	// FLOAT and DOUBLE are supported by CAST() as of MySQL 8.0.17.
	// For server versions before that, a workaround is applied, that casts to a DECIMAL,
	// that is then added to 0e0, which results in a DOUBLE.
	// REF: https://stackoverflow.com/a/32991084/2618319
	var useDecimalToDoubleFloatWorkaround = false;
	if (castMapping.StartsWith("double") && (!_options?.ServerVersion.SupportsDoubleCast ?? false))
	{
	useDecimalToDoubleFloatWorkaround = true;
	castMapping = "decimal(65,30)";
	}
	if (castMapping.StartsWith("float") && (!_options?.ServerVersion.SupportsFloatCast ?? false))
	{
	useDecimalToDoubleFloatWorkaround = true;
	castMapping = "decimal(65,30)";
	}
	if (useDecimalToDoubleFloatWorkaround)
	{
	Sql.Append("(");
	}
	Sql.Append("CAST(");
	Visit(sqlUnaryExpression.Operand);
	Sql.Append(" AS ");
	Sql.Append(castMapping);
	Sql.Append(")");
	if (useDecimalToDoubleFloatWorkaround)
	{
	Sql.Append(" + 0e0)");
	}

So Row.GetFloat() needs to cast down from double as well, as it already does for decimal here:

https://github.com/mysql-net/MySqlConnector/blob/c28126ed97700c4f71f3845c0f9f9cbcc12d6c47/src/MySqlConnector/Core/Row.cs#L353-L358

[bgrainger]

https://www.nuget.org/packages/MySqlConnector/0.59.0 is available to fix this conversion.