geddy / model

Datastore-agnostic ORM in JavaScript
265 stars 55 forks source link

Syntax Err when Removing with Empty ID array #166

Closed danfinlay closed 10 years ago

danfinlay commented 10 years ago

At least throws syntax error with postgres adapter. I'm willing to try to fix this one but I'm filing a bug to help myself remember to. Ex:

geddy.model.User.remove({id: [], function(err, data){
  // Throws err;
});
danfinlay commented 10 years ago

This also throws an error using Postgres if you model.remove({}, callback);, as is used in the automated tests.

danfinlay commented 10 years ago

I was able to do a cheap and easy fix like this in the postgres adapter:


  this.remove = function (query, callback) {
    if(!query.id || !query.id.length && query.id.length === 0){
      var sql = this._createDeleteStatementWithConditions(query);
      this.exec(sql, function (err, data) {
        if (err) {
          callback(err, null);
        }
        else {
          callback(null, true);
        }
      });
    }else{
      callback(null, true);
    }
  };

However this is "not a contribution", and I'm just suggesting one easy way this could be fixed. (My company's lawyers asked me to say not a contribution when I post code. I've written a big letter trying to get postbacks pre-approved but for now that's how I'm going to cover my ass).

ben-ng commented 10 years ago

Thank you for.. not contributing this suggestion? :D

Yes, we shouldn't break on an empty query.. this needs to be fixed.

ben-ng commented 10 years ago

Ok, along with the fix in b8099e88b34e173fe07ea8079efb5fc2f9c86d37 this should be a more robust solution to the problem.