Skip to content
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

Transaction API misleading #470

Open
timruffles opened this Issue Sep 9, 2014 · 18 comments

Comments

Projects
None yet
7 participants
@timruffles
Copy link
Collaborator

timruffles commented Sep 9, 2014

First off thanks for Knex! :)

The transaction API has a promise-like API (then(), catch()) that suggests it's exposing a promise for the eventual committal or rollback of the transaction. Instead, however, it's a stateful API with nothing to do with promises - every time you call it something happens and a different promise is returned!

If you consider the root idea of promises it is that they represent a future value, or a failure to provide that value. Calls to then() are entirely divorced from time - e.g you should be able to call then() any number of times, before, after, and during the asynchronous operation it represents, and get the same result every time.

I'd be happy to contribute to fix up this aspect of the API. I just wasted a heap of time because something that looked like a promise wasn't one.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Sep 9, 2014

Yeah, I'm not sure I've fully explained this in the docs... it's the same with knex calls in general - they're not actually promises, they're "thenables" - objects which return a promise and are able to be consumed / coerced to promises but themselves are not automatically promises... otherwise something like knex.select('*').from('accounts').toString() wouldn't work.

So the return value from knex.transaction(...).then(...) is an actual promise which fulfills the A+, but the transaction object isn't.

I don't know what would be the best approach is here... whether it's to mutate the internal state of the object to be able to return the original promise in case .then is called multiple... or to do something else.

I'll take a look into it because it's come up as a source of confusion a few times... but it'd have to be changed everywhere in the library, not just transactions, and I'm not sure if there is anything that currently depends on the current thenable behavior.

@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Sep 9, 2014

A transaction is either committed or rejected, so seems like good mapping to promise semantics. Each transaction is triggered via commit() at some future point, so it could create a deferred inside its constructor and expose its promise via then().

On the query front, it's clearly trickier as you want to be able to reuse the same builder. A solution is to put the execution behaviour behind something like an run() method: no more verbose and makes the builder/query separation clear:

// a builder
var userSelect = knex.select('name')
  .from('users');

// promises for the result of a query
var oldUsers = userSelect
  .where('age > ?', 90)
  .run();

var youngUsers = userSelect
  .where('age < ?', 25)
  .run();

var oldAndYoungUsers = Promise.all([oldUsers, youngUsers])
  .then(_.flatten);

Having things exposing a then() without promise semantics will just get more confusing as time goes on and promises are more widely used/understood (i.e ES6 adoption).

@bendrucker

This comment has been minimized.

Copy link
Collaborator

bendrucker commented Sep 9, 2014

Thenables are a useful feature and I don't see an explicit runner method as a step forward. Might be some memory issues with holding onto the promise but if anything that's the way to go.

@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Nov 18, 2014

@bendrucker it's no more code to write .run(cb) rather than .then(cb), and you make the library a good citizen in the promise eco-system. Re useful, could you give some examples? It just seems dangerous: if you pass a thenables to other code as a promise it's very likely to create bugs due to the 'not really a promise' issues, where you'd attach multiple .thens() as normal and actually kick off multiple delete queries!

@tgriesser I think it's really worth doing for the next major version number of knex. As you said, holding onto the promise would be problematic (less so with ES6 WeakMaps, but I assume you'd like to support older environments).

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Nov 18, 2014

@timruffles I guess the issue is that it's not .run(cb) vs .then(cb), it's that for every single query chain, you'd need to write .run().then(handler).

It just seems dangerous: if you pass a thenables to other code as a promise it's very likely to create bugs due to the 'not really a promise' issues, where you'd attach multiple .thens() as normal and actually kick off multiple delete queries!

I don't ever attach multiple .then's to the same original object... only ever to the return value of the original .then, otherwise it just feels counterintuitive. Can you give an example of where this ever occurs in practice?

Re: convenience, it's just a matter of having to call run every single time you end the query chain, it just feels less fluent, but I could be wrong on this.

knex.select('*').from('accounts').where({id: 1}).then(function(accts) {
  return knex.first('id').from('users').where({user_id: accts[0].id});
}).then(function() { 
  // ... 
})

vs:

knex.select('*').from('accounts').where({id: 1}).run().then(function(accts) {
  return knex.first('id').from('users').where({user_id: accts[0].id}).run();
}).then(function() { 
  // ... 
})

It's something I'm definitely still considering / weighing the pros and cons of. If it did change, it'd likely be to .exec() rather than run, which called with no arguments would return a promise otherwise it'd assume a node callback.

Also, re: being a good part of the promise ecosystem I guess this should be clarified in the docs, but knex was never intended to be a promise implementation, but instead act as "thenables", which I thought was a thing from 1.2 in the spec.

@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Nov 18, 2014

you'd need to write .run().then(handler).

Just make run:

function run(forResolve, forReject) {
  var query = runQuery();
  return forResolve || forReject ? query.then(forResolve, forReject) : query;
}

in other words, it proxies to the initial .then() of the query if you provide transformers.

I don't ever attach multiple .then's to the same original object... only ever to the return value of the original .then,

I attach multiple .then()s all the time e.g, from code I'm working on today:

var analyses = loadAnalyses(); 
var analysisA = analyses.then(_.first);
var analysisB = analyses.then(_.last);

Promises are great because they allow you to focus on logic (i.e information about the value, and relationships between that info) rather than mechanics (when we've got the value). Precisely the behaviour that explodes when something that should be immutable kicks off side-effects.

With .run(resolve, reject) people have a natural migration from using .then() (with .exec(nodeCallback) as well). This would hopefully keep the chain as fluent, and avoid confusion.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Nov 18, 2014

in other words, it proxies to the initial .then() of the query if you provide transformers.

But that'd eliminate the behavior of having the thenable chains returned inside promise handlers assimilate into the promise chain. That's one of the main benefits of using them in the first place.

var analyses = loadAnalyses(); 
var analysisA = analyses.then(_.first);
var analysisB = analyses.then(_.last);

I don't see any issues with doing that, as long as load analyses is a promise to begin with, which could be guaranteed with knex via:

var analyses = loadAnalyses().then(); 
var analysisA = analyses.then(_.first);
var analysisB = analyses.then(_.last);
@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Nov 18, 2014

But that'd eliminate the behavior of having the thenable chains returned inside promise handlers assimilate into the promise chain. That's one of the main benefits of using them in the first place.

I can't see why that's the case? The behaviour after run() should be precisely the same as before:

// old
someQuery()
.then(r, rj)
.catch();

// new
someQuery()
.run(r, rj)
.catch();

I don't see any issues with doing that, as long as load analyses is a promise to begin with, which could be guaranteed with knex

Using .then() as you suggest results in code like this:

var deleted = deleteRows().then();
var wasDeleted = deleted.then(_.toBoolean);
var ids = deleted.then(_.pluck("id"));

which will provoke this commit:

commit 25a43f34ea36b1af59305f7f5ca538fd54b2dc8a
Author: Tim Ruffles <....>
Date:   Tue Nov 18 12:34:45 2014 +0000

    remove tautological then - promises are immutable!

followed by my being fired, and the tears of many a user.

I believe code should explicit not implicit. Making an immutable API context-specifically mutable, near very dangerous side effects, doesn't seem wise.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Nov 18, 2014

Using .then() as you suggest results in code like this:

Yeah, ideally that .then() would live inside the function you're calling, or you could just wrap the function in Bluebird's Promise.method and call it a day, but either way... I get what you're saying

I can't see why that's the case? The behaviour after run() should be precisely the same as before:

For a single query, sure, but would you reproduce the example here using that api? Particularly:

// Then query the table...
.then(function() {
  return knex.insert({user_name: 'Tim'}).into('users');
})

// ...and using the insert id, insert into the other table.
.then(function(rows) {
  return knex.table('accounts').insert({account_name: 'knex', user_id: rows[0]});
})

// Query both of the rows.
.then(function() {
  return knex('users')
    .join('accounts', 'users.id', 'accounts.user_id')
    .select('users.user_name as user', 'accounts.account_name as account');
})

Also, divergent logic like that there seems like it'd be confusing outside of the very, very simple cases. if something fails in one codepath I feel like following the error propagation would get a bit tricky.

One potential compromise if we want to keep the existing api but prevent consumers from overlooking the fact that the return values of knex chains are not promises (since only the returned values of .then calls from knex are promises) would be to warn if .then is called twice within the same tick of the event loop (taking a cue from Bluebird's behavior with possiblyUnhandledRejection notification).

@bendrucker

This comment has been minimized.

Copy link
Collaborator

bendrucker commented Nov 18, 2014

Re: warnings, I know the idea of privately storing the promise on a query has been raised (as __promise or whatever) and then having then do this:

if (!this.__promise) this.__promise = new Runner(this).run(); //...
return this.__promise.then(onFulfilled, onRejected);

Can't remember if there was any consensus on that or what it was. That's ultimately going to produce the most predictable results with lazy unwrapping as exists now but also the immutability guarantee as if a query were a real promise.

@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Nov 18, 2014

@tgriesser Calling .run(): more typing traded for having the behaviour more explicit. To me the trade-off is worth it:

.then(function() {
  return knex.first('id').from('users').where({user_id: accts[0].id}).run()
});

Two possible alternatives:

  1. Having a alternative builder that does thenables, specifically for in-place usage:
var knexInline = require("knex").inline;

// with helper
.then(function() {
  return knexInline.first('id').from('users').where({user_id: accts[0].id});
});
  1. x helper, again specifically documented as "for inline use"
// 'x' for exec
Object.defineProperty(queryBuilderApi, "x", {
  get: function() {
    return this.run();
  },
});

// with helper
.then(function() {
  return knex.first('id').from('users').where({user_id: accts[0].id}).x
});
@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Nov 18, 2014

@bendrucker It's a nice API - without WeakMap however it would be a source of memory leaks. For ES6 environments it could be implemented safely.

@bmac

This comment has been minimized.

Copy link
Contributor

bmac commented Nov 18, 2014

The first time I used knex, I was also confused by the fact that calling .then would execute the query. However, once I learned how it worked I haven't had any issues and I enjoy the current api.

I'm in favor of @llambda suggestion of updating the docs to make the current behavior more clear over adding a new API.

elliotf pushed a commit to elliotf/knex that referenced this issue Nov 24, 2014

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Dec 28, 2014

@timruffles I've been thinking a bit about this one lately and with some changes I'm planning on making to streamline the internal query building a ton, I think I might have an idea for a solution that will keep the current API while being able to maintain proper promise behavior where it should.

What do you foresee as the main situations where memory leaks might occur with something similar to what @bendrucker suggested? I'm thinking if you're not using a query chain as both a promise and a partial which is assimilated into other queries, it shouldn't really be an issue.

@timruffles

This comment has been minimized.

Copy link
Collaborator Author

timruffles commented Jan 5, 2015

@tgriesser yes - that's precisely where I'd see it being a problem. e.g:

var active = knex('users').where('deleted_at IS NULL');

exports.active = function() { return active.then() };
exports.online = function() { return active.where('online', true).then() };

Also, thinking about it, you'd need a exec() to allow reuse of queries via the promise API. now and later will be the same value if promises are cached, and no exec() API exists.

var recentEvents = knex("projects").where("created_at > NOW() - 1000");

var now = recentEvents.then();
var later = timeout(2000).then(function() { return recentEvents.then() });

Another option to give builder/result distinction and retain the inline style: change Result#then() so if it is resolved with a query builder it will run the query and return a promise for its result.

@bruun

This comment has been minimized.

Copy link

bruun commented Jul 6, 2015

I just wasted a heap of time because something that looked like a promise wasn't one.

You are not alone. A .then in promise-land shouldn't have side-effects, but the knex API does.

@baltar

This comment has been minimized.

Copy link

baltar commented Feb 26, 2016

What's the take on this now? I'm all for an explicit run() method. I use bookshelfjs and avoid direct use of knex, but I just stumbled across a a case where that non-promise then() hit me. I really don't think that preventing a minimal amount of verbosity is enough justification to allow such confusing and in some cases dangerous behaviour.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Mar 14, 2016

@baltar As far as I know consensus was that QueryBuilder could be changed to not be thenable and has separate method to convert it to promise.

But this will be really big change so probably not coming any time soon.

It will be nice to be able to return QueryBuilder instances from promise without triggering them in the process :) Bad side is that it adds a bit more boilerplate to tell everywhere when query should be triggered (sounds fair enough drawback to increased control).

@bruun I couldn't find anything from spec saying that calling then multiple times for thenable should be immutable, but yeah it would be nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.