fix: support changing comments in MySQL columns (#6903)

in the mysql driver, column comments were only half supported -
adding them to new columns & new tables was possible, but updating
existing columns to include comments was not possible

there also was no escaping done on the column comments - so depending
on the contents of your comment, an invalid query could be generated

adds support to add comments to existing columns, adds simple comment
escaping code, & creates tests to verify the behavior

src/driver/mysql/MysqlDriver.ts

@@ -776,7 +776,7 @@ export class MysqlDriver implements Driver {
                 || tableColumn.unsigned !== columnMetadata.unsigned
                 || tableColumn.asExpression !== columnMetadata.asExpression
                 || tableColumn.generatedType !== columnMetadata.generatedType
-                // || tableColumn.comment !== columnMetadata.comment // todo
+                || (tableColumn.comment || "") !== columnMetadata.comment
                 || !this.compareDefaultValues(this.normalizeDefault(columnMetadata), tableColumn.default)
                 || (tableColumn.enum && columnMetadata.enum && !OrmUtils.isArraysEqual(tableColumn.enum, columnMetadata.enum.map(val => val + "")))
                 || tableColumn.onUpdate !== columnMetadata.onUpdate

src/driver/mysql/MysqlQueryRunner.ts

@@ -637,7 +637,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
                 oldColumn.name = newColumn.name;
             }
 
-            if (this.isColumnChanged(oldColumn, newColumn, true)) {
+            if (this.isColumnChanged(oldColumn, newColumn, true, true)) {
                 upQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} CHANGE \`${oldColumn.name}\` ${this.buildCreateColumnSql(newColumn, true)}`));
                 downQueries.push(new Query(`ALTER TABLE ${this.escapePath(table)} CHANGE \`${newColumn.name}\` ${this.buildCreateColumnSql(oldColumn, true)}`));
             }
@@ -1483,7 +1483,7 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
                     if (tableColumn.isGenerated)
                         tableColumn.generationStrategy = "increment";
 
-                    tableColumn.comment = dbColumn["COLUMN_COMMENT"];
+                    tableColumn.comment = (typeof dbColumn["COLUMN_COMMENT"] === "string" && dbColumn["COLUMN_COMMENT"].length === 0) ? undefined : dbColumn["COLUMN_COMMENT"];
                     if (dbColumn["CHARACTER_SET_NAME"])
                         tableColumn.charset = dbColumn["CHARACTER_SET_NAME"] === defaultCharset ? undefined : dbColumn["CHARACTER_SET_NAME"];
                     if (dbColumn["COLLATION_NAME"])
@@ -1783,6 +1783,22 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
         };
     }
 
+    /**
+     * Escapes a given comment so it's safe to include in a query.
+     */
+    protected escapeComment(comment?: string) {
+        if (comment === undefined || comment.length === 0) {
+            return `''`;
+        }
+
+        comment = comment
+            .replace("\\", "\\\\") // MySQL allows escaping characters via backslashes
+            .replace("'", "''")
+            .replace("\0", ""); // Null bytes aren't allowed in comments
+
+        return `'${comment}'`;
+    }
+
     /**
      * Escapes given table or view path.
      */
@@ -1824,8 +1840,8 @@ export class MysqlQueryRunner extends BaseQueryRunner implements QueryRunner {
             c += " PRIMARY KEY";
         if (column.isGenerated && column.generationStrategy === "increment") // don't use skipPrimary here since updates can update already exist primary without auto inc.
             c += " AUTO_INCREMENT";
-        if (column.comment)
-            c += ` COMMENT '${column.comment.replace("'", "''")}'`;
+        if (column.comment !== undefined && column.comment.length > 0)
+            c += ` COMMENT ${this.escapeComment(column.comment)}`;
         if (column.default !== undefined && column.default !== null)
             c += ` DEFAULT ${column.default}`;
         if (column.onUpdate)

test/functional/columns/comments/columns-comments.ts

@@ -0,0 +1,34 @@
+import {expect} from "chai";
+import "reflect-metadata";
+import {Connection} from "../../../../src";
+import {closeTestingConnections, createTestingConnections, reloadTestingDatabases} from "../../../utils/test-utils";
+import {Test} from "./entity/Test";
+
+describe("columns > comments", () => {
+
+    let connections: Connection[];
+    before(async () => connections = await createTestingConnections({
+        entities: [Test],
+        // Only supported on mysql
+        enabledDrivers: ["mysql"]
+    }));
+    beforeEach(() => reloadTestingDatabases(connections));
+    after(() => closeTestingConnections(connections));
+
+    it("should persist comments of different types to the database", () => Promise.all(connections.map(async connection => {
+        const table = (await connection.createQueryRunner().getTable("test"))!;
+
+        expect(table.findColumnByName("a")!.comment).to.be.equal("Hello World")
+        expect(table.findColumnByName("b")!.comment).to.be.equal("Hello\nWorld")
+        expect(table.findColumnByName("c")!.comment).to.be.equal("Hello World! It's going to be a beautiful day.")
+        expect(table.findColumnByName("d")!.comment).to.be.equal("Hello World! #@!$`")
+        expect(table.findColumnByName("e")!.comment).to.be.equal("Hello World. \r\n\t\b\f\v")
+        expect(table.findColumnByName("f")!.comment).to.be.equal("Hello World.\\")
+        expect(table.findColumnByName("g")!.comment).to.be.equal(" ")
+        expect(table.findColumnByName("h")!.comment).to.be.equal(undefined);
+        expect(table.findColumnByName("i")!.comment).to.be.equal(undefined);
+
+    })));
+
+
+});

test/functional/columns/comments/entity/Test.ts

@@ -0,0 +1,46 @@
+import {Entity} from "../../../../../src/decorator/entity/Entity";
+import {Column} from "../../../../../src/decorator/columns/Column";
+import {PrimaryGeneratedColumn} from "../../../../../src/decorator/columns/PrimaryGeneratedColumn";
+
+@Entity()
+export class Test {
+
+    @PrimaryGeneratedColumn()
+    id: number;
+
+    // Standard Comment
+    @Column({ comment: "Hello World"})
+    a: string;
+
+    // Comment with a newline
+    @Column({ comment: "Hello\nWorld"})
+    b: string;
+
+    // Comment with a single quote
+    @Column({ comment: "Hello World! It's going to be a beautiful day."})
+    c: string;
+
+    // Comment with special characters
+    @Column({ comment: "Hello World! #@!$`"})
+    d: string;
+
+    // Comment with control characters
+    @Column({ comment: "Hello World. \r\n\t\b\f\v\0"})
+    e: string;
+
+    // Comment that ends with a backslash
+    @Column({ comment: "Hello World.\\"})
+    f: string;
+
+    // Comment that is only whitespace
+    @Column({ comment: " "})
+    g: string;
+
+    // Comment that is empty
+    @Column({ comment: ""})
+    h: string;
+
+    // No comment.
+    @Column()
+    i: string;
+}

test/functional/migrations/generate-command/command.ts

@@ -7,7 +7,7 @@ describe("migrations > generate command", () => {
     let connections: Connection[];
     before(async () => connections = await createTestingConnections({
         migrations: [],
-        enabledDrivers: ["postgres"],
+        enabledDrivers: ["postgres", "cockroachdb", "mysql"],
         schemaCreate: false,
         dropSchema: true,
         entities: [Post, Category],

test/functional/schema-builder/change-column.ts

@@ -248,4 +248,35 @@ describe("schema builder > change column", () => {
 
     })));
 
+    it("should correctly change column comment", () => Promise.all(connections.map(async connection => {
+        // Skip thie contents of this test if not one of the drivers that support comments
+        if (!(connection.driver instanceof MysqlDriver)) {
+            return;
+        }
+
+        const teacherMetadata = connection.getMetadata("teacher");
+        const idColumn = teacherMetadata.findColumnWithPropertyName("id")!;
+
+        idColumn.comment = "The Teacher's Key";
+
+        await connection.synchronize();
+
+        const queryRunnerA = connection.createQueryRunner();
+        const teacherTableA = await queryRunnerA.getTable("teacher");
+        await queryRunnerA.release();
+
+        expect(teacherTableA!.findColumnByName("id")!.comment).to.be.equal("The Teacher's Key", connection.name);
+
+        // revert changes
+        idColumn.comment = "";
+
+        await connection.synchronize();
+
+        const queryRunnerB = connection.createQueryRunner();
+        const teacherTableB = await queryRunnerB.getTable("teacher");
+        await queryRunnerB.release();
+
+        expect(teacherTableB!.findColumnByName("id")!.comment).to.be.undefined;
+
+    })));
 });