.Net: [MEVD] Map DateTime to timestamptz on PostgreSQL#13514
.Net: [MEVD] Map DateTime to timestamptz on PostgreSQL#13514roji wants to merge 1 commit intomicrosoft:mainfrom
Conversation
adamsitnik
left a comment
There was a problem hiding this comment.
@roji PTAL at my comment. Thanks!
| /// </para> | ||
| /// </remarks> | ||
| /// <exception cref="NotSupportedException">Thrown if the <paramref name="storeType"/> is not a supported value.</exception> | ||
| public static TProperty WithStoreType<TProperty>(this TProperty property, string storeType) |
There was a problem hiding this comment.
"timestamp without time zone" is quite long magic string. And we don't expose it as any public const.
I have few alternative ideas and I would love to hear you thoughts on it @roji :
StoreAsTimestamp(bool withTimezone = true)
- rename the method to
StoreAsTimestamp(orWithTimestamp) - introduce an optional boolean parameter called
withTimezonewith default value set totrue
The usage would be following:
- property.WithStoreType("timestamp");
+ property.StoreAsTimestamp();
- property.WithStoreType("timestamp without time zone");
+ property.StoreAsTimestamp(withTimezone: false);StoreAsTimestamp and StoreAsTimestamptz
As an alternative, we could have two separate methods called StoreAsTimestamp and StoreAsTimestamptz. This perhaps would be ever better, as I assume the caller needs to know what are the differences between these two since he got here?
WithStoreType(NpgsqlDbType dbType)
Or keep the WithStoreType name and accept NpgsqlDbType?
- property.WithStoreType("timestamp");
+ property.WithStoreType(NpgsqlDbType.Timestamp);
- property.WithStoreType("timestamp without time zone");
+ property.WithStoreType(NpgsqlDbType.TimestampTz);I know it would expose public API that takes type specific to Npgsql, but what are the chances that we ever are going to use a different driver?
And it would allow for customizing other db types as well.
There was a problem hiding this comment.
Thanks @adamsitnik, here are some thoughts.
The point here is that the WithStoreType() API could be usable to set any PostgreSQL type, not just "timestamp without time zone". PostgreSQL (like any database) has its list of well-known types (bigint, text, timestamptz, uuid...); if we want to allow users to specify other types, then this API allows us to do that tomorrow without adding any new APIs (whereas StoreAsTimestamp() works for a single type only).
Accepting NpgsqlDbType enum is indeed better in that sense, but... at least in theory you can also create custom types in PostgreSQL: your own enum type, or maybe you install a PG extension that adds another type which I don't even know (the PG type system is very open/extensible). This is indeed a bit tricky; it's generally not enough just to give a store type - we can stick it in the CREATE TABLE definition easily enough, but then the PG provider also needs to know how to generate literals (inside filter expressions) or parameters in later operations. But I wouldn't want to artificially limit the API to only accept the well-known types listed in NpgsqlDbType, and thereby exclude all other types.
I'll also add that this is how databases APIs generally work: EF has a very similar API that just accepts a string for the database type, and it's the user's responsibility to know what to put there. Also when you're creating a table in raw SQL, you need to know the type for each column. So I think this is aligned with other .NET database APIs and isn't too bad.
DateTime now maps to the PG timestamptz type by default, which represents a UTC timestamp; this aligns with Npgsql and EF Core.
However, now that we have property annotations, I've also added a new
WithStoreType()that allows the user to specify a database type explicitly. For now, this is only supported for settingtimestamp with time zone(short:timestamp), which is the other PG date/time type, for unspecified timestamps (timezone is unknown or implicitly assumed). This allows users who do want to store non-UTC timestamp to do so.Closes #10641