refactor(lib): new `joinSQLFragments` util (#11865)

lib/data-types.js

@@ -10,6 +10,7 @@ const moment = require('moment');
 const { logger } = require('./utils/logger');
 const warnings = {};
 const { classToInvokable } = require('./utils/classToInvokable');
+const { joinSQLFragments } = require('./utils/join-sql-fragments');
 
 class ABSTRACT {
   toString(options) {
@@ -62,7 +63,10 @@ class STRING extends ABSTRACT {
     this._length = options.length || 255;
   }
   toSql() {
-    return `VARCHAR(${this._length})${this._binary ? ' BINARY' : ''}`;
+    return joinSQLFragments([
+      `VARCHAR(${this._length})`,
+      this._binary && 'BINARY'
+    ]);
   }
   validate(value) {
     if (Object.prototype.toString.call(value) !== '[object String]') {
@@ -97,7 +101,10 @@ class CHAR extends STRING {
     super(typeof length === 'object' && length || { length, binary });
   }
   toSql() {
-    return `CHAR(${this._length})${this._binary ? ' BINARY' : ''}`;
+    return joinSQLFragments([
+      `CHAR(${this._length})`,
+      this._binary && 'BINARY'
+    ]);
   }
 }
 

lib/dialects/abstract/query-generator.js

@@ -310,7 +310,19 @@ class QueryGenerator {
       returning += returnValues.returningFragment;
     }
 
-    return `INSERT${ignoreDuplicates} INTO ${this.quoteTable(tableName)} (${attributes}) VALUES ${tuples.join(',')}${onDuplicateKeyUpdate}${onConflictDoNothing}${returning};`;
+    return Utils.joinSQLFragments([
+      'INSERT',
+      ignoreDuplicates,
+      'INTO',
+      this.quoteTable(tableName),
+      `(${attributes})`,
+      'VALUES',
+      tuples.join(','),
+      onDuplicateKeyUpdate,
+      onConflictDoNothing,
+      returning,
+      ';'
+    ]);
   }
 
   /**
@@ -446,7 +458,15 @@ class QueryGenerator {
       updateSetSqlFragments.push(`${quotedField}=${escapedValue}`);
     }
 
-    return `UPDATE ${this.quoteTable(tableName)} SET ${updateSetSqlFragments.join(',')}${outputFragment} ${this.whereQuery(where)}${returningFragment}`.trim();
+    return Utils.joinSQLFragments([
+      'UPDATE',
+      this.quoteTable(tableName),
+      'SET',
+      updateSetSqlFragments.join(','),
+      outputFragment,
+      this.whereQuery(where),
+      returningFragment
+    ]);
   }
 
   /*
@@ -570,16 +590,19 @@ class QueryGenerator {
   }
 
   addConstraintQuery(tableName, options) {
-    options = options || {};
-    const constraintSnippet = this.getConstraintSnippet(tableName, options);
-
     if (typeof tableName === 'string') {
       tableName = this.quoteIdentifiers(tableName);
     } else {
       tableName = this.quoteTable(tableName);
     }
 
-    return `ALTER TABLE ${tableName} ADD ${constraintSnippet};`;
+    return Utils.joinSQLFragments([
+      'ALTER TABLE',
+      tableName,
+      'ADD',
+      this.getConstraintSnippet(tableName, options || {}),
+      ';'
+    ]);
   }
 
   getConstraintSnippet(tableName, options) {
@@ -660,7 +683,12 @@ class QueryGenerator {
       tableName = this.quoteTable(tableName);
     }
 
-    return `ALTER TABLE ${tableName} DROP CONSTRAINT ${this.quoteIdentifiers(constraintName)}`;
+    return Utils.joinSQLFragments([
+      'ALTER TABLE',
+      tableName,
+      'DROP CONSTRAINT',
+      this.quoteIdentifiers(constraintName)
+    ]);
   }
 
   /*
@@ -1486,7 +1514,11 @@ class QueryGenerator {
           alias = this._getMinifiedAlias(alias, includeAs.internalAs, topLevelInfo.options);
         }
 
-        return `${prefix} AS ${this.quoteIdentifier(alias, true)}`;
+        return Utils.joinSQLFragments([
+          prefix,
+          'AS',
+          this.quoteIdentifier(alias, true)
+        ]);
       });
       if (include.subQuery && topLevelInfo.subQuery) {
         for (const attr of includeAttributes) {
@@ -1750,9 +1782,11 @@ class QueryGenerator {
         alias = this._getMinifiedAlias(alias, throughAs, topLevelInfo.options);
       }
 
-      return `${this.quoteIdentifier(throughAs)}.${this.quoteIdentifier(Array.isArray(attr) ? attr[0] : attr)
-      } AS ${
-        this.quoteIdentifier(alias)}`;
+      return Utils.joinSQLFragments([
+        `${this.quoteIdentifier(throughAs)}.${this.quoteIdentifier(Array.isArray(attr) ? attr[0] : attr)}`,
+        'AS',
+        this.quoteIdentifier(alias)
+      ]);
     });
     const association = include.association;
     const parentIsTop = !include.parent.association && include.parent.model.name === topLevelInfo.options.model.name;
@@ -2130,15 +2164,17 @@ class QueryGenerator {
       return `CAST(${result} AS ${smth.type.toUpperCase()})`;
     }
     if (smth instanceof Utils.Fn) {
-      return `${smth.fn}(${smth.args.map(arg => {
-        if (arg instanceof Utils.SequelizeMethod) {
-          return this.handleSequelizeMethod(arg, tableName, factory, options, prepend);
-        }
-        if (_.isPlainObject(arg)) {
-          return this.whereItemsQuery(arg);
-        }
-        return this.escape(typeof arg === 'string' ? arg.replace('$', '$$$') : arg);
-      }).join(', ')})`;
+      return `${smth.fn}(${
+        smth.args.map(arg => {
+          if (arg instanceof Utils.SequelizeMethod) {
+            return this.handleSequelizeMethod(arg, tableName, factory, options, prepend);
+          }
+          if (_.isPlainObject(arg)) {
+            return this.whereItemsQuery(arg);
+          }
+          return this.escape(typeof arg === 'string' ? arg.replace('$', '$$$') : arg);
+        }).join(', ')
+      })`;
     }
     if (smth instanceof Utils.Col) {
       if (Array.isArray(smth.col) && !factory) {

lib/dialects/mariadb/data-types.js

@@ -51,7 +51,7 @@ module.exports = BaseTypes => {
 
   class DATE extends BaseTypes.DATE {
     toSql() {
-      return `DATETIME${this._length ? `(${this._length})` : ''}`;
+      return this._length ? `DATETIME(${this._length})` : 'DATETIME';
     }
     _stringify(date, options) {
       date = this._applyTimezone(date, options);

lib/dialects/mariadb/query-generator.js

 'use strict';
 
 const MySQLQueryGenerator = require('../mysql/query-generator');
+const Utils = require('./../../utils');
 
 class MariaDBQueryGenerator extends MySQLQueryGenerator {
   createSchema(schema, options) {
@@ -9,10 +10,13 @@ class MariaDBQueryGenerator extends MySQLQueryGenerator {
       collate: null
     }, options || {});
 
-    const charset = options.charset ? ` DEFAULT CHARACTER SET ${this.escape(options.charset)}` : '';
-    const collate = options.collate ? ` DEFAULT COLLATE ${this.escape(options.collate)}` : '';
-
-    return `CREATE SCHEMA IF NOT EXISTS ${this.quoteIdentifier(schema)}${charset}${collate};`;
+    return Utils.joinSQLFragments([
+      'CREATE SCHEMA IF NOT EXISTS',
+      this.quoteIdentifier(schema),
+      options.charset && `DEFAULT CHARACTER SET ${this.escape(options.charset)}`,
+      options.collate && `DEFAULT COLLATE ${this.escape(options.collate)}`,
+      ';'
+    ]);
   }
 
   dropSchema(schema) {
@@ -20,8 +24,22 @@ class MariaDBQueryGenerator extends MySQLQueryGenerator {
   }
 
   showSchemasQuery(options) {
-    const skip = options.skip && Array.isArray(options.skip) && options.skip.length > 0 ? options.skip : null;
-    return `SELECT SCHEMA_NAME as schema_name FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME NOT IN ('MYSQL', 'INFORMATION_SCHEMA', 'PERFORMANCE_SCHEMA'${skip ? skip.reduce( (sql, schemaName) => sql += `,${this.escape(schemaName)}`, '') : ''});`;
+    const schemasToSkip = [
+      '\'MYSQL\'',
+      '\'INFORMATION_SCHEMA\'',
+      '\'PERFORMANCE_SCHEMA\''
+    ];
+    if (options.skip && Array.isArray(options.skip) && options.skip.length > 0) {
+      for (const schemaName of options.skip) {
+        schemasToSkip.push(this.escape(schemaName));
+      }
+    }
+    return Utils.joinSQLFragments([
+      'SELECT SCHEMA_NAME as schema_name',
+      'FROM INFORMATION_SCHEMA.SCHEMATA',
+      `WHERE SCHEMA_NAME NOT IN (${schemasToSkip.join(', ')})`,
+      ';'
+    ]);
   }
 
   showTablesQuery(database) {

lib/dialects/mysql/data-types.js

@@ -50,7 +50,7 @@ module.exports = BaseTypes => {
 
   class DATE extends BaseTypes.DATE {
     toSql() {
-      return `DATETIME${this._length ? `(${this._length})` : ''}`;
+      return this._length ? `DATETIME(${this._length})` : 'DATETIME';
     }
     _stringify(date, options) {
       date = this._applyTimezone(date, options);

lib/utils.js

@@ -12,6 +12,7 @@ const operatorsSet = new Set(_.values(operators));
 let inflection = require('inflection');
 
 exports.classToInvokable = require('./utils/classToInvokable').classToInvokable;
+exports.joinSQLFragments = require('./utils/join-sql-fragments').joinSQLFragments;
 
 exports.Promise = Promise;
 

lib/utils/join-sql-fragments.js

+'use strict';
+
+function doesNotWantLeadingSpace(str) {
+  return /^[;,)]/.test(str);
+}
+function doesNotWantTrailingSpace(str) {
+  return /\($/.test(str);
+}
+
+/**
+ * Joins an array of strings with a single space between them,
+ * except for:
+ * 
+ * - Strings starting with ';', ',' and ')', which do not get a leading space.
+ * - Strings ending with '(', which do not get a trailing space.
+ * 
+ * @param {string[]} parts
+ * @returns {string}
+ * @private
+ */
+function singleSpaceJoinHelper(parts) {
+  return parts.reduce(({ skipNextLeadingSpace, result }, part) => {
+    if (skipNextLeadingSpace || doesNotWantLeadingSpace(part)) {
+      result += part.trim();
+    } else {
+      result += ` ${part.trim()}`;
+    }
+    return {
+      skipNextLeadingSpace: doesNotWantTrailingSpace(part),
+      result
+    };
+  }, {
+    skipNextLeadingSpace: true,
+    result: ''
+  }).result;
+}
+
+/**
+ * Joins an array with a single space, auto trimming when needed.
+ * 
+ * Certain elements do not get leading/trailing spaces.
+ * 
+ * @param {any[]} array The array to be joined. Falsy values are skipped. If an
+ * element is another array, this function will be called recursively on that array.
+ * Otherwise, if a non-string, non-falsy value is present, a TypeError will be thrown.
+ * 
+ * @returns {string} The joined string.
+ * 
+ * @private
+ */
+function joinSQLFragments(array) {
+  if (array.length === 0) return '';
+
+  // Skip falsy fragments
+  array = array.filter(x => x);
+
+  // Resolve recursive calls
+  array = array.map(fragment => {
+    if (Array.isArray(fragment)) {
+      return joinSQLFragments(fragment);
+    }
+    return fragment;
+  });
+
+  // Ensure strings
+  for (const fragment of array) {
+    if (fragment && typeof fragment !== 'string') {
+      const error = new TypeError(`Tried to construct a SQL string with a non-string, non-falsy fragment (${fragment}).`);
+      error.args = array;
+      error.fragment = fragment;
+      throw error;
+    }
+  }
+
+  // Trim fragments
+  array = array.map(x => x.trim());
+
+  // Skip full-whitespace fragments (empty after the above trim)
+  array = array.filter(x => x !== '');
+
+  return singleSpaceJoinHelper(array);
+}
+exports.joinSQLFragments = joinSQLFragments;

test/unit/dialects/mariadb/query-generator.test.js

@@ -71,11 +71,11 @@ if (dialect === 'mariadb') {
         },
         {
           arguments: [{ skip: ['test'] }],
-          expectation: 'SELECT SCHEMA_NAME as schema_name FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME NOT IN (\'MYSQL\', \'INFORMATION_SCHEMA\', \'PERFORMANCE_SCHEMA\',\'test\');'
+          expectation: 'SELECT SCHEMA_NAME as schema_name FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME NOT IN (\'MYSQL\', \'INFORMATION_SCHEMA\', \'PERFORMANCE_SCHEMA\', \'test\');'
         },
         {
           arguments: [{ skip: ['test', 'Te\'st2'] }],
-          expectation: 'SELECT SCHEMA_NAME as schema_name FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME NOT IN (\'MYSQL\', \'INFORMATION_SCHEMA\', \'PERFORMANCE_SCHEMA\',\'test\',\'Te\\\'st2\');'
+          expectation: 'SELECT SCHEMA_NAME as schema_name FROM INFORMATION_SCHEMA.SCHEMATA WHERE SCHEMA_NAME NOT IN (\'MYSQL\', \'INFORMATION_SCHEMA\', \'PERFORMANCE_SCHEMA\', \'test\', \'Te\\\'st2\');'
         }
 
       ],

test/unit/dialects/mssql/query-generator.test.js

@@ -136,12 +136,12 @@ if (current.dialect.name === 'mssql') {
 
       // With offset
       expectsql(modifiedGen.selectFromTableFragment({ offset: 10 }, { primaryKeyField: 'id' }, ['id', 'name'], 'myTable', 'myOtherName'), {
-        mssql: 'SELECT TOP 100 PERCENT id, name FROM (SELECT * FROM (SELECT ROW_NUMBER() OVER (ORDER BY [id]) as row_num, *  FROM myTable AS myOtherName) AS myOtherName WHERE row_num > 10) AS myOtherName'
+        mssql: 'SELECT TOP 100 PERCENT id, name FROM (SELECT * FROM (SELECT ROW_NUMBER() OVER (ORDER BY [id]) as row_num, * FROM myTable AS myOtherName) AS myOtherName WHERE row_num > 10) AS myOtherName'
       });
 
       // With both limit and offset
       expectsql(modifiedGen.selectFromTableFragment({ limit: 10, offset: 10 }, { primaryKeyField: 'id' }, ['id', 'name'], 'myTable', 'myOtherName'), {
-        mssql: 'SELECT TOP 100 PERCENT id, name FROM (SELECT TOP 10 * FROM (SELECT ROW_NUMBER() OVER (ORDER BY [id]) as row_num, *  FROM myTable AS myOtherName) AS myOtherName WHERE row_num > 10) AS myOtherName'
+        mssql: 'SELECT TOP 100 PERCENT id, name FROM (SELECT TOP 10 * FROM (SELECT ROW_NUMBER() OVER (ORDER BY [id]) as row_num, * FROM myTable AS myOtherName) AS myOtherName WHERE row_num > 10) AS myOtherName'
       });
     });
 

test/unit/dialects/postgres/query-generator.test.js

@@ -56,7 +56,7 @@ if (dialect.startsWith('postgres')) {
         {
           title: 'Should use the plus operator',
           arguments: ['+', 'myTable', {}, { foo: 'bar' }, {}, {}],
-          expectation: 'UPDATE "myTable" SET "foo"="foo"+ \'bar\'  RETURNING *'
+          expectation: 'UPDATE "myTable" SET "foo"="foo"+ \'bar\' RETURNING *'
         },
         {
           title: 'Should use the plus operator with where clause',
@@ -71,12 +71,12 @@ if (dialect.startsWith('postgres')) {
         {
           title: 'Should use the minus operator',
           arguments: ['-', 'myTable', {}, { foo: 'bar' }, {}, {}],
-          expectation: 'UPDATE "myTable" SET "foo"="foo"- \'bar\'  RETURNING *'
+          expectation: 'UPDATE "myTable" SET "foo"="foo"- \'bar\' RETURNING *'
         },
         {
           title: 'Should use the minus operator with negative value',
           arguments: ['-', 'myTable', {}, { foo: -1 }, {}, {}],
-          expectation: 'UPDATE "myTable" SET "foo"="foo"- -1  RETURNING *'
+          expectation: 'UPDATE "myTable" SET "foo"="foo"- -1 RETURNING *'
         },
         {
           title: 'Should use the minus operator with where clause',