Fix multiple thens running query builder multiple times

[MellowMelon]

The knex documentation has the following under the then method (emphasis mine):

Coerces the current query builder chain into a promise state, accepting the resolve and reject handlers as specified by the Promises/A+ spec. As stated in the spec, more than one call to the then method for the current query chain will resolve with the same value, in the order they were called; the query will not be executed multiple times.

This PR adds a test showing that the bolded statement is not correctly implemented by knex and fixes the bug.

[wubzz]

I never took note to this part of the docs before, but I don't like the sound of it, regardless of Promise spec and whatnot. To me this sounds like the example below will keep returning the same two rows from the initial .then call, or am I misunderstanding completely?

const queryBuilder = knex('accounts').select();

return queryBuilder
.then((result) => {
    console.log(result.length); // 2
    return queryBuilder.where('userid', '1').select(); // Add where clause
})
.then((result) => {
    console.log(result.length); // 2 again ??
});

I'd expect two different queries to be run in this example.

[elhigu]

Looks like the documentation is wrong at least and IIRC Promise A+ spec doesn't require that thenable returns always the same result, when its resolved multiple times. Here are some links where this has been discussed earlier:

#1952
#5816
#470
#1105

I had also missed this part of docs and indeed it would mean that one couldn't anymore trigger the same query multiple times, by calling .then again, but one would have to do something like: query.clone().then() to be able to reuse the same query builder for triggering multiple queries.

It would mean also, that all query builder's query building methods should start to throw an exception if one tries to add stuff to query after it has been triggered once.

This is so fundamental change to how knex is working currently, that it should first be discussed through and see what are up and down sides of each functionality in different use cases.

I'm not completely against this, but for now I would rather just fix the documentation with examples how to trigger the same query multiple times and how to actually just store promise, which will return the same results.

Current way supports for example:

const result = await query;
const refreshedResult await query;

With the change one would have to do:

const result = await query.clone();
const result2 = await query.clone();

With current system if one likes to store resulted promise and resolve that multiple times without triggering the query again, he can do

// this kind of example could be added to docs
const resultPromise = query.then(res => res); // or any other mapping to returned data
resultPromise.then(res => {
  // query was not triggered again
});

ps. Sorry I didn't have much time to write this or think this very clearly, so I might need to edit this later on with better argumentation and language :)

[MellowMelon]

No issues with the language, and I understand the reluctance to accept the PR if the change is breaking. Thank you for the quick response. I might prepare a PR to fix the docs instead, since those are clearly broken.

I do want to point out that Bluebird makes no such assumptions about calling then multiple times. The reason we discovered this bug is when we used Promise.each (source code) on some knex queries, which resulted in each query being executed twice. This is extremely surprising behavior, probably more surprising than if the samples in the comments above failed. But if the promises spec really doesn't care about then'ing multiple times, that might be for Bluebird to consider. (I haven't gone to them yet.)

Re wubzz's comment: I believe that piece of code is not an issue since the return of a then is a different object from the original. This issue is about when you carry the same promise around and .then it twice (not a common occurrence), not when you chain thens. I think there are several tests that would fail if this PR broke the piece of code you gave.

[wubzz]

@MellowMelon Actually I just applied this patch to 0.13.0 in a local project, and tested against the example code, and the result is as I suspected.

const queryBuilder = knex('Accounts').select();

queryBuilder.on('query', (a) => {
	console.log(a.sql);
});

queryBuilder
.then((rows) => {
	console.log(rows.length);
	return queryBuilder.whereRaw('1 = 0').select(); //Append where clause
})
.then((rows) => {
	console.log(rows.length);
});

Console output:

select * from "Accounts"
27
27

Only runs one query, expected two.

[elhigu]

I updated #5816 with better description. I think this can be closed for now and if this change is coming to for example 1.0 by @tgriesser then we should discuss it better how everything should work exactly when the behaviour is changed.

I'll add some tests which makes the current functionality official and which also tests that queryBuilder.then() returns a promise which resolves always to the same result.

[MellowMelon]

wubzz: My bad. I misinterpreted the intent of your example..