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(sqlite): parse date error #16355

Closed
wants to merge 2 commits into from
Closed

fix(sqlite): parse date error #16355

wants to merge 2 commits into from

Conversation

skypesky
Copy link

@skypesky skypesky commented Aug 7, 2023

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

Todos

  • null

@skypesky skypesky changed the title WIP: fix: #16340 WIP: fix(sqlite): parse Date Error Aug 7, 2023
@skypesky skypesky changed the title WIP: fix(sqlite): parse Date Error WIP: fix(sqlite): parse date error Aug 7, 2023
@skypesky skypesky changed the title WIP: fix(sqlite): parse date error fix(sqlite): parse date error Aug 7, 2023
DataTypes = require('sequelize/lib/dialects/sqlite/data-types')(BaseTypes);

if (dialect.match(/^sqlite/)) {
describe('[SQLITE Specific] DataTypes', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are these tests really sqlite specific? Is the behaviour different on other dialects?

Copy link
Author

Choose a reason for hiding this comment

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

Ok,Here's what I think。

  1. I don't know the details of how DATE is handled in other dialects, but it appears from the source code that different dialects handle dates differently from each other.
  2. the problem I'm having only occurs in sqlite, so I'm just fixing the parse function in sqlite and making sure it's working.

Copy link
Member

Choose a reason for hiding this comment

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

I think the other dialects should also handle these test cases the same way, so I'd like to see these tests running on all dialects and therefore be added to an existing test suite

@skypesky
Copy link
Author

skypesky commented Aug 17, 2023

Can this pr be merged?
This test(https://github.com/sequelize/sequelize/actions/runs/5786617083/job/15681735870?pr=16355) is probably wrong because of instability, and this change has nothing to do with postgress.

@ephys
Copy link
Member

ephys commented Aug 23, 2023

I'm not convinced we should do this. Our Date DataType inserts dates in the ISO format, not as numbers. If the goal is to fix #16340, it's queryInterface.insert that should be fixed instead.

Once that is fixed, any Date persisted to the database as a number will not be caused by us, therefore we can't know what level of precision it is meant to represent. It could be milliseconds, nanoseconds, seconds.

You can always extend DATE in your project to suit your needs but I don't think we should apply this to all users

@skypesky
Copy link
Author

skypesky commented Aug 23, 2023

I'm not convinced we should do this. Our Date DataType inserts dates in the ISO format, not as numbers. If the goal is to fix #16340, it's queryInterface.insert that should be fixed instead.

Once that is fixed, any Date persisted to the database as a number will not be caused by us, therefore we can't know what level of precision it is meant to represent. It could be milliseconds, nanoseconds, seconds.

You can always extend DATE in your project to suit your needs but I don't think we should apply this to all users

queryInterface.insert is just an example of the error it throws, even just querying for records will throw this error, I get this error querying for data when migrating prisma to sequelize (e.g. User.findAll(), User.findOne()). Just fixing it at queryInterface.insert is not enough.

@ephys
Copy link
Member

ephys commented Aug 23, 2023

Yes but we're not responsible for the format prisma used, that's what I was saying in my comment above: Prisma chose a precision of milliseconds, but another (or even us) could have chosen a different precision, like nanoseconds. We can't know what the precision should be because we were not responsible for the insertion of the data. This PR is specific to your needs so I would recommend a custom DataType instead (or migrating your data)

@WikiRik WikiRik added the v6 label Sep 23, 2023
@skypesky skypesky closed this Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants