MigrationsSqlGenerator: DefaultValue formats default value properly

[mirol-h]

Fix for issue #14457. Contains breaking change in MigrationsSqlGenerator (changed signature of DefaultValue method).

[dnfclas]

All CLA requirements met.

[bricelam]

LGTM. We should try and take this for 3.0 since it's consistent with the column type flowing we've added to the query pipeline in 3.0.

[ajcvickers]

Looks good other than my one comment.

[mirol-h]

Thanks for the feedback. I've incorporated your suggestion and also changed the call to FindMapping like this:

Dependencies.TypeMappingSource.FindMapping(defaultValue.GetType(), type)

This solves the issue for SQLite provider, where this test had failed:

https://github.com/aspnet/EntityFrameworkCore/blob/c2a39d700963f71c0035fa297aae6ec430578951/test/EFCore.Sqlite.FunctionalTests/SqliteMigrationSqlGeneratorTest.cs#L50

[ConditionalFact]
public virtual void DefaultValue_formats_literal_correctly()
{
    Generate(
        new CreateTableOperation
        {
            Name = "History",
            Columns =
            {
                new AddColumnOperation
                {
                    Name = "Event",
                    ClrType = typeof(string),
                    ColumnType = "TEXT",
                    DefaultValue = new DateTime(2015, 4, 12, 17, 5, 0)
                }
            }
        });

    AssertSql(
        @"CREATE TABLE ""History"" (
""Event"" TEXT NOT NULL DEFAULT '2015-04-12 17:05:00'
);
");
}

It tried to generate SQL literal for TEXT from DateTime and failed because of cast from DateTime to string (I guess this is necessary (for SQLite), because SQLite lacks proper date time type, right?).

[mirol-h]

@bricelam I've rebased branch onto release/3.0 and now lots of tests in SqlServer.FunctionalTests fail (e.g. Query.AsTrackingSqlServerTest.Applied_to_projection or Query.QueryNavigationsSqlServerTest.Project_single_scalar_value_subquery_in_query_with_optional_navigation_works).

Did I do something wrong, or is it something else?

[bricelam]

Almost certainly unrelated. (Re-running the CI)

[mirol-h]

@bricelam It's been some time, since I've rebased the branch on top of release/3.0. What's the current state of this PR (and #16960)? Is anything else required from me? In terms of 3.0 release, what are the odds at the moment that these PRs get merged?

[bricelam]

You’re good. I’m just slow. I’ll get it into the 3.0.0 release.