Add a better way to handle transactions by binding them to models

[lehni]

Description

Based on the way Objection.js is handling transactions by binding them to models, I was looking for a way to do the same in LoopBack. At first I thought I would have to make heavy use of Proxy objects, but luckily it turned out that there was already a mechanism in place, but only useable for memory and transient datasources:

https://github.com/strongloop/loopback-datasource-juggler/blob/6c6df15286400e0dfbc2a458e0a074acc1abeecd/lib/datasource.js#L2068-L2101

It works by making use of DataSource's copyModel() which creates a slave model bound to a given data source.

This PR extends this existing infrastructure and adds support for database transactions through the use of beginTransaction() and Transaction.commit() / Transaction.rollback().

Below the documentation I wrote about its functioning.

I believe this is pretty complete and sturdy , but I need to write unit tests for it still.

I couldn't find any unit tests for beginTransaction() and Transaction.commit() / Transaction.rollback(), so I think I could use some help with that...

And finally, an example of how this can be used in ES6 syntax with async / await:

// Rollback:
try {
  await app.dataSources.db.transaction(async models => {
    const {MyModel} = models
    console.log(await MyModel.count()) // 0
    await MyModel.create({foo: 'bar'})
    console.log(await MyModel.count()) // 1
    throw new Error('Oops')
  })
} catch (e) {
  console.log(e) // Oops
}
console.log(await app.models.MyModel.count()) // 0

// Commit:
await app.dataSources.db.transaction(async models => {
  const {MyModel} = models
  console.log(await MyModel.count()) // 0
  await MyModel.create({foo: 'bar'})
  console.log(await MyModel.count()) // 1
})
console.log(await app.models.MyModel.count()) // 1

/**
 * Run a transaction against the DataSource.
 *
 * This method can be used in different ways based on the passed arguments and
 * type of underlying data source:
 *
 * If no `execute()` function is provided and the underlying DataSource is a
 * database that supports transactions, a Promise is returned that resolves to
 * an EventEmitter representing the transaction once it is ready.
 * `transaction.models` can be used to receive versions of the DataSource's
 * model classes which are bound to the created transaction, so that all their
 * database methods automatically use the transaction. At the end of all
 * database transactions, `transaction.commit()` can be called to commit the
 * transactions, or `transaction.rollback()` to roll them back.
 *
 * If no `execute()` function is provided on a transient or memory DataSource,
 * the EventEmitter representing the transaction is returned immediately. For
 * backward compatibility, this object also supports `transaction.exec()`
 * instead of `transaction.commit()`, and calling `transaction.rollback()` is
 * not required on such transient and memory DataSource instances.
 *
 * If an `execute()` function is provided, then it is called as soon as the
 * transaction is ready, receiving `transaction.models` as its first
 * argument. `transaction.commit()` and `transaction.rollback()` are then
 * automatically called at the end of `execute()`, based on whether exceptions
 * happen during execution or not. If no callback is provided to be called at
 * the end of the execution, a Promise object is returned that is resolved or
 * rejected as soon as the execution is completed, and the transaction is
 * committed or rolled back.
 *
 * @param {Function} execute The execute function, called with (models). Note
 *     that the instances in `models` are bound to the created transaction, and
 *     are therefore not identical with the models in `app.models`, but slaves
 *     thereof (optional).
 * @options {Object} options The options to be passed to `beginTransaction()`
 *     when creating the transaction on database sources (optional).
 * @param {Function} cb Callback called with (err)
 * @returns {Promise | EventEmitter}
 */

Related issues

• none

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style
  guide

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[bajtos]

I love this idea 👏

I did a quick review of the proposed changes, see my comments below.

@raymondfeng @kjdelisle could you please take a more thorough look?

[bajtos]

Can you extract this long callback into a standalone function, to make the code easier to read?

Transaction.begin(transactionSource.connector, options, onTransactionCreated);

// ...

function onTransactionCreated(err, transaction) {
  // ...
}

[lehni]

Yes!

[bajtos]

When the connector does not support transactions, but the options or Model properties indicate that a transaction should be used, then I think we should throw an error. Otherwise our users will think their code is wrapped in a transaction while it is not. This applies to all DAO methods.

[lehni]

Model.getDataSource().isTransaction / transaction will only be set if transactions are supported and dataSource.transaction(execute) was used. So I don't think there is a need for exceptions?

[bajtos]

My concern is that the connection between the fact whether connector supports transactions and the number of argument of CRUD methods like connector.create is not obvious and easy to unintentionally break in the future. I personally prefer a defensive approach - if we assume that transactions are enabled only when connectors support options argument, then it's better to capture/assert that assumption.

For example:

options = addTransaction(Model, options);
if (connector.create.length === 4) {
   connector.create(modelName, ..., options, createCallback);
} else if (options.transaction) {
  throw new Error('The connector does not support create() within a transaction');
} else {
   connector.create(modelName, ..., createCallback);
}

[lehni]

Ok, that makes sense. I could easily move that to addTransaction() and call the function handleTransaction() instead, so there wouldn't any replication of code. The name of the calling function (create) in this example) could be passed to it as a 3rd argument.

[lehni]

I've just addressed this now in e3d47d8
Please have a look and let me know your thoughts.

[bajtos]

Uff, a return type that allows both a transaction and a promise is pretty confusing to me. Are we expecting all users of ds.transaction to check what has been returned? I agree backwards compatibility is important, can we perhaps use the number of arguments to decide whether this function was called in the old way (and the transaction should be returned), or in the new way (and the promise should be returned)?

[lehni]

Yes, that's definitely possible and makes more sense. I will get on it.

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[slnode]

Can one of the admins verify this patch?

[lehni]

@bajtos I've addressed your feedback and pushed a new version now.

[lehni]

I've worked some more on this now, and fully decoupled the dealing with transient/memory sources VS database sources from the handling of execute(). All combinations of these scenarios are now supported and should behave the same. When no execute() function is provided, the returned transaction object supports exec() (for backward compatibility), commit() and rollback() in either case.

[lehni]

Oh and I forgot to mention: I optimised the generation of these 'slave-models' that are bound to the transaction object by the use of getters: Only the models that are actually accessed are now created as bound versions through copyModel(), and only on first access. This should be much faster now on apps with a large amount of models.

[lehni]

I've further improved the code to handle all possible combinations of arguments and data-sources, and have improved the documentation to better describe these.

I've also switched to using TransactionMixin.beginTransaction() instead of Transaction.begin() to share code that handles timeouts and transaction ids, but I think it would be better to do what I wrote in this TODO:

// Reuse TransactionMixin.beginTransaction() through a mock model that
// returns the connector.
// TODO: Move timeout/id handling from TransactionMixin.beginTransaction()
// to loopback-connector's Transaction class instead?

I consider the code ready again for review now.

[bajtos]

This will most likely crash the process on an unhandled exception. Can we report the error via the callback please?

It may be easier (and probably more robust too) to change this method to take care of method invocation to.

Here is an example to illustrate my idea:

function invokeConnectorMethod(method, Model, args, options, cb) {
  const dataSource = Model.getDataSource();
  const numArguments = args.length + 3; 
  options = // add transaction if needed
  const optionsSupported = dataSource.connector[method].length >= numArguments;
  if (options.transaction && !optionsSupported) {
    return cb(new Error(/*...*/));
  }
  const connector = dataSource.connector;
  const fullArgs = [Model.modelName].concat(args);
  if (optionsSupported) fullArgs.push(options);
  fullArgs.push(cb);
  connector[method].apply(connector, fullArgs);
}

Thoughts?

[lehni]

You're probably right! I did it this way because of your suggestion to throw an Exception : )
I will look into this now, but the reason why we probably can't do this invokeConnectorMethod() that simply is because of irregular method signatures, see https://github.com/lehni/loopback-datasource-juggler/blob/e3d47d8ebb1b77e77ea638e10a512dfadb0aa066/lib/dao.js#L2473-L2475

But we could of course add this as an exception directly in invokeConnectorMethod()?

[lehni]

Implemented in 7d41a8f

[bajtos]

@slnode ok to test

[bajtos]

couldn't find any unit tests for beginTransaction() and Transaction.commit() / Transaction.rollback(), so I think I could use some help with that...

I found this test that's doing a very minimal test of the transaction feature: https://github.com/strongloop/loopback-datasource-juggler/blob/2ab4a26396691949627f582998153bc10ea3470d/test/schema.test.js#L36-L63

IMO, we should ideally test all DAO methods to verify that they correctly propagate the transaction. I am proposing to create a new file test/transactions.test.js and place all new tests there. Let's keep the existing test unchanged as a guard for backwards compatibility.

Could you please rebase your patch on top of the latest master? I added support for code coverage via nyc and integration with coveralls in #1481, it will help us to better asses what test cases we may be missing.

[kjdelisle]

Minor nitpick:
// It must've executed synchronously, so call done now.
or
// It must have executed synchronously, so call done now.

[lehni]

Addressed!

[kjdelisle]

IMO, we should ideally test all DAO methods to verify that they correctly propagate the transaction. I am proposing to create a new file test/transactions.test.js and place all new tests there. Let's keep the existing test unchanged as a guard for backwards compatibility.

+1
IMO, The *OrCreate helper methods need test coverage to ensure that they leverage this functionality as intended.

That and a single nitpick with one of the comments, and I'm more than satisfied.

Excellent pull request. 💯

[kjdelisle]

Forgot to add:
+9001 for the documentation-quality explanation of the purpose and function of the pull request!

[lehni]

@kjdelisle thanks for the feedback and the kind words! I'll get on it shortly.

In the meantime, I'd like to suggest that we get these two PRs merged, so that my code here can rely on that being present for timeouts to work, instead of the dirty trick with TransactionMixin.beginTransaction() as described above (#1472 (comment)):

#1484
loopbackio/loopback-connector#116

[lehni]

@bajtos, @kjdelisle regarding tests: I believe that testing the functionality of actual transaction handling os the business of loopback-connector and outside of the scope of this RP. What we need to test here is that the various scenarios outlined in the function's doc comment work as described, that the transaction's models are actually bound to the transaction, that commit() and rollback() get called in the right situations, etc. I am not quite sure how I'll do it yet, but it'll most probably be with a mock data-source that has a beginTransaction() method for database sources, and another that has transaction() for memory and transient sources. I'll get on it shortly!

[lehni]

@bajtos, @kjdelisle I've added some tests now. Once #1484 and loopbackio/loopback-connector#116 land, I'll add a test for timeouts also.

But I think generally, this is about how much I think we should test here. The rest really belongs to
loopback-connector and co., no?

[bajtos]

The rest really belongs to loopback-connector and co., no?

So far, we were keeping the shared test suite here in juggler - see test/persistence-hooks.suite.js for an example. Connectors then include these test files in their test suite, see e.g. mongodb connector.

I guess the situation is a bit tricky for transactions, because the only built-in connector (memory) does not support them, so we don't have an easy way how to run these tests as part of juggler's npm test.

@kjdelisle what's your take on this?

[bajtos]

I find this suspicious - we delete slaveModels[name] but don't set any new value. Should we store the result of dataSource.copyModel to slaveModels[name]?

[lehni]

dataSource.copyModel() sets the new value by itself. I need to delete it so that it can do that, because if it's still there, it wouldn't override. That's part of the caching mechanism, and works well.

[lehni]

I could store it additionally for clarity, but it's not required...

[bajtos]

Sounds good. Is there any test verifying this? (I.e. accessing the same slave model multiple times?)

[lehni]

That's a good point! I will add a test for it.

[zbarbuto]

@lehni Is there any way to get to the actual transaction from inside execute? If not, do you think this is something that should probably be added?

e.g

let instance = Model.findOne(); // instance I had loaded previously

await app.dataSources.db.transaction(async (models, cb, tx) => {
  const {MyModel} = models
  let target = await MyModel.create({foo: 'bar'})
  // I already have an instance but it isn't bound to the tx
  instance.updateAttributes({ targetId: target.id }, { transaction: tx});
})

[lehni]

I agree, this makes a lot of sense. I would suggest to add it as the optional 2nd parameter before cb though. I’m away until next week, will look into it then!

[lehni]

@zbarbuto I've started to look into this now. I see a few issues here currently that I'm not quite sure how to resolve yet:

1. The description of transaction() mentions a transaction object that is returned if no execute() is provided, giving access to models (the slave models bound to the transaction), commit(), rollback() etc. This isn't the actual transaction object that needs to be passed to the db functions though, it's a wrapper thereof. Wouldn't it be weird if execute() now received the actual transaction object (let's call it the "internal" transaction here onwards), which wouldn't be this wrapper object that the rest of the documentation talks about? If the wrapper object was passed instead, transaction.currentTransaction would give access to the internal transaction object. This naming was necessary since transaction itself inherits from the datasource, where transaction() already is a the main transaction function. This is far from ideal, but I had to work with what was already there...

2. It makes the most sense to provide this as the 2nd argument, pushing callback to the 3rd place, but this would mean we'd break compatibility with the current version. Since this just rolled out a few days ago I don't think that would be a problem, but it would merit a minor version increment I guess?

3. If we decide to insert a 2nd argument there, perhaps we could choose to provide a context object instead as elsewhere in LoopBack, with the properties transaction (in this case being the "internal" transaction object) and models, allowing us to add other things to it in the future, if needed? Although I can't really think of anything else right now, other than perhaps the initial options when calling transaction()...

@bajtos, @zbarbuto, @kjdelisle what do you think we should do here?

[bajtos]

2] It makes the most sense to provide this as the 2nd argument, pushing callback to the 3rd place, but this would mean we'd break compatibility with the current version. Since this just rolled out a few days ago I don't think that would be a problem, but it would merit a minor version increment I guess?

We are pretty strict about backwards compatibility. If a code working with the latest version published to npmjs.org stops working after upgrade, then such changes must be released as semver-major version.

Unfortunately, releasing a new major version of LoopBack is requires non-trivial amount of work which we are not willing to make now, considering the coming Beta release of https://github.com/strongloop/loopback-next (which we are planning to release as LoopBack 4.0).

3] If we decide to insert a 2nd argument there, perhaps we could choose to provide a context object instead as elsewhere in LoopBack, with the properties transaction (in this case being the "internal" transaction object) and models, allowing us to add other things to it in the future, if needed? Although I can't really think of anything else right now, other than perhaps the initial options when calling transaction()...

I am little bit confused. All CRUD methods (lib/dao.js) should be accepting an optional options argument that's typically used to provide the transaction. AFAICT, updateAttributes is already accepting it, see https://github.com/strongloop/loopback-datasource-juggler/blob/3a6ddf927dc7120efe9bdf149b78e4a5b6cd9d8d/lib/dao.js#L3155

IMO, we should continue using the convention of passing the transaction as an options property.

In which case, if I understand the problem correctly, we need to figure out how deal with two different kinds of "transaction" objects we can have in our app. I think transaction.currentTransaction (or perhaps transaction.current?) is a reasonable solution. Is it perhaps possible to reliably detect which kind of transaction object we have received and convert the "wrapper" to the "real" transaction automatically for the user?

Sorry if my comment is too terse a cryptic, I am short of time :(

[zbarbuto]

What about if the proxy models result had a getTransaction method on it or something similar that returned the transaction. That’s way it stays backwards compatible:

db.transaction(async models => {
  let tx = models.getTransaction();
  await instannce.updateAttributes(update, { transaction: tx });
})

[lehni]

Hmm that feels like a bit like a hack, no?

[zbarbuto]

Can’t really think of any other way unless a new method gets added to db with the signature that has the extra tx argument passed to the callback. But that’s just as hack-y imo. In any case I think it probably warrants its issue as it’s been a bit of a pain point for me when trying to use this (have to re-findById every instance I use and can’t pass the tx out to other helper functions, have to pass the whole models)

[zbarbuto]

Alternative (still hack but backwards compatible):

models.models = models
models.transaction = transaction

Allowing:

db.transaction(({ models, transaction}, cb) => {
 // ...
})

[lehni]

Yeah I was thinking of that too... I don't think it's a problem to insert a new 2nd argument since the feature itself just rolled out. Adoption is probably at 0.0001% right now : )

But what about the other question regarding the transaction object? I'm a bit split there...

[zbarbuto]

I think that is more a case of documentation. If you do not pass in an execute() function what the promise is resolving to isn't really the transaction, it's the transaction context (like you sort of hinted to before) which could have the actual transaction on it. So the flow would be something like:

// model was retrieved earlier outside the tx
const transactionCtx = await db.transaction();
const { transaction, models } = transactionCtx;
await model.updateAttributes(update, { context: transactionCtx.transaction })
await models.Foo.find();
await transactionCtx.commit();

Although I must admit looking at this it's a bit clumsy - since you're passing in transactions in some places and not in others, based on whether or not the model was retrieved using the proxy. Perhaps a better option would be to provide some method of binding existing model instances to the transaction so they behave like ones that were fetched from the proxy. Something like:

// model was retrieved earlier outside the tx
const transaction = await db.transaction();
const { models } = transaction;
transaction.bindToModel(model);
await model.updateAttributes(update) // is now part of the tx
await models.Foo.find();
await transaction.commit();

That way the first few lines of your tx execution are binding your models to the tx and then the rest behaves as-is.

[lehni]

I somehow didn't see @bajtos' reply earlier. If compatibility must remain, although the docs here don't actually mention this 2nd callback argument yet (they should!), the only options I see now is models.getTransaction() as @zbarbuto proposes (or perhaps more easily, just models.transaction ?), or a 2nd ctx argument instead of models, so that ctx = {models, transaction}. To remain backward compatible, ctx would also have to functions as the models argument, e.g. through JS-style inheritance, perhaps something like:

let ctx = Object.create(models);
ctx.models = models;
ctx.transaction = transaction;

But I don't think this is clean though...

I think models.transaction is actually the best solution now...

As for the two suggestions by @zbarbuto, neither of those are easily feasible with the current code-base, and I don't have the time to investigate them:

The 1st suggestion doesn't work since db.transaction() has been around, and has been returning what I called above the transaction wrapper object. It's in fact a EventEmitter object that inherits all properties of the original data-source, and is used internally on the created slave-models as part of the mechanism that binds the models to the transaction. All this code has already been in place from earlier work for transient and memory sources, I've just leveraged it in this PR to add support for db transactions.

The 2nd suggestion isn't possible either, because binding can't be changed on existing models without breaking things elsewhere, since in order to change this binding, we need to change the model's data-source (hence these bound slave models).

@bajtos to clarify: The db methods only ever receive the "internal" transaction object. The wrapper object is created to pretend to be a data-source that is in fact bound to the transaction, but inherits from the real data-source. I'm not explaining this well, but I'm also not the author of this approach. It was in place in the original dataSource.transaction() method that only worked with transient and memory sources.

I've called the reference to the internal transaction transaction.currentTransaction because transaction here really is the slave model's data-source, and is accessed through it in invokeConnectorMethod() (look for: {transaction: dataSource.currentTransaction}). If you're fine with this simply being called dataSource.current, we could add it to the documentation and expose it... In the context of let transaction = db.transaction(); ... {transaction: transaction.current} this would make sense, but elsewhere it would seem strange. And currently it's considered and internal property only.

[lehni]

A last option, and perhaps the most elegant one, could be to make every transaction object a proper data-source that inherits from the underlying data-source and works in the way expected by db.transaction(). There would be quite a bit of work involved, but I think it may clean up quite a bit of confusion and be clearer to maintain also... It could be done without breaking backward compatibility. This wouldn't solve the problem of exposing it to execute(), though, but that way, what is returned by dataSource.transaction() if no execute() method is provided could then be directly passed to all the db methods as the transaction object in the options like so:

const transaction = await dataSource.transaction();
await instance.updateAttributes(update, { tx: transaction });

This transaction objet would then also need a special getter for transaction.models that would create the structure that creates bound slave models only on request for efficiency.

But then we face the problem again that the "internal" transaction object is defined by loopback-connector, while all this extra functionality will have to be in loopback-datasource-juggler... Hence the reason for the wrapper... Oh dear.

[lehni]

@zbarbuto would this workaround work for? I haven't tested it, but it should work:

dataSource.transaction(async models => {
  let tx = models[Object.keys(models)[0]].getDataSource().currentTransaction;
  await instannce.updateAttributes(update, { transaction: tx });
})

[zbarbuto]

Thanks for the workaround. Is it right right to get the transaction directly from the dataSource? What if you have multiple concurrent transactions happening asynchronously - what is currentTransaction on the dataSource? The last one?

[lehni]

Since you're retrieving the model from the models object that is passed to execute() it only contains slave-models bound to this transaction. Getting the currentTransaction from one of the slave model's dataSource (which is the same as the transaction wrapper object as outlined above) will give you the right transaction. Note that these slave models are only ever bound to one transaction. If you're running multiple transactions concurrently, you end up with multiple versions of such bound slave models, each bound to only one transaction.

I hope this clarifies it.

[lehni]

@bajtos @zbarbuto to clarify my rambling above from yesterday:

We can speak of the transactionInstance of which the class Transaction is provided by loopback-connector and can be passed to all the database methods through their options, and the transactionDataSource, which is created by dataSource.transaction()when it creates a transactionDataSource that inherits from the main dataSource it is called on, connecting this slave dataSource with the created transaction, and uses it to bind the created slave-models to this transaction. This transactionDataSource is then also what's returned and through which the created transaction is exposed currently, indirectly... (this is a let-over from how transaction() was built predating my PR here) It exposes commit() and rollback() methods that abstract away the underlying type of transaction (memory, transient or database, of which only the last one actually use a transactionInstance internally).

This all makes sense I think, the only part in this that I don't think is elegant currently is that this transactionDataSource isn't actually a DataSource instance. It's a pure EventEmitter into which a bunch of properties are copied over.

I can look into creating a TransactionDataSource subclass that does all the right things, and then clean up the documentation. I can also look into supporting such a value in options: { transaction: transactionDataSource } so that it would retrieve the actual option from it and could be passed around, eliminating the only need I can see to access its currentTransaction property.

And the reason why I didn't call the transactionInstanceproperty transactionDataSource.transaction was that dataSoure.transaction() is the main method that we're discussing here, and it would override that. But calling it on a transactionDataSource doesn't make sense anyway, so maybe it's ok if this property then becomes the actual transactionInstance there?

Changing this I wouldn't consider breaking because currentTransaction wasn't documented and considered internal only so far.

I hope this clears up the discussion...

[bajtos]

Great discussion! I am afraid I don't have bandwidth to follow in details, I prefer to leave it up to you two to figure this out. From my limited understanding of the problem and your proposals, models.transaction looks like the best option to me, especially considering the implementation complexity of other options.

My only concern is about a possible name collision (what if the app has a model called Transaction, e.g. in banking or e-commerce). Perhaps models.tx would be less probable to collide? models.getTransaction() is probably the safest option, since model names usually don't start with a verb. Few more options that come to my mind: models.$transaction, models.$tx, models._transaction, models._tx.

[zbarbuto]

Yeah naming collisions was exactly why I suggested a getter in the first place. _transaction isn’t too bad as it fits the loopback pattern of prefacing private props with underscores.

[lehni]

In that case I think I prefer the getter actually, since accessing underscored properties form the outside always feels like I'm doing something I'm not supposed to be doing : /

But regarding naming collisions: models.transaction wouldn't collide with models.Transaction, and I don't think people use lowercased names for LB models?

If there was a case of a collision I think we'd simply not expose the transaction and give the transaction model the precedence.

[lehni]

Some more options

1. We could inspect the arguments of the passed execute() function:

   function getParameters(fn) {
     var str = fn.toString().match(/^\s*function[^\(]*\(([^\)]*)/)[1];
     return str ? str.split(/\s*,\s*/) : [];
   }
   
   console.log(getParameters(function(models, transaction, cb) {}))
   // ["models", "transaction", "cb"]

   We could simply look at parameter count (fn.length) and if fn.length === 2, then we'd look at the 2nd parameter name, and assume it's the transaction object if it is transaction or tx, and fall back to passing the callback in any other case. If fn.length === 3, then it's always the transaction.

   This would guarantee backward compatibility. But in the rare case where somebody uses code minification in Node.js, it would always behave in the old way, passing callback 2nd (only if there are 2 parameters)

2. We could call execute() with transactionDataSource as this. You won't be able to access this from a double-arrow functions then, but you could access the transactionInstance through this.currentTransaction inside a normal function() {} callback.

3. We could move the part of the code that "wraps" the code in the execute() with a transaction to a new function with the new, better behaviour, and call it something like dataSource.executeTransaction(models, transaction, callback), dataSource.wrapInTransaction(models, transaction, callback) or dataSource.runInTransaction(models, transaction, callback) or something the like? This function could then be used by db.transaction() to handle execute() also in a backward compatible way, but we could drop it from the documentation there then.

I quite like the last option as it is the cleanest... I'm also fine with the first, depending on what @bajtos thinks...

[bajtos]

But regarding naming collisions: models.transaction wouldn't collide with models.Transaction, and I don't think people use lowercased names for LB models?

Well, we are using user extending the built-in User model in our examples, I vaguely remember that I may have seen some other examples of lowercased model names in the issues people are reporting.

accessing underscored properties form the outside always feels like I'm doing something I'm not supposed to be doing

+1

---

Regarding models.getTransaction() vs. dataSource.executeTransaction() or runInTransaction() - both approaches are fine with me. I think I like the latter slightly more.

[lehni]

@bajtos what about suggestion 2, the use of this to run the execute method with? And what naming do prefer for suggestion 3? It could also simply be runTransaction()?

[bajtos]

what about suggestion 2

I find overloading this as confusing, it makes the code more difficult to read. I find the inability to use arrow functions as too limiting.

And what naming do prefer for suggestion 3? It could also simply be runTransaction()?

I don't have any strong opinion, please pick one that looks best to you.

[lehni]

Let's see. What feels the best? @zbarbuto, any suggestions?

dataSource.executeTransaction(async (models, transaction) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

dataSource.runTransaction(async (models, transaction) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

dataSource.runInTransaction(async (models, transaction) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

Or should we then also go with the idea of the ctx object to be future proof?

dataSource.executeTransaction(async ({transaction}) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

dataSource.runTransaction(async ({transaction}) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

dataSource.runInTransaction(async ({transaction}) => {
  await outsideInstance.updateAttributes(update, {transaction});
})

[zbarbuto]

My issue with adding another method is that it sort of just adds un-needed method bloat to the datasource object. You have .transaction and .executeTransaction which do effectively exactly the same thing but one is just a slightly less functional version of the other. Moreover beacuse of the nature of how the committing/rollback works it would make it hard to deprecate transaction and just call .executeTransaction under the hood. I definitely prefer adding more context to the current return value.

If there was a case of a collision I think we'd simply not expose the transaction and give the transaction model the precedence.

Doesn't seem fair that if you want to use a transaction lowercase model you don't get to use this method of using db transactions.

My vote goes to leaving the method named as-is but appending .getTransaction or .currentTransaction to the models proxy. The documentation could always be updated to avoid using the variable name models to avoid confusion:

db.transaction(async context /* or 'execution' */ => {
  const tx = context.getTransaction();
  const { User, Account } = context;
  // ...
})

[lehni]

Ok but in that case I'll suggest doing this:

We add context.models and context.transaction, and add all models that do not clash with either of these names to context directly as well (cheap, through thin getters that redirect to models[name]).

Then you can use it like this:

db.transaction(async ({models, transaction}) => {
  const { User, Account } = models
  // ...
})

And we're remain compatible as long as people don't deal with a model called models or transaction, so this would still work:

db.transaction(async (models) => {
  const { User, Account } = models
  // ...
})

After a while, we could deprecate this access on context and log a warning when it's used. And in a future major release it could be removed...

I really think this is fine, as this code has barely been out, and we'll remove the documentation that mentions the models argument in favour of context. It will also allow us to expose other things without changing the calling signature again, should we need them.

@bajtos do you approve?

[zbarbuto]

@lehni I thought the decision was to go for a getTransaction getter to retrieve the transaction rather than .transaction to avoid collisions. Transaction seems like a perfectly reasonable model name to want to use.

What if the app has a model called Transaction, e.g. in banking or e-commerce
Naming collisions was exactly why I suggested a getter in the first place
Doesn't seem fair that if you want to use a transaction lowercase model you don't get to use this method of using db transactions.

If you really want to allow the db.transaction(async ({models, transaction}) => syntax, perhaps add that transaction property to the context only if there is no existing transaction model in the app and have it implicitly call getTransaction under the hood.

[lehni]

No decision has been made yet. I liked the separate method best, getTransaction() on models feels like a hack to me...

context seems like the most future-proof approach, as it allows adding more things at a later point if required, but that would mean we'd need to start encouraging its use right away.