-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[6.0] Code literal support for DateOnly/TimeOnly #26159
Conversation
@@ -275,12 +276,52 @@ public override void String_literal_generated_correctly() | |||
Test_GenerateSqlLiteral_helper(GetMapping("varchar(max)"), "Text", "'Text'"); | |||
} | |||
|
|||
[ConditionalFact] | |||
public virtual void DateOnly_code_literal_generated_correctly() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: we should probably improve type mapping testing, see #26158
@@ -130,6 +130,7 @@ public CSharpHelper(ITypeMappingSource typeMappingSource) | |||
{ typeof(byte), (c, v) => c.Literal((byte)v) }, | |||
{ typeof(byte[]), (c, v) => c.Literal((byte[])v) }, | |||
{ typeof(char), (c, v) => c.Literal((char)v) }, | |||
{ typeof(DateOnly), (c, v) => c.Literal((DateOnly)v) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem sustainable that we add something here whenever any provider supports any new type. In this case, I can see that ultimately it does belong here since its essentially a new primitive, even though it's not supported by SQL Server yet. So, I just want to clarify that this kind of change isn't needed and that providers can do whatever is necessary without needing a change in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - I do think this makes sense for built-in BCL types, but type mappings can also override GenerateCodeLiteral. We can consider moving some of the things from here to the mappings.
Approved for EF Core 6.0 |
Fixes #26156
Description
EF Core 6.0 adds support for the new .NET DateOnly/TimeOnly types, but code literal generation support was left out.
Customer impact
Customers cannot scaffold migrations which seed DateOnly/TimeOnly data
How found
Customer
Regression
No, DateOnly/TimeOnly support is new in EF Core 6.0
Testing
Test for this scenario added in the PR.
Risk
Very low, the fix is trivial and affects migration scaffolding only (no runtime impact)