New Knex Events

[ngoctranfire]

Hi there,
I don't know if it's just me, but I feel like it would be really nice if we can have a 'sucess' event in knex. It would be great if we can catch a success event. I would like this because I want to know certain types of success events and react to them accordingly, not just right before its about to query.

[rhys-vdw]

Please define "success" and "types of success"? I'm guessing you mean a "response" event, ie. when a query is executed and receives a response (like the abandoned PR #825).

[ngoctranfire]

By "types of success", i meant to say that the response event should allow you to filter on events from a certain table. For example:
If I wanted to add a response so that I can listen to all changes made on the "users" table so that I can react to just changes made onto that table.
There should really be only one type of event, and that should be the "success" event and it should allow people to filter by events by table, or operations, such as (select, update, etc...)... Not sure what I should define it as yet.

[rhys-vdw]

@ngoctranfire I think the best way to achieve this would be to add a "response" event that returns the QueryBuilder and the response object.

Ultimately the QueryBuilder could have an API that can return table, method etc. These could be used for filtering.

Just adding a "response" event should be sufficent for now.

And thoughts on this one, @elhigu?

[wubzz]

To be consistent with existing events, may I suggest prefixing the event with query- ? So like query-response.

Looking at #825 I disagree where the event was emitted. It should have been emitted after processResponse has finished, not before. That way you get the response post-processing rather than only pre-processed on the obj (sql) object.

[ngoctranfire]

@wubzz I definitely agree and that's what actually I'm intending to do... This is what I'm thinking

        var response = runner.client.processResponse(resp, runner);
        runner.builder.emit('success', assign({__knexUid: this.connection.__knexUid}, response));
        return response;

However, I may also want to consider giving people the option of filtering the emissions, to have it default to turn success emissions to be always on, or to give them an option to maybe filter emissions to only emit from 'users' table or only 'insert' operations, etc... Or maybe we just emit all 'query-success' events and then let the user filter them automatically.

[elhigu]

@rhys-vdw I find the name of the event success really bad, query-response suggested by @wubzz sounds like correct... it is consistent with query-error added while ago.

I kind of understand people liking to have events where to hook own stuff after query... sometimes it just is really useful to be able to declare some global side effects (great power comes with great responsibility!). I think it is enough to have just one event, if one likes to do filtering with query type they can do it their own way...

Anyways we need to build some kind of spec what we are doing here... I had pretty hard time trying to understand what people want to achieve with this... currently I understood from thread that event should be emitted when result is got and it should contain the response rows and type of action and table name?

To me that sounds too limited data about the query which was ran. Should we pass also original query builder for the event handler so that one can check any type of info from the query builder? Only being able to trigger on certain tables / type of actions does sound too limited to me...

How does this work with transactions? Will there be query response for every response inside transaction? Or is the transaction instance passed also to handler?

[ngoctranfire]

@rhys-vdw I agree that we should rename it query-success and I'm curious about the questions you asked too and making a list of the specs we want, to what we want to data params we should pass and how we intended it to be used.

[ngoctranfire]

Currently, I feel that the event emission should send the following data:

query: Promise.method(function(obj) {
    this.builder.emit('query', assign({__knexUid: this.connection.__knexUid}, obj));
    var runner = this;
    return this.client.query(this.connection, obj)
      .then(function(resp) {
        var response = runner.client.processResponse(resp, runner);
        runner.builder.emit('query-response', assign({__knexUid: this.connection.__knexUid)}, runner.builder, response);
        return response;
      });
  })

To be consistent with the other event emissions, we are going to be sending the __knexUid, and adding the two following data objects:
QueryBuilder - this is the runner.builder being passed in.
Response - this is the response object that has been processed and is in a recognizable and familiar format.

[wubzz]

@ngoctranfire I could be reading this wrong, but from where I'm sitting it looks like you're extending an object with the querybuilder and the response, which makes no sense to me. I would have expected to see the 3 objects being emitted as seperate arguments to the listeners.

runner.builder.emit('query-response', {..}, runner.builder, response);

[ngoctranfire]

@wubzz Definitely see the error you pointed out. There is a misplaced parentheses. This is what I call bad copy and pasting >.>.

[elhigu]

@wubzz @ngoctranfire How about transaction of the query? Is this going to be triggered even if later on transaction fails and all changes in DB are rejected?

[wubzz]

@elhigu To have the event emit while inside a transaction one would have to append a listener here:
https://github.com/tgriesser/knex/blob/master/src/transaction.js#L212

Each query within the transaction that is successful should then trigger the event. But if a query fails causing a rollback, it should not emit the event since you would not end up in the runner's resolver function for processResponse. So regardless if the transaction is eventually rolled back, the event will have been emitted for every successful query up to that point.

Whether that's a good thing or not, I have no opinion. But it is consistent with how the query event currently handles this scenario.

[ngoctranfire]

@wubzz This makes an interesting point to maybe even have a "query-rollback" event.

[elhigu]

@wubzz for this issue I'm fine with functionality, which just runs callback regardless if query was inside transaction or not. It just need to be mentioned in documentation.

Probably it is enough for user to check which transaction is used from QueryBuilder got in query-response events data.

@ngoctranfire yep, transaction-begin, transaction-commit, transaction-rollback events could be useful. But I suppose it is another issue if someone needs them...