Merge pull request #92 from strongloop/feature/fix-issue-91

Fix the base sql connector to correct escape id values

lib/datasource.js

@@ -1568,6 +1568,17 @@ DataSource.prototype.idNames = function (modelName) {
   return this.getModelDefinition(modelName).idNames();
 };
 
+/**
+ * Find the id property definition
+ * @param {String} modelName The model name
+ * @returns {Object} The id property definition
+ */
+DataSource.prototype.idProperty = function (modelName) {
+  var def = this.getModelDefinition(modelName);
+  var idProps = def && def.ids();
+  return idProps && idProps[0] && idProps[0].property;
+};
+
 /**
  * Define foreign key to another model
  * @param {String} className The model name that owns the key

lib/model-definition.js

@@ -138,7 +138,7 @@ ModelDefinition.prototype.ids = function () {
     if (typeof id !== 'number') {
       id = 1;
     }
-    ids.push({name: key, id: id});
+    ids.push({name: key, id: id, property: props[key]});
   }
   ids.sort(function (a, b) {
     return a.key - b.key;
@@ -165,8 +165,8 @@ ModelDefinition.prototype.idName = function () {
   if (this.properties.id && this.properties.id.id) {
     return 'id';
   } else {
+    return id && id.name;
   }
-  return id && id.name;
 };
 
 /**

lib/sql.js

@@ -1,4 +1,6 @@
 var util = require('util');
+var async = require('async');
+var assert = require('assert');
 var Connector = require('./connector');
 
 module.exports = BaseSQL;
@@ -45,8 +47,10 @@ BaseSQL.prototype.command = function (sql, params, callback) {
 
 BaseSQL.prototype.queryOne = function (sql, callback) {
   return this.query(sql, function (err, data) {
-    if (err) return callback(err);
+    if (err) {
-    callback(err, data[0]);
+      return callback(err);
+    }
+    callback(err, data && data[0]);
   });
 };
 
@@ -112,7 +116,6 @@ BaseSQL.prototype.propertyName = function (model, column) {
  */
 BaseSQL.prototype.idColumn = function (model) {
   var name = this.getDataSource(model).idColumnName(model);
-  ;
   var dbName = this.dbName;
   if (typeof dbName === 'function') {
     name = dbName(name);
@@ -156,6 +159,17 @@ BaseSQL.prototype.columnEscaped = function (model, property) {
   return this.escapeName(this.column(model, property));
 };
 
+function isIdValuePresent(idValue, callback, returningNull) {
+  try {
+    assert(idValue !== null && idValue !== undefined, 'id value is required');
+    return true;
+  } catch (err) {
+    process.nextTick(function () {
+      callback && callback(returningNull ? null: err);
+    });
+    return false;
+  }
+}
 /**
  * Save the model instance into the backend store
  * @param {String} model The model name
@@ -163,11 +177,20 @@ BaseSQL.prototype.columnEscaped = function (model, property) {
  * @param {Function} callback The callback function
  */
 BaseSQL.prototype.save = function (model, data, callback) {
-  var sql = 'UPDATE ' + this.tableEscaped(model) + ' SET '
+  var idName = this.getDataSource(model).idName(model);
-    + this.toFields(model, data) + ' WHERE ' + this.idColumnEscaped(model) + ' = ' + Number(data.id);
+  var idValue = data[idName];
 
-  this.query(sql, function (err) {
+  if (!isIdValuePresent(idValue, callback)) {
-    callback(err);
+    return;
+  }
+
+  idValue = this._escapeIdValue(model, idValue);
+  var sql = 'UPDATE ' + this.tableEscaped(model) + ' SET '
+    + this.toFields(model, data) + ' WHERE ' + this.idColumnEscaped(model) + ' = '
+    + idValue;
+
+  this.query(sql, function (err, result) {
+    callback && callback(err, result);
   });
 };
 
@@ -178,12 +201,18 @@ BaseSQL.prototype.save = function (model, data, callback) {
  * @param {Function} callback The callback function
  */
 BaseSQL.prototype.exists = function (model, id, callback) {
+  if (!isIdValuePresent(id, callback, true)) {
+    return;
+  }
   var sql = 'SELECT 1 FROM ' +
-    this.tableEscaped(model) + ' WHERE ' + this.idColumnEscaped(model) + ' = ' + Number(id) + ' LIMIT 1';
+    this.tableEscaped(model) + ' WHERE ' + this.idColumnEscaped(model) + ' = '
+    + this._escapeIdValue(model, id) + ' LIMIT 1';
 
   this.query(sql, function (err, data) {
-    if (err) return callback(err);
+    if (err) {
-    callback(null, data.length === 1);
+      return callback && callback(err);
+    }
+    callback && callback(null, data.length >= 1);
   });
 };
 
@@ -194,20 +223,18 @@ BaseSQL.prototype.exists = function (model, id, callback) {
  * @param {Function} callback The callback function
  */
 BaseSQL.prototype.find = function find(model, id, callback) {
-  var idQuery = (id === null || id === undefined)
+  if (!isIdValuePresent(id, callback, true)) {
-    ? this.idColumnEscaped(model) + ' IS NULL'
+    return;
-    : this.idColumnEscaped(model) + ' = ' + id;
+  }
+  var self = this;
+  var idQuery = this.idColumnEscaped(model) + ' = ' + this._escapeIdValue(model, id);
   var sql = 'SELECT * FROM ' +
     this.tableEscaped(model) + ' WHERE ' + idQuery + ' LIMIT 1';
 
   this.query(sql, function (err, data) {
-    if (data && data.length === 1) {
+    var result =  (data && data.length >= 1) ? data[0] : null;
-      data[0].id = id;
+    callback && callback(err, self.fromDatabase(model, result));
-    } else {
+  });
-      data = [null];
-    }
-    callback(err, this.fromDatabase(model, data[0]));
-  }.bind(this));
 };
 
 /**
@@ -217,14 +244,31 @@ BaseSQL.prototype.find = function find(model, id, callback) {
  * @param {Function} callback The callback function
  */
 BaseSQL.prototype.delete = BaseSQL.prototype.destroy = function destroy(model, id, callback) {
+  if (!isIdValuePresent(id, callback, true)) {
+    return;
+  }
   var sql = 'DELETE FROM ' +
-    this.tableEscaped(model) + ' WHERE ' + this.idColumnEscaped(model) + ' = ' + id;
+    this.tableEscaped(model) + ' WHERE ' + this.idColumnEscaped(model) + ' = '
+    + this._escapeIdValue(model, id);
 
-  this.command(sql, function (err) {
+  this.command(sql, function (err, result) {
-    callback(err);
+    callback && callback(err, result);
   });
 };
 
+BaseSQL.prototype._escapeIdValue = function(model, idValue) {
+  var idProp = this.getDataSource(model).idProperty(model);
+  if(typeof this.toDatabase === 'function') {
+    return this.toDatabase(idProp, idValue);
+  } else {
+    if(idProp.type === Number) {
+      return idValue;
+    } else {
+      return "'" + idValue + "'";
+    }
+  }
+};
+
 /**
  * Delete all model instances
  *
@@ -232,12 +276,9 @@ BaseSQL.prototype.delete = BaseSQL.prototype.destroy = function destroy(model, i
  * @param {Function} callback The callback function
  */
 BaseSQL.prototype.deleteAll = BaseSQL.prototype.destroyAll = function destroyAll(model, callback) {
-  this.command('DELETE FROM ' + this.tableEscaped(model), function (err) {
+  this.command('DELETE FROM ' + this.tableEscaped(model), function (err, result) {
-    if (err) {
+    callback && callback(err, result);
-      return callback(err, []);
+  });
-    }
-    callback(err);
-  }.bind(this));
 };
 
 /**
@@ -253,7 +294,9 @@ BaseSQL.prototype.count = function count(model, callback, where) {
 
   this.queryOne('SELECT count(*) as cnt FROM ' +
     this.tableEscaped(model) + ' ' + buildWhere(where), function (err, res) {
-    if (err) return callback(err);
+    if (err) {
+      return callback(err);
+    }
     callback(err, res && res.cnt);
   });
 
@@ -279,7 +322,11 @@ BaseSQL.prototype.count = function count(model, callback, where) {
  * @param {Function} cb The callback function
  */
 BaseSQL.prototype.updateAttributes = function updateAttrs(model, id, data, cb) {
-  data.id = id;
+  if (!isIdValuePresent(id, cb)) {
+    return;
+  }
+  var idName = this.getDataSource(model).idName(model);
+  data[idName] = id;
   this.save(model, data, cb);
 };
 
@@ -287,17 +334,18 @@ BaseSQL.prototype.updateAttributes = function updateAttrs(model, id, data, cb) {
  * Disconnect from the connector
  */
 BaseSQL.prototype.disconnect = function disconnect() {
-  this.client.end();
+  // No operation
 };
 
 /**
  * Recreate the tables for the given models
- * @param {[String]|String} [models] A model name or an array of model names, if not present, apply to all models defined in the connector
+ * @param {[String]|String} [models] A model name or an array of model names,
+ * if not present, apply to all models defined in the connector
  * @param {Function} [cb] The callback function
  */
 BaseSQL.prototype.automigrate = function (models, cb) {
   var self = this;
-  var wait = 0;
+
   if ((!cb) && ('function' === typeof models)) {
     cb = models;
     models = undefined;
@@ -307,31 +355,23 @@ BaseSQL.prototype.automigrate = function (models, cb) {
     models = [models];
   }
 
-  models = models || Object.keys(this._models);
+  models = models || Object.keys(self._models);
-  models.forEach(function (model) {
+  async.each(models, function (model, callback) {
     if (model in self._models) {
-      wait++;
+      self.dropTable(model, function (err, result) {
-      self.dropTable(model, function () {
+        self.createTable(model, function (err, result) {
-        // console.log('drop', model);
+          if (err) {
-        self.createTable(model, function (err) {
+            console.error(err);
-          // console.log('create', model);
+          }
-          if (err) console.log(err);
+          callback(err, result);
-          done();
         });
       });
     }
-  });
+  }, cb);
-  if (wait === 0) cb();
-
-  function done() {
-    if (--wait === 0 && cb) {
-      cb();
-    }
-  }
 };
 
 /**
- * Drop the table for the given model
+ * Drop the table for the given model from the database
  * @param {String} model The model name
  * @param {Function} [cb] The callback function
  */