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

Prisma Migrate removes the results of custom migrations in the next migrate dev run #24180

Open
ApocalypseCalculator opened this issue May 14, 2024 · 13 comments
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. domain/schema Issue in the "Schema" domain: Prisma Schema, Introspection, Migrations etc. kind/bug A reported bug. topic: generated columns topic: migrate topic: prisma migrate dev --create-only CLI: prisma migrate dev --create-only topic: tsvector

Comments

@ApocalypseCalculator
Copy link

Bug description

For a multitude of different reasons users need to use custom SQL for unsupported Prisma features. One such feature I am trying to implement is native full text search, which requires me to add a tsvector column to my table and a GIN index. The documentation outlines how to customize a migration, which has worked for me. However, I am now trying to create another table, and when I try to create a migration for it, the generated migration drops my custom column and index.

Since my development database was populated with test data, I was able to catch on as Prisma warned of its attempt to push these changes onto the database itself. However, without data in dev, there is no warning and the migration is generated straight away. If deployed to production, this would wipe out the data I had, which is not really acceptable.

I believe that Prisma should a) warn of destructive actions like dropping columns/indices before generating migrations and b) not have this issue of dropping in the first place. Thoughts?

How to reproduce

  1. Create migration with given schema
  2. Generate a new migration without applying it with prisma migrate dev --create-only
  3. Add the given SQL into the empty migration
  4. Apply migration
  5. Optionally edit the schema in some way
  6. Create migration for changes (if no changes, a migration with only the drop commands are created)

Expected behavior

Prisma shouldn't drop my custom migration

Prisma information

datasource db {
    provider = "postgresql"
    url      = env("DB_URL")
}
generator client {
    provider = "prisma-client-js"
}
model Video {
    id        Int         @id @default(autoincrement())
    title     String
    alttitle  String
    summary   String
}
ALTER TABLE "Video" ADD search tsvector GENERATED ALWAYS AS 
    (setweight(to_tsvector('english', title), 'A') || ' ' || 
    setweight(to_tsvector('english', alttitle), 'B') || ' ' ||
    to_tsvector('english', summary)) STORED;

CREATE INDEX idx_search ON "Video" USING GIN(search);

Environment & setup

  • OS: Windows
  • Database: PostgreSQL
  • Node.js version: 18.17.1

Prisma Version

5.13.0
@ApocalypseCalculator ApocalypseCalculator added the kind/bug A reported bug. label May 14, 2024
@jkomyno jkomyno added bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. domain/schema Issue in the "Schema" domain: Prisma Schema, Introspection, Migrations etc. topic: migrate labels May 14, 2024
@janpio
Copy link
Member

janpio commented May 14, 2024

What exactly is the destructive migration that is being generated here?

Fundamentally the problem is that Prisma does not support what you are trying to use in Prisma Schema (a generated column I think), and hence will of course try to clean up your database to bring it into the state that the Prisma schema represents. But that is also why --create-only exists so you can review and clean up any generated migration.

@janpio janpio added the topic: prisma migrate dev --create-only CLI: prisma migrate dev --create-only label May 14, 2024
@ApocalypseCalculator
Copy link
Author

The "destructive actions" generated by Prisma that I mentioned are

-- DropIndex
DROP INDEX "idx_search";

-- AlterTable
ALTER TABLE "Video" DROP COLUMN "search";

I realize that --create-only exists, but I think the expectation is that I shouldn't need to go through every generated migration afterwards manually. If I need to add more custom features, or edit my Prisma schema further, it's not hard to see how it can become extremely messy. I had assumed (perhaps incorrectly) that it was a one and done sort of thing.

@janpio
Copy link
Member

janpio commented May 14, 2024

We would love to be able to offer that, but how should Prisma know what is expected in the database (because of a manually modified migration) and what is not (and hence needs to be fixed to get to the desired state)?

If I was you, I would probably add the search column and the Gin index on it to the Prisma schema, then you just need to see how Prisma deals with the generated column definition (it will possibly try to remove it, which then is something you need to modify with each future migration).

@ApocalypseCalculator
Copy link
Author

One possible solution is to tag each migration to the corresponding Prisma schema. Then a newly generated migration would diff the changes in the Prisma schema, as opposed to diff against what is in the current database. This, I believe, makes a bit more sense and is in my opinion a better system overall, but I understand it is likely time consuming to implement.

As for your solution, how can I add the column/index to the Prisma schema? Since tsvector and related functions aren't supported.

@janpio
Copy link
Member

janpio commented May 16, 2024

One possible solution is to tag each migration to the corresponding Prisma schema. Then a newly generated migration would diff the changes in the Prisma schema, as opposed to diff against what is in the current database. This, I believe, makes a bit more sense and is in my opinion a better system overall, but I understand it is likely time consuming to implement.

Yes, but that would be pretty much a completely different migration tool than what Prisma Migrate currently is.
Still, we are aware of the current challenges and are considering far reaching changes like that - but that will definitely take some time.

As for your solution, how can I add the column/index to the Prisma schema? Since tsvector and related functions aren't supported.

I was thinking about adding it as a supported field, and then manipulate the migration that would create it. But you are correct, in the details this might not actually work as hoped :/

@chrbala
Copy link

chrbala commented May 21, 2024

@ApocalypseCalculator, you might be interested in #24248, in which I suggest we make diffs based on changes to the Prisma schema in different git versions. You might be able to use this for your use case.

@TimMensch
Copy link

I'm trying to do something similar, but using the Unsupported() column type.

But it seems to be hitting a bug in Prisma's "shadow database" parser?

I add the following:

    textSearch Unsupported("GENERATED ALWAYS AS (to_tsvector('english', text)) STORED")
//...
    @@index([textSearch], type: Gin)

Which seems entirely reasonable, yes? And it generates:

/*
  Warnings:

  - Added the required column `textSearch` to the `TextRevision` table without a default value. This is not possible if the table is not empty.

*/
-- AlterTable
ALTER TABLE "TextRevision" ADD COLUMN     "textSearch" GENERATED ALWAYS AS (to_tsvector('english', text)) STORED NOT NULL;

-- CreateIndex
CREATE INDEX "TextRevision_textSearch_idx" ON "TextRevision" USING GIN ("textSearch");

This looks perfect!

But then before ever calling PostgreSQL (I checked the logs), it gets:

Database error:
ERROR: syntax error at or near "ALWAYS"

Position:
  3
  4   - Added the required column `textSearch` to the `TextRevision` table without a default value. This is not possible if the table is not empty.
  5
  6 */
  7 -- AlterTable
  8 ALTER TABLE "TextRevision" ADD COLUMN     "textSearch" GENERATED ALWAYS AS (to_tsvector('english', name text)) STORED NOT NULL;     

DbError { severity: "ERROR", parsed_severity: Some(Error), code: SqlState(E42601), message: "syntax error at or near \"ALWAYS\"", detail: None, hint: None, position: Some(Original(244)), where_: None, schema: None, table: None, column: None, datatype: None, constraint: None, file: Some("scan.l"), line: Some(1241), routine: Some("scanner_yyerror") }

Obviously the alternative approach of adding the migration manually doesn't work either, since Prisma will see it and kill it, as demonstrated by the bug report above. :|

This is a pretty serious bug/shortcoming/missing feature in Prisma. It's not working as expected, and this is a mission-critical feature for the app I'm using. More than half the reason I use Prisma is for the migrate feature, which could absolutely work even without being able to parse every field simply by skipping to the end of that column definition once it recognizes that it's an Unsupported() column.

If you need to know a type name, give Unsupported() another parameter so we can call it like:

Unsupported("ts_vector", "GENERATED AS ...")

Then internally it can treat the column as a ts_vector column, and maybe the full text search would even be able to work on it without us needing to create custom queries? I can dream...

@janpio
Copy link
Member

janpio commented Jun 10, 2024

@TimMensch The error message you are getting is straight from your database, not from Prisma. The generated SQL does not seem to be valid. As what you describe is also not about Prisma removing the results of custom migrations in the next mgirate dev run, I suggest you open a new discussion so we can dig into this. Thank you.

@janpio janpio changed the title Prisma Migrate Drops Custom Migrations Prisma Migrate removes the results of custom migrations in the next migrate dev run Jun 10, 2024
@TimMensch
Copy link

@janpio You were right. I missed the type in the Unsupported() call.

So flip this to a possible solution to the above issue: Add the column in the schema as an Unsupported() type and Prisma will happily add it for you.

The line that worked:

    textSearch Unsupported("tsvector GENERATED ALWAYS AS (to_tsvector('english', text)) STORED")

@TimMensch
Copy link

AND...now I am seeing a variation of the bug above. The next migrate dev did in fact emit illegal SQL attempting to ... "undo" the change in the previous migration?

Specifically, it is trying to "remove a default" from the column:

ALTER TABLE "TextRevision" ALTER COLUMN "textSearch" DROP DEFAULT;

So the Unsupported() column type above should be the fix for OP's issue, but it would instead try to erroneously "DROP DEFAULT" from the column on the very next migrate dev.

@janpio
Copy link
Member

janpio commented Jun 11, 2024

How is that illegal SQL @TimMensch?

There is no @default(...) defined for your column, so Prisma is trying to remove it - that is pretty much expected, isn't it? The question is what default is even there before? The migration SQL you shared would not create one. Or is that the bug? (If so, can you turn that into its own issue so we can reproduce and look at this? Thanks.)

@TimMensch
Copy link

@janpio It's illegal because the column in question is GENERATED. It has no DEFAULT, but somehow the way Prisma is parsing the response from PostgreSQL is fooling it into thinking there is a DEFAULT on the column. Thankfully DROP DEFAULT throws an error if the column is GENERATED ALWAYS or this would be even worse.

There's probably some flag associated with the column that indicates that yes, the column has a default value, because it's GENERATED ALWAYS and therefore will be created as if there's a DEFAULT clause. What's needed in Prisma is some logic that says "If the column type is Unsupported() then don't try to change it," or "If the Unsupported() column type is GENERATED, then don't treat it as having an unnecessary DEFAULT that needs to be removed," or equivalent.

In other words, you could decide that Unsupported() columns just shouldn't be messed with by Prisma. I'd get behind that choice. Or you could make a more targeted change that short-circuits the DEFAULT check if it's GENERATED, but then there's a danger that other Unsupported() columns could trigger other similar errors. My gut is that it would be better to treat Unsupported() columns as a black box, and only update them if the exact string used in the parameter changes.

@TimMensch
Copy link

OK, added #24496 to track this.

If #24496 is fixed, then the example code there can be used to fix OP's issue above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/1-unconfirmed Bug should have enough information for reproduction, but confirmation has not happened yet. domain/schema Issue in the "Schema" domain: Prisma Schema, Introspection, Migrations etc. kind/bug A reported bug. topic: generated columns topic: migrate topic: prisma migrate dev --create-only CLI: prisma migrate dev --create-only topic: tsvector
Projects
None yet
Development

No branches or pull requests

5 participants