-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Generate TEXT fields from String #4713
Comments
MacOS version: 10.13.6 |
This should be a pretty trivial case. @matthewmueller can you help out here? |
Just FYI on this one, all of the docs (Data Modelling, MySQL Data Modelling) say that I guess maybe This one is a big issue for us. |
@mavilein I think this is your area of expertise Looks like we need to adjust this line: https://github.com/prisma/prisma-engine/blob/bbe7af6959179dfc5feb98354d2d3d1096ccd9e2/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mysql_renderer.rs#L43 |
The comment in the code explains why we are using this type right now:
@matthewmueller : Let's discuss in person how we should handle this next week. |
Talked with @matthewmueller. He will write an update how we could tackle this. |
Thanks @mavilein. Quick brain dump in why this is not as easy as changing one line of code. Given the following example: model User {
id String @id
email String @unique
bio String
} If we do If we do The naive approach would be that whenever we see We're going to keep thinking about this but if you have any ideas, let us know! |
Thanks for the responses @mavilein and @matthewmueller! Maybe a better solution doesn't involve handling this implicitly, but rather requires the user to explicitly specify that they want to use a
You could prevent any annotations of Seems related to #335 as well For us, this is a very basic/important usecase, so any help you can provide would be greatly appreciated! |
@matthewmueller an alternative could be that we acknowledge this as a very common use case and introduce both a |
@schickling that's how I'm leaning as well, since the alternatives seem sketchy. But I am worried about a datatype explosion, making it harder for generators to do their job. We might just need to acknowledge this and deal with it. Actually, perhaps generators could have a different mapping (more restrictive) than lift. |
My proposal here is that given the following SQL: create extension if not exists citext;
create table if not exists blogs (
id serial primary key not null,
website text not null unique,
created_at timestamp not null default now()
);
create table if not exists users (
id serial primary key not null,
email citext not null unique,
location point not null,
first_name text not null,
last_name text not null,
created_at timestamp not null default now()
);
create table if not exists posts (
id serial primary key not null,
blog_id int references blogs (id) on delete cascade on update cascade,
author_id int references users (id) on delete cascade on update cascade,
title text not null,
created_at timestamp not null default now()
);
create table if not exists comments (
id serial primary key not null,
post_id int references posts (id) on delete cascade on update cascade,
comment text not null,
created_at timestamp not null default now()
); We generate a schema that looks like this: generator photon {
provider = "photonjs"
}
datasource pg {
provider = "postgresql"
url = "postgresql://m@localhost:5432/prisma-blog?schema=public"
}
model blogs {
id pg.serial @id
created_at pg.timestamp
posts posts[]
website pg.text @unique
}
model comments {
id pg.serial @id
comment pg.text
created_at pg.timestamp
post_id posts?
}
model migrate {
version pg.serial @id
}
model posts {
id pg.serial @id
author_id users?
blog_id blogs?
comments comments[]
created_at pg.timestamp
title pg.text
}
model users {
id pg.serial @id
created_at pg.timestamp
email pg.citext
location pg.point
first_name pg.text
last_name pg.text
posts posts[]
} Connectors will still know how to map to cc/ @schickling @sorenbs |
@matthewmueller : I don't think this is the way to go here. I see 2 major problems with this approach:
|
For 1. in our capability map, we'd have in the Postgres connector For 2. Yes, multiple datasources in a single model would be possible in this world: model users {
id pg.serial @id
created_at pg.timestamp
email pg.citext
location pg.point
first_name mongo.string
last_name mongo.string
posts posts[]
} I think the |
We discussed a solution for this. On MySQL we currently map the Hint: The number 191 stems from the fact that index key prefix length in MySQL is 767 bytes. We are using |
I just started working on this. I immediately hit the default values issue. Note: text columns can have a default value starting with MySQL 8.0 - I haven't looked very deep into this yet but it looks like there is no straightforward way to do this with older versions of MySQL, and prisma 1 handles this by doing the default value generation itself instead of leaving it to the database (?). Reference: https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html |
Hi @mavilein, Thanks for the update -- the reasoning makes sense and that solution would work for us! |
Dear @mavilein , wouldn't it be a good approach to add MORE predefined datatypes? i.e., not only have I would suggest to have different I added some examples (including code-references) to the linked issue. Maybe this can work out here as well!? In addition, we could rely on the All the best |
I think that sounds like a very good idea @johannesschobel, as there will be similar issues for the mapping from almost any Lift data type to its Persistent (DB) data type. Like you say, JSON support e.g. for MySQL would be amazing (we're really missing it), and seems to fall into a very similar category of feature. |
@johannesschobel @robmurtagh : We are currently actively working on specing out Prismas core datatypes and their mappings to the underlying database. Your suggestions are among the ones we have on the table right now 👍 . |
That is good to hear! If you have a dummy grammar available to peek, I can surely work on the PostgreSQL grammar |
One thing I'm curious/worried about is the performance implications of this change. I've had bad experiences with slow This seems like it would be addressed by @johannesschobel 's suggestion, however. |
Dear @nlarusstone , I am certainly aware, that the |
@nlarusstone : Can you elaborate on the some cases where |
Internal note: dropping from current sprint |
Any updates on this? Seeing it was dropped multiple times from the current sprin by divyenduz The possibility to add TEXT fields on schema is necessarily for all bigger projects. Please push this topic. Thank you! PS: Need help? Like to work on this topic to get this integration done. |
Bumping this because it's been open for a while, and it's pretty important. It's troublesome because |
Bumping! Also will this take into account if you wanted to set a custom character limit? Is it maybe a new @ directive?
Maybe? |
Checking in from the RedwoodJS community (along with Rob above). We're starting to see more conversation about this amongst the community -- it's moving up in priority as a request + need. If there's anything we can do to help, please don't hesitate to ask. Thanks again, Team Prisma! |
Just a quick sidenote: It's a bit weird that Prisma is on |
Definitely a priority for my team as well! There are a lot of other issues pointing to this same topic and my understanding is that it should be resolved as part of #446 It would be awesome if this could be pushed as a priority for the next release. |
It looks like it will never come and taking not very seriously. Other less important topics are more important for them, really disappointed for myself. Fixing bugs / problems, should be the highest priority on working. This topic blocks all my important projects now. My clients getting very uncomfortable with this new technology and thinking about to spent a lot of time and money to rewrite everything into other db adapter ... Please @janpio can you push this internally?!? |
Hi everyone, |
Hey @ChristophDietrich, @maoosi and others! Thanks a lot for sharing your thoughts and your activity here, this definitely helps us prioritize this feature better! As Hervé pointed out that this change comes with a number of implications, so it's not as trivial and quick to solve. However, I just want to point out that you can totally use any native DB types with Prisma (just not with Prisma Migrate, yet)! Taking the example from this issue of changing Assume you have the following Prisma model: model User {
id Int @id @default(autoincrement())
name String
} Prisma Migrate creates the following SQL table for you when running CREATE TABLE `User` (
`id` INT(11) NOT NULL AUTO_INCREMENT,
`name` VARCHAR(191) COLLATE utf8mb4_unicode_ci NOT NULL,
PRIMARY KEY (`id`)
); You can now manually change the the type of the ALTER TABLE `User` MODIFY `name` TEXT NOT NULL; Once you've done this, you can just keep using Prisma Client as before! |
@nikolasburk Sure i can change everything manually ... but when working with dev, stage and production and when you don't have permission to one of the latest system, you are not having a good deployment. No one likes to have a separate sql file to take care of and handling the update process by themselves. What about to introduce a possibility to add column type changes like you show here with ALTER Table in the "prisma migration" process unless Prisma team is not working on this topic in "full" detail? |
@ChristophDietrich : We are working currently very actively on Prisma Migrate. This can also be seen on our public roadmap. So this restriction will get lifted soonish. |
Tracking issue for |
Any update on this guys? Like is spec ready? |
Hi @sohailhaider |
it seems unacceptable to change field length for existing data.
|
@calidion can you elaborate? |
|
Hi,
I’m using
prisma2 lift
to generate my database, but it’s generating all my String fields asVARCHAR
with size 191. Unfortunately, when I go to populate these fields, some of my data has more than 191 characters. Is there a way I can getprisma2 lift
to generateTEXT
fields or specify my desired length ofVARCHAR
?Thanks!
The text was updated successfully, but these errors were encountered: