Skip to content

Use Microsoft.EntityFramework.SqlServer with the modern SqlClient #10424

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

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from

Conversation

ErikEJ
Copy link
Contributor

@ErikEJ ErikEJ commented Apr 25, 2025

Summary of the changes (in less than 80 characters):

  • Use the "modern" Entity Framework SQL Server provider
  • Could enable NuGet.Services.Sql to be removed?

Addresses #10416

Notes

@ErikEJ ErikEJ requested a review from a team as a code owner April 25, 2025 11:38
@joelverhagen
Copy link
Member

joelverhagen commented Apr 25, 2025

The change looks reasonable. This is just the kind of incremental step that we need, towards .NET Core.

There's a significant burden of testing here. In other words, we need to deploy all of the jobs that use SQL and verify everything is still working.

This is inevitable but it will take a while for me to do that and I'm going to block the merge on that to avoid risk to our ongoing deployments.

Please bear with me as I test the change. I'll respond back when I know more.

@joelverhagen joelverhagen self-assigned this Apr 25, 2025
@ErikEJ
Copy link
Contributor Author

ErikEJ commented Apr 25, 2025

There's a significant burden of testing here. In other words, we need to deploy all of the jobs that use SQL and verify everything is still working.

Yeah, I was expecting that

@joelverhagen
Copy link
Member

Test failures:

##[error][xUnit.net 00:00:01.06]     NuGet.Services.Sql.Tests.AzureSqlConnectionStringBuilderFacts.ExtractsAadSettingsFromConnectionString [FAIL]

  Failed NuGet.Services.Sql.Tests.AzureSqlConnectionStringBuilderFacts.ExtractsAadSettingsFromConnectionString [283 ms]
  Error Message:
   Assert.Equal() Failure: Strings differ
                                  ↓ (pos 128)
Expected: ···"0;Encrypt=True;Trust Server Certificate=F"···
Actual:   ···"0;encrypt=True;trustservercertificate=Fal"···
                                  ↑ (pos 128)
##[error][xUnit.net 00:00:19.67]     NuGet.Services.Validation.Tests.PendingMigrationsFacts.NoPendingMigrations [FAIL]

  Failed NuGet.Services.Validation.Tests.PendingMigrationsFacts.NoPendingMigrations [1 s]
  Error Message:
   System.AggregateException : One or more errors occurred.
---- System.ArgumentException : The ADO.NET provider with invariant name 'Microsoft.Data.SqlClient' is either not registered in the machine or application config file, or could not be loaded. See the inner exception for details.
-------- System.ArgumentException : Unable to find the requested .Net Framework Data Provider.  It may not be installed.
---- Microsoft.Data.SqlClient.SqlException : User does not have permission to alter database 'PendingMigrationsTest202504251444524495653Validation', the database does not exist, or the database is not in a state that allows access checks.
ALTER DATABASE statement failed.
Cannot drop the database 'PendingMigrationsTest202504251444524495653Validation', because it does not exist or you do not have permission.
  Stack Trace:

@joelverhagen
Copy link
Member

joelverhagen commented Apr 25, 2025

Also looks like NuGet.Jobs.sln is failing build. This is our solution file for the backend. NuGetGallery.sln is front-end focuses (mostly).

@joelverhagen
Copy link
Member

I fixed a build error and pushed it to your branch. It was only discoverable if you ran test.ps1 or the Update-Databases.ps1 script directly.

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Apr 28, 2025

@joelverhagen Wonder why the package inclusion was required?

@joelverhagen
Copy link
Member

It's used to determine where to find the ef6.exe tool in the user packages directory (restore destination). See:

if (!$efDirectory) {
# Read the current version of EntityFramework so that we can find the tools.
$cpmPath = Join-Path $PSScriptRoot "..\Directory.Packages.props"
[xml]$cpm = Get-Content $cpmPath
$efPackageReference = Select-Xml -Xml $cpm -XPath "//*[local-name()='PackageVersion']" `
| Where-Object { $_.Node.Attributes["Include"].Value -eq "EntityFramework" }
$efVersion = $efPackageReference.Node.Version
Write-Host "Using EntityFramework version $efVersion."
if ($env:NUGET_PACKAGES) {
$efDirectory = Join-Path $env:NUGET_PACKAGES "EntityFramework\$efVersion"
}
else {
$efDirectory = Join-Path $env:USERPROFILE ".nuget\packages\EntityFramework\$efVersion"
}
}

@ErikEJ
Copy link
Contributor Author

ErikEJ commented Apr 28, 2025

@joelverhagen Ah, the CLI tool is only in the base EF6 package!

@joelverhagen
Copy link
Member

Yes. Technically we could lookup the version using the Microsoft.EntityFramework.SqlServer package because that version matches the EntityFramework version, and EntityFramework is a transitive dependency, but I think it's better to be explicit here. We could also force the install with nuget.exe install... but this all will change when we move to EF Core so I think it's fine now.

@joelverhagen
Copy link
Member

I think no action is needed for these two. PreinstalledPackages.json is for a legacy job that is only used for bulk revalidation operations, of which we have only ever done one (we repository signed all existing packages on NuGet.org several years ago).

For the connection strings in our deployment pipelines, it looks like the old key names are still supported for back compat so we don't need to do anything. But we'll keep an eye on connection issues as we test the deployments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants