Skip to content
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

Wrap SQL for HasServiceTier and HasPerformanceLevel to be ignored when not on Azure #20682

Open
Tracked by #22945
Tim-Pohlmann opened this issue Apr 19, 2020 · 13 comments

Comments

@Tim-Pohlmann
Copy link

Tim-Pohlmann commented Apr 19, 2020

When using HasServiceTier and HasPerformanceLevel in OnModelCreating a migration can be created that looks like this:

public partial class ChangedDatabaseServiceTierToBasic : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.AlterDatabase()
            .Annotation("SqlServer:EditionOptions", "EDITION = 'Basic', SERVICE_OBJECTIVE = 'Basic'");
    }

    protected override void Down(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.AlterDatabase()
            .OldAnnotation("SqlServer:EditionOptions", "EDITION = 'Basic', SERVICE_OBJECTIVE = 'Basic'");
    }
}

If applied to a non-Azure database this causes an Exception. I see two problems with this

  1. Instead of throwing an Exception, I would expect EF to just not do anything on a non-Azure DB because I don't see an elegant way of using this feature in a situation where some of my DBs are in Azure (production) and some are local (dev). This might be more of a feature request than a bug report, though.
  2. The Exception is all but helpful (see below).

Steps to reproduce

Add the following lines to OnModelCreating:

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
    base.OnModelCreating(modelBuilder);

    modelBuilder.HasServiceTier("Basic");
    modelBuilder.HasPerformanceLevel("Basic");
}

Then use Add-Migration to create a migration and try to apply it to a local SQL Server instance.
You'll see an Exception like this:

Microsoft.EntityFrameworkCore.Migrations[20402]
      Applying migration '20200413102908_ChangedDatabaseServiceTierToBasic'.
Applying migration '20200413102908_ChangedDatabaseServiceTierToBasic'.
fail: Microsoft.EntityFrameworkCore.Database.Command[20102]
      Failed executing DbCommand (3ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      BEGIN
      DECLARE @db_name NVARCHAR(MAX) = DB_NAME();
      EXEC(N'ALTER DATABASE ' + @db_name + ' MODIFY ( 
      EDITION = ''Basic'', SERVICE_OBJECTIVE = ''Basic'' );');
      END
Failed executing DbCommand (3ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
BEGIN
DECLARE @db_name NVARCHAR(MAX) = DB_NAME();
EXEC(N'ALTER DATABASE ' + @db_name + ' MODIFY ( 
EDITION = ''Basic'', SERVICE_OBJECTIVE = ''Basic'' );');
END
Microsoft.Data.SqlClient.SqlException (0x80131904): Incorrect syntax near '.'.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean isAsync, Int32 timeout, Boolean asyncWrite)
   at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.UpdateDatabase(String targetMigration, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabaseImpl(String targetMigration, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabase.<>c__DisplayClass0_0.<.ctor>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)
ClientConnectionId:d9f92b81-9916-48ee-9686-6d0f567ab86f
Error Number:102,State:1,Class:15
Incorrect syntax near '.'.

I also asked for help in SO:
https://stackoverflow.com/questions/61205031/specify-azure-sql-server-edition-in-ef-core-without-breaking-local-development

Further technical details

EF Core version: 3.1.2
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Win 10
IDE: Visual Studio 2019 16.5.4

@ajcvickers
Copy link
Member

@thPion The best way to approach this would be to only include these in the model in OnModelCreating if the application is targeting Azure. This can usually be determined from the connection string.

That being said, I would consider not having EF create Azure databases automatically, but instead use the portal to provision the databases you need such that you have complete control over what kind of database is being used. Migrations can still be used to populate the schema of a database that exists but is empty.

@ajcvickers ajcvickers added the closed-no-further-action The issue is closed and no further action is planned. label Apr 20, 2020
@Tim-Pohlmann
Copy link
Author

Tim-Pohlmann commented Apr 21, 2020

Making it optional in OnModelCreating is not doing anything since that's not the code that throws the Exception. That's in the migration. If you can show me an elegant way to determine which DB I'm on from inside a connection, that would be helpful.
That being said, I still consider the Exception itself or at least its content a bug.
Also: Why do you introduce a new feature (HasServiceTier and HasPerformanceLevel were introduced in 3.1 in November) if your recommendation is not to use it?

@ajcvickers ajcvickers reopened this Apr 21, 2020
@ajcvickers
Copy link
Member

@thPion You are correct; my answer was not useful. I'll discuss with the team.

We introduced these so that people who are creating the database from code have a way to set these things, but we still recommend using the portal to create your Azure database so that you are fully aware of what you are getting, and won't get inappropriate defaults without realizing it. (The fact that the default for these things changed silently under the covers was, somewhat ironically, a motivating factor both to add this feature and at the same time recommend not using it.)

@Tim-Pohlmann
Copy link
Author

Thanks for revisiting the issue! So what you are saying is, you recommend doing it through the portal but added theses features for people who have to do it via code for one or another reason. Gotcha, thanks for clearing that up.

@ajcvickers
Copy link
Member

@thPion We discussed this again and we're going to start wrapping this in a check for Azure. For now, you can edit the migration to do this manually with something like:

migrationBuilder.Sql(@"IF SERVERPROPERTY('EngineEdition') = 5
EXEC(N'ALTER DATABASE [ThreeOne.SomeDbContext] MODIFY (EDITION = ''Basic'', SERVICE_OBJECTIVE = ''Basic'' );');
");

@ajcvickers ajcvickers added type-enhancement area-migrations and removed closed-no-further-action The issue is closed and no further action is planned. labels Apr 27, 2020
@ajcvickers ajcvickers added this to the Backlog milestone Apr 27, 2020
@ajcvickers ajcvickers changed the title HasServiceTier and HasPerformanceLevel causing not helpful exception messages on non-Azure DB Wrap SQL for HasServiceTier and HasPerformanceLevel to be ignored when not on Azure Apr 27, 2020
@redowl3
Copy link

redowl3 commented May 7, 2020

Which namespace are HasServiceTier and HasPerformanceLevel in? Can't find them anywhere.

Amazed more people are not looking for this functionality, since Microsoft made GEN5 the default database size, this has been a real issue for us.

@Tim-Pohlmann
Copy link
Author

namespace Microsoft.EntityFrameworkCore

@redowl3
Copy link

redowl3 commented May 7, 2020

Thanks - I have that installed but still can't see them? Just got off a call with a Microsoft engineer and he couldn't see them either.

@Tim-Pohlmann
Copy link
Author

Are you using the correct version of EF Core? They got introduced in 3.1 and are not present in earlier versions.

@redowl3
Copy link

redowl3 commented May 7, 2020

Yes, think so

image

@Tim-Pohlmann
Copy link
Author

@ajcvickers
Copy link
Member

@redowl3 Please open a new issue and post the csproj for your project that is not finding these APIs.

@danielleiszen
Copy link

If it helps to anyone, we are using EFCore migrations to generate databases in different environments. In order to generate an ALTER statement before each migration we use a custom SqlServerMigrationGenerator that replaces the original one by using ReplaceService<IMigrationSqlGenerator, MyMigrationSqlGenerator>() on DbContextOptionsBuilder in Startup.

Then in MyMigrationGenerator we can override the Generate function as

                base.Generate(operation, model, builder);

                if (operation is SqlServerCreateDatabaseOperation create)
                {
                    builder.AppendLine("IF SERVERPROPERTY('EngineEdition') = 5")
                        .AppendLine("BEGIN")
                        .AppendLine("EXEC(N'ALTER DATABASE CURRENT MODIFY (EDITION = ''Standard'', SERVICE_OBJECTIVE = ''S0'', MAXSIZE = 30 GB, BACKUP_STORAGE_REDUNDANCY = ''LOCAL'' );');")
                        .AppendLine("END;")
                        .EndCommand();
                }

Of course a production ready solution can be extended by checking the model (Dependencies.CurrentContext.Context), reading environment variables and so on.

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

No branches or pull requests

5 participants