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(repository): add beginTransaction API #3397

Merged
merged 1 commit into from Jul 31, 2019

Conversation

@b-admike
Copy link
Member

commented Jul 19, 2019

Closes #2614

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 馃憟

@b-admike b-admike self-assigned this Jul 19, 2019

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from 9d4d9b4 to 2bd6344 Jul 19, 2019

@b-admike b-admike referenced this pull request Jul 19, 2019
1 of 2 tasks complete

it('retrieves a newly created model in a transaction', async () => {
const tx: juggler.Transaction = await repo.beginTransaction({
isolationLevel: 'READ COMMITTED',

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

Do we have a constant/enum for the isolation level?

@@ -9,6 +9,7 @@ import {
juggler,
Options,
} from '@loopback/repository';
import {TransactionRepository} from '@loopback/repository/src';

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

This import from src seems to be problematic.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

+1

Suggested change
import {TransactionRepository} from '@loopback/repository/src';
import {TransactionRepository} from '@loopback/repository';
* A constructor of a class implementing CrudRepository interface,
* accepting the Entity class (constructor) and a dataSource instance.
*/
export type TransactionRepositoryCtor = new <

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

TransactionalRepositoryCtor?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

+1 for the name proposed by Raymond.

>(
entityClass: typeof Entity & {prototype: T},
dataSource: juggler.DataSource,
) => TransactionRepository<T, ID, Relations>;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

TransactionalRepository?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

+1 for TransactionalRepository

@@ -86,6 +85,10 @@ class CrudConnectorStub implements CrudConnector {
): Promise<Count> {
return Promise.resolve({count: this.entities.length});
}

beginTransaction(options: Options, cb: Callback) {

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

Do we allow Promise here?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Not yet, connectors must use callbacks now. See strongloop/loopback-datasource-juggler#1659

Let's add a code comment here to explain & include a link to that GH issue.

it('throws an error when beginTransaction() not implemented', async () => {
const repo = new DefaultCrudRepository(Note, ds);
await expect(repo.beginTransaction({})).to.be.rejectedWith(
'beginTransaction must be function implemented by the connector',

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

beginTransaction must be implemented?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

How about this:

Suggested change
'beginTransaction must be function implemented by the connector',
'Cannot start a new transaction - "memory" connector does not support transactions.'

(Replace memory with the actual connector name.)

This comment has been minimized.

Copy link
@b-admike

b-admike Jul 23, 2019

Author Member

This is coming from loopback-connector. See https://github.com/strongloop/loopback-connector/blob/master/lib/transaction.js#L96-L97. Perhaps we can improve the message there?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

Yes please, let's improve the message there. Ideally, the error object should have also a code property set to a reasonably-distinct string, e.g. 'TRANSACTIONS_NOT_SUPPORTED'. That will allow us to avoid brittle string check and verify just the error code in this test.

Anyhow, I am fine to leave this improvement out of scope of this pull request if you prefer.

* Commit the transaction
* @param callback
*/
commit(callback?: Callback<any>): void;

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

commit(callback?: Callback<any>): ValueOrPromise<void>?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Let's not introduce callbacks to LB4 APIs please. I believe we have already added support for promises to Transaction members in loopback-connector - see strongloop/loopback-connector#147.

IMO, we should use the following form:

Suggested change
commit(callback?: Callback<any>): void;
commit(): Promise<void>;

Regarding commit(callback?: Callback<any>): ValueOrPromise<void>: this does not offer enough type safety. It allows callers to provide a callback and await the returned value at the same time. Here is a more correct type definition:

commit(callback: Callback<any>): void;
commit(): Promise<void>;

This comment has been minimized.

Copy link
@b-admike

b-admike Jul 23, 2019

Author Member

Since we just want Promise style support in LB4, I've only included commit(): Promise<void>; signature for the methods.

@bajtos
Copy link
Member

left a comment

I think I may get confused about your intention regarding how to structure the test suite.

My expectation was that you will simply add or more new files to packages/repository-tests/src/crud, because Transactions are specific to Crud repository interface - we don't support them for KeyValue repositories.

I see that you are creating a whole new transaction-only test suite. What's your reasoning for that? What benefits do you see in that approach?

I am concerned that if the transaction test suite requires a new test file in each per-connector acceptance package, it is difficult to discover by connector authors and makes it easy to forget to run it for certain connectors.

So far, my vision was to add new tests to the single CRUD suite, so that connector authors are notified about new features required by our ORM via a new test that may start failing for their connector.

Let's discuss!

The comments below are based on the assumption that transactions should be tested as part of our CRUD test suite.

@@ -1,630 +0,0 @@
{
"name": "@loopback/test-repository-mysql",

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

It looks like you are trying to remove this package-lock file entirely. Why?

import {MYSQL_CONFIG, MYSQL_FEATURES} from './mysql.datasource';

describe('DefaultCrudRepository + mysql connector', () => {
transactionTestSuite(

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

This is not right. transactionTestSuite should be executed as part of running crudRepositoryTestSuite, which is already called from acceptance/repository-mysql/src/__tests__/mysql-default-repository.acceptance.ts See also packages/repository-tests/src/crud-test-suite.ts which is iterating over all files and automatically running the test suite(s) exported by them.

How to address this issue:

  • Remove this file (mysql.transaction.acceptance.ts)
  • Verify that the transaction-related tests are still executed for MySQL
@@ -1,32 +0,0 @@
{
"name": "@loopback/repository-tests",

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Ditto, why are you removing this package-lock file?

@@ -13,7 +13,7 @@
"build": "lb-tsc",
"clean": "lb-clean loopback-repository-tests*.tgz dist tsconfig.build.tsbuildinfo package",
"pretest": "npm run build",
"test": "lb-mocha \"dist/__tests__/**/*.js\"",
"test": "lb-mocha \"dist/__tests__/**/*.js\" --reporter=spec",

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Please revert this change. You can temporarily enable spec reporter as follows:

$ npm test -- --reporter=spec
@@ -4,6 +4,7 @@
// License text available at https://opensource.org/licenses/MIT

export * from './crud-test-suite';
export * from './transaction-test-suite';

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Transactions are CRUD specific, these tests are not going to work for KeyValue repositories.

Please move the new test file transaction-test-suite to packages/repository-tests/src/crud directory, where it will be automatically picked up by our infrastructure.

It looks like our developer documentation is insufficiently describing how to add new suites, please improve it to help the next person: https://github.com/strongloop/loopback-next/tree/master/packages/repository-tests#developer-guide

Please sync with @hacksparrow, he is working on this part of our docs too.

} from '../types.repository-tests';

// Core scenarios around creating new model instances and retrieving them back
// Please keep this file short, put any advanced scenarios to other files

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Please update the comment to match the intent of this file.

expect(created.id).to.be.ok();

// const found = await repo.findById(created.id);
await tx.commit();

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

How do you envision this test to fail when the database and/or the connector does not support transactions, or implement them incorrectly?

It seems to me that this test only verifies that create+retrieve preserves the data when both commands are executed in the same transaction. It will pass for any repository that implements beginTransaction API, regardless of whether it actually enforces the transaction or not.

Let's find more robust test scenarios please.

Here are few scenarios that come to my mind:

  1. Create a transaction, create a new model from that transaction, COMMIT the transaction, findById the created model outside of the transaction - model should be found. This is a kind of a smoke test, it passes even if transaction isolation is not implemented.

  2. Create a transaction, create a new model from that transaction, ROLLBACK the transaction, findById the created model outside of the transaction - no model should be returned. This verifies rollback feature, it does not require transactions to be isolated.

  3. Create a transaction, create a new model from that transaction. findById without any transaction - no model should be returned. findById from the same transaction - the created model should be returned. This verifies isolation of individual transactions.

@@ -9,6 +9,7 @@ import {
juggler,
Options,
} from '@loopback/repository';
import {TransactionRepository} from '@loopback/repository/src';

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

+1

Suggested change
import {TransactionRepository} from '@loopback/repository/src';
import {TransactionRepository} from '@loopback/repository';
* A constructor of a class implementing CrudRepository interface,
* accepting the Entity class (constructor) and a dataSource instance.
*/
export type TransactionRepositoryCtor = new <

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

+1 for the name proposed by Raymond.

@@ -86,6 +85,10 @@ class CrudConnectorStub implements CrudConnector {
): Promise<Count> {
return Promise.resolve({count: this.entities.length});
}

beginTransaction(options: Options, cb: Callback) {

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Not yet, connectors must use callbacks now. See strongloop/loopback-datasource-juggler#1659

Let's add a code comment here to explain & include a link to that GH issue.

});

const repo = new DefaultCrudRepository(Note, crudDs);
const res = await repo.beginTransaction({});

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

If we are accepting an empty array then let's make the argument optional please.

Suggested change
const res = await repo.beginTransaction({});
const res = await repo.beginTransaction();
it('throws an error when beginTransaction() not implemented', async () => {
const repo = new DefaultCrudRepository(Note, ds);
await expect(repo.beginTransaction({})).to.be.rejectedWith(
'beginTransaction must be function implemented by the connector',

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

How about this:

Suggested change
'beginTransaction must be function implemented by the connector',
'Cannot start a new transaction - "memory" connector does not support transactions.'

(Replace memory with the actual connector name.)

* Commit the transaction
* @param callback
*/
commit(callback?: Callback<any>): void;

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member

Let's not introduce callbacks to LB4 APIs please. I believe we have already added support for promises to Transaction members in loopback-connector - see strongloop/loopback-connector#147.

IMO, we should use the following form:

Suggested change
commit(callback?: Callback<any>): void;
commit(): Promise<void>;

Regarding commit(callback?: Callback<any>): ValueOrPromise<void>: this does not offer enough type safety. It allows callers to provide a callback and await the returned value at the same time. Here is a more correct type definition:

commit(callback: Callback<any>): void;
commit(): Promise<void>;
* Rollback the transaction
* @param callback
*/
rollback(callback?: Callback<any>): void;

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Member
Suggested change
rollback(callback?: Callback<any>): void;
rollback(): Promise<void>;

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from bbca2aa to 519592a Jul 23, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

Thank you for your thorough and thoughtful reviews @bajtos @raymondfeng 馃檱. I have applied most, if not, all your feedback.

@b-admike b-admike marked this pull request as ready for review Jul 23, 2019

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from 519592a to 4663995 Jul 23, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

I think I may get confused about your intention regarding how to structure the test suite.

My expectation was that you will simply add or more new files to packages/repository-tests/src/crud, because Transactions are specific to Crud repository interface - we don't support them for KeyValue repositories.

I see that you are creating a whole new transaction-only test suite. What's your reasoning for that? What benefits do you see in that approach?

I am concerned that if the transaction test suite requires a new test file in each per-connector acceptance package, it is difficult to discover by connector authors and makes it easy to forget to run it for certain connectors.

So far, my vision was to add new tests to the single CRUD suite, so that connector authors are notified about new features required by our ORM via a new test that may start failing for their connector.

Let's discuss!

The comments below are based on the assumption that transactions should be tested as part of our CRUD test suite.

I was doing it wrong :-). Thank you for the pointers, I agree with your POV.

@b-admike b-admike force-pushed the feat/repo-begintransaction branch 2 times, most recently from 7dfe932 to 52a4afa Jul 23, 2019

@bajtos bajtos added this to the July 2019 milestone milestone Jul 25, 2019

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from f54efd0 to cd32067 Jul 25, 2019

@bajtos
Copy link
Member

left a comment

Much better! I reviewed most of the changes but will need another round to review all details.

"integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==",
"version": "4.17.14",
"resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.14.tgz",
"integrity": "sha512-mmKYbW3GLuJeX+iGP+Y7Gp1AiGHGbXHCOh/jZmrawMmsE7MS4znI3RL2FsjbqOyMayHInjOeykW7PEajUk1/xw==",

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

Would you mind reverting this change? I prefer (deep) dependencies in package-lock files to be updated independently when they are not related to other changes made in the pull request.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 30, 2019

Member

You are effectively downgrading lodash to an OLDER version, similarly for loopback-connector-mysql later in this file.

Please revert all changes in all package-lock.json files.

IsolationLevel,
} from '@loopback/repository';
// assuming there is a Note model extending Entity class, and
// crudDs datasource which is backed by a transaction enabled

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

crudDs looks like a typo to me. On the third read of this line, it occurred to me that it may be a variable name "crud Dst". Let's use db, ds or crudDataSource instead.

docs/site/Transactions.md Outdated Show resolved Hide resolved
### Perform operations in a transaction

To perform create, retrieve, update, and delete operations in the transaction,
add a second argument consisting of the transaction object to the standard聽

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

options is not the second argument of all CRUD methods, e.g. it's the third argument for findById(id, filter, options).

You can specify a timeout (in milliseconds) to begin a transaction. If a
transaction is not finished (committed or rolled back) before the timeout, it
will be automatically rolled back upon timeout by default. The timeout event can
be trapped using the timeout hook.

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

The timeout event can be trapped using the timeout hook.

This may need more explanation - what is the "timeout hook" and how can it be used?

This comment has been minimized.

Copy link
@b-admike

b-admike Jul 26, 2019

Author Member

Actually I used LB3 docs as a base when writing the docs. After looking at our TransactionMixin class in juggler and comparing it to the Transaction class in loopback-connector, we still have support for timeout, but won't have support for hooks on transactions for observing events before commit, rollback, or timeout. Thus, I'll remove this bit.

);

it('retrieves model instance once transaction is committed', async () => {
const tx: juggler.Transaction = await repo.beginTransaction({

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 25, 2019

Member

We shouldn't be using juggler types here, let's use Transaction from @loopback/repository or don't specify the type at all.

Suggested change
const tx: juggler.Transaction = await repo.beginTransaction({
const tx = await repo.beginTransaction({

Same comment applies to all other tests too.

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from cd32067 to 32618cb Jul 26, 2019

docs/site/Transactions.md Outdated Show resolved Hide resolved
@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

@b-admike CI fails due to the following:

lerna ERR! npm ci exited 1 in '@loopback/test-repository-mysql'
lerna ERR! npm ci stderr:
npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
npm ERR! 
npm ERR! 
npm ERR! Invalid: lock file's loopback-connector-mysql@5.4.1 does not satisfy loopback-connector-mysql@^5.4.2
npm ERR! 

@bajtos bajtos dismissed their stale review Jul 30, 2019

the comments have been addressed

@bajtos
Copy link
Member

left a comment

The new version looks much better, you are getting close :)

I have few more comments to address.

No need to wait for another review from me before landing this change. Please get somebody from @strongloop/sq-lb-apex to do the review instead of me (and in addition to review by @raymondfeng).

"integrity": "sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==",
"version": "4.17.14",
"resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.14.tgz",
"integrity": "sha512-mmKYbW3GLuJeX+iGP+Y7Gp1AiGHGbXHCOh/jZmrawMmsE7MS4znI3RL2FsjbqOyMayHInjOeykW7PEajUk1/xw==",

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 30, 2019

Member

You are effectively downgrading lodash to an OLDER version, similarly for loopback-connector-mysql later in this file.

Please revert all changes in all package-lock.json files.

@@ -32,6 +32,7 @@ export function crudRepositoryTestSuite(
idType: 'string',
freeFormProperties: true,
emptyValue: undefined,
supportsTransactions: false,

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 30, 2019

Member

New feature flags should default to true. That way we include the new tests in all downstream repos (e.g. repository-mysql), which makes it much easier to discover a database/connector that does not support the new feature.

await tx.rollback();
await expect(repo.findById(created.id, {})).to.be.rejectedWith({
code: 'ENTITY_NOT_FOUND',
message: /Entity not found/,

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 30, 2019

Member

It's enough to assert on the code. The message can change in the future, we don't want this test to fail if that happens. Please remove this line.

@@ -411,9 +414,26 @@ describe('DefaultCrudRepository', () => {
});

it(`throws error when execute() not implemented by ds connector`, async () => {
const repo = new DefaultCrudRepository(Note, ds);
const repo = new DefaultTransactionalRepository(Note, ds);

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 30, 2019

Member

I don't see why is this change necessary - how is execute related to transactions? Can you revert this line please?

packages/repository/src/repositories/repository.ts Outdated Show resolved Hide resolved
packages/repository/src/transaction.ts Outdated Show resolved Hide resolved
@b-admike

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

You are effectively downgrading lodash to an OLDER version, similarly for loopback-connector-mysql later in this file.

Please revert all changes in all package-lock.json files.

Yeah I am not sure how I'm dealing with package-lock.json files. I always choose the versions in the latest master branch when rebasing. Maybe I should keep the files generated in my branch.

@b-admike b-admike referenced this pull request Jul 30, 2019
0 of 2 tasks complete

@b-admike b-admike force-pushed the feat/repo-begintransaction branch 3 times, most recently from 625d2bc to 77586c0 Jul 30, 2019


### Start transaction

Use the `beginTransaction()` method to start a new transaction.

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 30, 2019

Member

Do we allow dataSource.beginTransaction()? I don't see any examples for that.

This comment has been minimized.

Copy link
@b-admike

b-admike Jul 31, 2019

Author Member

I see that as an internal API because we have repository level beginTransaction which calls it. I guess users can technically do repo.dataSource.beginTransaction() too.

// Now we have a transaction (tx)
const tx = await repo.beginTransaction({
isolationLevel: IsolationLevel.READ_COMMITTED,
});

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 30, 2019

Member

Do we want to simplify it as follows?

const tx = await repo.beginTransaction(IsolationLevel.READ_COMMITTED);

@b-admike b-admike force-pushed the feat/repo-begintransaction branch 2 times, most recently from da301e1 to 1effff9 Jul 31, 2019

@b-admike

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2019

I think the coverage drop is because we don't run the transaction tests for all the downstream dependencies i.e. acceptance/repository-mongodb doesn't run them whereas acceptance/repository-mysql does:

COMMITTED 31 JUL 2019 - 17:38 COVERAGE DECREASED (-0.3%) TO 91.401%
docs/site/Transactions.md Outdated Show resolved Hide resolved
feat(repository): expose beginTransaction() API
Add TransactionalRepository for use with datasources
that support transactions. Create DefaultTransactionRepository
with CRUD methods and beginTransaction() method. Add Transaction
interface to describe transaction objects with commit() and
rollback() actions.

@b-admike b-admike force-pushed the feat/repo-begintransaction branch from 1effff9 to b9f8b21 Jul 31, 2019

@b-admike b-admike merged commit 0471315 into master Jul 31, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.3%) to 91.401%
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@delete-merged-branch delete-merged-branch bot deleted the feat/repo-begintransaction branch Jul 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.