Fix for 15256 - bad generated SQL for DefaultValue with line breaks.

[lajones]

Fix for #15256 - bad SQL was generated for DefaultValue with line breaks.

[AndriySvyryd]

Also apply the fix to ComputedColumnSql in SqlServerMigrationsSqlGenerator

[lajones]

@AndriySvyryd My mistake - I shouldn't have been applying it to DefaultValueSql in the first place - only to DefaultValue. For both DefaultValueSql and ComputedColumnSql it is assumed the string you are passing in is already in the form of valid SQL - and we shouldn't be trying to change it. But for DefaultValue if you pass in a string we should escape line breaks as otherwise we'll end up with invalid SQL.

[smitpatel]

For both DefaultValueSql and ComputedColumnSql it is assumed the string you are passing in is already in the form of valid SQL

I wonder if that assumption actually says anything about us trying to indent the SQL. Users may be totally unaware of. Even if a valid SQL passed in, our indented new line can break it.

[lajones]

@smitpatel I think the remark about indentation was a red-herring. The problem really was we would generate something like this:

CREATE TABLE [TestEntity] (
    [Id] int NOT NULL IDENTITY,
    [Name] nvarchar(max) NULL DEFAULT (Do not
do
this
)

And that's invalid SQL - so users of that would definitely notice. Now we'll generate something like this:

CREATE TABLE [TestEntity] (
    [Id] int NOT NULL IDENTITY,
    [Name] nvarchar(max) NULL DEFAULT CONCAT(N'Do not', CHAR (13), CHAR(10), N'do', CHAR (13), CHAR(10), N'this')
)

which is valid.

[AndriySvyryd]

@lajones Ok, but what do DefaultValueSql and ComputedColumnSql with line breaks produce when we script migrations?

[lajones]

@AndriySvyryd - they would produce bad SQL but that's the user's responsibility. The difference is that for DefaultValue() the user is assumed to be passing in a value, and EF is responsible for converting that value to valid SQL, whereas for DefaultValueSql() and ComputedColumnSql() the user is assumed to be passing in SQL and they are responsible for making sure that the SQL is valid.

There are an infinite number of ways that a user could pass invalid SQL to DefaultValueSql() and ComputedColumnSql() and us trying to guess and "fix" it will probably end up preventing someone else's scenario.

For instance, we cannot put single quotes around everything (like we do for strings passed to DefaultValue()) because it's perfectly possible that the user wants to call a function and end up with e.g. DEFAULT someFunction() which is very different from DEFAULT 'someFunction()'. Instead, if the user wants single quotes, the user is responsible for putting them in the string which they pass to DefaultValueSql() or ComputedColumnSql() - so we cannot be sure that replacing line breaks with CONCAT('abc', CHAR(13), CHAR(10), 'xyz') is the right thing to do as we're not sure that abc and xyz were meant to be strings to start with.

[smitpatel]

SELECT TOP(1) a.Column
From table as a
Where a.text = N'abc
   def
   ghi
end';

A valid SQL. If you indent it, gives you incorrect results.

[lajones]

OK. Agreed. This PR fixes the problem for DefaultValue - because the value will now always be on the same line as the DEFAULT statement. So please review that.

But I agree it does not address the issue for DefaultValueSql or ComputedColumnSql using SQL such as @smitpatel wrote above. We cannot solve that issue by the same method - for the reasons I stated above. So that will have to be a separate PR (presumably involving a different mode for IndentedStringBuilder).

[lajones]

Also note - a user of DefaultValueSql() or ComputedColumnSql() could solve the indentation problem right now. They could pass in the following:

SELECT TOP(1) a.Column
From table as a
Where a.text = CONCAT(N'abc', CHAR(13), CHAR(10), N'   def', CHAR(13), CHAR(10), N'   ghi', CHAR(13), CHAR(10), N'end');

which also makes clear their expectations of how newlines are stored in the database.

But a user of DefaultValue cannot solve the same problem - because EF is in charge of formatting the SQL. That's what this PR addresses.

[bricelam]

@smitpatel This is only an issue with idempotent scripts, so I'm not really worried about the migrationBuilder.Sql() cases since the workaround is just to write your SQL the same way we do.

I only feel obligated to fix the cases where we generate bad SQL for a literal value that the user correctly provided. Because in these cases, there isn't a good workaround.

[smitpatel]

@bricelam - I am not referring to migrationBuilder.Sql() API. I am referring to HasDefaultValueSql API. I don't have strong preference over trying to fix it or not. It would be just hard to detect errors.

[AndriySvyryd]

At least file an issue to track this

[lajones]

@bricelam @AndriySvyryd @smitpatel I'm happy to file the new issue to look into DefaultValueSql or ComputedColumnSql, but can we sign off on this please?

[lajones]

@AndriySvyryd I filed #20391 to track that issue.

[ajcvickers]

@smitpatel @AndriySvyryd @bricelam Given that we have an issue tracking SQL cases, is there anything left to do here? If not, can someone sign off?

[lajones]

Many thanks to @lauxjpn and in particular his update to the MySql provider for inspiration on this.