Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameter direction not supported in CommandType.Text #231

Closed
mguinness opened this issue Apr 8, 2017 · 35 comments
Closed

Parameter direction not supported in CommandType.Text #231

mguinness opened this issue Apr 8, 2017 · 35 comments

Comments

@mguinness
Copy link
Contributor

There's an issue when using parameter direction with Pomelo EF Core provider. Both FromSql() and ExecuteSqlCommand() can accept arrays of MySqlParameter (derived from DbParameter). If the Direction property is set to either InputOutput or Output it will not work as expected.

This issue was already touched on in issue PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#194 (comment).

This is due to the query being executed as CommandType.Text which uses TextCommandExecutor as opposed to the more advanced parameter support in StoredProcedureCommandExecutor.

While I understand the limitations of CommandType.Text, it does mean there is a disparity between MySqlConnector and System.Data.SqlClient which does handle output parameters. It does this by using sp_executesql to run the query. For example:

set @p1=NULL
exec sp_executesql N'EXEC MyTest @ParamOut OUTPUT',N'@ParamOut int output',@ParamOut=@p1 output
select @p1

Is there a way to do something similar using prepared statements in MySQL? For example:

SET @p1 := NULL;
SET @qry = 'CALL MyTest(@p1)'
PREPARE stmt FROM @qry
EXECUTE stmt
DEALLOCATE PREPARE stmt
SELECT @p1

If this isn't feasible to do or a design decision is made to not support it then I think an exception should be thrown when Direction property is set to something other than ParameterDirection.Input as it won't work as expected.

Anyway this is something which would benefit from discussion and I'd like to get other people's viewpoints on this issue.

@bgrainger
Copy link
Member

Can you give some ADO.NET code that illustrates the problem (particularly useful if you can contrast SqlClient vs MySqlConnector or Connector/NET).

@mguinness
Copy link
Contributor Author

Sure, no prob. The code snippets below using EF Core should help.

var outParam = new SqlParameter("@ParamOut", DbType.Int32)
    { Direction = ParameterDirection.Output };
ctx.Database.ExecuteSqlCommand("EXEC MyTest @ParamOut OUTPUT", outParam);

Inspecting outParam.Value will show the integer value returned from the stored proc.

var outParam = new MySqlParameter() { ParameterName = "@ParamOut",
    DbType = DbType.Int32, Direction = ParameterDirection.Output };
ctx.Database.ExecuteSqlCommand("CALL mytest(@ParamOut)", outParam);

This gives the following MySql.Data.MySqlClient.MySqlException:

'OUT or INOUT argument 1 for routine testdb.mytest is not a variable or NEW pseudo-variable in BEFORE trigger'

This is due to the following query being sent:
CALL mytest(NULL)

When it should really be the following SQL:
CALL mytest(@ParamOut)

@bgrainger
Copy link
Member

Sorry, I'm still not sure from that code snippet whether the problem is with this library, or with Pomelo EF Core's use of this library, or something else. Is there a way to reproduce this bug just by using ADO.NET methods?

@mguinness
Copy link
Contributor Author

mguinness commented Apr 8, 2017

OK, I'll take EF Core out of this altogether and just focus on ADO.NET. I'm not sure I'd classify this a bug, but more of a limitation.

Anyway, here is code for SQL Server and outParam.Value will yield 1.

using (var db = new SqlConnection("Server=127.0.0.1"))
{
    db.Open();

    var outParam = new SqlParameter("@ParamOut", DbType.Int32)
        { Direction = ParameterDirection.Output };

    var cmd = db.CreateCommand();
    cmd.CommandText = "SET @ParamOut = 1";
    cmd.Parameters.Add(outParam);
    cmd.ExecuteNonQuery();
}

Here is code for MySQL, a MySqlException will be raised.

'You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'NULL = 1' at line 1'

using (var db = new MySqlConnection("server=127.0.0.1"))
{
    db.Open();

    var outParam = new MySqlParameter() { ParameterName = "@ParamOut",
        DbType = DbType.Int32, Direction = ParameterDirection.Output };

    var cmd = db.CreateCommand();
    cmd.CommandText = "SET @ParamOut = 1";
    cmd.Parameters.Add(outParam);
    cmd.ExecuteNonQuery();
}

The reason it fails is that SET NULL = 1 is being sent to the server instead of SET @ParamOut = 1. It appears that variable names are being substituted with their values in TextCommandExecutor.

If that's the case, an option would be to include code like below to alert the user that this won't work:

if (parameterCollection.Any(p => p.Direction != ParameterDirection.Input))
    throw new Exception("Parameters can only be input when using CommandType.Text");

@bgrainger
Copy link
Member

Thank you; this is a very helpful example.

I'd agree that this is more of a limitation (with MySQL itself). The "text" protocol doesn't provide any way of returning parameter values to the client; you have to explicitly SELECT them (e.g., see documentation on stored procedures).

FWIW, I get the same "invalid SQL" MySqlException with Oracle's Connector/NET (so this is not a compatibility issue). A more descriptive exception message (that output parameters are not supported) is a good idea.

@mguinness
Copy link
Contributor Author

@bgrainger Thanks for confirming. Although, it appears that MySQL 5.5.3 has the ability to return the OUT parameter as an extra resultset. Is that something this library could take advantage of?

@bgrainger
Copy link
Member

This is the same as bug 75267 for Connector/NET.

@mguinness
Copy link
Contributor Author

@bgrainger Nice find, although it appears Oracle is in no hurry to fix it!

@pantonis
Copy link

pantonis commented Dec 2, 2019

Any update on this?

@mguinness
Copy link
Contributor Author

mguinness commented Dec 2, 2019

Are you referring to ADO.NET or EF Core? If the former, then you can use Using Stored Routines from Connector/NET to get the output values, otherwise no update.

@bgrainger Would you be able to grab the OUT params of SP from the OK packet when SERVER_PS_OUT_PARAMS is set? See WL#7766: Deprecate the EOF packet for more info.

UPDATE: Support for SERVER_PS_OUT_PARAMS is being tracked at #840.

@pantonis
Copy link

pantonis commented Dec 2, 2019

I am referring to EF Core. I have done already the ADO.NET solution. it works but it sucks. I have a huge proejct with dozens of stored procedures and I have to do the datareader stuff for every single field. Shame tha in year 2019 we are still forced to use ADO.NET

@bgrainger
Copy link
Member

Would you be able to grab the OUT params of SP from the OK packet when SERVER_PS_OUT_PARAMS is set?

MySqlCommand.Prepare isn't currently supported for CommandType.StoredProcedure; it should be able to be supported when that's implemented: #742.

@bgrainger
Copy link
Member

It seems like we might be talking about two separate things here, though?

A) executing (with CommandType.Text) SET @outParam = ... then accessing its value via the MySqlCommand.Parameters collection.
B) executing a stored procedure (prepared or not) and accessing the values of OUT parameters assigned by the stored procedure.

This issue covers (A). (B) is already implemented (when Prepare isn't called); #742 tracks supporting Prepare for stored procedures (with or without OUT parameters).

@mguinness
Copy link
Contributor Author

I have a huge project with dozens of stored procedures and I have to do the datareader stuff for every single field.

@pantonis Try creating an extension method on the database facade to facilitate calling these procedures for you. The code below isn't tested, but should get you started.

public static int CallStoredRoutine(this DatabaseFacade databaseFacade,
    string routineName, params DbParameter[] parameters)
{
    int rows;

    var cmd = databaseFacade.GetDbConnection().CreateCommand();
    cmd.CommandText = routineName;
    cmd.Parameters.AddRange(parameters);
    cmd.CommandType = CommandType.StoredProcedure;

    databaseFacade.OpenConnection();
    rows = cmd.ExecuteNonQuery();
    databaseFacade.CloseConnection();

    return rows;
}

@bgrainger Apologies for conflating the issues.

@pantonis
Copy link

pantonis commented Dec 3, 2019

@mguinness Big mess here are the datareader reads and cast to model properties one by one.

@mguinness
Copy link
Contributor Author

Using extension method you'll just read the Value property of the MySqlParameter that you passed in.

@bgrainger
Copy link
Member

I have a huge proejct with dozens of stored procedures and I have to do the datareader stuff for every single field.

@pantonis Can you give some example code? I'm not sure exactly what the problem is yet, and seeing the code might help me understand where MySqlConnector could be improved, or allow us to give some suggestions for improvement of your code.

@pantonis
Copy link

pantonis commented Dec 3, 2019

@mguiness I am not talking about the reading of output parameter but for the values returned from the SP.
This is the mess I described above, where I have dozens of SPs. Names are fictional:

using (var reader = await command.ExecuteReaderAsync())
{
	while (await reader.ReadAsync())
	{
		MyData myData = new MyData();

		myData.Prop1 = reader.GetString(reader.GetOrdinal("Prop1"));
		myData.Prop2 = reader.GetString(reader.GetOrdinal("Prop2"));
		myData.Prop3 = reader.GetString(reader.GetOrdinal("Prop3"));
		myData.Prop4 = reader.GetString(reader.GetOrdinal("Prop4"));
		myData.Prop5 = reader.GetString(reader.GetOrdinal("Prop5"));
		myData.Prop6 = reader.GetDouble(reader.GetOrdinal("Prop6"));
		myData.Prop7 = reader.GetInt64(reader.GetOrdinal("Prop7"));
		myData.Prop8 = reader.GetString(reader.GetOrdinal("Prop8"));
		myData.Prop9 = reader.GetDouble(reader.GetOrdinal("Prop9"));
		myData.Prop10 = reader.GetString(reader.GetOrdinal("Prop10"));
		myData.Prop11 = reader.GetString(reader.GetOrdinal("Prop11"));
		myData.Prop12 = reader.GetString(reader.GetOrdinal("Prop12"));
		myData.Prop13 = reader.GetString(reader.GetOrdinal("Prop13"));
		myData.Prop14 = reader.GetInt64(reader.GetOrdinal("Prop14"));
		myData.Prop15 = reader.GetString(reader.GetOrdinal("Prop15"));
		myData.Prop16 = reader.GetString(reader.GetOrdinal("Prop16"));
		myData.Prop17 = reader.GetString(reader.GetOrdinal("Prop17"));
		myData.Prop18 = reader.GetInt64(reader.GetOrdinal("Prop18"));
		myData.Prop19 = reader.GetString(reader.GetOrdinal("Prop19"));
		
		result.Add(myData);
	}
}

@bgrainger
Copy link
Member

bgrainger commented Dec 3, 2019

All that code could be replaced with one line using Dapper:

result = (await connection.QueryAsync<MyData>("spName", commandType: CommandType.StoredProcedure)).ToList();

I'd strongly suggest using a simple ORM library like Dapper to eliminate this repetitive boilerplate, while retaining very similar performance to the handwritten code above.

@mguinness
Copy link
Contributor Author

You need to create a model and use FromSql() to populate it from the stored proc, see Entity Framework Core, Calling Stored Procedures and Returning to a Model. No need to drop down to ADO.NET for this.

@pantonis
Copy link

pantonis commented Dec 3, 2019

@bgrainger I saw Dapper but I don't know if it is a good idea to mix 2 ORMS. I use EF Core for my business logic and I have been using it for years and I don't want to drop it for something I don't have experience with it.

@mguinness I was doing this until I discovered that EF Core using with MySql does not support output params. This is why I switched to ADO.NET

@mguinness
Copy link
Contributor Author

This probably isn't the right venue to get the answers you need. Ask a question on Stack Overflow with a Minimal, Reproducible Example and you'll likely get help for a solution that'll work for you.

@pantonis
Copy link

pantonis commented Dec 3, 2019

@mguinness It is not my intention to ask anything about dapper or ado.net. I initially wrote here to ask for the status of this issue that for the output parameters when using EF Core with MySql

@bgrainger
Copy link
Member

This repo (MySqlConnector) deals purely with ADO.NET; if you want an answer about EF Core, then https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql (or Stack Overflow) is the right place to raise the issue.

(It's still not clear to me what this issue--ADO.NET output parameters for CommandType.Text commands--has to do with your scenario.)

@pantonis
Copy link

pantonis commented Dec 5, 2019

@bgrainger I thought that the problem exists in MySqlConnect (Parameter direction not supported in CommandType.Text). I saw this thread as open and you added it an an enhancment.

@bgrainger
Copy link
Member

I thought that the problem exists in MySqlConnect (Parameter direction not supported in CommandType.Text)

Yes, that issue does exist. But AFAICT you're not using CommandType.Text (you're using CommandType.StoredProcedure) so, by definition, your problem has to be different than what this issue is tracking.

@pantonis
Copy link

pantonis commented Dec 5, 2019

So EF Core team or Pomelo team have to change their code to use CommandType.Text

@bgrainger
Copy link
Member

No... it's just that CommandType.Text and CommandType.StoredProcedure are fundamentally different types (even though they're both exposed via MySqlCommand), and behaviour/bugs associated with one aren't necessarily applicable to the other.

@mguinness
Copy link
Contributor Author

Just to reiterate that your issue has nothing to do with MySqlConnector but rather EF Core. I quickly looked at the code in RelationalCommand.cs and they don't specify CommandType so it'll default to CommandType.Text which means output parameters won't work.

Therefore you have three situations when dealing with stored procedures in EF Core:

  1. Stored proc w/ resultset but no output parameters. Use FromSql().
  2. Stored proc w/ output parameters but no resultset. Use ADO.NET w/ extension method.
  3. Stored proc w/ output parameters and resultset. Use ADO.NET and keep your existing code. However I'd look at using Getting schema information from the DataReader instead.

The one question I have regarding the MySQL text protocol is this - when using CommandType.Text and Prepare() with a MySqlParameterCollection does the OK_Packet contain SERVER_PS_OUT_PARAMS when CLIENT_PS_MULTI_RESULTS flag is used? Or is it simply that Prepared Statements aren't compatible with the MySQL text protocol?

@bgrainger
Copy link
Member

Prepared statements imply the binary protocol. (Unprepared, i.e., regular, statements use the text protocol.) Furthermore, I believe that SERVER_PS_OUT_PARAMS is only applicable to prepared statements that invoke stored procedures, not prepared arbitrary SQL. This will be covered by #742.

@mguinness
Copy link
Contributor Author

Looking at the example in C API Prepared CALL Statement Support it calls a stored proc with question mark placeholders.

status = mysql_stmt_prepare(stmt, "CALL p1(?, ?, ?)", 16);

In the documentation for mysql_stmt_prepare() I see no reference to it being limited to stored procedures, but the string must consist of a single SQL statement.

In prepared CALL statements used with PREPARE and EXECUTE, placeholder support for OUT and INOUT parameters is available beginning with MySQL 8.0.

Does that mean that MySqlCommand.Prepare() could support CommandType.Text?

@bgrainger
Copy link
Member

In the documentation for mysql_stmt_prepare() I see no reference to it being limited to stored procedures

It's not.

Does that mean that MySqlCommand.Prepare() could support CommandType.Text?

Yes, and it already does.

(But only stored procedures have "OUT parameters". SET @ParamOut = 1; (which could be executed as CommandType.Text, either prepared or not) is not an "out parameter"; it's just setting a user-defined variable in an ad-hoc SQL statement.)

@bgrainger
Copy link
Member

Back to the OP, there should be a way to make "OUT" parameters work with CommandType.Text. Consider the sample code:

    var outParam = new MySqlParameter() { ParameterName = "@ParamOut",
        DbType = DbType.Int32, Direction = ParameterDirection.Output };

    var cmd = db.CreateCommand();
    cmd.CommandText = "SET @ParamOut = 1";
    cmd.Parameters.Add(outParam);
    cmd.ExecuteNonQuery();

This could be handled as follows:

For each Output (or InputOutput) parameter, output a prologue similar to SET @paramName = NULL;. (For InputOutput, this probably needs to be initialised to the parameter value.)

Then output the SQL without substituting parameter names with their values.

Then append SELECT @paramName, ... for all (Input)Output parameters, read the result set and store the returned values in the corresponding MySqlParameter objects.

@bgrainger
Copy link
Member

I don't think the extra code in the previous comment is worth implementing in this library; setting "out" parameters just isn't natively supported by MySQL and we shouldn't pretend that it is.

If this isn't feasible to do or a design decision is made to not support it then I think an exception should be thrown when Direction property is set to something other than ParameterDirection.Input as it won't work as expected.

An exception with a link to documentation would help users in this situation.

@bgrainger
Copy link
Member

That was already implemented in #234.

throw new MySqlException("Only ParameterDirection.Input is supported when CommandType is Text (parameter name: {0})".FormatInvariant(parameter.ParameterName));

Since it hasn't come up much, a link to documentation may be unnecessary.

Closing this as an unsupported feature of the MySQL protocol; it doesn't have the capability to return named parameters from CommandType.Text statements without explicitly SELECTing them and reading an extra result set, which can be performed in user code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants