Skip to content
This repository was archived by the owner on Apr 25, 2023. It is now read-only.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

applicazza
Copy link

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
Copy link

huttj commented Nov 19, 2022

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
Copy link
Contributor

pimeys commented Nov 19, 2022 via email

@huttj
Copy link

huttj commented Nov 21, 2022

@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
Copy link
Contributor

pimeys commented Nov 21, 2022

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:

  • Have a special datetime mode for MySQL that lets us go around the weirdness of the database. This needs a design proposal.
  • Continue as-is, but return an error for the user that guides for better server settings.
  • Allow dates to be read as strings/integers, and let the user handle the types in the client.

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

@pimeys
Copy link
Contributor

pimeys commented Nov 21, 2022

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

@denisix
Copy link

denisix commented Jan 1, 2023

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
Copy link
Contributor

pimeys commented Jan 2, 2023

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

#419 (comment)

@denisix
Copy link

denisix commented Jan 2, 2023

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
Copy link

denisix commented Jan 3, 2023

"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
Copy link
Contributor

pimeys commented Jan 3, 2023

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
Copy link

denisix commented Jan 3, 2023

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

@janpio
Copy link
Contributor

janpio commented Jan 3, 2023

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 prisma/prisma#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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants