loopbackio / loopback-connector-postgresql

PostgreSQL connector for LoopBack.
Other
117 stars 180 forks source link

Transactions are broken on v3.0.1 #258

Closed StevenWinde closed 7 years ago

StevenWinde commented 7 years ago

Description/Steps to reproduce

This works fine in 3.0.0 but not in 3.0.1

this.beginTransaction( 'READ COMMITTED', ( err, tx )=>{
  this.updateAll( { foo: 'bar' },  { foo: 'bar2'},  { transaction: tx },
  ( err )=>{
    // err is Transaction is not active.
  }
}
cadesalaberry commented 7 years ago

Version 3.0.1 is also not working for me...

Do not use this version in production !

Error: Transaction is not active
at /opt/app/node_modules/loopback-connector-postgresql/lib/postgresql.js:205:18
at nextTickCallbackWith0Args (node.js:420:9)
at process._tickDomainCallback (node.js:390:13)
From previous event:
  at Object.createPromiseCallback (/opt/app/node_modules/loopback-datasource-juggler/lib/utils.js:492:17)
  at Function.DataAccessObject.update.DataAccessObject.updateAll (/opt/app/node_modules/loopback-datasource-juggler/lib/dao.js:2551:20)
jmls commented 7 years ago

really ? transactions are broken, and no-one from SL responds ? It's this sort of thing that really makes me worry about strongloop. Does anyone have a fix ?

cadesalaberry commented 7 years ago

@bajtos could you have a look at it please ?

bajtos commented 7 years ago

Hello, I am not familiar with PostgreSQL myself. The connector version 3.0.1 contains the following changes (see https://github.com/strongloop/loopback-connector-postgresql/releases/tag/v3.0.1):

  • Add docker setup (#256) (Sakib Hasan)
  • Allow explicit numeric datatype (#254) (Sakib Hasan)
  • Allow non-id serial properties (#198) (zbarbuto)
  • Revert PR #246 (#248) (Sakib Hasan)
  • Add loopback-connector as peer dependencies (#246) (Russ Tyndall)
  • Fix operations on ended transactions (zbarbuto)
  • dbdefaults: Cleanup InvalidDefault def after test (Kevin Delisle)
    • Reuse the data source to avoid too many clients (Raymond Feng)

I guess https://github.com/strongloop/loopback-connector-postgresql/pull/230 may be the cause of this regression?

@raymondfeng @jannyHou @zbarbuto @kjdelisle could you PTAL?

kjdelisle commented 7 years ago

As a stop-gap for now, I'll mark 3.0.0 as the latest release. ~Once we confirm, we'll deprecate 3.0.1 to inform users about this version.~

EDIT: I'll deprecate it now; if we determine an alternate cause, we can always back out that warning.

dhmlau commented 7 years ago

I can reproduce it using master, but not @2.8.0

zbarbuto commented 7 years ago

What does this refer to in the above context? Would it be possible to get a sandbox or fork+branch with test added to reproduce the issue?

I just wrote a test for updateAll which is working fine for me but that's using Transaction and the model directly. Hard to make a test to reproduce without knowing further info on context for this.

https://github.com/zbarbuto/loopback-connector-postgresql/commit/23d7a05dd1b6dc3f5566dd4c16d4fbd41263600e

dhmlau commented 7 years ago

I was able to reproduce by adding a js file under boot folder:

'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);
    });
  });
};

The error message Transaction is not active is coming from this line: https://github.com/strongloop/loopback-connector-postgresql/blob/master/lib/postgresql.js#L203. For some reason, transaction.txId, once was set, is undefined.

zbarbuto commented 7 years ago

Thanks for the steps. Have submitted a PR fix: #268

dhmlau commented 7 years ago

connected to strongloop/loopback-connector-postgresql/pull/268

dhmlau commented 7 years ago

Fixed in PR: https://github.com/strongloop/loopback-connector/pull/109