Fix #161 - Wrong queries causes unhandled exceptions

lib/sql.js

@@ -704,7 +704,15 @@ SQLConnector.prototype.save = function(model, data, options, cb) {
 
   let updateStmt = new ParameterizedSQL('UPDATE ' + this.tableEscaped(model));
   updateStmt.merge(this.buildFieldsForUpdate(model, data));
-  const whereStmt = this.buildWhere(model, where);
+  let whereStmt;
+  try {
+    whereStmt = this.buildWhere(model, where);
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   updateStmt.merge(whereStmt);
   updateStmt = this.parameterize(updateStmt);
   this.execute(updateStmt.sql, updateStmt.params, options,
@@ -731,7 +739,14 @@ SQLConnector.prototype.exists = function(model, id, options, cb) {
     'SELECT 1 FROM ' + this.tableEscaped(model) +
     ' WHERE ' + this.idColumnEscaped(model),
   );
-  selectStmt.merge(this.buildWhere(model, where));
+  try {
+    selectStmt.merge(this.buildWhere(model, where));
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   selectStmt = this.applyPagination(model, selectStmt, {
     limit: 1,
     offset: 0,
@@ -798,7 +813,15 @@ SQLConnector.prototype.buildDelete = function(model, where, options) {
  * @param {Function} cb The callback function
  */
 SQLConnector.prototype.destroyAll = function(model, where, options, cb) {
-  const stmt = this.buildDelete(model, where, options);
+  let stmt;
+  try {
+    stmt = this.buildDelete(model, where, options);
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   this._executeAlteringQuery(model, stmt.sql, stmt.params, options, cb || NOOP);
 };
 
@@ -924,7 +947,15 @@ SQLConnector.prototype._constructUpdateQuery = function(model, where, fields) {
  * @param {Function} cb The callback function
  */
 SQLConnector.prototype.update = function(model, where, data, options, cb) {
-  const stmt = this.buildUpdate(model, where, data, options);
+  let stmt;
+  try {
+    stmt = this.buildUpdate(model, where, data, options);
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   this._executeAlteringQuery(model, stmt.sql, stmt.params, options, cb || NOOP);
 };
 
@@ -939,7 +970,15 @@ SQLConnector.prototype.update = function(model, where, data, options, cb) {
  */
 SQLConnector.prototype._replace = function(model, where, data, options, cb) {
   const self = this;
-  const stmt = this.buildReplace(model, where, data, options);
+  let stmt;
+  try {
+    stmt = this.buildReplace(model, where, data, options);
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   this.execute(stmt.sql, stmt.params, options, function(err, info) {
     if (err) return cb(err);
     const affectedRows = self.getCountForAffectedRows(model, info);
@@ -960,6 +999,14 @@ function errorIdNotFoundForReplace(idValue) {
   return error;
 }
 
+function errorPropertyNotFound(key, model) {
+  const msg = g.f('Unknown property %s is used for model %s', key, model);
+  const error = new Error(msg);
+  error.statusCode = error.status = 400;
+  error.code = 'UNKNWON_PROPERTY';
+  return error;
+}
+
 SQLConnector.prototype._executeAlteringQuery = function(model, sql, params, options, cb) {
   const self = this;
   this.execute(sql, params, options, function(err, info) {
@@ -1113,9 +1160,11 @@ SQLConnector.prototype._buildWhere = function(model, where) {
     }
     const p = props[key];
     if (p == null) {
-      // Unknown property, ignore it
-      debug('Unknown property %s is skipped for model %s', key, model);
-      continue;
+      if (self.settings.ignoreUnknownProperties) {
+        debug('Unknown property %s is skipped for model %s', key, model);
+        continue;
+      }
+      throw errorPropertyNotFound(key, model);
     }
     // eslint-disable one-var
     let expression = where[key];
@@ -1207,13 +1256,22 @@ SQLConnector.prototype.buildOrderBy = function(model, order) {
   if (typeof order === 'string') {
     order = [order];
   }
+  const props = self.getModelDefinition(model).properties;
   const clauses = [];
   for (let i = 0, n = order.length; i < n; i++) {
     const t = order[i].split(/[\s,]+/);
+    const key = t[0];
+    if (!props[key]) {
+      if (self.settings.ignoreUnknownProperties) {
+        debug('Unknown property %s is skipped for model %s', key, model);
+        continue;
+      }
+      throw errorPropertyNotFound(key, model);
+    }
     if (t.length === 1) {
-      clauses.push(self.columnEscaped(model, order[i]));
+      clauses.push(self.columnEscaped(model, key));
     } else {
-      clauses.push(self.columnEscaped(model, t[0]) + ' ' + t[1]);
+      clauses.push(self.columnEscaped(model, key) + ' ' + t[1]);
     }
   }
   return 'ORDER BY ' + clauses.join(',');
@@ -1456,7 +1514,12 @@ SQLConnector.prototype.all = function find(model, filter, options, cb) {
   const self = this;
   // Order by id if no order is specified
   filter = filter || {};
-  const stmt = this.buildSelect(model, filter, options);
+  let stmt;
+  try {
+    stmt = this.buildSelect(model, filter, options);
+  } catch (err) {
+    return cb(err, []);
+  }
   this.execute(stmt.sql, stmt.params, options, function(err, data) {
     if (err) {
       return cb(err, []);
@@ -1537,7 +1600,14 @@ SQLConnector.prototype.count = function(model, where, options, cb) {
 
   let stmt = new ParameterizedSQL('SELECT count(*) as "cnt" FROM ' +
     this.tableEscaped(model));
-  stmt = stmt.merge(this.buildWhere(model, where));
+  try {
+    stmt = stmt.merge(this.buildWhere(model, where));
+  } catch (err) {
+    if (cb) {
+      cb(err);
+    }
+    return;
+  }
   stmt = this.parameterize(stmt);
   this.execute(stmt.sql, stmt.params, options,
     function(err, res) {

test/connectors/test-sql-connector.js

@@ -209,6 +209,10 @@ TestConnector.prototype.rollback = function(tx, cb) {
   tx.rollback(cb);
 };
 
+TestConnector.prototype.getCountForAffectedRows = function() {
+  return 1;
+};
+
 TestConnector.prototype.executeSQL = function(sql, params, options, callback) {
   const transaction = options.transaction;
   const model = options.model;

test/sql.test.js

@@ -14,16 +14,24 @@ const ds = new juggler.DataSource({
   connector: testConnector,
   debug: true,
 });
+const dsWithIgnore = new juggler.DataSource({
+  connector: testConnector,
+  debug: true,
+  ignoreUnknownProperties: true,
+});
 /* eslint-disable one-var */
 let connector;
+let connectorWithIgnore;
+let connectorNoSkipProp;
 let Customer;
 /* eslint-enable one-var */
 
 describe('sql connector', function() {
   before(function() {
     connector = ds.connector;
-    connector._tables = {};
-    connector._models = {};
+    connectorWithIgnore = dsWithIgnore.connector;
+    connectorWithIgnore._tables = connector._tables = {};
+    connectorWithIgnore._models = connector._models = {};
     Customer = ds.createModel('customer',
       {
         name: {
@@ -247,12 +255,27 @@ describe('sql connector', function() {
     });
   });
 
-  it('builds where and ignores invalid clauses in or', function() {
-    const where = connector.buildWhere('customer', {
-      name: 'icecream',
-      or: [{notAColumnName: ''}, {notAColumnNameEither: ''}],
-    });
-    expect(where.sql).to.not.match(/ AND $/);
+  it('builds where and throws if invalid clauses in or', function() {
+    expect(() => {
+      connector.buildWhere('customer', {
+        name: 'icecream',
+        or: [{notAColumnName: ''}, {notAColumnNameEither: ''}],
+      });
+    }).to.throw('Unknown property notAColumnName is used for model customer')
+      .to.include({
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+  });
+
+  it('builds where and ignores if invalid clauses in or', function() {
+    expect(() => {
+      connectorWithIgnore.buildWhere('customer', {
+        name: 'icecream',
+        or: [{notAColumnName: ''}, {notAColumnNameEither: ''}],
+      });
+    }).to.not.throw();
   });
 
   it('builds order by with one field', function() {
@@ -270,6 +293,57 @@ describe('sql connector', function() {
     expect(orderBy).to.eql('ORDER BY `NAME` ASC,`VIP` DESC');
   });
 
+  it('builds order by with non existent field throws', function() {
+    expect(() => {
+      connector.buildOrderBy('customer', ['nam?e', 'name']);
+    }).to.throw('Unknown property nam?e is used for model customer')
+      .to.include({
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+  });
+
+  it('builds order by with non existent field with direction throws', function() {
+    expect(() => {
+      connector.buildOrderBy('customer', ['nam?e ASC', 'name']);
+    }).to.throw('Unknown property nam?e is used for model customer')
+      .to.include({
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+  });
+
+  it('builds order by with only non existent fields throws', function() {
+    expect(() => {
+      connector.buildOrderBy('customer', ['nam?e', 'n?ame', '?name DESC']);
+    }).to.throw('Unknown property nam?e is used for model customer')
+      .to.include({
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+  });
+
+  it('builds order by with non existent field ignores', function() {
+    expect(() => {
+      connectorWithIgnore.buildOrderBy('customer', ['nam?e', 'name']);
+    }).to.not.throw();
+  });
+
+  it('builds order by with non existent field with direction ignores', function() {
+    expect(() => {
+      connectorWithIgnore.buildOrderBy('customer', ['nam?e ASC', 'name']);
+    }).to.not.throw();
+  });
+
+  it('builds order by with only non existent fields ignores', function() {
+    expect(() => {
+      connectorWithIgnore.buildOrderBy('customer', ['nam?e', 'n?ame', '?name DESC']);
+    }).to.not.throw();
+  });
+
   it('builds fields for columns', function() {
     const fields = connector.buildFields('customer',
       {name: 'John', vip: true, unknown: 'Random'});
@@ -503,4 +577,68 @@ describe('sql connector', function() {
     expect(function() { runExecute(); }).to.not.throw();
     ds.connected = true;
   });
+
+  it('should throw if invalid order by statement is used by all', function(done) {
+    connector.all('customer', {order: 'n?ame'}, {}, (err) => {
+      expect(err).to.include({
+        message: 'Unknown property n?ame is used for model customer',
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+      done();
+    });
+  });
+
+  it('should throw if invalid where statement is used by count', function(done) {
+    connector.count('customer', {'n?ame': 1}, {}, (err) => {
+      expect(err).to.include({
+        message: 'Unknown property n?ame is used for model customer',
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+      done();
+    });
+  });
+
+  it('should throw if invalid where statement is used by update', function(done) {
+    connector.update('customer', {'n?ame': 1}, {name: 5}, {}, (err) => {
+      expect(err).to.include({
+        message: 'Unknown property n?ame is used for model customer',
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+      done();
+    });
+  });
+
+  it('should throw if invalid where statement is used by destroyAll', function(done) {
+    connector.destroyAll('customer', {'n?ame': 1}, {}, (err) => {
+      expect(err).to.include({
+        message: 'Unknown property n?ame is used for model customer',
+        code: 'UNKNWON_PROPERTY',
+        statusCode: 400,
+        status: 400,
+      });
+      done();
+    });
+  });
+
+  it('should ignore if invalid order by statement is used by all', function(done) {
+    connectorWithIgnore.all('customer', {order: 'n?ame'}, {}, done);
+  });
+
+  it('should ignore if invalid where statement is used by count', function(done) {
+    connectorWithIgnore.count('customer', {'n?ame': 1}, {}, done);
+  });
+
+  it('should ignore if invalid where statement is used by update', function(done) {
+    connectorWithIgnore.update('customer', {'n?ame': 1}, {name: 5}, {}, done);
+  });
+
+  it('should ignore if invalid where statement is used by destroyAll', function(done) {
+    connectorWithIgnore.destroyAll('customer', {'n?ame': 1}, {}, done);
+  });
 });