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

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

Closed
roji opened this issue Mar 25, 2021 · 49 comments · Fixed by #1813
Closed

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

roji opened this issue Mar 25, 2021 · 49 comments · Fixed by #1813
Labels
💡 Enhancement New feature request

Comments

@roji
Copy link
Member

roji commented Mar 25, 2021

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.

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 25, 2021

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?

@mattjohnsonpint
Copy link
Contributor

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).

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 25, 2021

@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.

@mattjohnsonpint
Copy link
Contributor

They have the exact same range and precision.

@cheenamalhotra cheenamalhotra changed the title Support the new BCL DateOnly and TimeOfDay structs .NET 6 | Support the new BCL DateOnly and TimeOfDay structs Mar 25, 2021
@cheenamalhotra cheenamalhotra added the 💡 Enhancement New feature request label Mar 25, 2021
@cheenamalhotra
Copy link
Member

Acknowledged.
Looking forward to solidification in design.

@roji
Copy link
Member Author

roji commented Mar 26, 2021

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?).

@mattjohnsonpint
Copy link
Contributor

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

@roji
Copy link
Member Author

roji commented Mar 27, 2021

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.

@mattjohnsonpint
Copy link
Contributor

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.

@cheenamalhotra cheenamalhotra changed the title .NET 6 | Support the new BCL DateOnly and TimeOfDay structs .NET 6 | Support the new BCL DateOnly and TimeOnly structs Apr 15, 2021
@roji
Copy link
Member Author

roji commented May 29, 2021

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.

@CleanCodeX
Copy link

CleanCodeX commented Aug 2, 2021

"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?

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 2, 2021

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.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 2, 2021

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

@CleanCodeX
Copy link

CleanCodeX commented Aug 2, 2021

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.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 2, 2021

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

@John0King
Copy link

@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

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 7, 2021

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.

@John0King
Copy link

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

@StevenRasmussen
Copy link

@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.

@NetMage
Copy link

NetMage commented Nov 11, 2021

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?

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 11, 2021

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.

@AbstractionsAs
Copy link

AbstractionsAs commented Feb 24, 2022

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

@Wraith2
Copy link
Contributor

Wraith2 commented Feb 24, 2022

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

aQsu commented Mar 3, 2022

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.

@JRahnama
Copy link
Member

JRahnama commented Mar 3, 2022

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

@Grauenwolf
Copy link

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

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 7, 2022

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

@Grauenwolf
Copy link

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.

@mabster
Copy link

mabster commented Aug 8, 2022

@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.

@SimonCropp
Copy link
Contributor

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

@Pilchard123
Copy link

Pilchard123 commented Aug 9, 2022

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?

@Wraith2
Copy link
Contributor

Wraith2 commented Aug 9, 2022

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.

@nabenzine
Copy link

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

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 23, 2022

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.

@Grauenwolf
Copy link

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.

@MisinformedDNA
Copy link

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

roji commented Sep 23, 2022

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).

@Grauenwolf
Copy link

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.

@MisinformedDNA
Copy link

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

roji commented Sep 25, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement New feature request
Projects
None yet