microsoft / DACExtensions

DACExtensions contains samples that extend Data-Tier Applications using DacFx. These samples include deployment contributors and static code analysis rules that can be used with Visual Studio as well as examples of how to use the DacFx public mode
MIT License
125 stars 41 forks source link

DAC incorrectly sets containment type = Partial when restoring to SQL 2019 instance #36

Open IanKemp opened 4 years ago

IanKemp commented 4 years ago

I'm generating a bacpac file from a SQL Azure hosted database (compatibility level SQL Server 2017) and restoring it to a SQL Server localdb instance. This was working fine, but then I upgraded my localdb instance to SQL 2019, and now the restore fails:

Creating deployment plan
Initializing deployment
Verifying deployment plan
Analyzing deployment plan
Importing package schema and data into database
Updating database
*** Error importing database:Could not import package.
Error SQL72014: .Net SqlClient Data Provider: Msg 12824, Level 16, State 1, Line 5 The sp_configure value 'contained database authentication' must be set to 1 in order to alter a contained database.  You may need to use RECONFIGURE to set the value_in_use.
Error SQL72045: Script execution error.  The executed script:
IF EXISTS (SELECT 1
           FROM   [master].[dbo].[sysdatabases]
           WHERE  [name] = N'$(DatabaseName)')
    BEGIN
        ALTER DATABASE [$(DatabaseName)]
            SET FILESTREAM(NON_TRANSACTED_ACCESS = OFF),
                CONTAINMENT = PARTIAL
            WITH ROLLBACK IMMEDIATE;
    END

Error SQL72014: .Net SqlClient Data Provider: Msg 5069, Level 16, State 1, Line 5 ALTER DATABASE statement failed.      Error SQL72045: Script execution error.  The executed script:
IF EXISTS (SELECT 1
           FROM   [master].[dbo].[sysdatabases]
           WHERE  [name] = N'$(DatabaseName)')
    BEGIN
        ALTER DATABASE [$(DatabaseName)]
            SET FILESTREAM(NON_TRANSACTED_ACCESS = OFF),
                CONTAINMENT = PARTIAL
            WITH ROLLBACK IMMEDIATE;
    END

The important point to note here is that the SQL Azure database has containment type = None so I have no idea why (what I presume to be) the generated script is trying to alter the destination DB to have CONTAINMENT = PARTIAL. I believe that SQLPackage is assuming that a 2019 DB must always be set to have containment type = Partial, rather than using the source database's containment type - i.e. this is a bug.

The arguments I'm passing to SQLPackage are simple: /a:Import /sf:"${bacpacFile}" /tcs:"${targetConnectionString}". This happens with both the latest .NET Framework (15.0.4627.2) and .NET Core (15.0.4630.1) versions of SQLPackage.

IanKemp commented 4 years ago

I just tried to import a BACPAC via SSMS's "Import Data-tier Application" wizard and received the same error message. So this does not look like a SQLPackage issue, but rather a DAC one.

image

IanKemp commented 4 years ago

I believe the bug is in Microsoft.Data.Tools.Schema.Sql.dll, to be specific the Microsoft.Data.Tools.Schema.Sql.Deployment.Sql110DeploymentPlanGenerator#BuildAlterDatabaseStatements() method, in particular this block:

if (targetOptions != null && sourceOptions.Model.Platform == SqlPlatforms.SqlAzureV12 && SqlDeploymentPlanGenerator.IsPropertySupported(base.TargetModel.Platform, SqlDatabaseOptions.ContainmentClass) && !Sql110ModelComparer.CompareContainmentProperty(sourceOptions, targetOptions, SqlDatabaseOptions.ContainmentClass, null) && base.SupportsContainment)
{
    bool updateContainment = true;
    foreach (DatabaseOption option8 in setStatement.Options)
    {
        ContainmentDatabaseOption option2 = option8 as ContainmentDatabaseOption;
        if (option2 != null)
        {
            updateContainment = false;
            option2.Value = ContainmentOptionKind.Partial;
            break;
        }
    }
    if (updateContainment)
    {
        ContainmentDatabaseOption option = new ContainmentDatabaseOption
        {
            Value = ContainmentOptionKind.Partial
        };
        setStatement.Options.Add(option);
    }
}

I'm obviously no expert in this code, but it seems to me that either this block is being entered incorrectly (the outermost if has faulty logic and is evaluating when it should not be and/or the inner ifs in the foreach are not), or something upstream is setting a property incorrectly thus causing the if to be entered when it shouldn't be.

If this was an open-source project I would be able to pull the source, replicate the bug, make a fix, and submit a pull request... but it's not, and it's only due to the magic of ILSpy that I'm able to get this far.

IanKemp commented 4 years ago

This issue is present since at least 15.0.4538.1 (the oldest .NET Core Windows build of SQLPackage that is available for download).

IanKemp commented 4 years ago

Issue present as far back as 15.0.4200.2 (from the oldest downloadable version of the 15.x DAC package I could find).

Issue also present in 14.0.3881.1 (newest version of the 14 series that does not give me an "unable to connect to to master or target server" error).

IanKemp commented 4 years ago

Significant time with ILSpy has confirmed that the issue is due to the aforementioned block of code. However I now believe that the block itself is correct, and the condition causing it to be entered is the problem.

In particular, it seems that the method Microsoft.Data.Tools.Schema.Sql.SchemaModel.Sql110ModelComparer#CompareContainmentProperty is buggy. It's comparing containment properties for two models, neither of which has containment set or required, yet is returning false (indicating that it thinks the containment between the two is/should be different) when it should be returning true.

I think this is because that method only considers the source containment for SqlAzureV12 when it should also be considering the target containment.

llali commented 4 years ago

@IanKemp, One of the reasons DacFx changes target db to be containment is if the source db has contained user. Exporting a db with contained user cannot happen if target db is not changed to partial.