New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post processing hook for query result #2261

Merged
merged 1 commit into from Oct 12, 2017

Conversation

Projects
None yet
4 participants
@elhigu
Collaborator

elhigu commented Oct 9, 2017

Implement hook for allowing to add post processing hook to modify query results (for example to implement snake_case -> camelCase conversion for returned column names) Implements second part for #2084

Docs knex/documentation#58

@richraid21

This comment has been minimized.

Contributor

richraid21 commented Oct 10, 2017

Should this be setup as a queue of functions rather than a single assignable property?

What if the client needs multiple post-processing steps?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 10, 2017

Should this be setup as a queue of functions rather than a single assignable property?
What if the client needs multiple post-processing steps?

@richraid21 Actually I thought of that, but I wanted to keep knex side simple and robust and leave it up to client to add chaining functionality by themselves if they wish.

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Oct 11, 2017

@elhigu is it possible to pass the queryBuilder instance that generated the request to this hook, and also for the wrapIdentifier hook?

@@ -163,6 +163,13 @@ assign(Client.prototype, {
return sql;
},
postProcessResponse(resp, method) {

This comment has been minimized.

@joelmukuthu

This comment has been minimized.

@elhigu

elhigu Oct 11, 2017

Collaborator

I actually decided in the last moment when implementing this, that I don't want to pass method to that handler, since it would expose too much internal implementation details of knexto end user. If something extra is passed it can be specified and added later (I was planning to use that to allow people to distinct between normal and raw responses).

This comment has been minimized.

@elhigu

elhigu Oct 11, 2017

Collaborator

And good catch :) I'll remove this leftover of initial implementation before merging.

This comment has been minimized.

@joelmukuthu

joelmukuthu Oct 12, 2017

Contributor

Makes sense, and it probably isn't necessary to know which method generated some data that needs post-processing

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 11, 2017

@elhigu is it possible to pass the queryBuilder instance that generated the request to this hook, and also for the wrapIdentifier hook?

@joelmukuthu I'm not sure how hard that would be. I would consider that as additional feature though. What kind of use case you had for the query builder?

Maybe you could add new issue about it and the use cases why it would be good to have query builder reference passed to these hooks (there might be better more general ways to implement support for those use cases).

@wubzz

wubzz approved these changes Oct 12, 2017

this.builder.emit(
'query-response',
processedResponse,
postProcessedResponse,

This comment has been minimized.

@wubzz

wubzz Oct 12, 2017

Collaborator

Technically if a postProcessResponse function has been supplied in the config, this is no longer the query-response. But I'm not sure if it really matters given the context of the event. Just something I took note of.

This comment has been minimized.

@elhigu

elhigu Oct 12, 2017

Collaborator

It hasn't been really query response earlier either, because each knex dialect is processing the raw response from driver before passing it to this event.

@joelmukuthu

This comment has been minimized.

Contributor

joelmukuthu commented Oct 12, 2017

In both hooks, I need a some context about the identifier or data being processed. In knorm (an ORM sitting on top of knex), I need to know which model the data belongs to in the case of postProcessResponse. I thought the easiest thing was to add a reference to the model in the query builder and then I can use a standard hook for processing:

knex.client.config.wrapIdentifier = (identifer, wrap, builder) => {
  if (isField(identifier)) {
    const Model = builder._knormModel;
    const field = Model.fields[identifier];
    return wrap(field.column); // the user is allowed to specify a different column per field
  }
  return wrap(identifier);
};
knex.client.config.postProcessResponse = (data, builder) => {
  const Model = builder._knormModel;
  return new Model(data);
};

postProcessResponse is not a blocker since I have to overload the CRUD functions anyway. But with wrapIdentifier the only other way (I can think of) is to do the pre-processing before sending the identifiers to knex, which means overloading all the query-builder methods. Do you have some suggestions for this? I can as well create a new issue if it's necessary

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Oct 12, 2017

@joelmukuthu yeah, some kind of method for passing context would be useful. Not sure if that should be the query builder or some other context object. Currently builder._knormModel probably wont be copied along if one does builder.clone(). Could be better to have some context object there for passing data... that could be useful also for current query events.

@elhigu elhigu merged commit 9658c78 into tgriesser:master Oct 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

elhigu added a commit to ivanslf/knex that referenced this pull request Oct 16, 2017

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