Merge pull request #4796 from faceleg/feature/postgres-schema

Initial modifications to get enums working with postgres schemas

Merge pull request #4796 from faceleg/feature/postgres-schema
Initial modifications to get enums working with postgres schemas
Mick Hansen
Commit 9775f361 authored Nov 13, 2015 by Mick Hansen
Showing with 71 additions and 38 deletions
changelog.md
lib/dialects/abstract/query-generator.js
lib/dialects/postgres/query-generator.js
lib/instance-validator.js
lib/model.js
lib/query-interface.js
package.json
test/integration/dialects/postgres/dao.test.js
test/integration/dialects/postgres/query-generator.test.js
test/unit/sql/enum.test.js
--- a/changelog.md
+++ b/changelog.md
 # Next
 - [FIXED] Apply scopes to `aggregate` [#4764](https://github.com/sequelize/sequelize/issues/4764)
+- [FIXED] Improved postgres enum schema handling [#4796](https://github.com/sequelize/sequelize/issues/4796)
 - [ADDED/FIXED] Lower case `onDelete` option to allow the use of `onDelete: 'CASCADE', hooks: true`.
 # 3.13.0

--- a/lib/dialects/abstract/query-generator.js
+++ b/lib/dialects/abstract/query-generator.js
@@ -19,6 +19,16 @@ var throwMethodUndefined = function(methodName) {
 var QueryGenerator = {
  options: {},
+  extractTableDetails: function(tableName, options) {
+    options = options || {};
+    tableName = tableName || {};
+    return {
+      schema: tableName.schema || options.schema || 'public',
+      tableName: _.isPlainObject(tableName) ? tableName.table : tableName,
+      delimiter: tableName.delimiter || options.delimiter || '.'
+    };
+  },
  addSchema: function(param) {
    var self = this;

--- a/lib/dialects/postgres/query-generator.js
+++ b/lib/dialects/postgres/query-generator.js
@@ -5,8 +5,7 @@ var Utils = require('../../utils')
  , util = require('util')
  , DataTypes = require('../../data-types')
  , AbstractQueryGenerator = require('../abstract/query-generator')
-  , primaryKeys = {}
+  , primaryKeys = {};
-  , _ = require('lodash');
 var QueryGenerator = {
  options: {},
@@ -719,34 +718,36 @@ var QueryGenerator = {
  pgEnumName: function (tableName, attr, options) {
    options = options || {};
+    var tableDetails = this.extractTableDetails(tableName, options)
+      , enumName = '"enum_' + tableDetails.tableName + '_' + attr + '"';
-    var enumName = '"enum_' + (_.isPlainObject(tableName) ? tableName.table : tableName) + '_' + attr + '"';
+    // pgListEnums requires the enum name only, without the schema
+    if (options.schema !== false && tableDetails.schema) {
-    if (options.schema !== false && tableName && tableName.schema) {
+      enumName = this.quoteIdentifier(tableDetails.schema) + tableDetails.delimiter + enumName;
-      enumName = '"' + tableName.schema + '"' + tableName.delimiter + enumName;
    }
    return enumName;
  },
-  pgListEnums: function(tableName, attrName) {
+  pgListEnums: function(tableName, attrName, options) {
    var enumName = ''
-      , schema = tableName && tableName.schema ? tableName.schema : 'public';
+      , tableDetails = this.extractTableDetails(tableName, options);
-    if (!!tableName && !!attrName) {
+    if (tableDetails.tableName && attrName) {
-      enumName = ' AND t.typname=' + this.pgEnumName(tableName, attrName, { schema: false }).replace(/"/g, "'");
+      enumName = ' AND t.typname=' + this.pgEnumName(tableDetails.tableName, attrName, { schema: false }).replace(/"/g, "'");
    }
    var query = 'SELECT t.typname enum_name, array_agg(e.enumlabel) enum_value FROM pg_type t ' +
      'JOIN pg_enum e ON t.oid = e.enumtypid ' +
      'JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace ' +
-      "WHERE n.nspname = '" + schema + "'" + enumName + ' GROUP BY 1';
+      "WHERE n.nspname = '" + tableDetails.schema + "'" + enumName + ' GROUP BY 1';
    return query;
  },
  pgEnum: function(tableName, attr, dataType, options) {
-    var enumName = this.pgEnumName(tableName, attr)
+    var enumName = this.pgEnumName(tableName, attr, options)
      , values;
    if (dataType.values) {

--- a/lib/instance-validator.js
+++ b/lib/instance-validator.js
@@ -64,10 +64,12 @@ InstanceValidator.prototype.validate = function() {
  this.inProgress = true;
  var self = this;
-  return Promise.settle([
+  return Promise.all([
    self._builtinValidators(),
    self._customValidators()
-  ]).then(function() {
+  ].map(function(promise) {
+    return promise.reflect();
+  })).then(function() {
    if (self.errors.length) {
      return new sequelizeError.ValidationError(null, self.errors);
    }
@@ -98,7 +100,7 @@ InstanceValidator.prototype.hookValidate = function() {
 /**
 * Will run all the built-in validators.
 *
- * @return {Promise(Array.<Promise.PromiseInspection>)} A promise from .settle().
+ * @return {Promise(Array.<Promise.PromiseInspection>)} A promise from .reflect().
 * @private
 */
 InstanceValidator.prototype._builtinValidators = function() {
@@ -119,17 +121,17 @@ InstanceValidator.prototype._builtinValidators = function() {
    }
    if (self.modelInstance.validators.hasOwnProperty(field)) {
-      validators.push(self._builtinAttrValidate.call(self, value, field));
+      validators.push(self._builtinAttrValidate.call(self, value, field).reflect());
    }
  });
-  return Promise.settle(validators);
+  return Promise.all(validators);
 };
 /**
 * Will run all the custom validators.
 *
- * @return {Promise(Array.<Promise.PromiseInspection>)} A promise from .settle().
+ * @return {Promise(Array.<Promise.PromiseInspection>)} A promise from .reflect().
 * @private
 */
 InstanceValidator.prototype._customValidators = function() {
@@ -139,15 +141,16 @@ InstanceValidator.prototype._customValidators = function() {
    if (self.options.skip.indexOf(validatorType) >= 0) {
      return;
    }
    var valprom = self._invokeCustomValidator(validator, validatorType)
      // errors are handled in settling, stub this
-      .catch(function() {});
+      .catch(function() {})
+      .reflect();
    validators.push(valprom);
  });
-  return Promise.settle(validators);
+  return Promise.all(validators);
 };
 /**
@@ -184,16 +187,16 @@ InstanceValidator.prototype._builtinAttrValidate = function(value, field) {
    // Check for custom validator.
    if (typeof test === 'function') {
-      return validators.push(self._invokeCustomValidator(test, validatorType, true, value, field));
+      return validators.push(self._invokeCustomValidator(test, validatorType, true, value, field).reflect());
    }
    var validatorPromise = self._invokeBuiltinValidator(value, test, validatorType, field);
    // errors are handled in settling, stub this
    validatorPromise.catch(function() {});
-    validators.push(validatorPromise);
+    validators.push(validatorPromise.reflect());
  });
-  return Promise.settle(validators).then(this._handleSettledResult.bind(this, field));
+  return Promise.all(validators).then(this._handleReflectedResult.bind(this, field));
 };
 /**
@@ -299,7 +302,7 @@ InstanceValidator.prototype._validateSchema = function(rawAttribute, field, valu
 /**
- * Handles the returned result of a Promise.settle.
+ * Handles the returned result of a Promise.reflect.
 *
 * If errors are found it populates this.error.
 *
@@ -307,7 +310,7 @@ InstanceValidator.prototype._validateSchema = function(rawAttribute, field, valu
 * @param {Array.<Promise.PromiseInspection>} Promise inspection objects.
 * @private
 */
-InstanceValidator.prototype._handleSettledResult = function(field, promiseInspections) {
+InstanceValidator.prototype._handleReflectedResult = function(field, promiseInspections) {
  var self = this;
  promiseInspections.forEach(function(promiseInspection) {
    if (promiseInspection.isRejected()) {

--- a/lib/model.js
+++ b/lib/model.js
@@ -2436,6 +2436,7 @@ Model.prototype.update = function(values, options) {
        }
      });
    }
+    return null;
  }).then(function() {
    // Run before hook
    if (options.hooks) {

--- a/lib/query-interface.js
+++ b/lib/query-interface.js
@@ -93,7 +93,7 @@ QueryInterface.prototype.createTable = function(tableName, attributes, options, 
    for (i = 0; i < keyLen; i++) {
      if (attributes[keys[i]].type instanceof DataTypes.ENUM) {
-        sql = self.QueryGenerator.pgListEnums(tableName, attributes[keys[i]].field || keys[i]);
+        sql = self.QueryGenerator.pgListEnums(tableName, attributes[keys[i]].field || keys[i], options);
        promises.push(self.sequelize.query(
          sql, { plain: true, raw: true, type: QueryTypes.SELECT, logging: options.logging, transaction: options.transaction }
        ));

--- a/package.json
+++ b/package.json
@@ -32,7 +32,7 @@
    "url": "https://github.com/sequelize/sequelize/issues"
  },
  "dependencies": {
-    "bluebird": "^3.0.0",
+    "bluebird": "^3.0.5",
    "depd": "^1.0.0",
    "dottie": "^1.0.0",
    "generic-pool": "2.2.1",

--- a/test/integration/dialects/postgres/dao.test.js
+++ b/test/integration/dialects/postgres/dao.test.js
@@ -390,15 +390,15 @@ if (dialect.match(/^postgres/)) {
          return User.sync({
            logging: function (sql) {
              if (sql.indexOf('neutral') > -1) {
-                expect(sql.indexOf("ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE 'neutral' BEFORE 'happy'")).to.not.be.equal(-1);
+                expect(sql.indexOf("ALTER TYPE \"public\".\"enum_UserEnums_mood\" ADD VALUE 'neutral' BEFORE 'happy'")).to.not.be.equal(-1);
                count++;
              }
              else if (sql.indexOf('ecstatic') > -1) {
-                expect(sql.indexOf("ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE 'ecstatic' BEFORE 'meh'")).to.not.be.equal(-1);
+                expect(sql.indexOf("ALTER TYPE \"public\".\"enum_UserEnums_mood\" ADD VALUE 'ecstatic' BEFORE 'meh'")).to.not.be.equal(-1);
                count++;
              }
              else if (sql.indexOf('joyful') > -1) {
-                expect(sql.indexOf("ALTER TYPE \"enum_UserEnums_mood\" ADD VALUE 'joyful' AFTER 'meh'")).to.not.be.equal(-1);
+                expect(sql.indexOf("ALTER TYPE \"public\".\"enum_UserEnums_mood\" ADD VALUE 'joyful' AFTER 'meh'")).to.not.be.equal(-1);
                count++;
              }
            }

--- a/test/integration/dialects/postgres/query-generator.test.js
+++ b/test/integration/dialects/postgres/query-generator.test.js
@@ -181,7 +181,7 @@ if (dialect.match(/^postgres/)) {
        },
        {
          arguments: ['myTable', {title: 'ENUM("A", "B", "C")', name: 'VARCHAR(255)'}],
-          expectation: 'CREATE TABLE IF NOT EXISTS \"myTable\" (\"title\" \"enum_myTable_title\", \"name\" VARCHAR(255));'
+          expectation: 'CREATE TABLE IF NOT EXISTS \"myTable\" (\"title\" \"public\".\"enum_myTable_title\", \"name\" VARCHAR(255));'
        },
        {
          arguments: ['myTable', {title: 'VARCHAR(255)', name: 'VARCHAR(255)', id: 'INTEGER PRIMARY KEY'}],
@@ -205,7 +205,7 @@ if (dialect.match(/^postgres/)) {
        },
        {
          arguments: ['myTable', {title: 'ENUM("A", "B", "C")', name: 'VARCHAR(255)'}],
-          expectation: 'CREATE TABLE IF NOT EXISTS myTable (title "enum_myTable_title", name VARCHAR(255));',
+          expectation: 'CREATE TABLE IF NOT EXISTS myTable (title public."enum_myTable_title", name VARCHAR(255));',
          context: {options: {quoteIdentifiers: false}}
        },
        {

--- a/test/unit/sql/enum.test.js
+++ b/test/unit/sql/enum.test.js
@@ -5,7 +5,8 @@ var Support   = require(__dirname + '/../support')
  , DataTypes = require(__dirname + '/../../../lib/data-types')
  , expectsql = Support.expectsql
  , current   = Support.sequelize
-  , sql       = current.dialect.QueryGenerator;
+  , sql       = current.dialect.QueryGenerator
+  , expect    = require('chai').expect;
 describe(Support.getTestDialectTeaser('SQL'), function() {
@@ -24,6 +25,22 @@ describe(Support.getTestDialectTeaser('SQL'), function() {
        }
      });
+      describe('pgEnumName', function() {
+        it('does not add schema when options: { schema: false }', function() {
+          expect(sql.pgEnumName(PublicUser.getTableName(), 'mood', { schema: false }))
+            .to.equal('"enum_users_mood"');
+          expect(sql.pgEnumName(FooUser.getTableName(), 'theirMood', { schema: false }))
+            .to.equal('"enum_users_theirMood"');
+        });
+        it('properly quotes both the schema and the enum name', function() {
+          expect(sql.pgEnumName(PublicUser.getTableName(), 'mood', PublicUser.rawAttributes.mood.type))
+            .to.equal('"public"."enum_users_mood"');
+          expect(sql.pgEnumName(FooUser.getTableName(), 'theirMood', FooUser.rawAttributes.mood.type))
+            .to.equal('"foo"."enum_users_theirMood"');
+        });
+      });
      describe('pgEnum', function () {
        it('uses schema #3171', function () {
          expectsql(sql.pgEnum(FooUser.getTableName(), 'mood', FooUser.rawAttributes.mood.type), {
@@ -31,9 +48,9 @@ describe(Support.getTestDialectTeaser('SQL'), function() {
          });
        });
-        it('does not add schema when public', function () {
+        it('does add schema when public', function () {
          expectsql(sql.pgEnum(PublicUser.getTableName(), 'theirMood', PublicUser.rawAttributes.mood.type), {
-            postgres: 'CREATE TYPE "enum_users_theirMood" AS ENUM(\'happy\', \'sad\');'
+            postgres: 'CREATE TYPE "public"."enum_users_theirMood" AS ENUM(\'happy\', \'sad\');'
          });
        });
      });
@@ -45,8 +62,8 @@ describe(Support.getTestDialectTeaser('SQL'), function() {
          });
        });
-        it('uses the default schema if options given', function () {
+        it('uses the default schema if no options given', function () {
-            expectsql(sql.pgListEnums(), {
+          expectsql(sql.pgListEnums(), {
            postgres: 'SELECT t.typname enum_name, array_agg(e.enumlabel) enum_value FROM pg_type t JOIN pg_enum e ON t.oid = e.enumtypid JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace WHERE n.nspname = \'public\' GROUP BY 1'
          });
        });