prisma / quaint

SQL Query AST and Visitor for Rust
Apache License 2.0
582 stars 61 forks source link

Do not return error on `0000-00-00` date #419

Open applicazza opened 1 year ago

applicazza commented 1 year ago

Some legacy MySQL databases allow 0000-00-00 to be stored in a Date column. However when such a value is met by prisma the whole row is discarded and cannot be retrieved and further processed.

Converting an "invalid" date to NULL is a quick and dirty solution that wouldn't break anything.

huttj commented 1 year ago

Please land this. As it is now, it's impossible to use Prisma with legacy MySQL databases. I had to migrate to Sequelize. Sequelize! The horror.

pimeys commented 1 year ago

Hey,

I'm currently on vacation, but this makes a lot of sense to me. Back next week, going to discuss it with the team!

On Sat, Nov 19, 2022, at 9:27 AM, Joshua Hutt wrote:

Please land this. As it is now, it's impossible to use Prisma with legacy MySQL databases. I had to migrate to Sequelize. Sequelize! The horror.

— Reply to this email directly, view it on GitHub https://github.com/prisma/quaint/pull/419#issuecomment-1320833660, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIRF5OGOSPOBR2MTJNEK3WJCFNPANCNFSM6AAAAAASE36YEE. You are receiving this because you are subscribed to this thread.Message ID: @.***>

huttj commented 1 year ago

@pimeys Thank you for taking a look at it, and for the careful consideration of the situation under discussion.

The last option would be great – if there was a way to read them and write them as strings, and then parse them in a hook, it would work for determined users like myself who have no choice. A simple example, presented as a "workaround" for legacy databases would suffice. I wouldn't mind adding some extra code to the server to make it work–if it is possible.

I couldn't figure out how to do that effectively with the current implementation, if it's even possible. If you could at least support that approach, I think we could put this issue to rest, for good.

pimeys commented 1 year ago

I put the PR for a round of reviews, and there's one thing here that does not really work well.

Let's imagine you have a table:

CREATE TABLE foo (
  id INT NOT NULL AUTO_INCREMENT,
  date DATETIME NOT NULL,
  PRIMARY KEY (id)
)

Now, your server lets you to write 0000-00-00 00:00:00 due to server settings. This value gets stored if the input is an invalid datetime value, but only if explicitly enabling this setting in the MySQL configuration.

We can imagine the following model from the table:

model foo {
  id   Int      @id          @default(autoincrement())
  date DateTime @db.DateTime
}

As we see the date field is required. Our type contract forces the field to always return a value. With this change we'd be returning nulls for a field we say never can be null.

What options we would have here then? I'd say a few at least:

Anyhow, as it is right now we cannot merge the PR. The issue needs some product consideration before we move forward.

pimeys commented 1 year ago

@huttj could you open an issue to prisma/prisma for the feature/improvement. Then it goes to the normal rotation for the teams.

denisix commented 1 year ago

Hey, I'm currently on vacation, but this makes a lot of sense to me. Back next week, going to discuss it with the team!

Hey, when this pull request will be added, any ETA? Thank you!

pimeys commented 1 year ago

Not in this version, I don't think it fits to the Prisma type system. See my comment in

https://github.com/prisma/quaint/pull/419#issuecomment-1321862497

denisix commented 1 year ago

Not in this version, I don't think it fits to the Prisma type system. See my comment in

  1. it's not an option, because some legacy projects already have this '0000-00-00 00:00:00' in their production DB tables, so we need to handle such cases without any changes on DB side.
  2. currently prisma shows type range error, without any suggestion how can I fix it, it's weird.
  3. if I use DB inspection tool, db pull, it creates prisma.schema that is already have type range issues.
  4. you didn't provide any options to user to convert '0000-00-00 00:00:00' to NULL, or any other hooks or code solutions, just words that you cannot do this.
  5. you have PR with the desired solution, but you just ignoring it..

Not only me have such problems, just look how many comments..

denisix commented 1 year ago

"The default value of an optional field is null" as noted in https://www.prisma.io/docs/concepts/components/prisma-schema/data-model

so can we enforce "0000-00-00 00:00:00" be recognized as NULL for data model with field date DateTime?, please check the model:

model foo {
  id   Int      @id          @default(autoincrement())
  date DateTime? @db.DateTime
}

in this case developer can say this field is optional and can be NULL due to referenced documentation.

pimeys commented 1 year ago

I think the only thing we can do here is to let you use String @db.DateTime as the type for these fields, then convert manually. Also, this is not a Quaint issue anymore, the management is not reading these and the work is not scheduled to the normal sprints. I suggest somebody to open an issue to prisma/prisma.

We just cannot convert an invalid type to null, if the Prisma type is DateTime (a required field, that cannot be null by the definition). I hope with all this material you understand the issue and why we will not merge this PR. What I want to see here is somebody writing an article about the issue, and then suggesting a solution with all pros and cons listed.

denisix commented 1 year ago

it's a good starting point writing an article why we should not use prisma in our LTS projects :)

janpio commented 1 year ago

No need to bring the discussion to this level @denisix.

You could be good community member and add a comment for your use case to https://github.com/prisma/prisma/issues/5006. We prioritize things our community members find important - but as you did not comment or upvote on that we for example would not know.