cli icon indicating copy to clipboard operation
cli copied to clipboard

removeColumn on down action of migration not working

Open Epyon616 opened this issue 7 years ago • 7 comments

Hi all,

I've been using Sequelize and Sequelize CLI for a few weeks now and have come across a bit of an issue.

I have a migration that adds columns to an existing table in the up action and should remove them if the migration is rolled back however this is not happening and instead the rollback is claiming it was successful but it's actually leaving the columns in place. Which means when you run the migration again Sequelize complains that the columns already exist.

I'm sure This is bound to be a simple oversight on my part but I thought I would ask on here as based on all the documentation I have read it would appear everything is correct. Here is an the migration in question:

module.exports = {
  up: (queryInterface, Sequelize) => {
    return [
      queryInterface.addColumn('playlist', 'createdBy', Sequelize.INTEGER),
      queryInterface.addColumn('playlist', 'modifiedBy', Sequelize.INTEGER),
  },

  down: (queryInterface, Sequelize) => {
    return [
      queryInterface.removeColumn('playlist', 'createdBy'),
      queryInterface.removeColumn('playlist', 'modifiedBy'),
    ];
  },
};

Epyon616 avatar May 22 '17 10:05 Epyon616

Can you paste the output? Also, seems your "up" method is missing a trailing ];.

harmon avatar May 24 '17 16:05 harmon

Having same problem as StlthyLee, columns get added okay, but not removed:

module.exports = {
  up: (queryInterface, Sequelize) => {
    queryInterface.addColumn(
      'tableName',
      'custom_entity_id',
      Sequelize.INTEGER
    );
    queryInterface.addColumn(
      'tableName',
      'custom_entity_field_id',
      Sequelize.INTEGER
    );
  },

  down: (queryInterface) => {
    queryInterface.removeColumn('tableName', 'custom_entity_id');
    queryInterface.removeColumn('tableName', 'custom_entity_field_id');
  },
};

As far as I can tell these are exactly reciprocal actions but while the up adds the columns, the down does not remove them.

Output of me calling the undo

Sequelize CLI [Node: 7.5.0, CLI: 3.0.0-3, ORM: 4.8.3]

WARNING: This version of Sequelize CLI is not fully compatible with Sequelize v4. https://github.com/sequelize/cli#sequelize-support

Loaded configuration file "config/config.json".
Using environment "local".
== 20170916234050-custom-entities: reverting =======
== 20170916234050-custom-entities: reverted (0.042s)

Zechnophobe avatar Sep 22 '17 02:09 Zechnophobe

So, in your up and down migrations, you aren't returning any promises (something they say to do). Can you make sure to return something?

module.exports = {
  up: (queryInterface, Sequelize) => {
    let promise = queryInterface.addColumn(
      'tableName',
      'custom_entity_id',
      Sequelize.INTEGER
    );
    return promise.then(() => {
      return queryInterface.addColumn(
        'tableName',
        'custom_entity_field_id',
        Sequelize.INTEGER
      );
    });
  },

  down: (queryInterface) => {
    let promise = queryInterface.removeColumn('tableName', 'custom_entity_id');
    return promise.then(() => { return queryInterface.removeColumn('tableName', 'custom_entity_field_id'); });
  },
};

This way it doesn't try to execute them in parallel, which is an issue I've seen before when making modifications to some databases.

Not an expert, but I think if the migration does not return anything, it tries to close the db connection quickly, and may not execute your changes. I bet if you put catch() handlers on those down migration "removeColumn()" promises, it would show you some error.

harmon avatar Sep 22 '17 13:09 harmon

@harmon You nailed it! Thanks very much.

For anyone finding this page, I wrapped my queries in Promise.all like so:

module.exports = {
  up: (queryInterface, Sequelize) => {
    return Promise.all([
      queryInterface.addColumn(
        'tableName',
        'custom_entity_id',
        Sequelize.INTEGER
      ),
      queryInterface.addColumn(
        'tableName',
        'custom_entity_field_id',
        Sequelize.INTEGER
      ),
    ]);
  },

  down: (queryInterface) => {
    return Promise.all([
      queryInterface.removeColumn('tableName', 'custom_entity_id'),
      queryInterface.removeColumn('tableName', 'custom_entity_field_id'),
    ]);
  },
};

And this fixed things. I didn't need to do the drops in any order though, but I assume if I had to, say, drop tables that had constraints on each other I'd need to do promise chaining to make sure they went in a specific order. Thanks again for the assistance.

Zechnophobe avatar Sep 22 '17 17:09 Zechnophobe

@Zechnophobe Yup! That works too. Glad you figured it out.

harmon avatar Sep 22 '17 18:09 harmon

This is quite interesting and should be documented in the guide. I'm just testing out Sequelize and learning about making DB changes etc. and have run into this issue with even a single command.

My migration file as follows works when migrating up, but not down (no error thrown):

'use strict';
module.exports = {
  up: (queryInterface, Sequelize) => {
    return queryInterface.addColumn(
      'users',
      'test_column',
      {
        type: Sequelize.STRING,
        allowNull: true,
        defaultValue: 'yo new column brother',
        after: 'password'
      }
    );
  },
  down: (queryInterface, Sequelize) => {
    queryInterface.removeColumn('users', 'test_column');
  }
};

Simply adding return statements however, and it works.

'use strict';
module.exports = {
  up: (queryInterface, Sequelize) => {
    return queryInterface.addColumn(
      'users',
      'test_column',
      {
        type: Sequelize.STRING,
        allowNull: true,
        defaultValue: 'yo new column brother',
        after: 'password'
      }
    );
  },
  down: (queryInterface, Sequelize) => {
    return queryInterface.removeColumn('users', 'test_column');
  }
};

shane-deveire avatar Oct 12 '18 10:10 shane-deveire

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jun 21 '20 20:06 stale[bot]