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

Regression in EF Core 7? Nullable DateTime to Non-nullable DateTime conversion bug when using SqlServer type DateTime instead of DateTime2 #29239

Closed
eero-dev opened this issue Sep 30, 2022 · 6 comments

Comments

@eero-dev
Copy link

eero-dev commented Sep 30, 2022

File a bug

Given the code below, EF Core 7 seems to be generating invalid SQL when compared to EF Core 6
This seems to only happen when specifying that the database type for the field is 'datetime' instead of using 'datetime2', causing most of our old migrations to fail (made with EF Core 6).

I found similar issues but they were from EF Core 3.1.0, #12797

Include your code

DatetimeBugEfCore.zip

Include stack traces

The conversion of a varchar data type to a datetime data type resulted in an out-of-range value.
The statement has been terminated.

EF Core 6 Generated SQL:

ALTER TABLE [Blogs] ALTER COLUMN [TimeStamp] datetime NOT NULL;	
ALTER TABLE [Blogs] ADD DEFAULT '0001-01-01T00:00:00.000' FOR [TimeStamp];

EF Core 7 Generated SQL:

UPDATE [Blogs] SET [TimeStamp] = '0001-01-01T00:00:00.000' WHERE [TimeStamp] IS NULL;
ALTER TABLE [Blogs] ALTER COLUMN [TimeStamp] datetime NOT NULL;
ALTER TABLE [Blogs] ADD DEFAULT '0001-01-01T00:00:00.000' FOR [TimeStamp];

Include provider and version information

EF Core version: 7.0.100-*
Database provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer): Microsoft.EntityFrameworkCore.SqlServer
Target framework: (e.g. .NET 5.0): NET 7
Operating system: Windows 11
IDE: (e.g. Visual Studio 2019 16.3) VS 2022 17.4 Preview 2

Executed SQL against Sql Server 2019

@eero-dev eero-dev changed the title Nullable DateTime to Non-nullable DateTime conversion bug in EF7 RC1 Nullable DateTime to Non-nullable DateTime conversion bug in EF7 Sep 30, 2022
@eero-dev eero-dev changed the title Nullable DateTime to Non-nullable DateTime conversion bug in EF7 Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 6 Sep 30, 2022
@eero-dev eero-dev changed the title Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 6 Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 Sep 30, 2022
@eero-dev eero-dev changed the title Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 when using SqlServer type DateTime instead of DateTime2 Sep 30, 2022
@eero-dev eero-dev changed the title Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 when using SqlServer type DateTime instead of DateTime2 Regression? Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 when using SqlServer type DateTime instead of DateTime2 Sep 30, 2022
@eero-dev eero-dev changed the title Regression? Nullable DateTime to Non-nullable DateTime conversion bug in EF Core 7 when using SqlServer type DateTime instead of DateTime2 Regression in EF Core 7? Nullable DateTime to Non-nullable DateTime conversion bug when using SqlServer type DateTime instead of DateTime2 Sep 30, 2022
@roji roji self-assigned this Sep 30, 2022
@roji
Copy link
Member

roji commented Sep 30, 2022

This is a result of #21765: when making a column non-nullable, we now generate an UPDATE to convert all nulls to the CLR default value. But the CLR default for DateTime is in year 1, which is out of range for SQL Server datetime.

Note that previously the migration succeeded, but left the column configured with an out-of-range default. That means that whenever a new row was inserted without explicitly specifying the datetime, the INSERT would fail for the same reason. So while this is technically a regression, it is one only in the sense that applying the migration now fails where it previous succeeded, but that migration was invalid anyway. We can still consider this a regression, since if the user always provides a value, things did work previously.

As a workaround for 7.0, we could simply identify datetime in the migrations SQL generator, and provide a different default for it (probably 1753-01-01T:00:00:00, the minimal value). For 7.0, we can add a hook on the type mappings to return the default value (do we have other scenarios where this may be useful?).

@ajcvickers
Copy link
Contributor

@roji @bricelam Is this a place where the correct thing is for the migration to be edited? It's not clear that using a different "default" is really any better here. The fundamental problem is that .NET DateTime does not really map correctly to SQL Server datetime. Whatever we do, problems will always remain with this mapping. That's why we always default to datetime2 mappings.

@roji
Copy link
Member

roji commented Sep 30, 2022

Could be. I do think it makes sense to use the "lowest possible database value" as a default (following the .NET behavior); it's also what we do for datetime2, and it just so happens that the minimal CLR value is the same as the minimal database value.

For another data point... PostgreSQL timestamp has a range of 4713 BC to 294276 AD (the range is that big because the precision, is lower, 1 microsecond), but also supports special infinity and -infinity values. Npgsql (by default) maps DateTime.MinValue and DateTime.MaxValue to -infinity and infinity, with the rationale that DateTime.MinValue is used as a "timestamp that's smaller than all others", rather than an actual point of time that we want to represent. So the Npgsql type mapping for timestamp generates a literal of -infinity when it sees DateTime.MinValue, and the migration to make a timestamp column non-nullable emit the following (before the UPDATE change):

ALTER TABLE "Blogs" ALTER COLUMN "DateTime" SET NOT NULL;
ALTER TABLE "Blogs" ALTER COLUMN "DateTime" SET DEFAULT TIMESTAMPTZ '-infinity';

@ajcvickers
Copy link
Contributor

@eero-dev The correct thing to do here is to provide a default value that is valid for the SQL Server datetime type. For example:

        builder.Entity<Blog>()
            .Property(x => x.TimeStamp)
            .HasColumnType("datetime")
            .HasDefaultValue(new DateTime(1753, 1, 1));

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 4, 2022

Or even

    HasDefaultValue(SqlDateTime.MinValue)

@eero-dev
Copy link
Author

eero-dev commented Oct 5, 2022

Yeah that is the solution I was leaning on, editing the existing migration to replace the defaultValue and adding the HasDefaultValue for future migrations. Thank you for the solutions.

@eero-dev eero-dev closed this as completed Oct 5, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 5, 2022
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

4 participants