Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Open
danfinlay opened this Issue Mar 5, 2014 · 8 comments

Comments

Projects
None yet
2 participants
Contributor

danfinlay commented Mar 5, 2014

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.

Contributor

mde commented Mar 5, 2014

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.

Contributor

danfinlay commented Mar 5, 2014

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.

Contributor

danfinlay commented Mar 5, 2014

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();
          });
        });
      }
    });
  }
Contributor

mde commented Mar 5, 2014

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.

Contributor

danfinlay commented Mar 5, 2014

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.

Contributor

mde commented Mar 5, 2014

You mean scaffolded tests for a Geddy app?

Contributor

danfinlay commented Mar 5, 2014

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 :)

Contributor

mde commented Mar 5, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment