Add design time support

[StevenRasmussen]

Fixes #16

[ErikEJ]

I think you need to update the .csproj as well: https://github.com/npgsql/efcore.pg/blob/main/src/EFCore.PG.NodaTime/EFCore.PG.NodaTime.csproj#L23

[StevenRasmussen]

@ErikEJ - Just updated the project as well. Thanks for taking a look at this!

[ErikEJ]

You are welcome, having design time support for Nodatime with SQL Server will be so sweet!

[ErikEJ]

Could not make it work... Noticed that other packages use Nodatime 3.0.1 !?

[ErikEJ]

Wonder if you can try this iso the strange targets file?

https://github.com/ErikEJ/EFCorePowerTools/blob/master/src/GUI/ErikEJ.EntityFrameworkCore.SqlServer.Dacpac/Design/SqlServerDacpacDesignTimeServices.cs#L12

[StevenRasmussen]

Hmmm.... ok. Would you be able to put down the steps that you are going through to try and test this? That way I can try to troubleshoot on my end.

[ErikEJ]

As I wrote, you can simply test with the command line tooling and a file based nuget feed.

[StevenRasmussen]

Hmmm... which command specifically are you invoking with the EF Core CLI? I was under the impression that the migration commands used design-time features (see here). However, I just setup a new project referencing a local NuGet feed with the package, added some NodaTime properties to one of the models, ran both the commands: dotnet ef migrations add InitialCreate dotnet ef database update The commands executed successfully and the DB was created with the new date columns that map to the NodaTime properties. I'm probably not invoking the correct command that you are seeing an error with though.

[ErikEJ]

It's the scaffold command.

But I was thinking maybe the db column must be datetime2 to map to Nodatime, and I have only tested with datetime columns.

[ErikEJ]

OK, I tried with datetime2, and it finally worked - but is the type name Noda? I thought it was: Instant ?

namespace ScaffoldingTester.Models
{
    public partial class BoolNullableTest
    {
        public int Id { get; set; }
        public bool TestBool { get; set; }
        public DateTime? Noda { get; set; }
    }
}

[StevenRasmussen]

I'll try the scaffold command to see what happens and let you know. To answer your question, the DB type depends on the NodaTime type. For example, the Instant type maps to a datetime2 in SQL. The LocalDate type maps to a date and a LocalTime maps to time. Your class should use the NodaTime types... so in your example it would be:

namespace ScaffoldingTester.Models
{
    public partial class BoolNullableTest
    {
        public int Id { get; set; }
        public bool TestBool { get; set; }
        public Instant? CreatedOn { get; set; }
        public LocalDate SomeDate { get;set; }
    }
}

[ErikEJ]

Sorry, it got confused, it is still not working.

[ErikEJ]

Maybe someone from the EF Core team can see what is wrong?

@bricelam @ajcvickers

[StevenRasmussen]

Interesting... I've never used the DB first approach and scaffolding. I briefly scanned the documentation but didn't see how to specify using the NodaTime types. Is it part of the scaffold command? Is the NodaTime package supposed to create a new provider or something based on the SQL provider? Guessing here but is it something like:

dotnet ef dbcontext scaffold "Data Source=(localdb)\MSSQLLocalDB;Initial Catalog=YourDB" SimplerSoftware.EntityFrameworkCore.SqlServer.NodaTime

Or how else does the scaffold command know to reverse engineer the DB types to NodaTime types?

[ErikEJ]

You reference the nodatime package in your project, and that's it! Magic (when it works)

[ErikEJ]

So on the command line, just reference the standard SQL Server package name

[StevenRasmussen]

Wow! That does sound like magic :) Hopefully Brice or Arthur can provide some insight into what exactly the package needs to provide so that the magic happens!

[StevenRasmussen]

I wonder if the following has something to do with the magic in the Npgsql provider here:

NpgsqlConnection.GlobalTypeMapper.UseNodaTime();

[ErikEJ]

No idea, let's ask @roji :smile:

[bricelam]

I wonder if reverse-engineering to NodaTime types is more of a concern for code generation, and should be handled in templates somewhere instead of while transforming the database schema into an EF Core model...

[StevenRasmussen]

That's a valid point... I would defer to your expertise on this. Maybe @roji could comment on how it's currently handled by the Npgsql provider.

[roji]

Wrote some comments above, but tl;dr Npgsql's EF NodaTime plugin does support scaffolding via a single NpgsqlNodaTimeTypeMappingSourcePlugin (no replacing of the provider's TypeMappingSource neither in runtime nor in design-time).

From a cursory glance, the PR seems like it should work (though I'd consolidate the plugins into a single one). @ErikEJ @StevenRasmussen how have you attempted to test this? If you've tried taking a dependency on this plugin via <ProjectReference> from a test project, then IIRC that's not going to work; I think there's a bit of magic that makes the .targets file only work out-of-the-box when using a <PackageReference> with a nuget; to test with <ProjectReference> you need to manually import the plugin's .targets file in the test project's csproj.

NpgsqlConnection.GlobalTypeMapper.UseNodaTime();

This adds NodaTime support at the Npgsql ADO level (on top of which the EF support is implemented) - it is Npgsql-specific and you don't need it, since SqlServer support is entirely done at the EF level.

[roji]

BTW please ping me again if you still can't make it work, and I'll try it out.

[StevenRasmussen]

@roji - Thanks for stepping in here and providing some feedback.

From a cursory glance, the PR seems like it should work (though I'd consolidate the plugins into a single one)

Agreed. When I started this project in the 3.x timeframe I tried to follow the same pattern that existed in the EF Core codebase which I believe had them all separated out. It looks like during the 5.x timeframe they were consolidated into a single class. I think that's a cleaner approach and will look at doing that.

how have you attempted to test this?

@ErikEJ mentioned the issue regarding using a <ProjectReference> and <PackageReference> and so I've been using a local folder based NuGet feed so that a <PackageReference> is used. When I run the scaffold command to generate a DB context it executes successfully and even adds the UseNodaTime method as specified by the SqlServerNodaTimeCodeGeneratorPlugin (so I know its picking up this plugin as part of the generation) but the CLR types for the model are just DateTime instead of an Instant or a LocalDate per the existing DB schema (which map to a datetime2 and date column respectively in the DB).

I guess it's still not clear to me how the scaffolding command should be picking up and using each of the IRelationalTypeMappingSourcePlugin's defined in my SqlServerNodaTimeDesignTimeServices.ConfigureDesignTimeServices method in order to override the default type mappings from the SqlServerTypeMappingSource. This is where I was coming up with the idea that the SqlServerTypeMappingSource needed to be replaced somehow so that it would know that if it encountered a datetime2 column type that it should map to an Instant instead of a DateTime CLR type. Anyway, I'm sure I'm still missing something somewhere so if you have a chance to take a look that would be much appreciated. Thanks!

[ErikEJ]

@bricelam Not sure why you think templates are needed, as mentioned by @roji this works well in EF Core Power Tools for both MySQL and PostgreSQL, cannot see why we cannot make the SQL Server support equally great?

I am able to use the plugin functionality from a local package feed, but experience the same issue as @StevenRasmussen - the UseNodaTime call is added to the generated DbContext, but no mapping of datetime2 columns to Instant takes place, so if you could have a look, @roji that would be really great!

[roji]

I took a look, and the TypeMappingSourcePlugins simply don't seem to support finding a mapping based only on a store type - which is what scaffolding does. For example, here's InstantTypeMappingSourcePlugin.FindMapping:

public RelationalTypeMapping FindMapping(in RelationalTypeMappingInfo mappingInfo)
{
    var clrType = mappingInfo.ClrType ?? typeof(Instant);
    var storeTypeName = mappingInfo.StoreTypeName;

    return typeof(Instant).IsAssignableFrom(clrType)
        ? new InstantTypeMapping(storeTypeName ?? "datetime2", clrType)
        : null;
}

When scaffolding, mappingInfo.ClrType is always null, and so this returns null. You can take a look at NpgsqlNodaTimeTypeMappingSourcePlugin to see how to implement a type mapping source plugin that maps both ways (and handles all NodaTime types in a single plugin).

On an unrelated note, I noticed that the UseNodaTime extension is defined in the Microsoft.EntityFrameworkCore.SqlServer.NodaTime.Extensions namespace. We usually define such extensions under Microsoft.EntityFramework, so that no additional using directives are necessary. This is particularly important here, as the scaffolded DbContext has UseNodaTime, but the directive is missing.

[StevenRasmussen]

@roji - Thanks for checking and that makes sense now! I’ll work on refactoring the code to consolidate into a single plugin and at the same time address this issue. Hopefully it should “just work” after this :) Thanks again!

[roji]

I think it will :) I can already see "UseNodaTime" appearing in the scaffolded DbContext, so it really is only the type mapping that isn't working properly. Once that's done, it should achieve magic status as above...

[StevenRasmussen]

Ok. I just pushed several changes that I thought would do it but unfortunately it's still not working for some reason. The logic for selecting the RelationalTypeMapping has been consolidated into the SqlServerNodaTimeTypeMappingSourcePlugin.cs file. I checked and both the StoreTypeMappings and ClrTypeMappings collections are being populated correctly so that it should return a valid mapping based on only the store type so I'm not quite sure what the issue could be. Sorry to ping you on this again @roji but would you mind taking another look?

[roji]

Bad news - there's an ordering issue in SqlServerTypeMappingSource which makes it try plugins only if a built-in mapping isn't found - I've opened https://github.com/dotnet/efcore/issues/24660 to track. As a workaround, you should be able to replace SqlServerTypeMappingSource with your own class which extends it, and simply overrides FindMapping to switch the lookup order.

[ErikEJ]

@StevenRasmussen I can confirm that this did not work! @roji So this worked for other provider how?

[roji]

@ErikEJ our messages crossed each other :) Npgsql and Pomelo reversed the lookup order in their TypeMappingSource, SqlServer will need to do the same.

[StevenRasmussen]

I think we're finally there! I just pushed the changes for the suggested workaround and was able to reverse engineer a DB that produced properties that had NodaTime types! Thanks everyone for your help! @ErikEJ - If you want to double check that everything looks good on your end I will go ahead and merge and update the published NuGet package.

[ErikEJ]

Fantastic!

I will have a look soon, maybe bump version to 5.1 due to obsolete API etc?

[StevenRasmussen]

I had considered bumping to 5.1 but wanted to keep the library's Major.Minor in step with EFCore to reduce confusion when determining which package to install.

[ErikEJ]

[ErikEJ]

:shipit:

[ErikEJ]

LGTM

[roji]

Awesome that this is done! FYI https://github.com/dotnet/efcore/issues/24660 has also been merged on the EF Core side, so the SqlServerTypeMappingSource hack can be removed for 6.0 at some point.

[StevenRasmussen]

Thanks @roji !!