[Pg] Fix: all datetime mappings #1659

Angelelz · 2023-12-16T17:50:04Z

This PR will close #806, close #971, close #1176, close #1185, close #1407 and close #1587.
Most are the same. The problem is postgres.js, it didn't have a clear way of replace the default parsing into a Date object after the client have been instantiated.
I spent some time researching and found a solution.
Although it looks like postgres.js is still trimming the microsecond precision, even after bypassing the parsing.

I'll look into it further and possibly submit a new PR.

Added tests on all/most drivers that use postgres.

Edit: I found another workaround for the issue with trimming the microsecond precision. I added the commit.

Angelelz · 2023-12-17T01:34:32Z

This will also close #1061

Hebilicious · 2023-12-19T15:09:57Z

Just hit this issue, thanks for looking into it @Angelelz

Closing 7 issues in one 🚀 🚀 🚀 🚀 🚀 🚀 🚀

jakeleventhal · 2023-12-22T15:48:07Z

Am waiting for this to merge before giving a migration from prisma a test

AndriiSherman · 2023-12-27T15:16:22Z

@Angelelz, thanks for your PR. Are those changes compatible with the previous implementation?
For example, if the user already has timestamp columns with some data and upgrades to this version, will the selected data be retained in the same format?

Angelelz · 2023-12-27T15:53:10Z

@Angelelz, thanks for your PR. Are those changes compatible with the previous implementation? For example, if the user already has timestamp columns with some data and upgrades to this version, will the selected data be retained in the same format?

Before, the user was being lied to by the types when using the postgres.js driver. Drizzle was expecting to receive a string, and was reporting a string and not doing any mapping, but postgres was actually returning a Date. That's why so many issues were filed. With this PR the mapping made by postgres.js has been disabled, so drizzle maps correctly as the user requests with the mode.
We might need to add a warning on the new version, as the users might have implemented workarounds for this.

BTW, I see conflicts, I thought I resolved them all last night. Let me see if I can resolve here in GH.

ericodom · 2023-12-27T15:55:58Z

This is 100% what's happening...even if you set mode = "string" it returns a date object (but the type is a string). I'm having to do a quick transform from date -> string before returning results, just to get the data to sync with the types.

Really looking forward to this PR getting merged :-)

drizzle-orm/src/postgres-js/driver.ts

jakeleventhal · 2023-12-27T16:36:16Z

Does this resolve #1164

Angelelz · 2023-12-27T16:42:45Z

Does this resolve #1164

Based on this test added, if it passes then yes. But I think that test only runs when merging to beta or main. I didn't run it locally. Crucially, the mapping I did in timestamp.ts should be the fix.

jakeleventhal · 2023-12-27T16:49:31Z

8 issues resolved now

jakeleventhal · 2023-12-27T16:53:14Z

#1059 this one too @Angelelz

Angelelz · 2023-12-27T16:56:56Z

Does this resolve #1164

Based on this test added, if it passes then yes. But I think that test only runs when merging to beta or main. I didn't run it locally. Crucially, the mapping I did in timestamp.ts should be the fix.

@AndriiSherman Can you run the test on AWS locally to know for sure? I don't have an account, nor I want to create one lol

Angelelz · 2023-12-27T17:26:10Z

#1059 this one too @Angelelz

Actually, this might have already been fixed by this commit.

AndriiSherman · 2023-12-27T18:00:58Z

Does this resolve #1164

Based on this test added, if it passes then yes. But I think that test only runs when merging to beta or main. I didn't run it locally. Crucially, the mapping I did in timestamp.ts should be the fix.

@AndriiSherman Can you run the test on AWS locally to know for sure? I don't have an account, nor I want to create one lol

Yes, I wanted to test Serverless v2 anyway, so I'm going to run all tests for the AWS Data API and merge if successful. Also, @Angelelz, I guess mappings in the timestamp can be reverted. We will always get a string from the driver, as long as in both pg and postgres.js, we are receiving a raw response, which is always a string for dates. I think, in this case, it should be compatible with the previous version and still work as expected.

I also think that it may be a breaking change for someone who is doing custom mapping, so I guess we would need to make a minor release and not a patch release and add this to the docs. Maybe we can finally add release notes or tags to mark it somehow

jakeleventhal · 2023-12-27T19:52:24Z

@AndriiSherman related issue: #1686

AndriiSherman · 2023-12-28T10:27:21Z

I reverted the mappings to the way it was before, but kept the mapToDriverValue as it was dropping millisecond precision. The problem is that .toUTCString() method drops the millisecond and doesn't really add anything. See #1176

I guess you need to create another tests for timestamp with timezone:

First case(mode: string)

Insert date in string format with timezone in it
Select in string format and check that values are the same
Select as raw query and check that values are the same

Second case(mode: date)

Insert date as new Date()
Select as date and check that timezones are the same
Compare both dates(maybe by using .getTime())

Third case(mode: date) with timestamps inserted from different timezones:

Insert date as new Date() with one timezone
Insert date as new Date() with another timezone
Select both dates and compare both dates(maybe by using .getTime())

And a few more tests for just timestamp without timezone

First case(mode: string)

Insert date in string format without timezone
Select in string format and check that values are the same
Select as raw query and check that values are the same

Second case(mode: string)

Insert date in string format with timezone
Select as raw query and check that values are the same
Example: You have inserted 2023-12-28T09:29:43.824+03:00, You selected 2023-12-28 09:29:43.824. You need to trim T and timezone from the first date and check that values are the same

Third case(mode: date)

Insert date as new Date()
Select as raw query as string, make this as Date in UTC and compare both dates(maybe by using .getTime())
Compare both dates using orm mapping(maybe by using .getTime())

Angelelz · 2023-12-28T15:12:57Z

Hey Andrew,
I'll add the test, but when using mode: 'date', remember that the Date object doesn't store the timezone, it just stores the datetime as UTC, and just shows it to you in your local timezone.
There's very little test that can be done around timezones when using the Date object.
I'll add the tests later today and write some comments on the code for your reference.

Angelelz · 2023-12-29T03:38:43Z

@AndriiSherman I added the tests on all 5 pg drivers. the big problem is that I couldn't run the tests locally on AWS or Neon, because I don't have accounts on either of those. I basically guessed the types of the raw select when using db.execute.
I would suggest you to run them locally if you can, if not, let me know and I'll just erase them.

AndriiSherman

@Angelelz

integration-tests/tests/pg.test.ts

drizzle-orm/src/pg-core/columns/timestamp.ts

Angelelz · 2024-01-03T05:04:18Z

@AndriiSherman I have added the requested tests. I also decided to add the requested tests on the postgres.js.test.ts since that's the driver where I made the changes to the mappings.

AndriiSherman · 2024-01-03T12:17:19Z

@AndriiSherman I have added the requested tests. I also decided to add the requested tests on the postgres.js.test.ts since that's the driver where I made the changes to the mappings.

just left 1 comment

AndriiSherman

@Angelelz

AndriiSherman · 2024-01-04T12:41:40Z

integration-tests/tests/pg.test.ts

+	// 2, Select in string format and check that values are the same
+	const result = await db.select().from(table);
+
+	t.deepEqual(result, [{ id: 1, timestamp: '2022-01-01 04:00:00.123456+00' }]);


I think we don't need this specific test, because you have 2 proper tests with different timezones added

AndriiSherman · 2024-01-04T12:47:58Z

integration-tests/tests/pg.test.ts

+
+	// Please notice that postgres will transform the date to UTC and it saves it in UTC.
+	// even if set without time zone, just becuase it was provided a timezone offset, it will transform it to UTC
+	t.deepEqual(result.rows, [{ id: 1, timestamp_string: '2022-01-01 04:00:00.123456+00' }]);


As Postgres docs states

In a literal that has been determined to be timestamp without time zone, PostgreSQL will silently ignore any time zone indication. That is, the resulting value is derived from the date/time fields in the input value, and is not adjusted for time zone.

This means that when you insert a string with a timezone into a timestamp without time zone type, the timezone should be simply removed without any time conversions. For example, for 2022-01-01T02:00:00.123456-02, you should receive 2022-01-01 02:00:00.123456 in return

The reason why it happened - you have created timestamp(6) with time zone instead of timestamp(6)

AndriiSherman · 2024-01-04T12:48:12Z

integration-tests/tests/pg.test.ts

+	await db.execute(sql`
+		create table ${table} (
+					id serial primary key,
+					timestamp_string timestamp(6) with time zone not null


should be just timestamp(6)

AndriiSherman · 2024-01-04T12:48:45Z

integration-tests/tests/pg.test.ts

+	await db.execute(sql`
+		create table ${table} (
+					id serial primary key,
+					timestamp_string timestamp(3) with time zone not null


should be just timestamp(6)

AndriiSherman · 2024-01-04T13:01:31Z

@Angelelz, while you are fixing what I found, I'll start writing documentation for timestamps with explanations on how it should work with Postgres now. I want to make sure everybody will know exactly how we are handling it

Angelelz · 2024-01-04T15:19:42Z

@AndriiSherman sounds good, thank you for being so thorough. Will address these later today.

ggrantrowberry · 2024-01-04T18:42:43Z

Will this fix the issue where I have date columns in Postgres and Date objects that I am trying to insert but Drizzle makes me convert them to strings first?

Angelelz · 2024-01-04T23:36:27Z

@AndriiSherman You were totally right, I had made the mistake of setting the columns with time zone and thought that postgres were transforming them internally.

jakeleventhal · 2024-01-08T15:34:58Z

Nice work @Angelelz and thanks for the review @AndriiSherman

This one is really important for our team

AndriiSherman · 2024-01-09T09:11:47Z

This pull request will be merged into the beta and can be tested using drizzle-orm@beta before being merged into the latest. I will provide a detailed explanation of how it should work and will notify in every related issue that this pull request is closing

realmikesolo · 2024-01-22T16:36:07Z

This PR will also close 895

Angelelz added 4 commits December 16, 2023 01:50

[Pg] Patched Postgres.js driver to not parse the dates

826db5a

[Pg] Correct handling of the timestamps and timestamptz types

a21639b

[Pg] Added test in mostr drivers

8dab8a4

[Pg] Cleaned tests

064a252

Angelelz added the review-needed label Dec 16, 2023

This was referenced Dec 16, 2023

[FEATURE]: "Transparent" Mode for Datetime Handling compatible with the underlying driver (timezone) #1626

Open

Fix: placeholder on insert update #1663

Open

This was referenced Dec 17, 2023

[BUG]: Postgres insert into timestamp with time zone removes milliseconds #1061

Closed

[BUG]: defaultNow() in date mode should save new Date() #1148

Closed

Angelelz changed the title ~~Fix all datetime mappings~~ [Pg] Fix: all datetime mappings Dec 17, 2023

[Pg] [Postgres-js] fixed serializing timestamps as well

1b97252

AndriiSherman changed the base branch from main to beta December 27, 2023 15:04

Merge branch 'beta' into fix-all-datetime-mappings

46d6105

AndriiSherman reviewed Dec 27, 2023

View reviewed changes

drizzle-orm/src/postgres-js/driver.ts Show resolved Hide resolved

jakeleventhal mentioned this pull request Dec 27, 2023

[BUG]: update timestamp field (using AWS Data API) #1164

Open

jakeleventhal mentioned this pull request Dec 27, 2023

[BUG]:Unable to insert Date into Aws Postgres #1059

Closed

[Pg] Added requested additional tests

ce0c95f

AndriiSherman reviewed Jan 2, 2024

View reviewed changes

integration-tests/tests/pg.test.ts Outdated Show resolved Hide resolved

Merge branch 'beta' into fix-all-datetime-mappings

821548f

jakeleventhal reviewed Jan 2, 2024

View reviewed changes

drizzle-orm/src/pg-core/columns/timestamp.ts Show resolved Hide resolved

Added detailed tests

c7b07d1

Separated tests per cases as requested

c606ebe

AndriiSherman reviewed Jan 4, 2024

View reviewed changes

Fixed test without timezones

b29a5e1

AndriiSherman added ready-to-be-merged and removed review-needed labels Jan 8, 2024

AndriiSherman approved these changes Jan 8, 2024

View reviewed changes

AndriiSherman merged commit 8c0788a into drizzle-team:beta Jan 9, 2024
5 checks passed

This was referenced Jan 10, 2024

Fix jsonb inserts in PG when postgres.js is used as driver #1641

Closed

Fix: json and jsonb parsing in postgres-js #1785

Merged

AndriiSherman mentioned this pull request Jan 22, 2024

[BUG]: Drizzle Studio does not show correct value for timestamp #1147

Closed

Hebilicious mentioned this pull request Feb 19, 2024

[Bug]: date.getTime() is not a function (originating in oslo) -> but the core problem is that nodejs-adapter from Lucia doesn't convert expiresAt as a date lucia-auth/lucia#1424

Closed

gp27 mentioned this pull request Apr 21, 2024

[BUG]: Type error with postgresjs strings/dates in version 0.30 #1993

Open

gp27 mentioned this pull request Aug 24, 2024

[Pg] Fixed postgres-js serialization of Date objects #2839

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pg] Fix: all datetime mappings #1659

[Pg] Fix: all datetime mappings #1659

Angelelz commented Dec 16, 2023 •

edited

Loading

Angelelz commented Dec 17, 2023

Hebilicious commented Dec 19, 2023

jakeleventhal commented Dec 22, 2023

AndriiSherman commented Dec 27, 2023

Angelelz commented Dec 27, 2023

ericodom commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

Angelelz commented Dec 27, 2023 •

edited

Loading

jakeleventhal commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

Angelelz commented Dec 27, 2023

Angelelz commented Dec 27, 2023

AndriiSherman commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

AndriiSherman commented Dec 28, 2023

Angelelz commented Dec 28, 2023

Angelelz commented Dec 29, 2023

AndriiSherman left a comment

Angelelz commented Jan 3, 2024

AndriiSherman commented Jan 3, 2024

AndriiSherman left a comment

AndriiSherman Jan 4, 2024

AndriiSherman Jan 4, 2024

AndriiSherman Jan 4, 2024

AndriiSherman Jan 4, 2024

AndriiSherman commented Jan 4, 2024

Angelelz commented Jan 4, 2024

ggrantrowberry commented Jan 4, 2024

Angelelz commented Jan 4, 2024

jakeleventhal commented Jan 8, 2024

AndriiSherman commented Jan 9, 2024

realmikesolo commented Jan 22, 2024

[Pg] Fix: all datetime mappings #1659

[Pg] Fix: all datetime mappings #1659

Conversation

Angelelz commented Dec 16, 2023 • edited Loading

Angelelz commented Dec 17, 2023

Hebilicious commented Dec 19, 2023

jakeleventhal commented Dec 22, 2023

AndriiSherman commented Dec 27, 2023

Angelelz commented Dec 27, 2023

ericodom commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

Angelelz commented Dec 27, 2023 • edited Loading

jakeleventhal commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

Angelelz commented Dec 27, 2023

Angelelz commented Dec 27, 2023

AndriiSherman commented Dec 27, 2023

jakeleventhal commented Dec 27, 2023

AndriiSherman commented Dec 28, 2023

Angelelz commented Dec 28, 2023

Angelelz commented Dec 29, 2023

AndriiSherman left a comment

Choose a reason for hiding this comment

Angelelz commented Jan 3, 2024

AndriiSherman commented Jan 3, 2024

AndriiSherman left a comment

Choose a reason for hiding this comment

AndriiSherman Jan 4, 2024

Choose a reason for hiding this comment

AndriiSherman Jan 4, 2024

Choose a reason for hiding this comment

AndriiSherman Jan 4, 2024

Choose a reason for hiding this comment

AndriiSherman Jan 4, 2024

Choose a reason for hiding this comment

AndriiSherman commented Jan 4, 2024

Angelelz commented Jan 4, 2024

ggrantrowberry commented Jan 4, 2024

Angelelz commented Jan 4, 2024

jakeleventhal commented Jan 8, 2024

AndriiSherman commented Jan 9, 2024

realmikesolo commented Jan 22, 2024

Angelelz commented Dec 16, 2023 •

edited

Loading

Angelelz commented Dec 27, 2023 •

edited

Loading