-
-
Notifications
You must be signed in to change notification settings - Fork 689
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
[FEATURE]: "Transparent" Mode for Datetime Handling compatible with the underlying driver (timezone) #1626
Comments
Hi, Basically, if I map the date column the following way, it will revert to driver default behavior and comply to the connection timezone configuration: testdate.date_mode_date.mapFromDriverValue = (value: any) => new Date(value)
testdate.date_mode_date.mapToDriverValue = (value: any) => value With that in mind, I believe that adding the "transparent" or "driver" mode only involves the following changes in `mysql-core/columns/datetime.ts" file : |
I would be pleased to provide a PR however I'm not sure how the tests are organized. |
I don't this will be necessary because we'll start bypassing the driver's default behavior. Which means Edit: If the way I did it in that PR is approved, you can probably take a look at the implementation there and submit one for mysql2 or others. |
I don't know much about Postgres and am not sure if you are facing the same problem as with MySql (could be). |
On that PR, I basically disabled any mapping made by the driver. That means, that whatever is in the database is what drizzle will receive. MySql is a little different in the sense that it doesn't store timezone information. I guess we shall see if the drizzle team likes that approach, then you could submit a PR with those changes. I could do it too. |
driverI have the feeling that by driver you mean "Drizzle"? When I'm using the term "driver" I mean the underlying driver (ie: MySql2)? "string" formatThe problem is that, even though it looks similar at first sight, it is not the same thing to have the date in your prCorrect me if I'm wrong but it looks to me like your PR constitutes a breaking change. |
No, by driver I mean (in your case) Mysql2. I think the way I'm handling dates makes strings only necessary when microsecond precision is necessary (Date objects are not that precise). My PR is not breaking changes, is fixing a lot of reported bugs. The idea of drizzle manipulating the types for you makes a lot of sense, after all it's an Object Relational Mapper. |
Having to convert dates into strings before updating your database and strings into dates after retrieving data from a database is not really a great experience. How can there be a lot of reported bugs on date handling? If they are that many bugs related to dates, people have certainly found a workaround. In that respect, fixing bugs sometimes involves breaking changes. We will have to disagree here. The idea of drizzle manipulating the values returned by the underlying driver does not make any sense unless for additional optional features. Those drivers let you already query SQL data and have already a solution to those timezone problems. |
The |
Of course, I would be fine with that change but, because it was clearly said (see screenshot above) that UTC was the behavior of the date mode, changing the behavior would constitute a breaking change in my book. I have nothing against a Breaking change. This is certainly more elegant than having a third mode. However it should be clearly explained and documented. |
Yes, I implemented that PR. |
I just ran a quick test in a mysql database: const table = mysqlTable("test_Dates", {
id: serial("id").primaryKey(),
date1: datetime("date1", { mode: "date", fsp: 3 }).default(
sql`current_timestamp`,
),
date2: datetime("date2", { mode: "string", fsp: 3 }).default(
sql`current_timestamp`,
),
date3: timestamp("date3", { mode: "date", fsp: 3 }).default(
sql`current_timestamp`,
),
date4: timestamp("date4", { mode: "string", fsp: 3 }).default(
sql`current_timestamp`,
),
});
const parse1 = table.date1.mapFromDriverValue;
const parse2 = table.date2.mapFromDriverValue;
const parse3 = table.date3.mapFromDriverValue;
const parse4 = table.date4.mapFromDriverValue;
table.date1.mapFromDriverValue = (val: any) => {
console.log(val);
return parse1(val);
};
table.date2.mapFromDriverValue = (val: any) => {
console.log(val);
return parse2(val);
};
table.date3.mapFromDriverValue = (val: any) => {
console.log(val);
return parse3(val);
};
table.date4.mapFromDriverValue = (val: any) => {
console.log(val);
return parse4(val);
};
const created = await db.execute(sql`CREATE TABLE IF NOT EXISTS ${table} (
id serial primary key,
date1 datetime(3) default current_timestamp(3),
date2 datetime(3) default current_timestamp(3),
date3 timestamp(3) default current_timestamp(3),
date4 timestamp(3) default current_timestamp(3)
)`);
console.log(created);
const inserted = await db.insert(table).values({});
console.log(inserted);
console.log(await db.select().from(table));
await db.execute(sql`drop table if exists ${table}`); I got correct results: [
{
id: 1,
date1: 2023-12-17T05:18:26.446Z,
date2: "2023-12-17 05:18:26.446",
date3: 2023-12-17T05:18:26.446Z,
date4: "2023-12-17 05:18:26.446",
}
] |
What timezone did you configure at the connection level ? I had not paid that much attention to your example but you don't even write in the database which only displays half of the story. But still, write some data even manually in the mySql table, set the timezone at the connection level (ie: timezone: "+03:00") and compare the date retrieved by a mysql2 query and a drizzle query. I will try to come up with a prog that highlights the problem. |
Changing the timezone would complicate you even more IMO, do you apply the change when writing, reading, both? The current behavior is to write all dates to the DB in UTC. Since that's how they're represented inside a Date object, the conversion is trivial. Also, datetime in MySql doesn't store any information of the timezone, making it more error prone to attempt to store any other timezone other than UTC. const exampleDate = new Date("2002-03-23T10:11:12.123Z");
const inserted = await db.insert(table).values({
date1: exampleDate,
date2: exampleDate.toISOString().replace("T", " ").replace("Z", ""),
date3: exampleDate,
date4: exampleDate.toISOString().replace("T", " ").replace("Z", ""),
});
console.log(inserted);
console.log(await db.select().from(table)); And this was the result, which is correct: [
{
id: 1,
date1: 2002-03-23T10:11:12.123Z,
date2: "2002-03-23 10:11:12.123",
date3: 2002-03-23T10:11:12.123Z,
date4: "2002-03-23 10:11:12.123",
}
] |
Starting from your program, I made a couple of changes. import { sql } from "drizzle-orm"
import { mysqlTable, int, varchar, datetime, serial, timestamp } from "drizzle-orm/mysql-core"
import { drizzle } from "drizzle-orm/mysql2"
import mysql from "mysql2/promise"
// If you don't want to configure any timezone and rely on default driver behavior,
// just set connectionTimezone to an empty object
const connectionTimezone = {timezone: getTimeZone()}
const connection = await mysql.createConnection({ uri: process.env.DATABASE_URL, ...connectionTimezone })
const db = drizzle(connection)
const table = mysqlTable("test_Dates", {
id: serial("id").primaryKey(),
description: varchar("description", { length: 255 }),
date1: datetime("date1", { mode: "date", fsp: 3 }).default(sql`current_timestamp`),
date2: datetime("date2", { mode: "string", fsp: 3 }).default(sql`current_timestamp`),
date3: timestamp("date3", { mode: "date", fsp: 3 }).default(sql`current_timestamp`),
date4: timestamp("date4", { mode: "string", fsp: 3 }).default(sql`current_timestamp`),
})
// drop the table at start so that we can check the table after each run
await db.execute(sql`drop table if exists ${table}`)
const created = await db.execute(sql`CREATE TABLE IF NOT EXISTS ${table} (
id serial primary key,
description varchar(255),
date1 datetime(3) default current_timestamp(3),
date2 datetime(3) default current_timestamp(3),
date3 timestamp(3) default current_timestamp(3),
date4 timestamp(3) default current_timestamp(3)
)`)
// Current datetime
const myDate = new Date()
const myDateStr = toLocaleISOString(myDate)
// Insert a row using Drizzle
const inserted = await db
.insert(table)
.values({ description: "inserted with drizzle", date1: myDate, date2: myDateStr, date3: myDate, date4: myDateStr })
// Insert a row directly with mysql2
connection.execute(`insert into test_Dates (description, date1, date2, date3, date4) values (?, ?, ?, ?, ?)`, [ "inserted with mysql2", myDate, myDateStr, myDate, myDateStr])
console.log('Info')
console.log({ current_timezone: getTimeZone(), connectionTimezone, myDate, myDateStr })
console.log("Select with Drizzle")
console.log(await db.select().from(table))
console.log("select with raw sql")
const rows = await connection.query(`select * from test_Dates`)
console.log(rows[0])
/**
* Converts a Date object to a localized ISO string representation.
* @param date - The Date object to convert.
* @returns The localized ISO string representation of the Date object.
*/
function toLocaleISOString(date: Date) {
const pad = (num: number) => num.toString().padStart(2, "0")
const year = date.getFullYear()
const month = pad(date.getMonth() + 1) // Months are 0-indexed in JavaScript
const day = pad(date.getDate())
const hours = pad(date.getHours())
const minutes = pad(date.getMinutes())
const seconds = pad(date.getSeconds())
return `${year}-${month}-${day}T${hours}:${minutes}:${seconds}`
}
/**
* Returns the current time zone offset in the format "+HH:MM" or "-HH:MM".
* @returns {string} The current time zone offset.
*/
function getTimeZone() {
const offset = new Date().getTimezoneOffset(),
o = Math.abs(offset)
return (
(offset < 0 ? "+" : "-") +
("00" + Math.floor(o / 60)).slice(-2) +
":" +
("00" + (o % 60)).slice(-2)
)
} |
Let me run your example and share some thoughts |
With how drizzle currently works, people having a database where dates are not stored in UTC (whether this is a good thing or not) have the following options:
testdate.date_mode_date.mapFromDriverValue = (value: any) => new Date(value)
testdate.date_mode_date.mapToDriverValue = (value: any) => value
I believe Drizzle current way is not correct but still works as long as you are having dates in UTC format in your database. |
I am currently maintaining the timeclock app in my company, the app is used to track the employee's hours. Our field technicians have to go out of state and into different timezones sometimes. In our opinion, managing timezone per connections would have had so many foot guns, especially because the server will maintain a pool of connections, we decided to keep the connection in UTC and manage the timezone changes ourselves. It is surprisingly easy, we have this one helper in the codebase that we use in the client, both in the web app and the mobile apps: export function removeTimezone(
time: Date,
offsetHrs: number = new Date().getTimezoneOffset() * 60 * 1000
) {
return new Date(time.valueOf() - offsetHrs);
} I submitted the UTC PR because it would represent an unique/centralized way of storing dates in Mysql (taking into account that MySql doesn't store timezone information). Without sounding pedantic (hopefully), and without knowing your application's requirements/features, you could envision in your application a similar helper, where you give it the offset and it would store the date in your datebase with the offset that you please. And you'd be working with Dates the whole time (as we do). You've also noticed that I haven't said that drizzle shouldn't support this. My opinion is that it shouldn't, but it will be the drizzle team to decide. |
Why would I do anything like what you are suggesting when the driver works perfectly well perfectly for the scenario you are describing ant the one I am describing as well? The Timezone configuration tells the driver in which Timezone dates are stored in the database. Period. I'm rather surprised by your opinion that Drizzle should not support a proven approach to timezone handling (on a technical standpoint - functional issues are something else) . Typeorm relies on Mysql2, Kysely relies on MySql2, like Knex and most tools on top of MySql2. Other drivers like maria implements this same timezone approach as well. Unless you are saying storing dates in a format different than UTC is WRONG and people using my tool should not be able to do that, I can't see why Drizzle would diverge and be incompatible with all those tools. |
That is just my opinion. In any case, it seems like there will be an option to add an offset to the options per column. See this comment on my PR. Edit: I didn't see an issue tracking that, not sure if you'll like to submit it? |
Of course, all opinions are respectable, but I'd be lying if I didn't say I find it completely illogical. Honestly, I don't think adding an offset is a good idea. MySql2 is already doing this. I can't see any valuable reason to do, another way, exactly the job that the underlying driver is doing. Most ORM and Query Builders on top of MySql2 are just doing that. And it works. One could argue that by doing this, it will make up for the initial design error in a more elegant way than adding a third mode. Not completely wrong. However, I'm not really fond of having even more date manipulation on top of the current one. My proposal is plain simple a "driver" or "transparent" mode that let's the driver do it's things. That would ensure that if you are coming from mysql2, typeorm, knex and many other tools that you won't face any weird date problems. This is not the most elegant approach to have 3 modes but the best approach for not breaking compatibility. I'm talking of MySql only. I don't know how dates in Postgres are working and I don't know how the drivers in PG ecosystem usually handle TZ conversion. I heard that PG dates include tz information. It must certainly change things. However, it would be surprising if they were not already proposing a valid solution. I'm not part of the Drizzle team. But if I were, I would take time to reason about date handling, if they took it the wrong way or not and maybe rethink the approach. I can't see why all the other tools would be wrong. |
Alright, I'll explain why I don't think the approach you suggest is the best. Now, just please remember that this is just some guy-on-the-internet's opinion, IDK what would the drizzle team think about this.
An offset configuration on the column, would solve all the problems, it would give you the flexibility to change your setting per column and will make sure your types are not lying to you. This problem is not only present for dates, you simply cannot properly represent some mysql fixed-point types (like decimal) in javascript due to the inherit flaws of floating point numbers. Some drivers could give you back the strings, others could just give you a number. Then there are bigints, some drivers might give you strings, others might give you number and others might give you js Bigints. An user should, per column, be able to select what typescript type he'll like, and this is only possible if the ORM does the mapping. That's what the M stands for. |
Yes, no problem. Your comments are well taken. Those are interesting points but let me answer to them:
The problem of an offset configuration is that it does not solve the problem of DST. How do you handle the fact that part of the year you are GMT+1 and another part or the year, GMT+2 ? It's not possible with an offset. That's what the "local" mode is for when you use MySql2 configuration. By the way, I made a mistake in my previous posts, 'local' is the default MySql2 driver option and not 'Z' (UTC). Regarding the last part of you post, I'm not sure exactly what you mean. All the drivers I have been using are providing data that I can use in the context of typescript. There is a difference between having a string or a numeric and having a different value than what you get when using the driver directly with the same query. |
You can use mysql2 if you want to connect to planescale, but if you're in a serverless environment, you'll need to use the planetscale severless driver. That driver will return your dates as a string. I'm not saying it's impossible, I just don't think it should be the way to solve this problem. Just because the driver had to implement mapping to a Date object, probably because their users were asking for the ergonomics, doesn't mean it's an ORM. Keep in mind the Anyway, Let's just see what they say. |
Yes, you are right regarding the necessity to map the driver value to a certain type. That being said, if the driver returns a string do whatever is fine to provide the additional possibility to get it directly as a date object. If the driver returns a string do a best effort (I have no specific expectations anyway). If the driver returns a date, do not change it's value, If the driver does not provide a date by itself, it means I have anyway an internal approach to handle dates. And if I move to Drizzle, I can anyway create a custom column to implement my internal approach. Yes, MySql does not store Timezone information with dates. That's why the Timezone configuration is an essential feature if you map MySql dates into JS dates and vice-versa. Either you decide to just return string without any interpretation or you need to provide a way to configure the Timezone. Implementing an offset is not enough. You will need to restart your application everyday to take the DST into account. If they want to the offset way, they should provide a "local" option that will apply the proper offset depending on the DST. |
Any update on this matter ? I'm currently using this approach on each table to work around the problem. It's somewhat ugly though: ;[table.created, table.modified, ].forEach((col) => {
col.mapFromDriverValue = (value: any) => new Date(value)
col.mapToDriverValue = (value: any) => value
}) |
For us the above workaround isn’t enough - it will only work when the local timezone matches the db timezone. Our backend runs on UTC but the DB is Sydney time (for legacy reasons). Drizzle only gets the raw string back from the driver so calling We’ve created a custom type that reads/writes the time into the expected timezone (using @date-fns/tz). It’s a bit of a gotcha going forward to remember to never to use the standard |
@mlev Yes, but the date returned will depend on the driver TZ configuration. Can't you configure the timezone at this (driver) level? @Angelelz Why not implementing the requested "transparent" mode for those who would like the dates to be retuned exactly like they would be with a SQL query made using MySql2 directly ? Seems a pretty legit request to me. |
@FredericLatour that wasn't the case when I was testing - it seems drizzle bypasses the driver to return a string for all date types. I'm creating the connection using I tracked it down to this code in the drizzle session: drizzle-orm/drizzle-orm/src/mysql2/session.ts Lines 76 to 78 in c8359a1
If I commented out that line then the value passed to |
Describe what you want
Context
I've been working with Drizzle for a possible adoption and have noticed that Drizzle is manipulating date values in a way that doesn't align with the behavior of the underlying MySql2 driver or other ORM/query builders that rely on it.
Indeed, MySql2 makes it possible to configure the timezone at the connection/pool level (other drivers, at least in MySql space offer a similar option): Connection Options
This timezone configuration option makes it possible to manage scenarios where dates in a MySQL database are not stored in the UTC format (which is more frequent that one may imagine).
Essentially, this option ensures that all dates being sent to MySQL are converted from JavaScript's internal UTC format to the configured timezone. Similarly, all dates retrieved from MySQL are first converted back to UTC, before being transformed into JavaScript date objects.
This is crucial because the JavaScript Date constructor expects an input string to be in UTC format. Therefore, this feature ensures seamless conversion and compatibility between MySQL and JavaScript date formats, regardless of the timezone configuration.
Problem
I've conducted several tests attempting to modify the timezone configuration during the creation of a MySql2 connection. However, these changes didn't seem to have any effect. Without having looked into the source code, it appears as though this option is being bypassed or short-circuited by Drizzle.
In my opinion, it would have been preferable if Drizzle didn't manipulate dates and leave the driver data untouched. Many SQL drivers, including MySql2, already have a built-in solution for handling timezones.
A Universally Appealing Proposed Solution :)
That being said, given Drizzle's current approach to date handling, I believe a potential solution could be the introduction of a "transparent" mode along the "date" and "string" modes. This mode would allow users to opt out of Drizzle's date manipulation and rely on the underlying driver's date handling instead.
This new mode would provide a non breaking soloution that would make it possible to adopt drizzle in a smooth fashion for those having existing apps/database that rely on non utc stored dates.
Hopefully, this is a straigthforward change that will be accepted.
The text was updated successfully, but these errors were encountered: