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

FromSqlRaw() and ParameterDirection.Output #21433

Open
Tracked by #21888
mguinness opened this issue Jun 27, 2020 · 12 comments
Open
Tracked by #21888

FromSqlRaw() and ParameterDirection.Output #21433

mguinness opened this issue Jun 27, 2020 · 12 comments

Comments

@mguinness
Copy link

This is tangentially related to Get output parameter value of a stored procedure using EF Core FromSql(...) is always null.

With SQL Server you can use code shown in How to use a SQL output parameter with FromSqlInterpolated? to get output parameters from a stored procedure.

However as discussed in Parameter direction not supported in CommandType.Text this is not possible in MySQL when CommandType in ADO.NET is set to CommandType.Text.

However setting CommandType to CommandType.StoredProcedure is not an option when using FromSqlRaw() in EF Core as RelationalCommand class doesn't set CommandType and will default to CommandType.Text.

Could there be a mechanism when executing a stored procedure via raw SQL query to set CommandType to CommandType.StoredProcedure? Maybe a new method FromSqlProcRaw()?

@roji
Copy link
Member

roji commented Jun 28, 2020

@mguinness the value of FromSql really is to materialize entities coming out of the raw SQL query (and possibly even to compose LINQ on top of it). If I'm understanding your scenario correctly, you just want to get out non-entity data via an output parameter - is there any value here in using EF rather than dropping down to ADO.NET?

@mguinness
Copy link
Author

mguinness commented Jun 28, 2020

It's true that FromSql() is intended to primarily materialize entities, but it's also not unusual for stored procs to return both a resultset and output parameters (like a total or count).

The output parameter got a value 'null' of Stored Procedure demonstrates that use case with SQL Server. This is not possible when using MySQL without changing ADO.NET CommandType.

The example I provided in the MySqlConnector repo didn't make that use case clear since I tried to simplify the issue.

FWIW, it's also not possible to use output parameters with ExecuteSqlCommand(), but as you say dropping down to ADO.NET is a workaround.

@ajcvickers
Copy link
Member

@lauxjpn @bgrainger Could we get your input here? It's not clear to us whether the choice to use Text or StoredProcedure must come from the application developer, or whether there is some way to handle this more automatically possibly at the ADO.NET layer. (Related to this is whether or not this is only an issue with MySQL, or whether other providers might have the same problem.)

@mguinness
Copy link
Author

Although executing a stored proc does work with CommandType.Text it probably would be best practice to use CommandType.StoredProcedure for all providers regardless. It would also avoid using database keywords like EXEC or CALL which does cause some confusion.

When executing a stored procedure, what is the benefit of using CommandType.StoredProcedure versus using CommandType.Text?

Maybe both FromSql() and ExecuteSqlCommand() could be overloaded to include an optional parameter indicating type?

@bgrainger
Copy link
Contributor

So, I know about the MySQL protocol and how MySqlConnector implements it, but not a lot about EFCore or other providers. I'll try to provide some technical background info about MySQL and hopefully someone else can synthesise this into a coherent whole?

When a client uses ADO.NET to execute a CommandType.StoredProcedure command, MySqlConnector takes the CommandText, treats it as a sproc name, and queries the MySQL Server for sproc metadata (parameters, etc.); it then aligns the MySqlCommand.Parameters collection against the sproc and builds (essentially) the following SQL statement behind the scenes:

SET @p1 = <INLINED PARAM VALUE1>; -- set input parameter
SET @p2 = <INLINED PARAM VALUE2>; -- set input/output parameter
CALL <CommandText>(@p1, @p2, @p3); -- call stored procedure; it may return an arbitrary number of result sets
SELECT @p2, @p3; -- select input/output and output parameter

In this example, @p1 is an Input parameter, @p2 is InputOutput and @p3 is Output. The MySQL "text protocol" doesn't have any concept of parameters, so the user's actual parameter values are escaped and inlined into the SQL; @p1, @p2, @p3 are treated as user-defined variables by the server. The final SELECT is handled inside MySqlConnector, which assigns the values of the single row in that result set to the MySqlParameter objects of the MySqlCommand; this can only happen after all other result sets are read.

This is all slightly more complicated when the user Prepares the MySqlCommand, because that causes MySQL's "binary protocol" to be used, which can optionally have support for automatically returning the output parameters without the extra SELECT statement. (MySqlConnector doesn't implement this yet.)

Based on mysql-net/MySqlConnector#231 (comment), I'm assuming the desired behaviour in EFCore (for this issue) is for this to work:

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

(Or FromSql? Or FromSqlRaw? I don't yet know what the difference is.)

It seems like there could be a lot of ways this could be handled. However, given that MySqlConnector is a low-level ADO.NET library with lots of different consumers, it doesn't seem appropriate for it to do any "query rewriting" when CommandType is CommandType.Text and CommandText starts with CALL .

Instead, if ExecuteSqlCommand (or FromSql, etc.) is supposed to set output parameters when a CALL statement is executed, I suggest that:

  1. Pomelo detect this situation and rewrite the query to SELECT the output parameters; this would basically be duplicating (and maintaining) all the similar code MySqlConnector already has to do this, so this seems highly undesirable. Or:
  2. If FromSqlRaw("sproc_name", CommandType.StoredProcedure) (or something similar) can be supported in EFCore, then EF users would get access to the underlying MySqlConnector behaviour that makes this all appear to "just work". It seems that @mguinness is proposing this in the OP.

Hope this helps.

@smitpatel
Copy link
Member

In case, we decide to add a method in EF Core to support this, few things we should investigate.

  • Are sproc composible on any provider? e.g. in above example, if sproc is composible and becomes a subquery what would become the command type since it is not sproc anymore but text does not work either.
  • If it cannot be composed over then consider returning IEnumerable rather than IQueryable
  • Given different return type, we can also name it differently rather than an overload of FromSql.

@mguinness
Copy link
Author

mguinness commented Jun 29, 2020

Are sproc composible on any provider? e.g. in above example, if sproc is composible and becomes a subquery what would become the command type since it is not sproc anymore but text does not work either.

The documentation for Raw SQL Queries has the following paragraph:

SQL Server doesn't allow composing over stored procedure calls, so any attempt to apply additional query operators to such a call will result in invalid SQL. Use AsEnumerable or AsAsyncEnumerable method right after FromSqlRaw or FromSqlInterpolated methods to make sure that EF Core doesn't try to compose over a stored procedure.

So maybe having a separate method would avoid this issue and make the intent clearer and is there any advantage to a deferred return type as users commonly try to access output parameters before enumerating.

This is all slightly more complicated when the user Prepares the MySqlCommand, because that causes MySQL's "binary protocol" to be used, which can optionally have support for automatically returning the output parameters without the extra SELECT statement. (MySqlConnector doesn't implement this yet.)

See Support SERVER_PS_OUT_PARAMS for more info.

@lauxjpn
Copy link
Contributor

lauxjpn commented Jul 1, 2020

@mguinness For a quick workaround, you can just use an interceptor:

Sample code
using System.Data;
using System.Data.Common;
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.Extensions.Logging;
using MySql.Data.MySqlClient;

namespace IssueConsoleTemplate
{
    public class IceCream
    {
        public int IceCreamId { get; set; }
        public string Name { get; set; }
        public int Stock { get; set; }
    }

    public class StoredProcedureOutputParameterSupportInterceptor : DbCommandInterceptor
    {
        public override InterceptionResult<DbDataReader> ReaderExecuting(
            DbCommand command,
            CommandEventData eventData,
            InterceptionResult<DbDataReader> result)
        {
            if (command.CommandType != CommandType.StoredProcedure &&
                command.Parameters.Cast<DbParameter>()
                    .Any(p => p.Direction == ParameterDirection.Output ||
                              p.Direction == ParameterDirection.InputOutput))
            {
                command.CommandType = CommandType.StoredProcedure;

                // TODO: Strip "CALL" prefix and parameters in parenthesis, in case those where supplied as
                // part of the CommandText.
            }
            
            return result;
        }
    }

    public class Context : DbContext
    {
        public DbSet<IceCream> IceCreams { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder
                .UseMySql(
                    "server=127.0.0.1;port=3306;user=root;password=;database=IssueEF21433_01",
                    b => b.ServerVersion("8.0.20-mysql"))
                .AddInterceptors(new StoredProcedureOutputParameterSupportInterceptor())
                .UseLoggerFactory(
                    LoggerFactory.Create(
                        b => b
                            .AddConsole()
                            .AddFilter(level => level >= LogLevel.Information)))
                .EnableSensitiveDataLogging()
                .EnableDetailedErrors();
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<IceCream>()
                .HasData(
                    new IceCream
                    {
                        IceCreamId = 1,
                        Name = "Vanilla",
                        Stock = 500,
                    },
                    new IceCream
                    {
                        IceCreamId = 2,
                        Name = "Chocolate",
                        Stock = 800,
                    },
                    new IceCream
                    {
                        IceCreamId = 3,
                        Name = "Matcha",
                        Stock = 20,
                    });
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            using var context = new Context();

            context.Database.EnsureDeleted();
            context.Database.EnsureCreated();

            context.Database.ExecuteSqlRaw(@"
CREATE PROCEDURE sp_GetIceCreamsAtLeastInStock (IN atLeastInStock int, OUT count int)
BEGIN
    SELECT COUNT(*) INTO count FROM IceCreams
    WHERE Stock >= atLeastInStock;

    SELECT * FROM IceCreams
    WHERE Stock >= atLeastInStock;
END");

            var atLeastInStockParameter = new MySqlParameter("@atLeastInStock", MySqlDbType.Int32)
            {
                Direction = ParameterDirection.Input,
                Value = 100
            };

            var countParameter = new MySqlParameter("@count", MySqlDbType.Int32)
            {
                Direction = ParameterDirection.Output
            };

            var iceCreams = context.IceCreams
                .FromSqlRaw(
                    "sp_GetIceCreamsAtLeastInStock", // can only contain the stored procedure name
                    atLeastInStockParameter,
                    countParameter)
                .ToList();

            Debug.Assert(iceCreams.Count == 2);
            Debug.Assert(iceCreams.Count == (int)countParameter.Value);
        }
    }
}

@ajcvickers
Copy link
Member

@bgrainger Are you saying that out parameters are already supported in Text mode, but are only populated after the result set has been consumed? If so, then this is the same behavior as SqlClient.

@bgrainger
Copy link
Contributor

Are you saying that out parameters are already supported in Text mode, but are only populated after the result set has been consumed?

Yes, but only if the CommandType is StoredProcedure. (You can't set cmd.CommandText = "SET @outParam = 1;" and retrieve its value by adding a parameter with that name to the collection then calling ExecuteNonQuery().)

@lauxjpn
Copy link
Contributor

lauxjpn commented Jul 2, 2020

Another solution could be to introduce an additional extension method, that could be chained like so:

var iceCreams = context.IceCreams
    .FromSqlRaw(
        "sp_GetIceCreamsAtLeastInStock",
        atLeastInStockParameter,
        countParameter)
    .UsingCommandType(CommandType.StoredProcedure) // or With..., or just .AsStoredProcedure() ?
    .ToList();

This would more or less move the responsibility to the user, to choose the right CommandType if necessary. Of course this is less discoverable, but it might still circumvent headaches about composability issues in providers that support composable stored procedures (if there are any). Not sure, if this would be any better than just adding an overload with an additional parameter for FromSqlRaw and co.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 3, 2020

That would be a great addition to the API, as several have complained about the inability to do this (CommandType.Storedproc) with EF/ EF Core. https://stackoverflow.com/questions/9267078/when-executing-a-stored-procedure-what-is-the-benefit-of-using-commandtype-stor

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

No branches or pull requests

7 participants