db-migrate / node-db-migrate

Database migration framework for node
Other
2.32k stars 360 forks source link

createTable errors out when you specify length and autoincrement in postgres #323

Closed zilkey closed 6 years ago

zilkey commented 8 years ago

When using the postgres driver, and using the migration from the docs:

'use strict';

var dbm;
var type;
var seed;

/**
  * We receive the dbmigrate dependency from dbmigrate initially.
  * This enables us to not have to rely on NODE_PATH.
  */
exports.setup = function(options, seedLink) {
  dbm = options.dbmigrate;
  type = dbm.dataType;
  seed = seedLink;
};

exports.up = function(db) {
  db.createTable( 'product_variant',
  {
      id:
      {
        type: 'int',
        unsigned: true,
        notNull: true,
        primaryKey: true,
        autoIncrement: true,
        length: 10
      },
      product_id:
      {
        type: 'int',
        unsigned: true,
        length: 10,
        notNull: true,
        foreignKey: {
          name: 'product_variant_product_id_fk',
          table: 'product',
          rules: {
            onDelete: 'CASCADE',
            onUpdate: 'RESTRICT'
          },
          mapping: 'id'
        }
      },
  });
  return null;
};

exports.down = function(db) {
  return null;
};

And you run it with node node_modules/.bin/db-migrate up you'll get:

[ERROR] error: syntax error at or near "("
    at Connection.parseE (/Users/galvanize/workspace/exercises/distributed-memories-services/node_modules/pg/lib/connection.js:539:11)
    at Connection.parseMessage (/Users/galvanize/workspace/exercises/distributed-memories-services/node_modules/pg/lib/connection.js:366:17)
    at Socket.<anonymous> (/Users/galvanize/workspace/exercises/distributed-memories-services/node_modules/pg/lib/connection.js:105:22)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at TCP.onread (net.js:523:20)

When you run it with node node_modules/.bin/db-migrate up --dry-run you'll see:

CREATE TABLE  "product_variant" ("id"  (10) SERIAL PRIMARY KEY NOT NULL, "product_id" INTEGER (10) NOT NULL);

Notice the "id" (10) in there. It's caused by this line:

https://github.com/db-migrate/pg/blob/master/index.js#L43

var type = spec.autoIncrement ? '' : this.mapDataType(spec.type);
var len = spec.length ? util.format('(%s)', spec.length) : '';

If autoincrement is true, type is an empty string. If you leave the limit off, it produces this:

CREATE TABLE  "product_variant" ("id"   SERIAL PRIMARY KEY NOT NULL, "product_id" INTEGER (10) NOT NULL);

Solution 1

Raise an error when both are specified.

There's no real reason to specify a length and autoincrement. Postgres allows you to specify bigserial as a datatype, but does not (to my knowledge) allow you to specify a serial/bigserial table with a length.

Solution 2

Silently ignore / warn users when length + autoincrement are specified in postgres.

Solution 3

Any other ideas?


Regardless, it seems like we should update the docs to not use the combination, since it's a bummer for new users when they copy the example and it blows up.

I'm happy to write the tests / submit the pull request / update the docs, but would like some guidance on how to handle this case.

Thanks!

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/28018706-createtable-errors-out-when-you-specify-length-and-autoincrement-in-postgres?utm_campaign=plugin&utm_content=tracker%2F73887&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F73887&utm_medium=issues&utm_source=github).
wzrdtales commented 8 years ago

You're using the migrations in a wrong way!

This should be like this instead:

'use strict';

var dbm;
var type;
var seed;

/**
  * We receive the dbmigrate dependency from dbmigrate initially.
  * This enables us to not have to rely on NODE_PATH.
  */
exports.setup = function(options, seedLink) {
  dbm = options.dbmigrate;
  type = dbm.dataType;
  seed = seedLink;
};

exports.up = function(db) {
  return db.createTable( 'product_variant',
  {
      id:
      {
        type: 'int',
        unsigned: true,
        notNull: true,
        primaryKey: true,
        autoIncrement: true,
        length: 10
      },
      product_id:
      {
        type: 'int',
        unsigned: true,
        length: 10,
        notNull: true,
        foreignKey: {
          name: 'product_variant_product_id_fk',
          table: 'product',
          rules: {
            onDelete: 'CASCADE',
            onUpdate: 'RESTRICT'
          },
          mapping: 'id'
        }
      },
  });
};

exports.down = function(db) {
  return null;
};

the createTable method returns a Promise, also to ask: Do you use v0.10.x-beta already, this is not true for v0.9.x though.

And yes you're absolutely right, I'm currently mainly focusing on the docs and think a lot about how to structure them better and make the entrance as easy as possible. Up to the final release of v0.10.x the docs should be up to date though.

wzrdtales commented 8 years ago

For the real issue with this issue, this needs that to be added to the pg driver.

andris310 commented 7 years ago

this bug is still hapening

stale[bot] commented 6 years ago

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.