Generate DbContext Class in a Different Location

[tonysneed]

Address #1836 by generating DbContext class to a different location.

For my first pass, I refactored ReverseEngineeringScaffolder.Generate to accept a outputDbContextPath parameter for specifying a different location for the DbContext class.

Next I'll look at how to refactor OperationExecutor.ScaffoldContext.

[tonysneed]

The CI build failed because of a timeout. I ran the build and tests locally and they all passed, although it took more than an hour for them to complete. Is there a more efficient way to run the tests?

Update: I figure this is something I'll have to live with. But I'm using dotnet test to run specific tests. ReSharper is good for debugging tests, but it takes up so much memory, VS becomes unstable.

[tonysneed]

@bricelam I implemented the update for the C# generator, but I'll probably need to do the same for VB and F#. Can you point me in the right direction?

[tonysneed]

Currently there are a couple of failing tests, most likely because I'm doing something untoward.

One test, DbContextScaffoldCommand_accepts_separate_context_output_path, fails with LocalDB is not supported on this Platform. However, I've seen other tests using LocalDB without any problems.

The other test, OperationExecutor_ScaffoldContext_generates_separate_context_output_path, fails with a null reference exception on line 106, probably because the "ContextFile" is not being generated. This works for me locally, so there must be something about the build environment that is interfering with scaffolding the DbContext class.

Any help diagnosing these would be greatly appreciated! 😄

[bricelam]

I implemented the update for the C# generator, but I'll probably need to do the same for VB and F#. Can you point me in the right direction?

I think the scaffolding code generators are responsible for way too much at the moment. I'm going to play around with the factoring a bit and try to move out the code that writes the files to disk. In Migrations, the flow looks something like this:

var scaffoldedMigration = migrationsScaffolder.ScaffoldMigration(); // Calls IMigrationsCodeGenerator
var files = migrationsScaffolder.Save(scaffoldedMigration);

This also helps with testing since you don't need to intercept the I/O using something like IFileService.

[bricelam]

P.S. We should also try and get some of this code out of the .Internal namespace for 2.1 so you can reliably depend on it.

[tonysneed]

Excellent - I’m looking forward to the new flow and glad to play a part in it! And it will make testing a lot easier too. :)

[tonysneed]

Waiting on #10459 to merge, then I'll refactor.

[smitpatel]

@tonysneed - If you can rebase on latest dev. 😄

[tonysneed]

I should have time for this later this week.

[bricelam]

Don't bother with adding end-to-end tests yet. We have #9111 to figure out the best approach for this.

[tonysneed]

@bricelam You can review this, now that I've merged upstream commits and refactored.

I've added outputDbContextDir parameter to IReverseEngineerScaffolder.Save, which can be:

1. Null (defaults to outputDir)
2. Folder name under projectPath
3. Path relative to projectPath
4. Absolute path

Please let me know what you think.

[tonysneed]

@bricelam There used to be an IFileService interface that abstracted the file system so you could mock it. Is there an alternative to that now?

[tonysneed]

I fixed the failing test in commit 4977d9f, and all the tests pass for me locally when running build.cmd. So I'm not sure why the CI build would still be failing.

[tonysneed]

Wondering if someone might be able to review this sometime soon?

[bricelam]

Sure, I think I'm done making breaks in this area. 😉 Can you rebase again?

[tonysneed]

@bricelam I rebased again and resolved conflicts. I think the code it working properly at this point. However, one of my tests (Microsoft.EntityFrameworkCore.Design OperationExecutor_ScaffoldContext_generates_separate_context_output_path) is getting an exception on DatabaseOperations.ScaffoldContext when it calls services.GetRequiredService<IReverseEngineerScaffolder>():

Unable to resolve service for type 'Microsoft.EntityFrameworkCore.Storage.Converters.IValueConverterSelector' while attempting to activate 'Microsoft.EntityFrameworkCore.Storage.CoreTypeMapperDependencies'.

It looks like a dependency needs to be registered, but I'm not sure how to do that in the context of my unit test. Would you be able to have a look?

[bricelam]

Changes look good. I suspect we just forgot to add the following to DesignTimeServiceCollectionExtensions.AddEntityFrameworkDesignTimeServices().

.AddSingleton<IValueConverterSelector, ValueConverterSelector>()

[tonysneed]

Is this something I can fix? Or maybe open a separate issue?

[bricelam]

I can fix it as part of merging this.

[bricelam]

(We know we need #9111 to catch things like this.)

[tonysneed]

@bricelam Can you also remove the Skip parameter on the [Theory] attribute on my unit test?

Or I can re-test after you have registered IValueConverterSelector with DI.

[tonysneed]

Maybe this feature will make it into the next release?

[bricelam]

Yep. I'll try to get it merged for 2.1.0-preview1. (Working on some 2.0.x patch things at the moment.)

[tonysneed]

Thanks! I think this tweak to the tooling will be appreciated by a lot of folks, especially those using repository patterns. /cc @lelong37

[bricelam]

@tonysneed Forgive me. I'm tweaking this slightly since #8434 will want to put the configuration classes next to the DbContext. There were also a couple of missing things (the corresponding PowerShell changes and a resource). I removed some tests too 🐑 (sheepishly) since we need to figure out the best way to write functionals for RevEng (issue #9111), but I manually verified everything is working end to end.

Let me know if you have any concerns with my updates.

I also filed #10727 to clarify the semantics of this option.

[tonysneed]

@bricelam Looks great - thanks!

[tonysneed]

Glad to see this merged! 🎉

[weitzhandler]

Thanks @tonysneed!