Can't convert byte[] to string

[IndigoHealth]

I changed the Type of a property from byte[] to string. The resultant migration code in the Down routine is:

 migrationBuilder.AlterColumn<byte[]>(
        name: "Data",
        table: "ScheduledTasks",
        nullable: true,
        oldClrType: typeof(string),
        oldNullable: true);

When I attempt to update-database to an earlier migration, which runs the above code, I get this error:

Implicit conversion from data type nvarchar(max) to varbinary(max) is not allowed. Use the CONVERT function to run this query.

The full error message is:

fail: Microsoft.EntityFrameworkCore.Database.Command[20102]
      Failed executing DbCommand (103ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      DECLARE @var0 sysname;
      SELECT @var0 = [d].[name]
      FROM [sys].[default_constraints] [d]
      INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
      WHERE ([d].[parent_object_id] = OBJECT_ID(N'[ScheduledTasks]') AND [c].[name] = N'Data');
      IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [ScheduledTasks] DROP CONSTRAINT [' + @var0 + '];');
      ALTER TABLE [ScheduledTasks] ALTER COLUMN [Data] varbinary(max) NULL;
System.Data.SqlClient.SqlException (0x80131904): Implicit conversion from data type nvarchar(max) to varbinary(max) is not allowed. Use the CONVERT function to run this query.
   at System.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at System.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at System.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at System.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean async, Int32 timeout, Boolean asyncWrite)
   at System.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean asyncWrite, String methodName)
   at System.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Execute(IRelationalConnection connection, DbCommandMethod executeMethod, IReadOnlyDictionary`2 parameterValues)

Steps to reproduce

Further technical details

EF Core version:
Database provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer)
Target framework: (e.g. .NET Core 3.0)
Operating system:
IDE: (e.g. Visual Studio 2019 16.3)

[ajcvickers]

@sbsw What would you expect to happen with the existing binary data?

[IndigoHealth]

As the warning says, “you’ve scaffolded a change that might result in loss of data”. I understand that I’ve made a breaking change and I expect the data to be deleted. I worked around the problem by modifying the generated code to first delete the string column, then to add back in the byte[] column.

Now that I think about it, I don't understand why I don't see the same problem with the Up migration, since the conversion from byte[] to string is just as ambiguous as the conversion from string to byte[].

In addition to the existing (highlighted) warning that I get when I run add-migration (that's a GREAT feature, BTW), it would be super great if the generated migration code contained a comment immediately preceding the code that deletes data, warning that data is being deleted.

[ajcvickers]

@sbsw We discussed this, but it's not something we think we can/should change at the EF level. (We could implement column re-building, and then replicate the (quite strange) conversion rules that SQL Server has to do a rebuild when necessary, but the value seems low since we still wouldn't be able to convert the data automatically.)

@bricelam I think we discussed in the past adding a comment to the generated migration for potential data loss. Do you remember the conclusion?

[IndigoHealth]

@ajcvickers I don't know what "this" is that you discussed. Just to be clear, I'm not asking for the migration code to do anything fancy. I understand that I've introduced a breaking change, and I'm fine with it deleting the existing data in the affected column. My issue is simply that the scaffolded migration code fails with an exception, so I can't execute the scaffolded migration. The scaffolded migration code should execute without error.

If the code throws away data (or may throw away out-of-range or otherwise incompatible data) then it would be nice if we were told about it, in a warning from the scaffolder (as it does today), in a comment in the code, and in a warning from the update-database command that executes the scaffolded code.

[ajcvickers]

@sbsw "This" refers to "The scaffolded migration code should execute without error."

[IndigoHealth]

@ajcvickers Ouch! So, by design, the scaffolded migration code may look correct (in this case, it appears to be converting a column from string to byte[]), but the code may fail when it is executed. And, as I gather from your commentary, the EF team doesn't want to identify and work around all of the odd corner cases where scaffolded code like this might fail with an exception (or, maybe worse, produce unexpected results).

Ok, so, instead of giving us the generic "migration may cause loss of data" warning (which usually means I've deleted a column or some such), could you give us a stern, clear warning when the migration code might fail in a more unexpected way? Something like:

Warning, the scaffolded code may not produce the results you expect, or it may fail with an exception. Test this migration to see what it actually does!

[ajcvickers]

@sbsw It is always the case that you should test migrations before using them. Ideally, we would report any issue that any database provider might have with a given migration, but that's not practical. That's why we have a tool to scaffold a starting point for a migration, which the developer must review, test, and edit appropriately before using. (This is also why they aren't marked as "auto-generated code"--they are intended as a starting point only.

Unfortunately, this often isn't how people use migrations. We have discussed this mismatch in understanding before, but I think it's worth us discussion again. I'll bring it up with the team.

[IndigoHealth]

@ajcvickers Hehe... Yeah, we get to the point were we rely on your tools to do what we "expect" them to do. That perception is reinforced by the generally excellent warnings that we get when the tool suspects that something may require our attention. Maybe you want to just be more explicit in setting expectations via a comment block at the beginning of the scaffolded code:

This auto-generated code is provided as a starting point. Feel free to modify it as necessary.

Due to idiosyncrasies in database providers, this auto-generated code may produce unexpected results, or it may fail with an exception from the underlying database provider. You are strongly encouraged to test both the Up and Down migration code before including it in your project.

[bricelam]

If we had more information in the provider about implicit conversions between store types (also needed for #15586), we could handle this better.

[roji]

Comment made by @Brar (who is sitting next to me): we could also scaffold a commented-out proposal of a migration, with the conversion encoding as a placeholder. This would go a long way to help the user deal with this, while forcing them to actually think about it (and put in a valid encoding). This makes sense especially since we're saying we expect users to inspect our migrations etc.

[IndigoHealth]

The problem (from my perspective as a developer who relies on the tooling to just get the job done) is that the migration code is a high-level abstraction (add this column; change that column from byte[] to string). Visual inspection of the migration code gives me no obvious clue what operations may cause the underlying provider to choke. Obviously, I now know that type conversions may be problematic, so I'll look at them carefully. But what other operations deserve increased scrutiny?

The only realistic option is for the user to test both the Up and Down migration every time to see if it throws an exception. If it passes that simple test then I assume that the database schema was correctly modified. It would be really helpful if the migration methods were smart enough to guess what operations MIGHT cause corruption or data loss, and emit a warning during the update-database operation to tell the user what data to examine after the migration.

[roji]

@sbsw note that there are many ways in which you can add a migration which would fail; for example, you could set a default SQL value that isn't valid, EF Core would create that migration and SQL Server would error when you tried to apply the migration. It's simply not possible for EF Core to attempt to identify all cases in which a migration would cause an error upon application.

As @ajcvickers wrote above, in any real-world production scenario, migrations must be inspected and applied against a test/preproduction database, surfacing this kind of issue.

[ajcvickers]

Notes from team triage:

• Make the documentation about checking migrations and testing more explicit: Document clearly that migrations need to be tested EntityFramework.Docs#2188
• Put this on the backlog to see if we can do anything better for this case