make Query a public api #171
Conversation
|
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. |
|
hmm...are you able to run these tests locally? This pull request caused a test to fail. |
|
@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:
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); |
|
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. |
|
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. ❤️ |
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:
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.