Reverse engineering: Add IsConfigured check to avoid using hard-coded connection when previously configured

[cbrianball]

I may be misunderstanding the intent of the tool: once I run dotnet ef dbcontext scaffold ..., all of my files are generated as expected, but the provided connection string is hard-coded in the dbcontext class itself (with a warning directing the developer to follow best-practices on storing a connection string).

I had planned to do my database schema updates externally (more specifically from a SQL Server database project), so I will need to run this tool whenever there are any schema changes, which means, each time it runs, I'll have to manually modify the generated database context (or script the change).

Is there an option/pattern I'm missing, or I am not following the intended usage of the tool?

[divega]

I think we could add an option to skip generating the connection string which you could use if you already have it in the right place. Would that help?

[cbrianball]

Yes, that would be great!

[rowanmiller]

We would have to generate code to wire up the context to use the existing connection string though, so depending on the app the code we'd have to generate could be ConfigurationManager in OnConfiguring, or if it's an ASP.NET core app then the code is probably already there in Startup.cs. Perhaps we need to take another look at whether we can realistically work out what kind of app we are generating into.

[cbrianball]

I don't know the details around the experience you are looking to provide, but as a developer, I think it's perfectly reasonable for it to be my responsibility to worry about how the connection string is set on the DbContext.

[rowanmiller]

My main point was that if we skipped generating the connection string then you wouldn't have to delete it from code... but you would still be in the position where you have to hand edit the context file to wire back up the connection string (or add the DI-friendly constructor if you are using ASP.NET Core).

[divega]

@rowanmiller Fair point. I think having an option to generate the constructor that takes external DbContextOptions<TContext> and no OnConfuguring() method is a better way to go about this.

[rowanmiller]

We decided we aren't going to do anything right now, but opened a more general item to look at better support for the different connection string patterns #6746. We think this may be intertwined with our reverse engineer template story, possibly with some built-in templates you can easily chose between.

[ElanHasson]

@rowanmiller am I to understand this discussion as "the connection string will be hard coded for now" ?

Why not add a flag to disable it as @divega suggested?

[ElanHasson]

Being unfamiliar with the EF Core codebase, I could have probably implemented this better-- but it seems pretty straight forward... ElanHasson@2d2c0dd

I didn't yet implement any tests, and have not submitted a PR for it.

Based on the above discussion, would it even be accepted? If so, I'll add tests.

[rowanmiller]

@ElanHasson suppressing OnConfiguring only gets you part of the way there, as you need to also add the constructor that takes options. We discussed adding this flag (implemented as you did) but decided it was just a stop gap that we would want to remove once we have proper support for doing something better with connection strings (i.e. something that tailors to the specific app model you are using).

[ElanHasson]

@rowanmiller sorry, didn't get an alert for your response.

Scaffolding is a major part of the DB first workflow especially with regard to automation.. currently the only other options I can see are using Roslyn/String manipulation to remove the method and this is not ideal.

Regarding the "something better", is that even slated/planned/identified? I'd be happy to take a stab at it if it isn't.

Perhaps the low-effort, half-measure is what I've implemented in ElanHasson@2d2c0dd .

It is already generating all the classes as partials, so perhaps it's an exercise left to the implementor?
In my scenario, we're injecting DbContextOptions into the ctor in a different partial class, and having a hard-coded connection string in the OnConfiguring method is actually preventing the injected DbContextOptions<> from being used due to configuration priority.

[ElanHasson]

Perhaps we can modify the generated method to include if (!optionsBuilder.IsConfigured)... then use the hardcoded connection string.

This should be a great low-effort modification that can satisfy the DI use case without introducing additional overhead, such as documentation, and the creation of many net-new unit tests, etc.

[rowanmiller]

Perhaps we can modify the generated method to include if (!optionsBuilder.IsConfigured)... then use the hardcoded connection string.

Interesting idea, re-opening to discuss in triage

[ElanHasson]

Great to see this moving along...

[ElanHasson]

@rowanmiller and @smitpatel thanks for taking care of this so quickly.

Glad I was able come up with a simple, fast solution!!