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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: leverage isactive for transaction #4474

Merged
merged 1 commit into from Jan 29, 2020
Merged

feat: leverage isactive for transaction #4474

merged 1 commit into from Jan 29, 2020

Conversation

@jannyHou
Copy link
Contributor

jannyHou commented Jan 21, 2020

Implements #3471
Previous PRs see:
strongloop/loopback-connector#164
strongloop/loopback-connector#165

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 21, 2020

Checking why postgresql doesn't remove transaction's connection when it's released

@jannyHou jannyHou changed the title feat: leverage isactive for transaction [WIP]feat: leverage isactive for transaction Jan 21, 2020
Copy link
Contributor

nabdelgadir left a comment

Why are package-locks being updated in the PR?

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 22, 2020

@nabdelgadir I released a new version for loopback-connector, which contains a new function that this PR depends on, so I also updated the dependencies in the package-lock.json file to fetch the latest loopback-connector

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 22, 2020

The tx on this line is an instance of different Transaction for postgresql and mysql tests:

packages/repository/node_modules/lb-connector/transaction.js

packages/repository/node_modules/lb-datasource-juggler/transaction.js, which should be the expected behavior

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 22, 2020

When transaction is created from lb-connector's Transaction.begin, the Transaction's __proto__ has juggler's transaction mixin:

Screen Shot 2020-01-22 at 2 44 18 PM

But when transaction is created from lb-connector-postgresql's beginTransaction, the Transaction's __proto__ DOES NOT include juggler's transaction mixin:

Screen Shot 2020-01-22 at 2 40 47 PM

So the problem turns to be lb-connector's function Transaction behaves differently when invoked by different callers.

@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Jan 23, 2020

Found the reason.

packages/repository has a juggler and its loopback-connector as dep, and juggler's Transaction mixin is applied to its lb-connector's Transaction.
So tests in mysql creates transaction with the Transaction that has mixin's overriden prototype methods.

While for postgresql, create transaction uses the Transaction in acceptance/repository-postgresql's dependency, which doesn't have the juggler's Transaction mixin applied. And therefore won't delete the connection reference for tx

@jannyHou jannyHou force-pushed the use-isactive branch from 4aacaa9 to cccea26 Jan 28, 2020
@jannyHou jannyHou changed the title [WIP]feat: leverage isactive for transaction feat: leverage isactive for transaction Jan 28, 2020
/**
* Check if the transaction has an active connection
*/
isActive(): Promise<boolean>;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 28, 2020

Member

Is it boolean or Promise<boolean>?

if (ds.connector && ds.connector.name === 'postgresql') {
tx = undefined;
}
if (tx?.isActive()) {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jan 28, 2020

Member

This test will be always true if tx.isActive() returns a promise.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 28, 2020

Author Contributor

@raymondfeng good catch!
it is not a promise, fixed.

Copy link
Contributor

emonddr left a comment

Great work, Janny

@jannyHou jannyHou force-pushed the use-isactive branch from bc573ac to 8ab782c Jan 29, 2020
@jannyHou jannyHou merged commit fc94437 into master Jan 29, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 92.368%
Details
@jannyHou jannyHou deleted the use-isactive branch Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can鈥檛 perform that action at this time.