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

Create specification tests for EF Core with NodaTime #20114

Open
StevenRasmussen opened this issue Mar 1, 2020 · 13 comments
Open

Create specification tests for EF Core with NodaTime #20114

StevenRasmussen opened this issue Mar 1, 2020 · 13 comments

Comments

@StevenRasmussen
Copy link

StevenRasmussen commented Mar 1, 2020

It seems to me that NodaTime is becoming a defacto library for many C# projects, much like Json.Net due to its superior handling of DateTime concepts. I feel it would be very beneficial if there was native support for NodaTime built into EFCore. I have attempted to make it work with ValueConverters but there are some current limitations: #10434, #11156. Even once those issues are resolved it would still only support the converting of the types but not necessarily allow using any SQL functions related to DateTime types, ie DATEPART, DATEADD etc.

To that end I have created a repo here that is an initial attempt at adding support for the major types:

  • Instant
  • OffsetDateTime
  • LocalDateTime
  • LocalDate
  • LocalTime
  • Duration

The reason for this ticket is 2-fold:

  1. Put this out there for the community to help develop/contribute to.
  2. Create a placeholder for adding this functionality directly into EntityFrameworkCore.
@rynowak
Copy link
Member

rynowak commented Mar 1, 2020

Moving to efcore repo

@rynowak rynowak transferred this issue from dotnet/aspnetcore Mar 1, 2020
@ajcvickers
Copy link
Member

@StevenRasmussen Yep, this has been on our mind for a while now. Note that Npgsql already has support for NodaTime. /cc @roji

@bricelam
Copy link
Contributor

bricelam commented Mar 2, 2020

@ajcvickers ajcvickers added this to the Backlog milestone Mar 2, 2020
@StevenRasmussen
Copy link
Author

StevenRasmussen commented Mar 3, 2020

I wanted to provide an update on the project. As of v1.2.0 I was able to get the most common operations working for all of the types, ie DATEADD and DATEPART is supported for Instant, OffsetDateTime, LocalDateTime, LocalDate, LocalTime, and Duration. Thanks @bricelam for your work on a solution for HierarchyId which I used as a resource for getting this all hooked up... and of course thanks to everyone on the EF Core team for a great codebase that allows for this plugin scenario without needing to modify any of the source!
For more information see the repo here: https://github.com/StevenRasmussen/EFCore.SqlServer.NodaTime

@roji
Copy link
Member

roji commented Mar 3, 2020

@StevenRasmussen really nice to see this! Some quick comments:

  • Note that the method/member translations are SQL Server-specific, so it seems like that should be part of your project/nuget name (EFCore.SqlServer.NodaTime?).
  • Your type mappings are currently lacking code literal generation, which means it's not possible to use them for seeding. This isn't a big problem, you may want to look at my support in Npgsql and copy from there. Note that for some types this isn't implemented (Code literal generation for NodaTime Instant and ZonedDateTime npgsql/efcore.pg#854 tracks that).

Just for background, in Npgsql NodaTime support is built into the ADO layer (via a plugin system separate from EF Core's). This allows the EF Core provider to pass NodaTime types down directly without any value converters.

@StevenRasmussen
Copy link
Author

@roji - Thanks for the feedback! Definitely agree on the name so I have updated it to EFCore.SqlServer.NodaTime as you suggested. Regarding your second point: Is this with regards to the HasData concept? Or DataMigrations? Currently the unit test project uses HasData to successfully seed some data in a localDb. I also just tested creating a migration and that appears to have worked as well... but I'm probably missing something. Thanks!

@roji
Copy link
Member

roji commented Mar 4, 2020

@StevenRasmussen right, sorry - because your type mappings use value converters, you don't need to worry about code literal generation - migrations get code literals for the converted types (e.g. DateTime). In the Npgsql case, since NodaTime instances are passed as-is to the ADO layer, code literal generation must be taken care of.

@jskeet
Copy link

jskeet commented Mar 20, 2021

I've only just become aware of this feature request - do please get in touch (via issues on the repo probably) if there's anything Noda Time could do to become more "EF Core friendly" (without disrupting other users, of course).

@roji roji removed this from the Backlog milestone Mar 20, 2021
@roji
Copy link
Member

roji commented Mar 20, 2021

@jskeet thanks! I think @StevenRasmussen's package is out and working, so I don't think anything in particular is needed from your side.

@dotnet/efteam now that there's a community package for this, we can probably close this issue...

@bricelam
Copy link
Contributor

bricelam commented Mar 22, 2021

FYI, @khellang also recently created an extension that adds support for NodaTime to the SQLite provider.

@ajcvickers ajcvickers changed the title Native support for NodaTime Create specification tests for EF Core with NodaTime Mar 23, 2021
@ajcvickers
Copy link
Member

Repurposing this to add NodaTime tests to the EF Core provider test suite. We would implement the tests and run for SQL Server and SQLite in this repo, and then external providers can run the same tests for their providers. This will help create a consistent experience across providers.

@khellang
Copy link
Member

Nice 😎 Is there anything we can do to help? Are there any similar tests in the code base already?

@roji
Copy link
Member

roji commented Mar 23, 2021

Here are some of the NodaTime query tests in Npgsql. Note that these aren't structured like a good EF test - with AssertQuery - but can be a starting point.

@khellang if you want to work on this, you can look at one of the test classes - GearsOfWarQueryTestBase may be a good example), and use that as a model. Each provider's test suite extends the that TestBase - along with the fixture (GearsOfWarQueryFixtureBase) - providing the necessary infrastructure to run all the tests on that specific database etc. I'd be happy to help out if needed.

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

8 participants