Stop adding connection string into code when scaffolding

[KallDrexx]

When I run the command:

dotnet ef dbcontext scaffold "<connection-string>" Microsoft.EntityFrameworkCore.SqlServer -o "Data" -c "ApiDbContext" -f

It generates a ApiDbContext.cs file with the following:

       protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            #warning To protect potentially sensitive information in your connection string, you should move it out of source code. See http://go.microsoft.com/fwlink/?LinkId=723263 for guidance on storing connection strings.
            optionsBuilder.UseSqlServer(@"<connection string>");
        }

I have literally just spent 2 hours trying to figure out why my our staging environments were hitting our development databases instead of our production databases. We run this command every time we have any database schema changes (because we do database first instead of code first) and this time I forgot to remove this.

It got lost in a code review because of other changes in that file as well as lots of changes in other areas of the code.

It got lost in the warnings message because the current build has a lot of warnings because of [Obsolete] tags that we cannot remedy immediately.

To me this whole situation is not good, if for no other reason then you are encouraging users to put their sensitive connection strings in source control while at the same time saying "this is a bad practice but we will still shove this in your code every time you run this command". All it takes is one person to miss that message and it's in source control forever (or can be a royal pain to remove if it's an active code base).

While running dotnet ef dcontext scaffold --help I do not see any option to help remedy this either.

[ErikEJ]

I will investigate adding the option to have the connectionstring removed to my "EF Core Power Tools" ErikEJ/SqlCeToolbox#582 - is the best approach to simply remove the 2 lines of code in OnConfiguring?

[ErikEJ]

@KallDrexx Implemented in the latest daily build: https://github.com/ErikEJ/SqlCeToolbox/wiki/EF-Core-Power-Tools

[KallDrexx]

That's neat and I 100% appreciate it, but unfortunately most of our developers do not use Visual Studio proper anymore, we are mixed between Rider and VS Code (mostly for cross-platform support) :(.

[ajcvickers]

@KallDrexx This was actually discussed at great length a few years ago. The problem is that we want the result of reverse engineering to be runnable. If it isn't then people tend to give up immediately. We also couldn't come up with a secure way of doing this that would work across platforms/environments/frameworks. So after much debate we went with putting the connection string in, but with a warning comment. This certainly isn't ideal, but as of now it seems the best compromise between the competing requirements.

Note that the work planned around update-model-from-database #831 should make this much less of a problem for your scenario because it will not be necessary to run a complete , new scaffolding every time.

[josh-sachs]

For what it is worth, it seems to me the best compromise would have been to include an additional, optional flag that would result in the current behavior - and to have the default behavior be something that is suitable for use against a production code base.

The inclusion of the connection string is bad practice, and the tooling should work in a way that encourages people to wean themselves off of functionality that was included for the sake of mitigating any learning curve that may exist.

People following a guide can easily include a --hold-my-hand flag... they are probably copying and pasting from the web anyway. When present, the included warning comment should then advise them to remove that line of code, and omit the flag when they are ready to scaffold for use in a production environment.

It is discouraging to me that this isn't obvious, and that the current behavior is actually the product of intentional deliberation, rather than just an oversight.

Can we at least consider adding a flag to the CLI tool to prevent this behavior? The warning comment could be updated to inform developers of the option.

[KallDrexx]

The likelihood of someone coming to EF Core to just try it out and just stumbling on how to scaffold C# entities from an existing database without looking at any documentation seems pretty slim to me. If they are already following documentation to perform the scaffold I don't see why it's that difficult to instruct them how to pass a connection string to a DbContext. That's only one extra step they have to use to execute and gives them insight how to properly extract their connection string into a more secure area.

[smitpatel]

After looking at this more closely,
the very first code snippet is actually not what we are generating with 2.0 packages.
#6686 explicitly converted code to following style.

       protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            if (!optionsBuilder.IsConfigured)
            {
            #warning To protect potentially sensitive information in your connection string, you should move it out of source code. See http://go.microsoft.com/fwlink/?LinkId=723263 for guidance on storing connection strings.
                optionsBuilder.UseSqlServer(@"<connection string>");
            }
        }

Which makes sure that if the DbContext is configured already, it won't be reconfigured. So scaffolding actually does not change connection string and does not cause any issue of hitting wrong database. The problem does not exist actually.

Further, re-running scaffolding erases all custom modifications #831 fixes it. This is just one of the points of that issue.

It is quite ironic thread though.
Beginners should read whole documentation line by line but reading code reviews in detail is not necessary.
Beginners should learn to pass in extra flag as an extra step when removing 2 lines of code is huge step.
It is unacceptable to put connectionstring in code when it is acceptable to ignore warnings during the build time.

[smitpatel]

Given that after removing OnConfiguring first time, "Update model from database" will preserve that modification as part of #831 and #831 being slated for 2.1.0 release, adding a flag in 2.1.0 release provides very little value.

[josh-sachs]

What is ironic is you missed the part where nobody claimed beginners read documentation line-by-line, the assumption was made they are copy pasting. The warning should educate them on how to alleviate the concern instead of requiring them to remember to revisit to the same constructor every time they update their database outside of code-first.

What is ironic is that the content of the message also describes the security risks associated with having the connection string in the code, adding an if statement doesn't alleviate this concern whatsoever.

What is Ironic is you are referencing an issue that may resolve it which has been open now for what - 3 years? Also, if I'm not mistaken the 2.1 .net core sdk isn't even released yet?

Tooling should do predictable things, and hard-wiring the codebase to the reference database during scaffold is unpredictable. The current behavior causes issues for teams who don't use code first to manage their database (e.g. that have an actual DBA) and seems trivial to address (a PR has already been submitted). The sarcasm at the end of your post @smitpatel is unnecessary and IMO violates your own repository guidelines around decorum.

[ralmsdeveloper]

@josh-sachs , I do not see sarcasm in the text @smitpatel , if you look, quotes being programmed to 2.1.

You have your opinion and your point of view, but I do not find your text productive.
@smitpatel just wanted to show something, and I believe that when he asked the beginners to see the documentation, surely the intention was good and did not disqualify anyone!

to complement: @smitpatel is one of the people who help most here, I say that because the same, like everyone on the team, always point me in the right direction.