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

Migration generation drops and creates columns instead of altering resulting in data loss #3357

Open
ryuuji3 opened this issue Jan 3, 2019 · 55 comments · May be fixed by #10891
Open

Migration generation drops and creates columns instead of altering resulting in data loss #3357

ryuuji3 opened this issue Jan 3, 2019 · 55 comments · May be fixed by #10891

Comments

@ryuuji3
Copy link

ryuuji3 commented Jan 3, 2019

Issue type:

[ ] question
[x] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[ ] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[x ] postgres
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[ ] latest
[ ] @next
[x] 0.2.11 (or put your version here)

Steps to reproduce or a small repository showing the problem:

I apologize- I can't help but think I've seen a similar issue but I couldn't find it in opened or closed issues.

Let's say I have a varchar column with length of 50;

@Column({
   length: 50
})
public example: string;

Which results in this migration:

CREATE TABLE "bug" ("text" character varying(50))

And then I go ahead and change the length to 51;

@Column({
   length: 51
})
public example: string;

The resulting generated migration code is:

ALTER TABLE "bug" DROP COLUMN "example"
ALTER TABLE "bug" ADD "example" character varying(51)

Which obviously would result in data loss.
I would have expected;

ALTER TABLE "bug" ALTER COLUMN "example" character varying(51)

I did see something in #3352 which I think could be related? Although its for schema sync.

@pleerock
Copy link
Member

pleerock commented Jan 4, 2019

no, its not related to #3352. We already had this question before, I'll provide answer once again.

Its by design. Let's say you have length equal to 50. What will happen if you change it to 10? Right, data loss again. Some databases even don't allow to do it without dropping the column. Some allow however without any warning. We could handle all possible cases, but this is a bit more complex in implementation, that's why decided to make it simple this way.

@ryuuji3
Copy link
Author

ryuuji3 commented Jan 4, 2019

Thanks for getting back to me!

It makes sense that data truncation would result in some data loss; but not the entire column. However, something like increasing the size of a column would surely not result in data loss? I think this is worth pursuing.

Understandably something like SQLite which does not support ALTER COLUMN would not benefit from this feature... but that's also only one of many RMBDS' that do.

I think migration generation is a killer feature of many ORMs and TypeORM should not be left out. We personally chose TypeORM for our production app based on the fact that it at has migration generation where something like Sequelize has very limited support.
If this isn't something high on the road map, I understand. But I do not agree that it shouldn't be up for consideration whatsoever.

@pleerock
Copy link
Member

pleerock commented Jan 4, 2019

I'll left it open, maybe some day we'll find time to improve this already quite complex part. Everything comes down into resources. You probably have seen - we have a separate topic for that (#3267).

@ryuuji3
Copy link
Author

ryuuji3 commented Jan 4, 2019

Thank you! I understand completely.

@Sturgelose
Copy link

Sturgelose commented Jan 9, 2019

We are having exactly the same issue, where we had set a PK of a table to a length, and now after increasing it to a larger one, we got this column deleted and recreated across all the rest of tables (this column is a relation used in many other tables). Luckily I saw it in the migration before applying it, though, not sure if anyone does pre checks of autogenerated code!

I also see the logic (just a proposal at the moment) that:

  1. If length increases (as long as the DB/charset supports it), there should be only an ALTER.
  2. Otherwise, it would delete and create again the column (maybe throw a warning in the CLI when generating the migration?)

I'd be willing to find some time in a couple of weeks to try to discuss/implement this feature as I see it could be beneficial to TypeORM :) I cannot give time just right now, but by mid February I should have some free time to do so, if its okay for you :)

@ryuuji3
Copy link
Author

ryuuji3 commented Jan 11, 2019

I think that ALTER COLUMN (or equivalent) should always be used when available- even if the column is being truncated. If it is being truncated, then that's on purpose.

A warning when a column is being dropped would be nice though!

@dueldanov
Copy link

Current behavior leads to MySQL error when trying to alter primary key column referenced by one or more foreign keys in other tables.

@AlexMesser AlexMesser self-assigned this Mar 13, 2019
@jyang0110
Copy link

@pleerock Do you have any documentation on what cases will result in drop column? Does renameColumn method have possibility to drop column? Thanks.

@hgezim
Copy link

hgezim commented May 5, 2019

To make matters worse, even QueryRunner.changeColumn has the same behaviour where it drops old column and adds a new one. Completely not what you'd expect.

Please take throw hard errors/exceptions instead of silently deleting data.

@ryuuji3
Copy link
Author

ryuuji3 commented May 11, 2019

Edit: This implementation is unique per driver! The tedious part is that each driver needs to have its implementation re-worked!

await this.dropColumn(table, oldColumn);

@perzanko
Copy link

We have same problem. Sorry guys, I love your work, but migrations really suck for now. 👎

@dima11221122
Copy link

I confirm, migration generator is broken. I wanted to remove only startDate column in my entity. After remove I've run typeorm migration:generate -n PostRefactoring and got such migration file:

import {MigrationInterface, QueryRunner} from "typeorm";

export class RemoveStartDateSubscription1564382101607 implements MigrationInterface {

    public async up(queryRunner: QueryRunner): Promise<any> {
        await queryRunner.query(`ALTER TABLE "subscription" DROP COLUMN "startDate"`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "name"`);
        await queryRunner.query(`ALTER TABLE "project" DROP COLUMN "description"`);
        await queryRunner.query(`ALTER TABLE "team" DROP COLUMN "description"`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "username"`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "displayName"`);
        await queryRunner.query(`ALTER TABLE "project" ADD "description" character varying`);
        await queryRunner.query(`ALTER TABLE "team" ADD "description" character varying`);
        await queryRunner.query(`ALTER TABLE "user" ADD "username" character varying`);
        await queryRunner.query(`ALTER TABLE "user" ADD "displayName" character varying`);
        await queryRunner.query(`ALTER TABLE "subscription" ADD "startDate" date NOT NULL DEFAULT now()`);
        await queryRunner.query(`ALTER TABLE "user" ADD "name" character varying`);
        await queryRunner.query(`ALTER TABLE "subscription" ALTER COLUMN "usersCapacity" DROP DEFAULT`);
        await queryRunner.query(`ALTER TABLE "subscription" ALTER COLUMN "endDate" SET NOT NULL`);
    }

    public async down(queryRunner: QueryRunner): Promise<any> {
        await queryRunner.query(`ALTER TABLE "subscription" ALTER COLUMN "endDate" DROP NOT NULL`);
        await queryRunner.query(`ALTER TABLE "subscription" ALTER COLUMN "usersCapacity" SET DEFAULT 1`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "name"`);
        await queryRunner.query(`ALTER TABLE "subscription" DROP COLUMN "startDate"`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "displayName"`);
        await queryRunner.query(`ALTER TABLE "user" DROP COLUMN "username"`);
        await queryRunner.query(`ALTER TABLE "team" DROP COLUMN "description"`);
        await queryRunner.query(`ALTER TABLE "project" DROP COLUMN "description"`);
        await queryRunner.query(`ALTER TABLE "user" ADD "displayName" character varying`);
        await queryRunner.query(`ALTER TABLE "user" ADD "username" character varying`);
        await queryRunner.query(`ALTER TABLE "team" ADD "description" character varying`);
        await queryRunner.query(`ALTER TABLE "project" ADD "description" character varying`);
        await queryRunner.query(`ALTER TABLE "user" ADD "name" character varying`);
        await queryRunner.query(`ALTER TABLE "subscription" ADD "startDate" date NOT NULL DEFAULT now()`);
    }

}

As you can see, the generator is changed ALL of the entity columns (drop and add them again).

typeorm: 0.2.18
postgresql: 11.2

@rmainwork
Copy link

I'm seeing something similar in #4577. Can someone confirm if this is the same bug?

@sam-gronblom-rj
Copy link

I'm seeing something similar when trying to run migrations:generate on an empty Postgres database. I have two entities, user and lease. The first three statements of the up migration are fine. They create an enum as expected, create a table for lease, create a table for users.

However, after that the migration seems to get confused and proceeds to DROP all the columns one by one for the lease table. And then re-adding them back again. This does not seem to happen for the user table.

Finally the down migration also seems weird. It's also dropping all columns of lease first one by one, then re-adding them one by one, and then finally simply dropping the tables.

I tried removing most of the attributes from my lease entity to see if there was some feature I'm using that seems to throw off typeorm. I found it was quite easy to confuse the migration generator and make it add seemingly useless statements.

@arimus
Copy link

arimus commented Oct 10, 2019

This is a pretty bad wrinkle in an otherwise nice migration system. Would seem that running the alter column is a pretty reasonable solution for databases that support this (of which many do).

It's would be a good idea to take the path of least data loss for any operation, so naively drop + add'ng would seem to be the worst choice when alter column is available. Using the alter is the equivalent of what a user would expect when hand-writing an SQL query to change the size of a column.

Btw, there's also the situation where the user increases the size of the column (which is vastly more common and which should result in NO data loss). Naively dropping and adding is simple, but has pretty huge ramifications for anyone looking to generate migration files that are useful (without modification) in a non-dev environment. Here's what a migration looked like after simply setting a property to have a length of 129 (from 128). Interestingly, it also saw fit to add in a no-op alter for the 'sub_type' property, which had no actual changes at all.

import {MigrationInterface, QueryRunner} from "typeorm";

export class MigrationName1570747617830 implements MigrationInterface {

    public async up(queryRunner: QueryRunner): Promise<any> {
        await queryRunner.query("ALTER TABLE `model_property` CHANGE `sub_type` `sub_type` varchar(16) NULL DEFAULT null");
        await queryRunner.query("ALTER TABLE `model_property` DROP COLUMN `default`");
        await queryRunner.query("ALTER TABLE `model_property` ADD `default` varchar(129) NULL");
    }

    public async down(queryRunner: QueryRunner): Promise<any> {
        await queryRunner.query("ALTER TABLE `model_property` DROP COLUMN `default`");
        await queryRunner.query("ALTER TABLE `model_property` ADD `default` varchar(128) NULL");
        await queryRunner.query("ALTER TABLE `model_property` CHANGE `sub_type` `sub_type` varchar(16) NULL");
    }

}

Understandably, resources may not be able to tackle this improvement right now.

@jprules321
Copy link

When I change anything and generate migrations it always drop the datas in the many to many tables related to that model, even if what I changed has nothing to do with it. Is this related to this issue or is there a fix for this

@EvanDarwin
Copy link

I'm also experiencing this issue, do we know if there's a temporary workaround for this issue?

@ghost
Copy link

ghost commented Feb 4, 2020

I'm also experiencing this on MySQL which is making migration:generate impossible to use.

@sunapi386
Copy link

sunapi386 commented Apr 28, 2020

I've run into this as well, on 0.2.24. I probably would need to edit the migration file and hand craft it. Is that how you guys above dealt with it?

edit:
Added example; existing column data would be lost.

    public async up(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "listing" ADD "reviews" integer`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "ratings" integer`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "beds"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "beds" integer`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "baths"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "baths" integer`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "price"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "price" integer NOT NULL`, undefined);
    }

    public async down(queryRunner: QueryRunner): Promise<void> {
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "price"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "price" numeric(10,2) DEFAULT 0`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "baths"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "baths" numeric(10,0) DEFAULT 0`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "beds"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" ADD "beds" numeric(10,0) DEFAULT 0`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "ratings"`, undefined);
        await queryRunner.query(`ALTER TABLE "listing" DROP COLUMN "reviews"`, undefined);
    }

@kayvanbree
Copy link

I am also experiencing this issue on MySQL. Can't use migrations this way.

This is a pretty old issue. Is there any information on if this will be fixed in the near future?

@mtford90
Copy link

I've had to drop back to writing the migrations by hand. The migration generator drops and adds columns and constraints willy nilly. It's a nice idea but I suspect the complexities make it difficult to maintain.

@gregdillon
Copy link

I'm just starting to learn Typeorm in relation to NestJS. Took me a while to find this issue, and I agree. It seems odd that the default behavior of a migration is dataloss. Even when renaming a column, the generator does a DROP/ADD.

@zbyte64
Copy link

zbyte64 commented Sep 8, 2020

Using postgres as well, I have noticed that on a new database I run schema:sync and it will create the tables, drop all id columns, drop constrains, recreate all dropped columns & constraints.

@ottoo
Copy link

ottoo commented Aug 10, 2022

I noticed this behavior when trying to change a time column to timetz. It's ridiculous that the column would be dropped in this case since the data for PSQL would stay intact with just an alter table. I think having this behavior as the default is very dangerous and can catch people off guard.

@Maia-Everett
Copy link

Maia-Everett commented Aug 25, 2022

I've experienced this issue with MariaDB.

I realize it's impossible to cover all specific cases, but lengthening a varchar column seems to me a common enough and uncontroversially data-preserving change to warrant special handling in the migration generator. Would it be possible to add?

@iiisak
Copy link

iiisak commented Sep 9, 2022

Has this still not been fixed since 2019? We're having the same issues on better-sqlite3 (which supports altering tables) and it's causing some nasty FOREIGN KEY constraint failed errors.

@lijinglun-1
Copy link

I understand it's hard to cover all situation.But at least, a common and simple length-changing shouldn't always just drop columns.

@phytter
Copy link

phytter commented Dec 24, 2022

It's a shame this hasn't been resolved yet . I faced this issue today and the way of this is treated in the core is so bad. I hope team is thinking a workaround for the problem.

@alexey-sh
Copy link

So the best choice for now would be synchronize: false and make all migrations by hands.
The issue was created 4 years ago and it seems like there's no light at the end of the tunnel.

@maitrungduc1410
Copy link

maitrungduc1410 commented Mar 25, 2023

I'm facing same issue.

Now I have to manually check for any migration which I generate to make sure that in the case it has DROP command, I'll have no data lost. Funny :)

@sangimed
Copy link

sangimed commented Mar 31, 2023

Same bug using 0.2.24. Has anyone faced a similar issue on the latest version ?

@AlexGusevEpam
Copy link

Same issue on typeorm v0.3.16

@Distortedlogic
Copy link

Data loss is the worst solution .. renders migrations as worthless

@RAFALAMAO
Copy link

I think may be a good solution would be get an option to set if we want or not DROP when create a migration, any ways, thanks for this.

@Lamarcke
Copy link

I'm also facing the same issue. I'm glad i discovered this in the development phase.
I will be checking every line of the migrations i run from now on. I still think migrations are a huge plus for Typeorm. Most ORMs don't have something like that, and those that do, do it very badly.

@Nikita1917s
Copy link

Nikita1917s commented Jul 17, 2023

I had the same problem - changing column length from 2000 -> 20000

So, I used this SQL method - and it worked for me. The length was changed - no error was shown during the migration

  public async up(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(
      `ALTER TABLE "notes" ALTER COLUMN "text" TYPE VARCHAR(20000)`,
    );
  }
  public async down(queryRunner: QueryRunner): Promise<void> {
    await queryRunner.query(
      `ALTER TABLE "notes" ALTER COLUMN "text" TYPE VARCHAR(2000)`,
    );
  }

@nodegin
Copy link

nodegin commented Sep 14, 2023

Can we have an option having rename the column to <original_name>_backup then add the new column instead of drop & add?

@Laurensdc
Copy link

Changing a column type from varchar to text also drops the column when generating a migration.

@basemkhirat
Copy link

It's 2024, let's wait another four years :(

@Edison4mobile
Copy link

it must be fixed. it is big big big problem.

@vs-t
Copy link

vs-t commented Jan 16, 2024

+1 it's a problem for next migration and a terrible tripwire

@milad-afkhami
Copy link

It's been 5 years.

I guess yuuji3 has switched to Prisma by now. Nest.js should really consider acquiring TypeORM to clean this mess.

@cezar-tech
Copy link

A dangerous problem that almost got into production !!!!!
I had to wrote the SQL myself

@opiredev
Copy link

@RubenRuCh created a $1,000.00 reward using Opire

How to earn this reward?

Since this project does not have Opire bot installed yet 😞 you need to go to Opire and claim the rewards through your programmer's dashboard once you have your PR ready 💪

@RubenRuCh
Copy link

RubenRuCh commented Apr 28, 2024

This issue has been affecting me for several years. Had to write custom migrations to avoid losing data. And a few times I forgot about this and almost screw production.

Seeing that the issue has been open for several years and the maintainers have been clear about lack of resources, I think it's time for me to stop complaining about the problem and try to actually collaborate to see it solved.

I created a $1000 reward for the developer (either a TypeORM maintainer or an external dev) that manage to solve this.

I'm aware it's a complex issue and that it will probably take more effort than $1k worth's. But it's all I can afford at the moment.

Bear in mind that if this get fixed, we all will benefit from it. So if every one of us who has asked for this issue to be solved put a small amount ($50, $100...) it can actually sum to be a great reward that could help to incentive having this issue solved.

@Rafael-rgsousa
Copy link

This issue has been affecting me for several years. Had to write custom migrations to avoid losing data. And a few times I forgot about this and almost screw production.

Seeing that the issue has been open for several years and the maintainers have been clear about lack of resources, I think it's time for me to stop complaining about the problem and try to actually collaborate to see it solved.

I created a $1000 reward for the developer (either a TypeORM maintainer or an external dev) that manage to solve this.

I'm aware it's a complex issue and that it will probably take more effort than $1k worth's. But it's all I can afford at the moment.

Bear in mind that if this get fixed, we all will benefit from it. So if every one of us who has asked for this issue to be solved put a small amount ($50, $100...) it can actually sum to be a great reward that could help to incentive having this issue solved.

This is how I solved this.

https://github.com/Rafael-rgsousa/typeorm-safer-migrations

I've created a code to allow me to backup the columns' data into a temporary table and restore it after the migration is completed.

The "start" command starts the backup process
The "done" command starts the restoration process

Example:

import { MigrationInterface, QueryRunner } from 'typeorm';
import { SaferMigrations, TableBackupConfig } from './path-to/safer-migration';

export class ExampleMigration implements MigrationInterface {
  tableConfigs: TableBackupConfig[] = [
    { tableName: 'products', primaryKeyColumns: ['id'], backupColumns: ['description'] },
    { tableName: 'orders', primaryKeyColumns: ['order_id', 'product_id'], backupColumns: ['quantity'] }
  ];

  public async up(queryRunner: QueryRunner): Promise<void> {
    await SaferMigrations.start(queryRunner, this.tableConfigs);
    // Your migration code...
    await SaferMigrations.done(queryRunner, this.tableConfigs);
  }

  public async down(queryRunner: QueryRunner): Promise<void> {
    await SaferMigrations.start(queryRunner, this.tableConfigs);
    // Your migration code...
    await SaferMigrations.done(queryRunner, this.tableConfigs);
  }
}

@opiredev
Copy link

🔥 The user @Rafael-rgsousa has claimed all rewards for this issue using Opire.
🎯 You can see the PR here
💰 @RubenRuCh you can pay the related rewards here

@RubenRuCh
Copy link

Thanks @Rafael-rgsousa!

Let's see if someone with permissions to merge can take a look at your PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment