Change AllowUserVariables default to true

[bgrainger]

As mentioned in #193, the default value for Allow User Variables is false, to match MySql.Data. Is this still a good default? (It feels like unexpected behaviour to me.) Would it be a major breaking change to make it true by default?

[bgrainger]

Looks like this was a breaking change introduced in 5.2.x that's caused problems for people because it's not obvious why disallowing user variables should be the default behaviour.

All AllowUserVariables=false does is throw an exception early before the SQL is sent to the server. I can't think of a good reason to do this instead of just sending the SQL to MySQL Server and letting it determine if it's valid or not. (Certainly for ADO.NET users coming from SQL Server there's no need to opt in to using variables.)

[bgrainger]

More users assuming this is a bug in the MySql.Data connector:

• https://bugs.mysql.com/bug.php?id=44599
• http://stackoverflow.com/questions/5524632/how-can-i-use-a-mysql-user-defined-variable-in-a-net-mysqlcommand/

[caleblloyd]

I would like to look into this a little more, should have time in a few more days

[bgrainger]

More users reporting this default behaviour as a bug:

• https://bugs.mysql.com/bug.php?id=42776
• https://bugs.mysql.com/bug.php?id=46054

In the first bug, a maintainer explains:

This is not a bug. The default behavior is that the provider will look for variables (strings starting with ? or @) and assume they are parameters that should be set. If not, it throws an error. If you want to run sql that contains these strings, then you need to set 'Allow User Variables=true' on your connection string. This will cause the provider to find the variables and see if there is a parameter of that name. If there is, it uses it. If not, it assumes it is a user variable and accepts it.

However, this still doesn't explain why the default behaviour was chosen.

[bgrainger]

More users reporting it as a bug:

• https://bugs.mysql.com/bug.php?id=40765
• https://bugs.mysql.com/bug.php?id=52785
• https://bugs.mysql.com/bug.php?id=67613

From bug 40765:

The default mode for Connector/Net assumes you will not be using user variables (as most users don't) and so it sees @data and expects to find a parameter named @data.

This isn't a very convincing rationale.

There's a hint in this archived documentation:

Using the '@' symbol for parameters is now the preferred approach although the old pattern of using '?' is still supported.
Please be aware however that using '@' can cause conflicts when user variables are also used. To help with this situation please see the documentation on the Allow User Variables connection string option

And from Connection String Options Reference:

OldSyntax
This option was deprecated in Connector/NET 5.2.2. All code should now be written using the '@' symbol as the parameter marker.

Then, from the 5.2.0 release notes:

• Marked connection string option 'Use Old Syntax' as obsolete and changed code to allow both @ and ? as parameter markers.
• Added Allow User Variables connection string option so that users can use user variables without getting missing parameter exceptions. This feature is related to our move back to using @ as parameter marker. (see release notes)

From an old mailing list post:

The syntax for setting parameters has changed. Now you use ? instead of @ to mark parameters. This change was necessary for proper compatibility with MySQL.

And one final piece of old documentation:

Note. Prior versions of the provider used the '@' symbol to mark parameters in SQL. This is incompatible with MySQL user variables, so the provider now uses the '?' symbol to locate parameters in SQL. To support older code, you can set 'old syntax=yes' on your connection string. If you do this, please be aware that an exception will not be throw if you fail to define a parameter that you intended to use in your SQL.

Here's what appears to have happened:

• An extremely old version of the connector used @ for parameters.
• It then changed to use ? for parameters and @ for variables. (At this point the OldSyntax option was added to opt into the old behaviour for existing code; new code should have used ?.) I'm assuming at this point that because ? only meant parameter, the connector threw an exception if you used a named parameter without supplying the corresponding value; this was a client-side programming error.
• In 5.2.0, both ? and @ were permitted for parameters and the OldSyntax option was deprecated. Assuming the the exception for missing parameters already existed, it was now thrown for both ? and @ parameters. However, this caused a problem if you did want to use variables, so a new Allow User Variables option was added to enable them.

This may have made sense for users who upgraded existing code through multiple versions of Connector/Net, but I think it's confusing for new users, particularly if they're coming from a different ADO.NET provider.

(Note that Connector/J has a lot of properties but it doesn't expose any option like this as far as I can tell. It seems to have come from the development history of Connector/Net.)

My recommendation is to default this connection string setting to true.

[caleblloyd]

The default mode for Connector/Net assumes you will not be using user variables (as most users don't) and so it sees @data and expects to find a parameter named @data.

I would argue that this is in fact a convincing rationale. Most use cases that I see do not use user variables.

Consider what happens when the programmer forgets to bind a variable in their code or has a typo in the bound variable name:

// Forgot to bind a variable, @title is not bound
cmd.CommandText = @"INSERT INTO `BlogPost` (`Title`, `Content`) VALUES (@title, @content);";
cmd.Parameters.Add(new MySqlParameter
{
	ParameterName = "@content",
	DbType = DbType.String,
	Value = "Test",
});

// Typo, @titl instead of @title in bound parameter
cmd.CommandText = @"INSERT INTO `BlogPost` (`Title`, `Content`) VALUES (@title, @content);";
cmd.Parameters.Add(new MySqlParameter
{
	ParameterName = "@titl",
	DbType = DbType.String,
	Value = "Test",
});
cmd.Parameters.Add(new MySqlParameter
{
	ParameterName = "@content",
	DbType = DbType.String,
	Value = "Test",
});

With AllowUserVariables=False, the programmer gets a nice exception message with both of these:

MySql.Data.MySqlClient.MySqlException: Parameter '@title' must be defined.

With AllowUserVariables=True, the @title parameter is evaluated as a User Variable, which happens to be null in my session. If the Title field is NOT NULL, you get an exception and if it is NULL, you get silent data loss:

// Title field is NOT NULL
MySql.Data.MySqlClient.MySqlException: Column 'Title' cannot be null

// Title field is NULL
mysql> SELECT * FROM BlogPost;
+----+---------+-------+
| Id | Content | Title |
+----+---------+-------+
|  1 | test    | NULL  |
+----+---------+-------+
1 row in set (0.00 sec)

Comparison with other languages:

Maybe this is a bit of a non-issue in drivers for other languages because bound parameters do not use the leading @. In PHP's PDO driver, named bound parameters use a leading :, such as :title and :content, so it is easy to discern between a missing bound parameter and a user variable.

Recommendation

My recommendation is to leave the default at false. I think that most programmers do not use user variables and this has a lot more potential to create problems for the programmers who forget to bind variables or have typos. We may see an increase in false issues reported as bugs when in reality they are typos.

For those programmers who do need user variables, the issues that @bgrainger has pointed are indexed on Google and the likes, and should hopefully be quick to figure out.

[bgrainger]

I had the (incorrect) assumption that MySQL Server itself would reject @title for not being defined. (I meant to test this but forgot to come back and do so.) Since MySQL Server will silently ignore typos, I agree that Allow User Variables = false has value.

We could even add something like "Set AllowUserVariables=true in your connection string..." to the exception message to help people solve the problem without googling.

[Fruchuxs]

Comparison with other languages:

Maybe this is a bit of a non-issue in drivers for other languages because bound parameters do not use the leading @. In PHP's PDO driver, named bound parameters use a leading :, such as :title and :content, so it is easy to discern between a missing bound parameter and a user variable.

Can't this be changed by configuration? So we can use : to declare bound parameters and can throw away the Allow User Variables=True in the connection string? We use a mechanism in our application updater, which make problems if allow user variables is false. And for the appication administrator who defines the connection string it's really confusing to add this. A concartination of the string to add this in the application is also rather a hack.
IMHO you have fixed here a design fault with another design fault.

[bgrainger]

Can't this be changed by configuration?

In what way? Are you proposing that a new UseColonForParameters=True connection string setting be supported (which would automatically enable Allow User Variables)? That doesn't seem like it solves any of the problems of needing a specific connection string that you mentioned. Are you thinking of a different way of configuration?

A concartination of the string to add this in the application is also rather a hack.

I disagree (well, as long as it's not actually done via string concatenation). Here's an example of another library adjusting the user-provided connection string to enable the settings it needs: #600 (comment)

(One could choose to silently update it, throw an exception, etc.)

[Fruchuxs]

Are you thinking of a different way of configuration?

Possibility to configure programatically the use of : or ? for prepared statement variables. Because the root of the problem is the @-sign and not the user defined variables. But this seems to be a general Problem in the .net DbParameter class?
I don't want to globally set Allow User Variables because of the reasons you figured out here.

As you already metioned, it's an edge case and I dont want break our abstraction for one reason.