Redo timestamp handling #1992
Redo timestamp handling #1992
Conversation
roji
force-pushed
the
UntilTheEndOfTime
branch
6 times, most recently
from
bb36860
to
be01865
Compare
| if (method == SystemClock_GetCurrentInstant) | ||
| { | ||
| return NpgsqlNodaTimeTypeMappingSourcePlugin.LegacyTimestampBehavior | ||
| ? _sqlExpressionFactory.AtTimeZone( |
There was a problem hiding this comment.
So previously there was always a timezone conversion here?
There was a problem hiding this comment.
Not exactly... The old (and now legacy) translation for GetCurrentInstant was NOW() AT TIME ZONE 'UTC' (with constant UTC), which returns a UTC timestamp but typed as timestamp without time zone, since that is how we were mapping Instant. So no timezone conversion - just a PG type conversion.
|
|
||
| // We support getting a local DateTime via DateTime.Now (based on PG TimeZone), but there's no way to get a non-UTC | ||
| // DateTimeOffset. | ||
| nameof(DateTime.Now) when type == typeof(DateTimeOffset) |
There was a problem hiding this comment.
member.name DateTime.Now can be coming from DateTimeOffset? How does this work?
There was a problem hiding this comment.
Will clean up (this is just string name matching so no bug or anything)
There was a problem hiding this comment.
Didn't end up getting cleaner 🤣
| storeType = "INT"; // Shorthand that looks better in SQL | ||
| var storeType = sqlUnaryExpression.TypeMapping.StoreType switch | ||
| { | ||
| "integer" => "INT", |
There was a problem hiding this comment.
Only because it's been this way... There's already lots of inconsistency around upper/lower case in type and function names... We can look at this separately (don't want to cause even more test changes in this PR :/)
| { | ||
| var dateTime = (DateTime)value; | ||
|
|
||
| if (!NpgsqlTypeMappingSource.LegacyTimestampBehavior && dateTime.Kind == DateTimeKind.Utc) |
There was a problem hiding this comment.
I'm definitely expecting us to have to loosen this restriction in npgsql. But again, let's wait for the complaints :)
There was a problem hiding this comment.
We'll see :)
For example, Where(b => b.TimestampTzColumn == new DateTime(2012, 1, 1)) compares a timestamptz (so UTC timestamp) to an unspecified timestamp. This really makes no sense, and the user really should change to comparing against new DateTime(2012, 1, 1, 0, 0, 0, DateTimeKind.Utc). I do think we're doing our users a service by enforcing this, but there may some some cases out of their control which could cause them to block.
Am indeed curious to see the fallout.
| #endif | ||
|
|
||
| static NpgsqlNodaTimeTypeMappingSourcePlugin() | ||
| => LegacyTimestampBehavior = AppContext.TryGetSwitch("Npgsql.EnableLegacyTimestampBehavior", out var enabled) && enabled; |
There was a problem hiding this comment.
Not a big fan seeing this compat switch twice, it will probably never happen but you could technically have value drift between the moment this one is initialized in the static constructor and the one in NpgsqlTypeMappingSource.
There was a problem hiding this comment.
Technically true, but it's very very hard to imagine a non-intentional scenario where this can happen... In general, an application flipping around an appcontext switch is not a normal scenario, and I suspect various other .NET components check switches in multiple parts of the codebase rather than capturing it once...
Do you think we should make the flag a public API in Npgsql? Seems a bit much to me...
test/EFCore.PG.FunctionalTests/Query/ManyToManyQueryNpgsqlFixture.cs
Outdated
Show resolved
Hide resolved
roji
force-pushed
the
UntilTheEndOfTime
branch
from
b4b4fe8
to
7062d6c
Compare
Closes #1970
Closes #873
Closes #473
Closes #1974
Closes #1983