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 return array failed to execute #185

Closed
greatghoul opened this issue Dec 5, 2018 · 16 comments · Fixed by #325
Closed

migration return array failed to execute #185

greatghoul opened this issue Dec 5, 2018 · 16 comments · Fixed by #325

Comments

@greatghoul
Copy link

error details

== 20180828102922-add-sponsor-intro-to-lotteries: migrating =======

ERROR: Error: Migration 20180828102922-add-sponsor-intro-to-lotteries.js (or wrapper) didn't return a promise
    at /Users/greatghoul/Workspace/Miidii/Sweetfoot/node_modules/umzug/lib/migration.js:132:15
    at Generator.next (<anonymous>)
    at step (/Users/greatghoul/Workspace/Miidii/Sweetfoot/node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
    at /Users/greatghoul/Workspace/Miidii/Sweetfoot/node_modules/babel-runtime/helpers/asyncToGenerator.js:28:13
    at process._tickCallback (internal/process/next_tick.js:68:7)

migration file

'use strict';

module.exports = {
  up: (queryInterface, Sequelize) => {
    return [
      queryInterface.addColumn('Lotteries', 'sponsor_intro', {
        type: Sequelize.TEXT,
        allowNull: true,
      }),
      queryInterface.addColumn('Lotteries', 'prefs', {
        type: Sequelize.JSONB,
        allowNull: false,
        defaultValue: {},
      }),
      queryInterface.addColumn('Lotteries', 'award_usage', {
        type: Sequelize.TEXT,
        allowNull: true,
      }),
      queryInterface.addColumn('Lotteries', 'award_price', {
        type: Sequelize.STRING,
        allowNull: true,
      }),
    ];
  },

  down: (queryInterface, Sequelize) => {
    return [
      queryInterface.removeColumn('Lotteries', 'sponsor_intro'),
      queryInterface.removeColumn('Lotteries', 'prefs'),
      queryInterface.removeColumn('Lotteries', 'award_usage'),
      queryInterface.removeColumn('Lotteries', 'award_price'),
    ];
  }
};
@Lax
Copy link

Lax commented Dec 8, 2018

I guess it should be using Promise instead of Array.

return queryInterface.addColumn(...)
             .then(() => {
               return queryInterface.addColumn(...)
             })
             .then(...)

@greatghoul
Copy link
Author

promise style always works, but array style is now broken.

@akenger
Copy link

akenger commented Dec 12, 2018

Ran into this today as well...

@spathon
Copy link

spathon commented Dec 13, 2018

Was an easy fix for me to just change return [ to return Promise.all([ and ending ])
Should probably have been a Major semver update as it's a breaking change?

@greatghoul
Copy link
Author

I locked umzug to 2.1.0.

{
  "dependencies": {
    ...
    "umzug": "2.1.0",
    ...
  }
}

@samathan
Copy link

Hi,

Below is my migration script,
'use strict';

module.exports = {
up: (queryInterface, Sequelize) => {
return
queryInterface.addColumn( 'tablename', 'columnname1', Sequelize.BLOB)
.then(() => {
return queryInterface.addColumn( 'tablename', 'columnname2', Sequelize.STRING);
})
},

down: (queryInterface, Sequelize) => {
return
queryInterface.removeColumn( 'tablename1', 'columnname1' )
.then(() => {
return queryInterface.removeColumn( 'tablename1', 'columnname2' );
});
}
}
Below error while executing the above migration ,
== 20181203091310-nameassets-addcolumns: migrating =======

ERROR: Migration 20181203091310-nameassets-addcolumns.js (or wrapper) didn't return a promise

Can some help me with this?
Thank you.

@greatghoul
Copy link
Author

'use strict';

module.exports = {
  up: (queryInterface, Sequelize) => {
    return // <---- it returns here, code below this line not executed.
    queryInterface.addColumn('tablename', 'columnname1', Sequelize.BLOB)
      .then(() => {
        return queryInterface.addColumn('tablename', 'columnname2', Sequelize.STRING);
      })
  },

  down: (queryInterface, Sequelize) => {
    return
    queryInterface.removeColumn('tablename1', 'columnname1')
      .then(() => {
        return queryInterface.removeColumn('tablename1', 'columnname2');
      });
  }
}

@samathan you should change it to

'use strict';

module.exports = {
  up: (queryInterface, Sequelize) => {
    return queryInterface.addColumn('tablename', 'columnname1', Sequelize.BLOB)
      .then(() => {
        return queryInterface.addColumn('tablename', 'columnname2', Sequelize.STRING);
      })
  },

  down: (queryInterface, Sequelize) => {
    return queryInterface.removeColumn('tablename1', 'columnname1')
      .then(() => {
        return queryInterface.removeColumn('tablename1', 'columnname2');
      });
  }
}

@Ricardonacif
Copy link

Question is: do we wanna keep the array support or are we dropping it?

@greatghoul
Copy link
Author

I vote for the array support.

@toddpla
Copy link

toddpla commented Apr 15, 2019

After reinstalling version sequelize 4.36.1 & sequelize-cli 4.0.0 then running migrations locally I came across this issue. It would be useful to know whether there is a way to revert back to how it was before this change was implemented. Are there any release notes on this?

@thutv18
Copy link

thutv18 commented May 25, 2019

Hi @toddpla ,

I've tried re-install the sequelize. It still does not work.
This is error:
image
This is my migration:
image

@spathon
Copy link

spathon commented May 27, 2019

@thutv18 You could easily fix your migration by wrapping it in a Promise.all
Eg return Promise.all([ queryInterface.addColumn ... ])

@thutv18
Copy link

thutv18 commented Jun 20, 2019

Thank @spathon ,
I knew this solution. But, i'm working on the existed database with many files migration. So the effort to update all files is very much.
However, I found other solution for this case. Using "node_modules/.bin/sequelize db:migrate" to run. It works for me

dennissivia pushed a commit to integrations/slack that referenced this issue Aug 21, 2019
* Update direct dependencies and fix migrations

Updating sequelize to a newer minor version broke our migrations since
they did not return promises in case of multiple changes. I fixed this
in the least intrusive way by wrapping the commands in promises.
See sequelize/umzug#185 for all details.


* Run npm audit fix

Apply npm audit fix to make ensure all low-hanging security patches
are applied.

* Remove `async` from describe.

Having `async` in `describe` statements causes the following error:

```
● Test suite failed to run

  Returning a Promise from "describe" is not supported. Tests must be defined synchronously.
  Returning a value from "describe" will fail the test in a future version of Jest.
```
Thus I removed it and the tests pass without that issue.

* Disable issue comment test due to stability issues 💔

Currently comments are processed more than once under certain
circumstances. This happens even more often on test/CI. Thus I am
disabling this test for now to unblock pending PRs.

I am going to fix this bug and re-enable the tests ASAP.
See #914 for details.
@enbermudas
Copy link

This error is happening to me while using the db:seed:undo:all command. The Promise.all([...]) solution is not working either.

Sequelize version: ^5.21.5

Current code:

const baseRecord = {
  description: 'Description...',
  createdAt: new Date(),
  updatedAt: new Date(),
};

const records = ['Fire', 'Water', 'Earth', 'Wind'];

module.exports = {
  up: queryInterface => {
    return queryInterface.bulkInsert(
      'Elements',
      records.map(name => ({ name, ...baseRecord })),
    );
  },

  down: () => queryInterface =>
    Promise.all([queryInterface.bulkDelete('Elements', null, {})]),
};

@sushantdhiman
Copy link
Contributor

@enbermudas down function is returning another function instead of promise. Here is the correct version

down: queryInterface =>
    Promise.all([queryInterface.bulkDelete('Elements', null, {})]),

@papb papb self-assigned this Mar 28, 2020
@papb papb removed their assignment Apr 20, 2020
@mmkal mmkal mentioned this issue Oct 1, 2020
5 tasks
@mmkal mmkal closed this as completed in #325 Oct 7, 2020
mmkal added a commit that referenced this issue Oct 7, 2020
Closes #233 (replacement PR)
Closes #106 - multiple folders now supported via globbing
Closes #169 - shouldn't be as necessary anymore, but potentially further work could be done to make it more convenient to handle many different kinds of migrations
Closes #188 - migration class removed
Closes #193 - since we're now using `glob`, we can find migrations in symlinked directories. If necessary, in a follow-up we can expose the symlink-related glob options, but there are gotchas there so let's wait for a user request

Fixes #302 - `pattern` option is removed in favour of explicit globs
Fixes #259 - user is now responsible for globbing/ignoring migration files
Closes #185 - although closes as "wontfix" - array support is still gone. `Promise.all` should be used
Fixes #171
Touches on #167 - but should continue discussion there as this still doesn't introduce a config option for silently skipping already-applied migrations that are explicitly specified
Fixes #33

This is more-or-less a rewrite of the `Umzug` class, consolidating and simplifying several options. 

Minimal usage now:

```js
import { Umzug } from 'umzug'

const umzug = new Umzug({
  migrations: {
    glob: 'path/to/migrations/*.js'
  },
  logger: console,
})
```

Note: the `umzug.ts` file is collapsed in GitHub's diff, but that's where the main change is, so it should be opened!

TODO:
- [x] audit existing issues - many can likely be closed by this
- [ ] decide on whether we should make sure consumer is returning a promise. increases test overhead but stops users shooting themselves in the foot. see #233 (comment)
- [ ] decide if we want to allow no-oping already-run migrations per #167
- [x] narrow logger interface to only take string messages - can be widened in future, but not narrowed
- [ ] document how to support multiple folders. this is a common feature request and globbing supports it via `'{path1/*.js,path2/*.js}'`

Summary of the changes, from the readme in the changeset:

___

The Umzug class should be imported as a named import, i.e. `import { Umzug } from 'umzug'`.

The `MigrationMeta` type, which is returned by `umzug.executed()` and `umzug.pending()`, no longer has a `file` property - it has a `name` and *optional* `path` - since migrations are not necessarily bound to files on the file system.

The `migrations.glob` parameter replaces `path`, `pattern` and `traverseDirectories`. It can be used, in combination with `cwd` and `ignore` to do much more flexible file lookups. See https://npmjs.com/package/glob for more information on the syntax.

The `migrations.resolve` parameter replaces `customResolver`. Explicit support for `wrap` and `nameFormatter` has been removed - these can be easily implemented in a `resolve` function.

The constructor option `logging` is replaced by `logger` to allow for `warn` and `error` messages in future. NodeJS's global `console` object can be passed to this. To disable logging, replace `logging: false` with `logger: undefined`.

The `Umzug#execute` method is removed. Use `Umzug#up` or `Umzug#down`.

The options for `Umguz#up` and `Umzug#down` have changed:
- `umzug.up({ to: 'some-name' })` and `umzug.down({ to: 'some-name' })` are still valid.
- `umzug.up({ from: '...' })` and `umzug.down({ from: '...' })` are no longer supported. To run migrations out-of-order (which is not generally recommended), you can explicitly use `umzug.up({ migrations: ['...'] })` and `umzug.down({ migrations: ['...'] })`.
- name matches must be exact. `umzug.up({ to: 'some-n' })` will no longer match a migration called `some-name`.
- `umzug.down({ to: 0 })` is still valid but `umzug.up({ to: 0 })` is not.
- `umzug.up({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.up(['m1', 'm2'])` has been removed.
- `umzug.down({ migrations: ['m1', 'm2'] })` is still valid but the shorthand `umzug.down(['m1', 'm2'])` has been removed.
- `umzug.up({ migrations: ['m1', 'already-run'] })` will throw an error, if `already-run` is not found in the list of pending migrations.
- `umzug.down({ migrations: ['m1', 'has-not-been-run'] })` will throw an error, if `has-not-been-run` is not found in the list of executed migrations.
- `umzug.up({ migrations: ['m1', 'm2'], force: true })` will re-apply migrations `m1` and `m2` even if they've already been run.
- `umzug.down({ migrations: ['m1', 'm2'], force: true })` will "revert" migrations `m1` and `m2` even if they've never been run.
- `umzug.up({ migrations: ['m1', 'does-not-exist', 'm2'] })` will throw an error if the migration name is not found. Note that the error will be thrown and no migrations run unless _all_ migration names are found - whether or not `force: true` is added.

The `context` parameter replaces `params`, and is passed in as a property to migration functions as an options object, alongs side `name` and `path`. This means the signature for migrations, which in v2 was `(context) => Promise<void>`, has changed slightly in v3, to `({ name, path, context }) => Promise<void>`. The `resolve` function can also be used to upgrade your umzug version to v3 when you have existing v2-compatible migrations:

```js
const { Umzug } = require('umzug');

const umzug = new Umzug({
  migrations: {
    glob: 'migrations/umzug-v2-format/*.js',
    resolve: ({name, path, context}) => {
      // Adjust the migration from the new signature to the v2 signature, making easier to upgrade to v3
      const migration = require(path)
      return { up: async () => migration.up(context), down: async () => migration.down(context) }
    }
  },
  context: sequelize.getQueryInterface(),
  logger: console,
});
```

Similarly, you no longer need `migrationSorting`, you can use `Umzug#extend` to manipulate migration lists directly:

```js
const { Umzug } = require('umzug');

const umzug =
  new Umzug({
    migrations: { glob: 'migrations/**/*.js' },
    context: sequelize.getQueryInterface(),
  })
  .extend(migrations => migrations.sort((a, b) => b.path.localeCompare(a.path)));
```
@renanmachad
Copy link

The use of Promise.all ( [ queryInterfaces here] ) , resolves the problem.

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

Successfully merging a pull request may close this issue.