brandonryan commented 1 year ago

Problem

The new DateOnly CLR type maps nicely to the SQL date for input parameters now, but there is no way for me to tell the sql driver that I want sql date output parameters to be DateOnly. (Potentially the same with TimeOnly instead of time span).

Example

Exec Stored Procedure

using Microsoft.Data.SqlClient;
using Microsoft.EntityFrameworkCore;

namespace Brandon.Expirements
{
    public class Program
    {
        private const string ExecProcCmd = "EXEC test.DateOnlyOutputParam @in, @out OUTPUT";

        public static void Main()
        {
            var ctx = new ExpContext();

            var inParam = new SqlParameter("@in", new DateOnly(1996, 06, 27));
            var outParam = new SqlParameter()
            {
                ParameterName = "@out",
                Direction = System.Data.ParameterDirection.InputOutput,
                DbType = System.Data.DbType.Date,
            };

            ctx.Database.ExecuteSqlRaw(ExecProcCmd, inParam, outParam);

            Console.WriteLine(outParam.Value.GetType().Name); //Logs: DateTime
        }
    }
}

Stored Procedure

CREATE PROC [test].[DateOnlyOutputParam] (
    @in Date, 
    @out Date OUTPUT
)
AS
BEGIN
    SET @out = @in;
END;

Proposed Solution

It would be nice if there was some kind of configuration flag, or custom converter interface, but I'm not sure where you would put it. An alternative solution could be using the Value parameter as a hint of what type is expected out. This feels a bit like magic behavior though, more meant as a last resort if configuration isn't an option.

var outParam = new SqlParameter()
{
    ParameterName = "@out",
    Direction = System.Data.ParameterDirection.InputOutput,
    DbType = System.Data.DbType.Date,
    Value = DateOnly.MaxValue, //hints to driver that we want a DateOnly Value
};

brandonryan commented 1 year ago

I could also see some kind of generic type parameter solution helping here, but that probably increases the scope quite a bit.

Wraith2 commented 1 year ago

DateOnly and TimeOnly were implemented in https://github.com/dotnet/SqlClient/pull/1813 by @ErikEJ so he may have some feedback.

ErikEJ commented 1 year ago

Yeah, I will have a look, but I doubt there is a simple, non-breaking fix.

roji commented 1 year ago

Yeah, I think this is a fundamental limitation: when defining an output parameter, you specify which database type you're expecting out (e.g. DbType.Date), but not what CLR type (i.e. DateOnly vs. DateTime).

It's true that SqlClient could use the CLR type of the input parameter (Value = DateOnly.MaxValue) above as a user statement of what's desired as the output CLR type. However, it does feel like a hack; it would also possibly applications currently passing DateOnly in but expecting DateTime out (since that's the current behavior).

mabrahamsen commented 1 year ago

You have the same issue with ExecuteScalar as well. Seems the only working paths are SqlDataReader->GetValue / SqlParameter input values.

Client code can build extensions methods / helper methods for converting DateTime to DateOnly and TimeSpan to TimeOnly, such as a GetValue<T> for SqlParameter and ExecuteScaler - but it is not optimal. This is the way I solved it for some test cases.

It is also strange that an Input/Output SqlParameter takes a TimeOnly/DateOnly on the way in, but returns the result as DateTime/TimeSpan for the same parameter for a stored procedure. It is a narrow use-case, but that could perhaps be easily fixable to get a certain parity on the same object instance.

If you start adding GetValue<T> to these objects you also start getting into a discussion about what conversations to support for other data types as well. You could do a Value = nullable DateOnly as well for the SqlParameter case, but still a hack - but I think this might be a bit cleaner than using DateOnly.MaxValue as it implies that you want to push a value through.

With the exception of the input/output SqlParameter case, I struggle to find a good solution.

roji commented 1 year ago

You have the same issue with ExecuteScalar as well.

With ExecuteScalar, a user always has alternative of calling ExecuteReader instead and then using GetFieldValue<T> to get any specific type they want. There has been some thought on adding a generic ExecuteScalar<T> (https://github.com/dotnet/runtime/issues/26511), but it generally hasn't seemed very important so far.

For output parameters, the way forward here is probably the introduction of a generic DbParameter (https://github.com/dotnet/runtime/issues/17446), which is something that needs to be done anyway to allow writing parameters without boxing. At that point, you'd have a SqlParameter<T> whose Value (or similar) is typed as a DateOnly, and the database driver would know to return that.

brandonryan commented 1 year ago

For output parameters, the way forward here is probably the introduction of a generic DbParameter (https://github.com/dotnet/runtime/issues/17446), which is something that needs to be done anyway to allow writing parameters without boxing. At that point, you'd have a SqlParameter<T> whose Value (or similar) is typed as a DateOnly, and the database driver would know to return that.

This is the optimal solution imo. Allows for very nice type safe constructors.