Improve query composability and support for utility functions

[jevakallio]

Recently moved from Bookshelf to writing raw knex queries and loving the experience. One thing that's bugging me is that there doesn't seem to be a great way to extract commonly used domain-specific query patterns and reuse them as functions.

This spike adds .compose(fn) method to the query builder to achieve this. The method takes in a callback function, whose this context is bound to the current query builder. From syntax point of view it works similarly with .whereWrapped(fn), but additionally allows the function to add arbitrary operations to the query. See example below for details.

Just testing waters here, are you interested in this type of new functionality? If yes, I can contribute tests and documentation. Naming and semantics of the method can be changed if you see fit.

Example

In my domain many tables have a foreign key image_asset_id referencing image_assets table, and I would like to include certain fields from this table into my query results.

Currently I have this pattern in multiple queries:

  return knex('some_table')
    //...snip...
    .leftJoin('image_assets', some_table.image_asset_id', 'image_assets.id')
    .select(
      'some_table.*',
      'image_assets.asset_url',
      'image_assets.asset_height',
      'image_assets.asset_width');
    .then(rows => rows.map(modelify));

With compose I could do the following

  return knex('some_table')
    //...snip...
    .select('some_table.*')
    .compose(withImageAssetOf, 'some_table')
    .then(rows => rows.map(modelify));

Where the withImageAssetOf function can be defined reusably e.g. as follows:

function withImageAssetOf(tableName) {
  this
    .leftJoin('image_assets', tableName + '.image_asset_id', 'image_assets.id')
    .select(
      'image_assets.asset_url',
      'image_assets.asset_height',
      'image_assets.asset_width');
}

Thanks for your great work on knex!

[StreetStrider]

@jevakallio, great idea. I have some things to say:

• (1) It would be better for composability if transformer function will take query builder as its first parameter (instead of using this).
• (2) Transformer function should not take any other arguments. If some parameters is required it can be done with hi-order function.

From this two points your example can look like this:

function withImageAssetOf(tableName) {
  return function(query) {
    return query
        .leftJoin('image_assets', tableName + '.image_asset_id', 'image_assets.id')
        .select(
          'image_assets.asset_url',
          'image_assets.asset_height',
          'image_assets.asset_width');
  }
}

Other, not parametrized transformers can be done without hi-order function (they just refer to query builder).

• (3) I'm not sure the name compose is good, since it's associated with functional composition. I think the name apply is better, but it can be mixed up with Function.prototype.apply, so it is not good enough too :(. Maybe the name transform is better than both.

Here's example which reveals the power of this three points:

/* simple transformer */
function isPublic (query)
{
  return query
  .where('is_public', true)
  .where('rank', '>', 0)
}
/* parametrized transformer */
function byUser (author_or_reviewer_id)
{
  return function (query)
  {
    return query
    .where('author', author_or_reviewer_id)
    .orWhere('reviewer', author_or_reviewer_id)
  }
}

/* now some query */
db.table('publications')
.select()
.transform(isPublic)
.transform(byUser(1))

/* thanks to (1) & (2) transformer functions can be composed*/
var compose = require('lodash').compose

var userRelatedPublic = compose(byUser(1), isPublic)
/* ... */
query.transform(userRelatedPublic)

[jevakallio]

@StreetStrider thanks for the feedback! My thoughts in order

It would be better for composability if transformer function will take query builder as its first parameter (instead of using this).

this-less style would be my personal preference too (and that's how I initially implemented it), but I believe parity with existing API is more important. If you look at how e.g. where, whereWrapped etc. works, binding the query to this. I would like to follow the same convention for clarity, anything else would be surprising to the user.

Transformer function should not take any other arguments. If some parameters is required it can be done with hi-order function.

Whether we like it or now, higher order functions and returning closures is still something that many javascript developers are not very familiar or comfortable with, and because the rest of knex does not require that style of programming, I don't think we should force it on to the user either.

You can, however, choose not to use the optional provided parameters and use higher order functions if you want:

function byUser (author_or_reviewer_id)
{
  return function ()
  {
    return this
    .where('author', author_or_reviewer_id)
    .orWhere('reviewer', author_or_reviewer_id)
  }
}

In this case concerns about traditional functional composability isn't really that relevant, IMO. The "composition" is achieved with chaining multiple .compose calls on a query builder, not f(g(x)).

Which brings us to...

I'm not sure the name compose is good, since it's associated with functional composition.

You're probably right. I don't like the proposed apply or transform either. How about includes, imports, helper, fn...

In any case I would like to hear from a core contributor whether there is interest in the feature in the first place, and we can fine tune based on their feedback.

[StreetStrider]

But I believe parity with existing API is more important. If you look at how e.g. where, whereWrapped etc. works, binding the query to this.

That is correct. But knex also supports an argument-style. I've not found the issue when I've replied to you for the first time, but I've found it now: #863. I'm not sure which is the preferred way. I think we should wait for maintainers' judgement here.

I don't think we should force it on to the user either.

OK. To be clear, in your exmaple tableName is the only, but not the only possible argument. So signature of original compose would be:

function compose (composer[, arg1, arg2, … argN])

Where arg* are passed as params to composer. Right?
If so, it is ok, since it covers all use-cases for hi-order function. And also this style leave a place for hi-order functions, as you mentioned. So it is a double win.

I think the name must represent that we're using some complex attachment to query builder. It is like a «plugin», maybe use as in middlewares and third-party plugins. Or helper, or leverage :)

[jevakallio]

Supporting query as arg would make it more cumbersome to receive parameters to the function (not hard, mind you, just syntactically error prone, as you might forget to receive the query arg first in the callback signature).

In any case, I'll wait to hear interest from maintainer. I think this would be a very useful feature with a very low code footprint, but no point bikeshedding on it if it's not going to ship. I'll be AFK until Monday, will catch up then if there is progress on this.

On 1 Jul 2015, at 12:04, Strider notifications@github.com wrote:

But I believe parity with existing API is more important. If you look at how e.g. where, whereWrapped etc. works, binding the query to this.

That is correct. But knex also supports an argument-style. I've not found the issue when I've replied to you for the first time, but I've found it now: #863. I'm not sure which is the preferred way. I think we should wait for maintainers' judgement here.

I don't think we should force it on to the user either.

OK. To be clear, in your exmaple tableName is the only, but not the only possible argument. So signature of original compose would be:

function compose (composer[, arg1, arg2, … argN])
Where arg* are passed as params to composer. Right?
If so, it is ok, since it covers all use-cases for hi-order function. And also this style leave a place for hi-order functions, as you mentioned. So it is a double win.

I think the name must represent that we're using some complex attachment to query builder. It is like a «plugin», maybe use as in middlewares and third-party plugins. Or helper, or leverage :)

—
Reply to this email directly or view it on GitHub.

[StreetStrider]

Any way, I'm 👍 on it. This would be a great tool for composability.

[whitelynx]

@jevakallio The unit tests seem to be failing, but it doesn't seem to be related to your changes... You could try rebasing this off master, and see if that fixes them.

Also, in order to get this merged, you'll need to add some unit tests for the compose function you added. (see CONTRIBUTING.md)

[jevakallio]

@whitelynx rebased, updated pull request with tests and documentation.

[rhys-vdw]

@jevakallio, nice concept.

Just a comment on the compose name. It seems to me that this function is the same concept as tap in lodash/underscore and bluebird chains, so I'd be tempted to suggest that name. QueryBuilder already exposes tap however, as a shortcut to Promise.tap.

Perhaps compose is fine, but if I were looking for this functionality I'd probably search for tap, so maybe tapQuery or something? Thoughts?

[jevakallio]

@rhys-vdw interesting, I had not considered that indeed technically speaking this feature is equivalent to tap, because typically I would expect a tap operation on promise/underscore chains to exist purely for side-effects, never to modify the chain.

That's my only argument against tap semantics: It doesn't communicate the fact that this feature is most useful when used to modify the query chain. Not sure how useful it is to tap into a query for side effects?

That said, I am fine with any naming the core contributors prefer, my interest is primarily to get the functionality in, since IMO it would be very useful, and simplify the codebases where I use knex myself.

Other names off the top of my head, for your consideration: use, using, partial, helper, fragment, snippet, combine, arrange, construct...

[rhys-vdw]

Hey @jevakallio. Sorry, I got sick and forgot about this. I think this is a great feature and I will definitely merge it. Hope I'm not bikeshedding now, but I do find the compose name a bit off. Both for the functional composition reason @StreetStrider mentioned, but also because to me it sounds like it should take a QueryBuilder instance as argument rather than a callback (ie. composing two queries).

Your point on tapQuery was convincing. apply is best but invalid for previously mentioned reasons.

Perhaps something explicit like mutate or modify? I think they express the intent explicitly without any potentially confusing connotations.

If you prefer compose I wont waste your time any longer and I'll just merge. Cheers.

[jevakallio]

@rhys-vdw, as they say, two hard things in programming: naming, cache invalidation and off-by-one errors...

I can very well see the point about the compose name not being perfect. I changed it to modify, as per your latest suggestion.

Let me know if further modifications are necessary. Cheers!

[rhys-vdw]

@rhys-vdw, as they say, two hard things in programming: naming, cache invalidation and off-by-one errors...

😆

Let me know if further modifications are necessary.

Thanks. It looks solid to me. Clean code, docs and tests.

[rhys-vdw]

Actually, I just looked again and realized there were a couple of things I missed. Could you make the QueryBuilder the first argument to the callback? The where callback does receive a QueryBuilder as its first argument (as well as binding this) so it would be more consistent. Also the code example in the docs still uses compose instead of modify.

Sorry!

[jevakallio]

I already fixed the documentation naming omission with --amend before you merged the PR, so that's correct in master.

The query builder parameter added in #901