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

MigrationsSqlGenerator: DefaultValue formats default value properly #16909

Merged
merged 2 commits into from
Aug 13, 2019

Conversation

mirol-h
Copy link

@mirol-h mirol-h commented Aug 2, 2019

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

@dnfclas
Copy link

dnfclas commented Aug 2, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than my one comment.

@bricelam bricelam self-assigned this Aug 2, 2019
@mirol-h
Copy link
Author

mirol-h commented Aug 3, 2019

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
Copy link
Author

mirol-h commented Aug 6, 2019

@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
Copy link
Contributor

bricelam commented Aug 6, 2019

Almost certainly unrelated. (Re-running the CI)

@mirol-h
Copy link
Author

mirol-h commented Aug 10, 2019

@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
Copy link
Contributor

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

…pe of the column, for which the default value should be generated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants