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

DateOnly Output SqlParameter #2092

Open
brandonryan opened this issue Jul 20, 2023 · 10 comments
Open

DateOnly Output SqlParameter #2092

brandonryan opened this issue Jul 20, 2023 · 10 comments
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain.

Comments

@brandonryan
Copy link

brandonryan commented Jul 20, 2023

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
Copy link
Author

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

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 23, 2023

DateOnly and TimeOnly were implemented in #1813 by @ErikEJ so he may have some feedback.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jul 23, 2023

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

@roji
Copy link
Member

roji commented Jul 23, 2023

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
Copy link

mabrahamsen commented Jul 23, 2023

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
Copy link
Member

roji commented Jul 23, 2023

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> (dotnet/runtime#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 (dotnet/runtime#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
Copy link
Author

For output parameters, the way forward here is probably the introduction of a generic DbParameter (dotnet/runtime#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.

@Kaur-Parminder Kaur-Parminder added the 💡 Enhancement Issues that are feature requests for the drivers we maintain. label Jul 25, 2023
@Kaur-Parminder
Copy link
Contributor

@brandonryan Thanks for reporting. I am adding this to our enhancement list for future semester planning.

@takthetank
Copy link

Any news on this? It's been a almost year now...

@kf-gonzalez2
Copy link

5.1.0 Preview 2 has a change that might be related to this to address issue #1009 , please review and let us know if it fixes this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain.
Projects
None yet
Development

No branches or pull requests

8 participants