make Query a public api #171

Closed
wants to merge 1 commit into
from

Conversation

2 participants
@troyk

troyk commented Aug 10, 2012

I'd like to propose having Query available as a public API. This solves a few pain points for my use case and I hope it will for others as well. I needed better control of the connection in a multi-tenant web app and want to use the built-in connection pool. Transactions and server session state (e.g. schema, timezone, user, etc) we're causing a lot of grief until I opened up the Query API. An excerpt of the code I'm using might help explain:

function DBSession(username,password) {
  this.transaction = null;
}

DBSession.prototype.query = function(config, values, callback) {
  var query = new pg.Query(config, values, callback),
      self  = this;
  if (!this.transaction) {
    pg.connect(function(err,client){
      if (err) return query.handleError(err);
      client.query(query);
    });
  } else {
    query.on('error',function(){
      self.rollback();
    });
    transaction.client.query(query);
  }
  return query;
};

var sql = 'SET search_path to crm1,public; SET TIME ZONE \'EST5EDT\'; select row_to_json(users) from users limit 1';
var db = new DBSession();
db.query(sql,function(e,r){
  console.log(e,r);
});

I moved the config creation of client.query to the query class and allow an instance of Query to be passed in addition to the current string and config options. All test passed on my machine and it didn't seem like additional tests were needed, but let me know if you want a test file and if this should be tested in one of the existing tests.

Also notice how I'm able to create a DBSession and issue queries in a more synchronous manner.

Also, with this API I have something I'd like to release as a plugin that console logs the SQL in the REPL, but it depends on getting to the Query obj as well.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Aug 21, 2012

Owner

I like this mucho. Probably a cleaner design overall than what I initially had done. It's totally backwards compatible, and it doesn't introduce any "clutter" across the surface area of the module. Testing it now.

Owner

brianc commented Aug 21, 2012

I like this mucho. Probably a cleaner design overall than what I initially had done. It's totally backwards compatible, and it doesn't introduce any "clutter" across the surface area of the module. Testing it now.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Aug 21, 2012

Owner

hmm...are you able to run these tests locally? This pull request caused a test to fail.


***Testing Pure Javascript***
api-tests.js..........
array-tests.js...........
big-simple-query-tests.js....
cancel-query-tests.js..Message: undefined
AssertionError: 52 == 0
    at Object._onTimeout (/Users/brian/src/node-postgres/test/integration/client/cancel-query-tests.js:36:10)
    at Timer.list.ontimeout (timers.js:101:19)

 AssertionError: 52 == 0
    at Object._onTimeout (/Users/brian/src/node-postgres/test/integration/client/cancel-query-tests.js:36:10)
    at Timer.list.ontimeout (timers.js:101:19)

make: *** [test-integration] Error 1
Owner

brianc commented Aug 21, 2012

hmm...are you able to run these tests locally? This pull request caused a test to fail.


***Testing Pure Javascript***
api-tests.js..........
array-tests.js...........
big-simple-query-tests.js....
cancel-query-tests.js..Message: undefined
AssertionError: 52 == 0
    at Object._onTimeout (/Users/brian/src/node-postgres/test/integration/client/cancel-query-tests.js:36:10)
    at Timer.list.ontimeout (timers.js:101:19)

 AssertionError: 52 == 0
    at Object._onTimeout (/Users/brian/src/node-postgres/test/integration/client/cancel-query-tests.js:36:10)
    at Timer.list.ontimeout (timers.js:101:19)

make: *** [test-integration] Error 1
@troyk

This comment has been minimized.

Show comment
Hide comment
@troyk

troyk Aug 21, 2012

@brianc found the issue but wanted to ping your opinion. The issue is the test is creating a single query instance and then re-using it multiple times, and since it is using the emitter API query3 which is not cancelled is emitting row events and incrementing rows1 .. rows4. The test passed before because client.query would always return a NEW query instance (by design or accident, Query obj uses the same property names as client.query({})), but now it will return the same query instance if you called it with an actual query instance.

So a decision is in order:

  1. Keep API as is, change test. (see code below, test passes - note how it does not use the same Query instance)
  2. Change API to return a new cloned Query obj if you pass it a Query obj (not a fan of this)
  3. Add a method to Query to clone the appropriate properties and return a new instance and make this the API for using a Query instance multiple times
  var client = helper.client();

  //var qry = client.query("select name from person order by name");
  var qry = {text:"select name from person order by name"};

  client.on('drain', client.end.bind(client));

    var rows1 = 0, rows2 = 0, rows3 = 0, rows4 = 0;

    var query1 = client.query(qry);
    query1.on('row', function(row) {
        rows1++;
    });
    var query2 = client.query(qry);
    query2.on('row', function(row) {
        rows2++;
    });
    var query3 = client.query(qry);
    query3.on('row', function(row) {
        rows3++;
    });
    var query4 = client.query(qry);
    query4.on('row', function(row) {
        rows4++;
    });

    helper.pg.cancel(helper.config, client, query1);
    helper.pg.cancel(helper.config, client, query2);
    helper.pg.cancel(helper.config, client, query4);

    setTimeout(function() {
        assert.equal(rows1, 0);
        assert.equal(rows2, 0);
        assert.equal(rows4, 0);
    }, 2000);

troyk commented Aug 21, 2012

@brianc found the issue but wanted to ping your opinion. The issue is the test is creating a single query instance and then re-using it multiple times, and since it is using the emitter API query3 which is not cancelled is emitting row events and incrementing rows1 .. rows4. The test passed before because client.query would always return a NEW query instance (by design or accident, Query obj uses the same property names as client.query({})), but now it will return the same query instance if you called it with an actual query instance.

So a decision is in order:

  1. Keep API as is, change test. (see code below, test passes - note how it does not use the same Query instance)
  2. Change API to return a new cloned Query obj if you pass it a Query obj (not a fan of this)
  3. Add a method to Query to clone the appropriate properties and return a new instance and make this the API for using a Query instance multiple times
  var client = helper.client();

  //var qry = client.query("select name from person order by name");
  var qry = {text:"select name from person order by name"};

  client.on('drain', client.end.bind(client));

    var rows1 = 0, rows2 = 0, rows3 = 0, rows4 = 0;

    var query1 = client.query(qry);
    query1.on('row', function(row) {
        rows1++;
    });
    var query2 = client.query(qry);
    query2.on('row', function(row) {
        rows2++;
    });
    var query3 = client.query(qry);
    query3.on('row', function(row) {
        rows3++;
    });
    var query4 = client.query(qry);
    query4.on('row', function(row) {
        rows4++;
    });

    helper.pg.cancel(helper.config, client, query1);
    helper.pg.cancel(helper.config, client, query2);
    helper.pg.cancel(helper.config, client, query4);

    setTimeout(function() {
        assert.equal(rows1, 0);
        assert.equal(rows2, 0);
        assert.equal(rows4, 0);
    }, 2000);
@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Aug 31, 2012

Owner

I have no idea why I was using the same query multiple times in the test. Honestly probably was being lazy that night when I was coding. Sorry about that! They're not intended to be "reused" (they're incredibly cheap to create) and so your insight into using a different query for each test is spot on. Could you send over a pull request w/ the test fixes as well as the code fixes? Much easier to view in diff form.

Owner

brianc commented Aug 31, 2012

I have no idea why I was using the same query multiple times in the test. Honestly probably was being lazy that night when I was coding. Sorry about that! They're not intended to be "reused" (they're incredibly cheap to create) and so your insight into using a different query for each test is spot on. Could you send over a pull request w/ the test fixes as well as the code fixes? Much easier to view in diff form.

@grncdr grncdr referenced this pull request Dec 11, 2012

Closed

Premade query object #228

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Dec 11, 2012

Owner

finally got this thing merged in! There was some weird stuff going on w/ the binary tests via an old pull request. I repaid that technical debt and got the test suite green again. Thanks so much. Sorry again for the delay in merging. ❤️

Owner

brianc commented Dec 11, 2012

finally got this thing merged in! There was some weird stuff going on w/ the binary tests via an old pull request. I repaid that technical debt and got the test suite green again. Thanks so much. Sorry again for the delay in merging. ❤️

@brianc brianc closed this Dec 11, 2012

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