-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(repository): add beginTransaction API #3397
Conversation
9d4d9b4
to
2bd6344
Compare
|
||
it('retrieves a newly created model in a transaction', async () => { | ||
const tx: juggler.Transaction = await repo.beginTransaction({ | ||
isolationLevel: 'READ COMMITTED', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import from src
seems to be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
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 < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransactionalRepositoryCtor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for the name proposed by Raymond.
>( | ||
entityClass: typeof Entity & {prototype: T}, | ||
dataSource: juggler.DataSource, | ||
) => TransactionRepository<T, ID, Relations>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransactionalRepository
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we allow Promise here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, connectors must use callbacks now. See loopbackio/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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beginTransaction must be implemented
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
'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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit(callback?: Callback<any>): ValueOrPromise<void>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 loopbackio/loopback-connector#147.
IMO, we should use the following form:
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>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we just want Promise style support in LB4, I've only included commit(): Promise<void>;
signature for the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
-
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. -
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. -
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
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 < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, connectors must use callbacks now. See loopbackio/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({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are accepting an empty array then let's make the argument optional please.
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this:
'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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 loopbackio/loopback-connector#147.
IMO, we should use the following form:
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rollback(callback?: Callback<any>): void; | |
rollback(): Promise<void>; |
bbca2aa
to
519592a
Compare
Thank you for your thorough and thoughtful reviews @bajtos @raymondfeng 🙇. I have applied most, if not, all your feedback. |
519592a
to
4663995
Compare
I was doing it wrong :-). Thank you for the pointers, I agree with your POV. |
7dfe932
to
52a4afa
Compare
f54efd0
to
cd32067
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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==", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/site/Transactions.md
Outdated
IsolationLevel, | ||
} from '@loopback/repository'; | ||
// assuming there is a Note model extending Entity class, and | ||
// crudDs datasource which is backed by a transaction enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
### 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
is not the second argument of all CRUD methods, e.g. it's the third argument for findById(id, filter, options)
.
docs/site/Transactions.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using juggler
types here, let's use Transaction
from @loopback/repository
or don't specify the type at all.
const tx: juggler.Transaction = await repo.beginTransaction({ | |
const tx = await repo.beginTransaction({ |
Same comment applies to all other tests too.
cd32067
to
32618cb
Compare
@b-admike CI fails due to the following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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==", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why is this change necessary - how is execute
related to transactions? Can you revert this line please?
packages/repository/src/__tests__/unit/repositories/legacy-juggler-bridge.unit.ts
Show resolved
Hide resolved
Yeah I am not sure how I'm dealing with |
625d2bc
to
77586c0
Compare
docs/site/Transactions.md
Outdated
|
||
### Start transaction | ||
|
||
Use the `beginTransaction()` method to start a new transaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we allow dataSource.beginTransaction()
? I don't see any examples for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
docs/site/Transactions.md
Outdated
// Now we have a transaction (tx) | ||
const tx = await repo.beginTransaction({ | ||
isolationLevel: IsolationLevel.READ_COMMITTED, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to simplify it as follows?
const tx = await repo.beginTransaction(IsolationLevel.READ_COMMITTED);
da301e1
to
1effff9
Compare
I think the coverage drop is because we don't run the transaction tests for all the downstream dependencies i.e.
|
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.
1effff9
to
b9f8b21
Compare
Closes #2614
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈