Fix tranaction operations when called from the model #268

zbarbuto · 2017-06-26T23:01:09Z

Description

I was unable to reproduce #258 exactly (when I hear back I will look into this further) but I did find issues when transaction operations where called when Transaction is called from the model.

When called directly on the model, commit and rollback do not go through transaction.js in loopback-connector so what the Postgresql connector is the Transaction rather than the connection. This PR checks for that case.

Related issues

Transactions are broken on v3.0.1 #258

connect to #258

Checklist

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

slnode · 2017-06-26T23:01:16Z

Can one of the admins verify this patch?

slnode · 2017-06-26T23:01:16Z

Can one of the admins verify this patch?

slnode · 2017-06-26T23:01:16Z

Can one of the admins verify this patch?

zbarbuto · 2017-06-26T23:05:23Z

@raymondfeng @jannyHou @kjdelisle @dhmlau

If anyone can add a test reproducing the original issue in #258 I'll investigate further.

dhmlau · 2017-06-27T00:44:44Z

@zbarbuto, thanks for your fix.
I can reproduce the problem by creating the following js in boot folder:
(Customer model with name property)

'use strict';

module.exports = function(app) {
  var customer = app.models.Customer;
  customer.beginTransaction('READ COMMITTED', (err, tx)=>{
    customer.updateAll({name: 'aaa'}, {name: 'bbb'}, {transaction: tx}, (err)=>{
      console.log('err = ', err);
    });
  });
};

I can try out your fix in a few hours.

dhmlau · 2017-06-27T01:54:43Z

@zbarbuto while I believe your changes might fix some other problem, I don't think it's the cause of #258. Error was thrown before commit function. Please see my comments in #258.

jannyHou · 2017-06-27T15:08:06Z

Thank you @zbarbuto I will review it asap, and some investigation @dhmlau did yesterday shows the problem is:

In postgresql connector's beginTransaction function, the transaction instance is created from the one in:
loopback-connector-postgresql/node_modules/loopback-connector,
see var tx = new Transaction(connector, connection);

While when the postgresql's beginTransaction function calls back, it goes to begin function in:
loopback-datasource-juggler/node_modules/loopback-connector,

if (!(connection instanceof Transaction)) {
      tx = new Transaction(connector, connection);
}

the code above is get executed since the connection is an instanceof the first lb-connector, NOT instanceof the second lb-connector, and when instantiate Transaction again, the txId is removed, and therefore the connector considers the transaction is not active.
see code

zbarbuto · 2017-06-27T22:40:46Z

@dhmlau Thanks for the steps to reproduce. Can see it happen myself now and am investigating.

zbarbuto · 2017-06-27T23:07:13Z

@jannyHou You're correct. This is caused when the version of loopback-connector required by the app is different to that required by loopback-connector-postgresql resulting in the connector's instanceof returning false. Frustrating. The real fix would probably be to ensure that all components are using the same version (e.g. for the postgres connector require loopback-connector as a peer dep or avoid requiring loopback-connector at the app level).

However, I've fixed it to avoid using Transaction to track transaction state and instead use an object on the connector. It's a bit of a dirty fix but it means that the issue of #230 is still handled but allows differing versions of loopback-connector.

Not sure the best way to add a test to avoid the regression in the future. Perhaps someone else could advise?

dhmlau · 2017-06-28T02:06:24Z

@zbarbuto @jannyHou , the cause of the problem is because there are different definition of Transaction which causes this line (https://github.com/strongloop/loopback-connector/blob/master/lib/transaction.js#L88) to fail. Would it make sense to change the if-statement from
if (!(connection instanceof Transaction))
to
if (connection.txId === undefined)

cc @raymondfeng

zbarbuto · 2017-06-28T02:10:40Z

@dhmlau connection.txId is only added by this connector (was part of #230). Changing would probably break other sql connectors (such as MySQL)

The definition of Transaction is the file required in can be different because the version is different (loopback-connector-postgresql requires the one in its own ./node_modules folder if the version is different from the one in the parent package's ../node_modules which is why instanceof isn't working).

dhmlau · 2017-06-28T13:26:02Z

@jannyHou @zbarbuto , I've verified the code with and without transaction.
@jannyHou , PTAL.

jannyHou · 2017-06-28T14:46:58Z

test/postgresql.transaction.test.js

+
+    before(function(done) {
+      Post.create(p1Content, function(err, p1) {
+        Post.create(p1Content, function(err, p2) {


I think here it should be p2Content?

raymondfeng · 2017-06-28T22:25:55Z

To avoid the failure of instanceof Transaction, we probably should use duck-typing by testing the properties such as if (typeof transaction.txId === 'string').

I'm not sure we need to use activeTransactions to keep track of the valid transactions. If we have to do so, we can set connector to Transaction object and test if tx.connector === this.

zbarbuto · 2017-06-28T22:41:52Z

@raymondfeng But loopback-connector is what checks instanceof Transaction, not loopback-connector-postgresql so it does not know about txId - its unique to this connector (see #268 (comment)). The only reason txId was added to this connector was to avoid executing CRUD operations on a connection that originally had a transaction but has since been committed (#230).

Moving to loopback-connector as a peer-dependency is probably the 'cleaner' solution as it doesn't require any code changes but is potentially more breaking.

dhmlau · 2017-06-30T03:21:52Z

It looks like only Transaction has connector. How about we check:
if (connection.connector === undefined)
in this line https://github.com/strongloop/loopback-connector/blob/master/lib/transaction.js#L88

raymondfeng · 2017-06-30T15:28:18Z

@dhmlau We can probably fix the code at https://github.com/strongloop/loopback-connector/blob/master/lib/transaction.js#L88 as follows:

    var tx = connection;
    if (tx.connector !== this) {
      // The connector returns a connection instead of transaction
      tx = new Transaction(connector, connection);
    } else {
      tx = new Transaction(tx.connector, tx.connection);
    }

@dhmlau Can you verify?

dhmlau · 2017-06-30T16:51:17Z

Verified. Works with and without transaction.
@zbarbuto , are you ok with making the change suggested by @raymondfeng?

dhmlau · 2017-07-05T19:02:28Z

@zbarbuto, do you have a chance to review the suggested changes? Thanks!

dhmlau · 2017-07-05T21:33:10Z

Apologize that there has been some environment issue, therefore, the changes were not got reflected when I was doing the testing. I will have to do the verification again.
If only changes in loopback-connector is needed, I'll create a separate PR. Thanks!

zbarbuto · 2017-07-05T23:41:08Z

@dhmlau I'll PR the suggested change to loopback-connector shortly.

zbarbuto · 2017-07-05T23:51:57Z

@dhmlau Update: I just tested with @raymondfeng 's suggested change (with the above changes removed) and tests start failing after the bulk transactions (database deadlocks).

https://github.com/strongloop/loopback-connector-postgresql/blob/master/test/postgresql.transaction.test.js#L62

dhmlau · 2017-07-06T02:12:25Z

right. there's some problem with the suggested change. @jannyHou proposed something else, let me try it out and let you know.

dhmlau · 2017-07-06T03:08:14Z

@zbarbuto , I put @jannyHou 's proposed fix in loopbackio/loopback-connector#107. However, currently there's some issue with the downstream loopback-connector-dashdb CI. We're working on that.

dhmlau · 2017-07-14T16:01:17Z

@zbarbuto, we've implemented a fix in loopback-connector which seems to be a better fix than patching it on the postgreql repo, and also include the test cases in there. loopbackio/loopback-connector#109

I think the tests that you've added in postgresql.transaction.test.js would be useful. If you agree, could you please remove the changes in lib*.js and incorporate the review comments in the test.js? Thanks a lot!

zbarbuto · 2017-07-16T22:40:46Z

@dhmlau Updated tests, removed active transaction tracking and rebased off latest master.

dhmlau · 2017-07-17T14:07:04Z

lib/transaction.js

  PostgreSQL.prototype.commit = function(connection, cb) {
    debug('Commit a transaction');
    var self = this;
+    if (connection instanceof Transaction) {


I've tested our changes in loopback-connector and it works without any changes in this repo.
Any reason we need to make this change?

Actually i think CI failed because of this.

Missed this in the revert. Have removed now.

dhmlau · 2017-07-17T14:07:25Z

lib/transaction.js

  PostgreSQL.prototype.rollback = function(connection, cb) {
    debug('Rollback a transaction');
    var self = this;
+    if (connection instanceof Transaction) {


same as above.

dhmlau · 2017-07-17T14:08:25Z

lib/transaction.js

  PostgreSQL.prototype.commit = function(connection, cb) {
    debug('Commit a transaction');
    var self = this;
+    if (connection instanceof Transaction) {


Actually i think CI failed because of this.

Resolves loopbackio#258

This reverts commit b1a76b3.

zbarbuto · 2017-07-17T22:47:05Z

Rebased again.

dhmlau · 2017-07-18T13:33:11Z

@slnode test please

dhmlau · 2017-07-18T13:33:40Z

lib/transaction.js

 'use strict';
 var debug = require('debug')('loopback:connector:postgresql:transaction');
 var uuid = require('uuid');
-var Transaction = require('loopback-connector').Transaction;


the last one.. i think this is needed.

zbarbuto · 2017-07-18T22:55:15Z

Done

dhmlau · 2017-07-19T01:53:53Z

Thanks for going through all this with us! Your PR has landed!

zbarbuto mentioned this pull request Jun 27, 2017

Transactions are broken on v3.0.1 #258

Closed

dhmlau added the apex label Jun 28, 2017

dhmlau requested a review from jannyHou June 28, 2017 13:25

dhmlau added #in-progress in-progress labels Jun 28, 2017

dhmlau assigned dhmlau and zbarbuto Jun 28, 2017

dhmlau added review and removed in-progress #in-progress labels Jun 28, 2017

jannyHou reviewed Jun 28, 2017

View reviewed changes

kjdelisle removed the apex label Jul 5, 2017

dhmlau mentioned this pull request Jul 6, 2017

Fix postgresql transaction loopbackio/loopback-connector#107

Closed

2 tasks

dhmlau mentioned this pull request Jul 14, 2017

Fix Postgresql transaction loopbackio/loopback-connector#109

Merged

2 tasks

zbarbuto force-pushed the hotfix/258_transactions branch from a620839 to b4dd9b2 Compare July 16, 2017 22:40

dhmlau reviewed Jul 17, 2017

View reviewed changes

zbarbuto added 8 commits July 18, 2017 08:16

Add test for loopbackio#258

c09e846

Add missing return

1e8a2c5

Fix operations directly on model loopbackio#258

0557e46

Avoid connector for transaction tracking

b6fb9db

Resolves loopbackio#258

Remove require of Transaction

cfd40de

Update transaction test

bee6bb2

Revert transaction tracking

d001396

This reverts commit b1a76b3.

Revert change to commit and rollback

f81e206

zbarbuto force-pushed the hotfix/258_transactions branch from 0881c5a to f81e206 Compare July 17, 2017 22:46

dhmlau reviewed Jul 18, 2017

View reviewed changes

Add missing require

8583c8c

dhmlau approved these changes Jul 19, 2017

View reviewed changes

dhmlau merged commit 07129cd into loopbackio:master Jul 19, 2017

dhmlau added this to the Sprint 40 - Apex milestone Jul 19, 2017

Fix tranaction operations when called from the model #268

Fix tranaction operations when called from the model #268

Uh oh!

Conversation

zbarbuto commented Jun 26, 2017 • edited by ssh24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

slnode commented Jun 26, 2017

Uh oh!

slnode commented Jun 26, 2017

Uh oh!

slnode commented Jun 26, 2017

Uh oh!

zbarbuto commented Jun 26, 2017

Uh oh!

dhmlau commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhmlau commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jannyHou commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbarbuto commented Jun 27, 2017

Uh oh!

zbarbuto commented Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhmlau commented Jun 28, 2017

Uh oh!

zbarbuto commented Jun 28, 2017

Uh oh!

dhmlau commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Jun 28, 2017

Uh oh!

zbarbuto commented Jun 28, 2017

Uh oh!

dhmlau commented Jun 30, 2017

Uh oh!

raymondfeng commented Jun 30, 2017

Uh oh!

dhmlau commented Jun 30, 2017

Uh oh!

dhmlau commented Jul 5, 2017

Uh oh!

dhmlau commented Jul 5, 2017

Uh oh!

zbarbuto commented Jul 5, 2017

Uh oh!

zbarbuto commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhmlau commented Jul 6, 2017

Uh oh!

dhmlau commented Jul 6, 2017

Uh oh!

dhmlau commented Jul 14, 2017

Uh oh!

zbarbuto commented Jul 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zbarbuto commented Jul 17, 2017

zbarbuto commented Jun 26, 2017 •

edited by ssh24

Loading

dhmlau commented Jun 27, 2017 •

edited

Loading

dhmlau commented Jun 27, 2017 •

edited

Loading

jannyHou commented Jun 27, 2017 •

edited

Loading

zbarbuto commented Jun 27, 2017 •

edited

Loading

zbarbuto commented Jul 5, 2017 •

edited

Loading