Skip to content
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

fix(postgres): parse timestamp dates less than year 1000 #5071

Merged
merged 3 commits into from Jan 5, 2024

Conversation

Robert-Schirmer
Copy link
Contributor

Bug

Parsing a timestamp column in postgresql with a date less than year 1000 will create an invalid Date when using the forceUtcTimezone set to true.

Reproduction

Created test for reproduction, run without the fix to PostgresSqlConnection.ts.

Fix

Use the date parsing function from postgres-date that is used by default in pg for parsing dates but add the Z to force the utc timezone. new Date('0022-01-01 09:17:58.000Z') when parsing the timestamp column str creates an invalid date in js.

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6667656) 99.49% compared to head (d7d2bfa) 99.49%.

Additional details and impacted files
@@           Coverage Diff           @@
##              5.x    #5071   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files         217      217           
  Lines       14732    14733    +1     
  Branches     3576     3576           
=======================================
+ Hits        14657    14658    +1     
  Misses         71       71           
  Partials        4        4           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@B4nan
Copy link
Member

B4nan commented Jan 4, 2024

please target master branch, then once we merge, we can cherry pick to 5.x

@Robert-Schirmer
Copy link
Contributor Author

please target master branch, then once we merge, we can cherry pick to 5.x

Thats what I originally wanted to do but there seems to be a refractor of the PostgreSqlConnection in master, it will require two different fixes. How do you want to do that?

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few notes about the test

const something = orm.em.create(TimestampTest, { createdAtTimestamp: createdAt });
await orm.em.persistAndFlush(something);

const res = await orm.em.find(TimestampTest, something.id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not making any query, as that entity is already in the identity map

Suggested change
const res = await orm.em.find(TimestampTest, something.id);
const res = await orm.em.fork().find(TimestampTest, something.id);

import { PostgreSqlDriver } from '@mikro-orm/postgresql';

@Entity()
export class TimestampTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to export anything

Suggested change
export class TimestampTest {
class TimestampTest {

Comment on lines 23 to 25
await orm.schema.ensureDatabase();
await orm.schema.execute('drop table if exists timestamp_test');
await orm.schema.createSchema();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await orm.schema.ensureDatabase();
await orm.schema.execute('drop table if exists timestamp_test');
await orm.schema.createSchema();
await orm.schema.refreshDatabase();

Comment on lines 16 to 21
const orm = await MikroORM.init({
entities: [TimestampTest],
dbName: `mikro_orm_test_timestamp`,
driver: PostgreSqlDriver,
forceUtcTimezone: true,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should go into beforeAll hook, and the orm.close should go to afterAll, that way the connection will be closed even if the test fails (and the whole test suite wont get stuck, which can sometimes end with the CI running for 6 hours)

@B4nan
Copy link
Member

B4nan commented Jan 4, 2024

Thats what I originally wanted to do but there seems to be a refractor of the PostgreSqlConnection in master, it will require two different fixes. How do you want to do that?

Then I'd like to first see a PR for master/v6, keep this one targeting v5. Once we have it fixed in v6, we can merge this one, but I don't want to fix things only here.

@B4nan
Copy link
Member

B4nan commented Jan 4, 2024

Btw, just curious, what kind of dates are you storing that you've hit this? :]

@B4nan
Copy link
Member

B4nan commented Jan 5, 2024

gave this a closer look, and its not just about postgres, neither it is about year 1000, just about some particular years close to 0. looks like its enough to add T into the datetime string instead of the space to fix this. fixing this in v6 is actually much easier than v5, as date parsing happens on a single place for all drivers. since nobody else asked about this, i am fine with merging this in for v5, and keeping the proper fix to all drivers in v6 only

edit: in the end its postgres problem only, as mysql wont even allow storing such a date, and sqlite handles this with timestampts, so this is not a problem there either

B4nan added a commit that referenced this pull request Jan 5, 2024
@B4nan B4nan merged commit 63eb5c5 into mikro-orm:5.x Jan 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants