dotnet/SqlClient

.NET 6 | Support the new BCL DateOnly and TimeOnly structs

roji opened this issue ยท 49 comments

roji commented

New DateOnly and TimeOnly structs are being introduced to .NET 6 as alternatives to DateTime (dotnet/runtime#49036). SqlClient and add support for these as perfect mappings for SQL Server date and time.

Are they likely to have ADO surface area, e.g. DbDataReader.GetDateOnly etc or do you think that providing only GetFieldValue(Async)<T> is appropriate because that wouldn't need any new methods?

Given that the SQL type is date, I suggest making a method DBDataReader.GetDate even if the type it returns in .NET is called DateOnly (which is still being discussed).

@mattjohnsonpint I think that would depend on the ranges supported. If it maps directly then I agree if there are any differences we may want to call out the DataOnly type is not the same as sql DATE. Mapping for clr to sql types is already complicated and a bit messy imo I think we should take care not to make it worse for the save of brevity.

They have the exact same range and precision.

Acknowledged.
Looking forward to solidification in design.

roji commented

Are they likely to have ADO surface area, e.g. DbDataReader.GetDateOnly etc or do you think that providing only GetFieldValue(Async) is appropriate because that wouldn't need any new methods?

I'm not aware of a reason to add new GetDate/GetTime APIs to DbDataReader - the generic GetFieldValue (and Async) should be sufficient for retrieving the new types (unless you guys have a reason?).

No strong opinions here, but a reason would be discoverability for those used to APIs like DbDataReader.GetDateTime.

roji commented

No strong opinions here, but a reason would be discoverability for those used to APIs like DbDataReader.GetDateTime.

That's true, but don't think it's sufficient... Note how there are no async equivalents for the Get* methods (e.g. no GetDateTimeAsync), and users must use the generic GetFieldValueAsync. Basically I think the Get* methods are a bit of a remnant from the days before generics existed.

Please note that the final names for these types are DateOnly and TimeOnly and that they are now merged into the main branch for the next version of .NET 6 (preview 4 likely). Please rename the title of this issue accordingly. Thanks.

roji commented

FYI preview4 is out and these types are now available (I've added support in Npgsql). If you guys can add support at your level, I can do that in EF Core for SQL Server as well.

"If you guys can add support at your level, I can do that in EF Core for SQL Server as well."

What's the current status here? It seems, DateOnly support ist not given yet (in NET 6 preview 6)

Any workarounds for that yet (ConfigureConventions?) except not using DateOnly in models?

Did you mean to ask in EFCore?

Status in SqlClient is that it's waiting for the NET6 release, CI update, new project targets, then work, then testing, then release which is every 6 months.

TBH, do not expect anything to be available until EF Core 7 (November 2022)

Did you mean to ask in EFCore?

Yes in Net Core 6

If it takes that long to support it, then how can it be used then in models except changing the property type? Can a conversion (HaveConversion/HasConversion) be used?

This:

builder.Properties<DateOnly>().HaveConversion(typeof(DateTime));

does not work.

If you have questions about EF Core conversions, you should post them in the EF Core repo.

@ErikEJ

TBH, do not expect anything to be available until EF Core 7 (November 2022)

what make SqlClient's development this kind of slow ?
and why can not just release it now ? from above discussion , I can not see any issue block the development

To start with .net 6 hasn't been released yet. And then there's all the work involved that I mentioned above., CI update, new project targets, implement, testing, documentation, then release. If you want to help with that then go ahead.

.NET6 will be release next month, and anything new on this ?

@John0King - You can see from the thread (several posts above) that this will not hit for at least 6 months or so after .Net 6 RTM.

To start with .net 6 hasn't been released yet. And then there's all the work involved that I mentioned above., CI update, new project targets, implement, testing, documentation, then release. If you want to help with that then go ahead.

Why does an update to SqlClient need to start after the FTM of .Net 6? Why didn't the development start in conjunction with .Net 6 preview and then be ready to release the next available release after .Net 6 RTM (which must be before Nov 22)? Is the release schedule listed anywhere?

Because the MS team is small and adding this feature wasn't a priority. It can't be added by entirely be an external contributor because all the work requires review by the MS team prior to merging. There are also very few external contributors because most people seem to prefer complaining to helping.

Any idea of when DateOnly and TimeOnly will be supported in SQL Server. This can cause many problems in businesses if they need the new DateOnly and TimeOnly using SQL Server.

"Because the MS team is small and adding this feature wasn't a priority."
A small "MS"-Team? Why is the whole (I guess) Microsoft-Team small? Because of the pandemic or was it already small before? I mean, it's a multi billion company which spent more than a billion for developing .net. It surprises that the team is too small to integrate Date/TimeOnly support for MS's own product SQL-Server.

It's an inaccurate perspective that people have of microsoft as a single immense monolithic entity. As you follow the open source projects here on github you realise that there are actually fairly small teams of people working on these projects. Even runtime which is about as big as it gets isn't nearly as many people as you'd expect.

Why is the team small? Because that's what is seen as needed, I expect. As an outsider I speculate that since there is no direct revenue stream from this library (or other drivers) there is no particular incentive to hire more people to deal with it. Having a team of support people for the driver is probably a loss offset again the sql server licensing so why wouldn't the business want as few people as possible?

Now the next question you're likely to ask is why isn't this handled with the same sort of gung-ho spirit as the runtime and aspnet products which are also open source? I think answer to that is it's a different division inside MS with different priorities. How do you make this feature a priority? If you're not paying then you're probably stuck with the usual visual studio feedback channels or social media rabble rousing. If you're a business then try reaching out through whatever channels are available and use your wallet clout to back up your feedback.

Also, .NET 6 was only released few days ago, and I already see indications of efforts to support .NET 6.

@ErikEJ thats good but it would be great if in the future this kind of work is done beforehand.

MS advertised the handy new DateOnly and TimeOnly fields as part of .NET 6 so you don't need to store any unneeded info.
But the problem everyone is running into is that they can't store it anywhere because MS their own storage technologie like SQL server client or EF Core can't handle it.

@ErikEJ thats good but it would be great if in the future this kind of work is done beforehand.

MS advertised the handy new DateOnly and TimeOnly fields as part of .NET 6 so you don't need to store any unneeded info. But the problem everyone is running into is that they can't store it anywhere because MS their own storage technologie like SQL server client or EF Core can't handle it.

That's not really a problem as MS SQL Server does support Date and Time colum types. The only need here is to use a custom converter like this.
There's also a nuget package from someone else for that.

All you need is:

protected override void ConfigureConventions(ModelConfigurationBuilder builder)
{
    builder.Properties<DateOnly>()
        .HaveConversion<DateOnlyConverter>()
        .HaveColumnType("date");
}

/// <summary>
/// Converts <see cref="DateOnly" /> to <see cref="DateTime"/> and vice versa.
/// </summary>
public class DateOnlyConverter : ValueConverter<DateOnly, DateTime>
{
    /// <summary>
    /// Creates a new instance of this converter.
    /// </summary>
    public DateOnlyConverter() : base(
            d => d.ToDateTime(TimeOnly.MinValue),
            d => DateOnly.FromDateTime(d))
    { }
}

It would be nice if this comparer could be integrated into EF Core, so people would only declare something like this:

    protected override void ConfigureConventions(ModelConfigurationBuilder builder)
    {
        builder.Properties<DateOnly>().HaveConversion<DateTime>();
    }
roji commented

@CleanCodeX that's an EF Core solution - SqlClient needs to support DateOnly and TimeOnly in order for non-EF users to be able to work with it.

@CleanCodeX i did see your converter solution in the ef core issue and i do appreciate it but is still sad it's not supported out of the box.

The problem is not that i have a problem implementing a simple converter, the problem is my collegeau ran into this when trying to update to .net 6. As a consequence he now doubts the quality of this framework leading to more pointless discussions around upgrading and witch tech stack to use...

The problem is this only works if you are using the ef core. Does not work if you are using dapper.

I'm currently all kinds of blocked by this. I'm not using EF, so EF solutions are no help..

How are you blocked? These are new types that can be replaced with a little work using the existing DateTime type. No timescale has been given for supporting these types so there is no missed deadline that you were expecting anyone to hit.

aQsu commented

I guess coders have been waiting for sane date and time related types for years . When they are finally available, we want to use them.

@aQsu it is in our to do list. Hopefully if everything works as planned we will try to have this for version 5 release.

Will this be added to System.Data.SqlClient as well or is it only going to be a Microsoft.Data.SqlClient feature?

@Grauenwolf System.Data.SqlClient will not receive any new features, only security updates.

I'm currently all kinds of blocked by this. I'm not using EF, so EF solutions are no help..

I ended up building an adapter layer into my ORM, Tortuga Chain. I'm assuming the other ORM developers are going to have to do the same.

@aQsu it is in our to do list. Hopefully if everything works as planned we will try to have this for version 5 release.

Hi @aQsu! I see that v5 was released a few days ago. Did this make the cut? I didn't see a mention of TimeOnly or DateOnly in the release notes.

@mabster no use of DateOnly in the code https://github.com/dotnet/SqlClient/search?q=DateOnly&type=code
so i would assume no

Of course, that now raises the question "when is it likely to be done?". Is there an agreed API that is just waiting on someone to make a PR, or is it still in the planning stage? Is it something you'd accept a PR for if there was one?

The discussion above indicates that the expected additional surface area is to GetFieldValue and GetFieldValueAsync. Other places like parameters and the internal SqlBuffer types will need updating to handle the extra data type but those aren't externally visible.

There is currently no build of this library that targets anything beyond netcore 3.1. As soon as there is one then the new functionality can be added. Until then it will wait. There is no indication of where this is in terms of priority but there are other tasks like the merging of netfx and netcore codebases which imo should take priority over adding new features.

Any update on this ? is this included in Ef core 7 previews ?

No. There is no update. There is still no net6 build target for this library and that is still required to begin work on the feature. If there were an update I'm sure someone would post about it here.

You don't really need support for it at this level. You could handle it all in the ORM. (That's what my ORM did.)

I'm not even sure what support would look like. Suddenly changing columns that were materialized as DateTime to be DateOnly or TimeOnly would be a breaking change.

You may not need it, but it's handier then everyone adding their own value converters. I don't see why there would need to be a breaking change. If a user changes their code to use one of the new objects, then they are changing their own functionality.

roji commented

I'm not even sure what support would look like. Suddenly changing columns that were materialized as DateTime to be DateOnly or TimeOnly would be a breaking change.

I think we discussed this above; support would mean allowing asking GetFieldValue<T> for a DateOnly/TimeOnly, and allowing SqlParameter.Value to be DateOnly/TimeOnly. This is completely non-breaking, and would also allow seamless support in ORMs layered over SqlClient.

These types are now a standard part of the .NET API, database drivers really should just support them at this point. The lack of native support at the ADO.NET layer also causes various issues and difficulties at upper (ORM) layers).

When you call GetValues, can DateOnly appear in the array for Date columns?

If yes, it's a breaking change and people will be mad.

If no, it doesn't faithfully represent what's in the database and people will be mad.

Does it make sense to have a switch that controls this? Maybe, but it would be non-obvious and that's close to not existing.

Hence my concern about not knowing what this is going to look like.

When you call GetValues, can DateOnly appear in the array for Date columns?

Based on previous comments, I think they are only going to support GetFieldValue(Async)<T>:

No strong opinions here, but a reason would be discoverability for those used to APIs like DbDataReader.GetDateTime.

That's true, but don't think it's sufficient... Note how there are no async equivalents for the Get* methods (e.g. no GetDateTimeAsync), and users must use the generic GetFieldValueAsync. Basically I think the Get* methods are a bit of a remnant from the days before generics existed.

roji commented

When you call GetValues, can DateOnly appear in the array for Date columns?
If yes, it's a breaking change and people will be mad.
If no, it doesn't faithfully represent what's in the database and people will be mad.

@Grauenwolf, the Npgsql, Microsoft.Data.Sqlite, and MySqlConnector drivers have all implemented this change. At least for the first two, this allows you to send DateOnly values in DbParameter, and use GetFieldValue to read them. This isn't a breaking change, and absolutely nobody got mad that GetValues hasn't changed. Modern usage of ADO.NET generally uses GetFieldValue anyway - it's generic, and so doesn't require additional casting and also doesn't box value type. In my experience supporting Npgsql users, not many still use the non-generic read APIs (GetValue/GetValues) - these really make sense mainly in fully dynamic scenarios, where your program sends arbitrary SQL queries and doesn't know what types will be coming back.

Sure, it's possible to add an opt-in to also change the "default" CLR type for dates from DateTime to DateOnly; that would indeed bring this change to the non-generic APIs as well. I've personally not seen one request for it so far, so I'd really start off with the non-breaking change (parameters and generic GetFieldValue), and see from there.