Postgres Adapter .remove({}, cb) doesn't work

[danfinlay]

Constructs a query that looks like:

DELETE FROM comments WHERE ;

This returns an error from postgres like:

syntax error at or near ";"

I tried to write a test for this, but I think the test doesn't test against a real pg database, and that's probably why this doesn't get caught.

[mde]

All the tests in test/integration/adapters tests against real databases. To run just the postgres tests, do jake test[postgres]. But this is a test we should run against all adapters.

[danfinlay]

I found the problem, and can recreate it and explain it exactly!:

Currently the test for remove is nested in an all() call, getting passed an array of ids.

Meanwhile, the scaffolded test for removing all items looks a bit different, and therein lies the problem:

'after': function (next) {
    // cleanup DB
    Person.remove({}, function (err, data) {
      if (err) { throw err; }
      next();
    });
  }

So basically this could be solved on either end: A convenience remove({}) on the model, or just scaffolding an all() call that then passes remove() an array of ids as a query.

I'll be switching my own code to the latter for now, will look at the generator code next.

[danfinlay]

This makes the scaffolded after test look a bit messier, but this is approximately what I'd propose:

  'after': function (next) {
    Comment.all({}, function(err, data){
      assert.ifError(err);
      if(data.length > 0){
        Comment.remove({
          id: data.map(function(dat){ return dat.id; })
        }, function (err, data) {
          if (err) { throw err; }
          Comment.all({}, function(err, data){
            assert.ifError(err);
            assert(data.length === 0, 'Removing all should remove all');
            next();
          });
        });
      }
    });
  }

[mde]

I would assume that passing an empty query object should result in no removals, not removing everything. (You didn't pass it any properties to match.) Some DBs we support don't even provide a way to truncate a table. We could provide a convenience remove-all (perhaps "truncate" is better?) method for the adapters that allow it. In any case, since you're sometimes dynamically constructing query objects, passing an empty one should be a no-op, not throw an error.

[danfinlay]

That makes sense to me now, so all the more I think this is actually a problem with the scaffolded tests (especially when treated as an example), not the adapter. If that's a different repository, you can close this.

[mde]

You mean scaffolded tests for a Geddy app?

[danfinlay]

Well currently geddy gen scaffold something will scaffold tests as well as models and controllers. So currently the scaffolded test templates are a little wrong, I'll go searching for them once I update all my model tests :)

[mde]

Excellent, thanks. I'm going to leave this open as a reminder to check empty query objects, and consider a convenience truncate method.