fix: drop column if not defined in attributes when alter table (#296)

also (partially) added column comment support to close #294
cyjake · Mar 22, 2022 · 17733f6 · 17733f6
1 parent 90bb2c8
commit 17733f6
Show file tree

Hide file tree

Showing 15 changed files with 173 additions and 40 deletions.
diff --git a/src/bone.js b/src/bone.js
@@ -37,10 +37,12 @@ function looseReadonly(props) {
 
 function compare(attributes, columnMap) {
   const diff = {};
+  const columnNames = new Set();
 
   for (const name in attributes) {
     const attribute = attributes[name];
     const { columnName } = attribute;
+    columnNames.add(columnName);
 
     if (!attribute.equals(columnMap[columnName])) {
       diff[name] = {
@@ -50,6 +52,12 @@ function compare(attributes, columnMap) {
     }
   }
 
+  for (const columnName in columnMap) {
+    if (!columnNames.has(columnName)) {
+      diff[columnName] = { remove: true };
+    }
+  }
+
   return diff;
 }
 
@@ -254,8 +262,8 @@ class Bone {
 
   /**
    * get actual update/insert columns to avoid empty insert or update
-   * @param {Object} data 
-   * @returns 
+   * @param {Object} data
+   * @returns
    */
   static _getColumns(data) {
     if (!Object.keys(data).length) return data;

diff --git a/src/data_types.js b/src/data_types.js
@@ -17,17 +17,20 @@ class DataType {
     const {
       STRING, TEXT,
       DATE, DATEONLY,
-      TINYINT, SMALLINT, MEDIUMINT, INTEGER, BIGINT,
+      TINYINT, SMALLINT, MEDIUMINT, INTEGER, BIGINT, DECIMAL,
       BOOLEAN,
       BINARY, VARBINARY, BLOB,
     } = this;
-    const [ , dataType, appendix ] = columnType.match(/(\w+)(?:\((\d+)\))?/);
-    const length = appendix && parseInt(appendix, 10);
+    const [ , dataType, ...matches ] = columnType.match(/(\w+)(?:\((\d+)(?:,(\d+))?\))?/);
+    const params = [];
+    for (let i = 0; i < matches.length; i++) {
+      if (matches[i] != null) params[i] = parseInt(matches[i], 10);
+    }
 
     switch (dataType) {
       case 'varchar':
       case 'char':
-        return new STRING(length);
+        return new STRING(...params);
       // longtext is only for MySQL
       case 'longtext':
         return new TEXT('long');
@@ -40,30 +43,31 @@ class DataType {
       case 'datetime':
       case 'timestamp':
         // new DATE(precision)
-        return new DATE(length);
+        return new DATE(...params);
       case 'decimal':
+        return new DECIMAL(...params);
       case 'int':
       case 'integer':
       case 'numeric':
-        return new INTEGER(length);
+        return new INTEGER(...params);
       case 'mediumint':
-        return new MEDIUMINT(length);
+        return new MEDIUMINT(...params);
       case 'smallint':
-        return new SMALLINT(length);
+        return new SMALLINT(...params);
       case 'tinyint':
-        return new TINYINT(length);
+        return new TINYINT(...params);
       case 'bigint':
-        return new BIGINT(length);
+        return new BIGINT(...params);
       case 'boolean':
         return new BOOLEAN();
       // mysql only
       case 'binary':
       // postgres only
       case 'bytea':
-        return new BINARY(length);
+        return new BINARY(...params);
       // mysql only
       case 'varbinary':
-        return new VARBINARY(length);
+        return new VARBINARY(...params);
       case 'longblob':
         return new BLOB('long');
       case 'mediumblob':
@@ -273,6 +277,41 @@ class BIGINT extends INTEGER {
   }
 }
 
+/**
+ * fixed-point decimal types
+ * @example
+ * DECIMAL
+ * DECIMAL.UNSIGNED
+ * DECIMAL(5, 2)
+ * @param {number} precision
+ * @param {number} scale
+ * - https://dev.mysql.com/doc/refman/8.0/en/fixed-point-types.html
+ */
+class DECIMAL extends INTEGER {
+  constructor(precision, scale) {
+    super();
+    this.dataType = 'decimal';
+    this.precision = precision;
+    this.scale = scale;
+  }
+
+  toSqlString() {
+    const { precision, scale, unsigned, zerofill } = this;
+    const dataType = this.dataType.toUpperCase();
+    const chunks = [];
+    if (precision > 0 && scale >= 0) {
+      chunks.push(`${dataType}(${precision},${scale})`);
+    } else if (precision > 0) {
+      chunks.push(`${dataType}(${precision})`);
+    } else {
+      chunks.push(dataType);
+    }
+    if (unsigned) chunks.push('UNSIGNED');
+    if (zerofill) chunks.push('ZEROFILL');
+    return chunks.join(' ');
+  }
+}
+
 const rDateFormat = /^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}(?:[,.]\d{3,6})$/;
 
 class DATE extends DataType {
@@ -494,6 +533,7 @@ const DataTypes = {
   MEDIUMINT,
   INTEGER,
   BIGINT,
+  DECIMAL,
   DATE,
   DATEONLY,
   BOOLEAN,

diff --git a/src/drivers/abstract/attribute.js b/src/drivers/abstract/attribute.js
@@ -66,13 +66,17 @@ function createType(DataTypes, params) {
   switch (type.constructor.name) {
     case 'DATE':
       return new DataType(type.precision, type.timezone);
+    case 'DECIMAL':
+      return new DataType(type.precision, type.scale);
     case 'TINYINT':
     case 'SMALLINT':
     case 'MEDIUMINT':
     case 'INTEGER':
     case 'BIGINT':
     case 'BINARY':
     case 'VARBINARY':
+    case 'CHAR':
+    case 'VARCHAR':
       return new DataType(type.length);
     default:
       return new DataType();

diff --git a/src/drivers/abstract/schema.js b/src/drivers/abstract/schema.js
@@ -19,9 +19,12 @@ module.exports = {
     const chunks = [ `ALTER TABLE ${escapeId(table)}` ];
 
     const actions = Object.keys(attributes).map(name => {
-      const attribute = new Attribute(name, attributes[name]);
+      const options = attributes[name];
+      // { [columnName]: { remove: true } }
+      if (options.remove) return `DROP COLUMN ${escapeId(name)}`;
+      const attribute = new Attribute(name, options);
       return [
-        attribute.modify ? 'MODIFY COLUMN' : 'ADD COLUMN',
+        options.modify ? 'MODIFY COLUMN' : 'ADD COLUMN',
         attribute.toSqlString(),
       ].join(' ');
     });

diff --git a/src/drivers/mysql/attribute.js b/src/drivers/mysql/attribute.js
@@ -15,8 +15,13 @@ class MysqlAttribute extends Attribute {
   }
 
   toSqlString() {
-    const { allowNull, defaultValue, primaryKey } = this;
-    const { columnName, type, columnType } = this;
+    const {
+      columnName,
+      type, columnType,
+      allowNull, defaultValue,
+      primaryKey,
+      comment,
+    } = this;
 
     const chunks = [
       escapeId(columnName),
@@ -33,6 +38,10 @@ class MysqlAttribute extends Attribute {
       chunks.push(`DEFAULT ${escape(defaultValue)}`);
     }
 
+    if (typeof comment === 'string') {
+      chunks.push(`COMMENT ${escape(comment)}`);
+    }
+
     return chunks.join(' ');
   }
 }

diff --git a/src/drivers/mysql/schema.js b/src/drivers/mysql/schema.js
@@ -38,7 +38,7 @@ module.exports = {
       columns.push({
         columnName: row.column_name,
         columnType: row.column_type,
-        columnComment: row.column_comment,
+        comment: row.column_comment,
         defaultValue: row.column_default,
         dataType: row.data_type,
         allowNull: row.is_nullable === 'YES',

diff --git a/src/drivers/postgres/schema.js b/src/drivers/postgres/schema.js
@@ -28,6 +28,10 @@ function formatAddColumn(driver, columnName, attribute) {
   return `ADD COLUMN ${attribute.toSqlString()}`;
 }
 
+function formatDropColumn(driver, columnName) {
+  return `DROP COLUMN ${driver.escapeId(columnName)}`;
+}
+
 module.exports = {
   ...schema,
 
@@ -85,7 +89,9 @@ module.exports = {
     const { escapeId } = this;
     const chunks = [ `ALTER TABLE ${escapeId(table)}` ];
     const actions = Object.keys(changes).map(name => {
-      const attribute = new Attribute(name, changes[name]);
+      const options = changes[name];
+      if (options.remove) return formatDropColumn(this, name);
+      const attribute = new Attribute(name, options);
       const { columnName } = attribute;;
       return attribute.modify
         ? formatAlterColumns(this, columnName, attribute).join(', ')

diff --git a/src/drivers/sqlite/schema.js b/src/drivers/sqlite/schema.js
@@ -108,7 +108,8 @@ module.exports = {
     for (let i = 0; i < tables.length; i++) {
       const table = tables[i];
       const { rows } = results[i];
-      const columns = rows.map(({ name, type, notnull, dflt_value, pk }) => {
+      const columns = rows.map(row => {
+        const { name, type, notnull, dflt_value, pk } = row;
         const columnType = type.toLowerCase();
         const [, dataType, precision ] = columnType.match(rColumnType);
         const primaryKey = pk === 1;
@@ -145,6 +146,8 @@ module.exports = {
     const { escapeId } = this;
     const chunks = [ `ALTER TABLE ${escapeId(table)}` ];
     const attributes = Object.keys(changes).map(name => {
+      const options = changes[name];
+      if (options.remove) return { columnName: name, remove: true };
       return new Attribute(name, changes[name]);
     });
 
@@ -157,7 +160,12 @@ module.exports = {
     // SQLite can only add one column a time
     // - https://www.sqlite.org/lang_altertable.html
     for (const attribute of attributes) {
-      await this.query(chunks.concat(`ADD COLUMN ${attribute.toSqlString()}`).join(' '));
+      if (attribute.remove) {
+        const { columnName } = attribute;
+        await this.query(chunks.concat(`DROP COLUMN ${this.escapeId(columnName)}`).join(' '));
+      } else {
+        await this.query(chunks.concat(`ADD COLUMN ${attribute.toSqlString()}`).join(' '));
+      }
     }
   },
 

diff --git a/test/integration/suite/basics.test.js b/test/integration/suite/basics.test.js
@@ -344,7 +344,7 @@ describe('=> Basic', () => {
       // should return false after first persisting
       expect(user.previousChanged('realname')).to.be(true);
       assert.deepEqual(user.previousChanged().sort(), [ 'id', 'email', 'nickname', 'status', 'level', 'createdAt', 'realname' ].sort());
-      assert.deepEqual(user.previousChanges(), { 
+      assert.deepEqual(user.previousChanges(), {
         id: [ null, user.id ],
         email: [ null, '[email protected]' ],
         level: [ null, 1 ],
@@ -386,7 +386,7 @@ describe('=> Basic', () => {
       const post = new Post({ title: 'Untitled' });
       assert.deepEqual(post.previousChanges(), {});
       await post.save();
-      assert.deepEqual(post.previousChanges(), { 
+      assert.deepEqual(post.previousChanges(), {
         id: [ null, post.id ],
         createdAt: [ null, post.createdAt ],
         isPrivate: [ null, Post.driver.type === 'mysql'? 0 : false ] ,
@@ -930,7 +930,7 @@ describe('=> Basic', () => {
       expect(foundPost2.id).to.equal(post.id);
       expect(foundPost2.updatedAt.getTime()).to.be.above(foundPost1.updatedAt.getTime());
 
-      await Post.update({ title: 'Yhorm' }, { updatedAt: new Date() }, { silent: true });
+      await Post.update({ title: 'Yhorm' }, { updatedAt: Date.now() + 1 }, { silent: true });
       const foundPost3 = await Post.findOne({ title: 'Yhorm' });
       expect(foundPost3.id).to.equal(post.id);
       expect(foundPost3.updatedAt.getTime()).to.be.above(foundPost2.updatedAt.getTime());
@@ -968,7 +968,7 @@ describe('=> Basic', () => {
       const updatedAt3 = post.updatedAt.getTime();
       expect(updatedAt3).to.be.above(updatedAt2);
 
-      await post.update({ updatedAt: new Date() }, { silent: true });
+      await post.update({ updatedAt: Date.now() + 1 }, { silent: true });
       await post.reload();
       const updatedAt4 = post.updatedAt.getTime();
       expect(updatedAt4).to.be.above(updatedAt3);
@@ -1230,7 +1230,7 @@ describe('=> Basic', () => {
       const post1 = await Post.findOne('id = ?', posts[0].id).unparanoid;
       assert(post1.deletedAt);
 
-      deleteCount = await Post.remove({ 
+      deleteCount = await Post.remove({
         word_count: {
           $gte: 0
         }

diff --git a/test/integration/suite/definitions.test.js b/test/integration/suite/definitions.test.js
@@ -140,20 +140,22 @@ describe('=> Bone.sync()', () => {
     await Bone.driver.dropTable('notes');
   });
 
-  after(async () => {
-    await Bone.driver.dropTable('notes');
-  });
-
   it('should create table if not exist', async () => {
     class Note extends Bone {};
-    Note.init({ title: STRING, body: TEXT });
+    Note.init({
+      title: { type: STRING, comment: '标题' },
+      body: TEXT,
+    });
     assert(!Note.synchronized);
 
     await Note.sync();
     assert(Note.synchronized);
     assert.equal(Note.table, 'notes');
     await checkDefinitions('notes', {
-      title: { dataType: 'varchar' },
+      title: {
+        dataType: 'varchar',
+        comment: Bone.driver.type === 'mysql' ? '标题' : undefined,
+      },
     });
   });
 
@@ -240,6 +242,24 @@ describe('=> Bone.sync()', () => {
       body: { dataType: 'text' },
     });
   });
+
+  it('should drop column if removed with alter', async () => {
+    await Bone.driver.createTable('notes', {
+      title: { type: STRING, allowNull: false },
+      body: { type: STRING },
+      summary: { type: STRING },
+    });
+    class Note extends Bone {};
+    Note.init({ title: STRING, body: TEXT });
+    assert(!Note.synchronized);
+    await Note.sync({ alter: true });
+    assert(Note.synchronized);
+    await checkDefinitions('notes', {
+      title: { dataType: 'varchar', allowNull: true },
+      body: { dataType: 'text' },
+      summary: null,
+    });
+  });
 });
 
 describe('=> Bone.drop()', () => {

diff --git a/test/unit/bone.test.js b/test/unit/bone.test.js
@@ -411,7 +411,6 @@ describe('=> Bone', function() {
       // MySQL 5.x returns column type with length regardless specified or not
       assert.ok(result.word_count.columnType.startsWith('mediumint'));
       assert(!result.halo);
-
     });
   });
-Original file line number
+Diff line change
@@ Expand Up / @@ -411,7 +411,6 @@ describe('=> Bone', function() { @@
           // MySQL 5.x returns column type with length regardless specified or not
           assert.ok(result.word_count.columnType.startsWith('mediumint'));
           assert(!result.halo);
         });
       });
@@ Expand Down @@