PgPromise adapter: add default tx options

[sam-artuso]

This PR adds the ability to specify default options for the @Transactional decorator, so that they don't need to be specified repeatedly.

[Papooch]

Thank you for the PR, I left some suggestions. Could you also add some tests for the default options?

[sam-artuso]

Thanks for your comments. I should have realized myself I needed to merge the default options, but that's what happens when you code while drinking beer...

On a more serious note, I need help. The tests for @nestjs-cls/transactional are failing for me. Even in the main branch. What am I doing wrong?

$ yarn test
[nestjs-cls]: Process started
[nestjs-cls]: PASS test/proxy-providers/for-feature.spec.ts
[nestjs-cls]: PASS test/plugin/plugin.spec.ts
[nestjs-cls]: PASS test/proxy-providers/for-root.spec.ts
[nestjs-cls]: PASS test/rest/for-root-async.spec.ts
[nestjs-cls]: PASS src/lib/cls.service.spec.ts
[nestjs-cls]: PASS test/proxy-providers/resolve-proxy-providers.spec.ts
[nestjs-cls]: PASS src/utils/value-from-path.spec.ts
[nestjs-cls]: PASS src/lib/cls-initializers/use-cls.decorator.spec.ts
[nestjs-cls]: PASS test/proxy-providers/proxy-providers.spec.ts
[nestjs-cls]: PASS test/rest/http-fastify.spec.ts (10.842 s)
[nestjs-cls]: PASS test/gql/gql-apollo.spec.ts (11.197 s)
[nestjs-cls]: PASS test/gql/gql-mercurius.spec.ts (11.582 s)
[nestjs-cls]: PASS test/rest/http-express.spec.ts (13.116 s)
[nestjs-cls]: 
[nestjs-cls]: Test Suites: 13 passed, 13 total
[nestjs-cls]: Tests:       153 passed, 153 total
[nestjs-cls]: Snapshots:   0 total
[nestjs-cls]: Time:        13.322 s
[nestjs-cls]: Ran all test suites.
[nestjs-cls]: Process exited (exit code 0), completed in 13s 926ms

[@nestjs-cls/transactional]: Process started
[@nestjs-cls/transactional]: FAIL test/inject-transaction.spec.ts
[@nestjs-cls/transactional]:   ● InjectTransaction with multiple named connections › when using the @Transactional decorator › should start two transactions independently with decorator
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     TypeError: this.cls.setProxy is not a function
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       221 |         this.cls.set(this.transactionInstanceSymbol, txInstance);
[@nestjs-cls/transactional]:       222 |         if (this._options.enableTransactionProxy) {
[@nestjs-cls/transactional]:     > 223 |             this.cls.setProxy(
[@nestjs-cls/transactional]:           |                      ^
[@nestjs-cls/transactional]:       224 |                 getTransactionToken(this._options.connectionName),
[@nestjs-cls/transactional]:       225 |                 txInstance,
[@nestjs-cls/transactional]:       226 |             );
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at TransactionHost.setTxInstance (src/lib/transaction-host.ts:223:22)
[@nestjs-cls/transactional]:       at src/lib/transaction-host.ts:193:37
[@nestjs-cls/transactional]:           at async Promise.all (index 0)
[@nestjs-cls/transactional]:       at CallingService.twoUnrelatedTransactionsWithDecorators (test/inject-transaction.spec.ts:58:26)
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:   ● InjectTransaction with multiple named connections › when using the @Transactional decorator › should start two transactions independently with startTransaction
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     TypeError: this.cls.setProxy is not a function
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       221 |         this.cls.set(this.transactionInstanceSymbol, txInstance);
[@nestjs-cls/transactional]:       222 |         if (this._options.enableTransactionProxy) {
[@nestjs-cls/transactional]:     > 223 |             this.cls.setProxy(
[@nestjs-cls/transactional]:           |                      ^
[@nestjs-cls/transactional]:       224 |                 getTransactionToken(this._options.connectionName),
[@nestjs-cls/transactional]:       225 |                 txInstance,
[@nestjs-cls/transactional]:       226 |             );
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at TransactionHost.setTxInstance (src/lib/transaction-host.ts:223:22)
[@nestjs-cls/transactional]:       at src/lib/transaction-host.ts:193:37
[@nestjs-cls/transactional]:           at async Promise.all (index 0)
[@nestjs-cls/transactional]:       at CallingService.twoUnrelatedTransactionsWithStartTransaction (test/inject-transaction.spec.ts:78:26)
[@nestjs-cls/transactional]:       at Object.<anonymous> (test/inject-transaction.spec.ts:170:17)
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:   ● InjectTransaction with multiple named connections › when using the @Transactional decorator › ignore transactions for other named connection
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     TypeError: this.cls.setProxy is not a function
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       221 |         this.cls.set(this.transactionInstanceSymbol, txInstance);
[@nestjs-cls/transactional]:       222 |         if (this._options.enableTransactionProxy) {
[@nestjs-cls/transactional]:     > 223 |             this.cls.setProxy(
[@nestjs-cls/transactional]:           |                      ^
[@nestjs-cls/transactional]:       224 |                 getTransactionToken(this._options.connectionName),
[@nestjs-cls/transactional]:       225 |                 txInstance,
[@nestjs-cls/transactional]:       226 |             );
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at TransactionHost.setTxInstance (src/lib/transaction-host.ts:223:22)
[@nestjs-cls/transactional]:       at src/lib/transaction-host.ts:193:37
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]: FAIL test/multiple-connections.spec.ts
[@nestjs-cls/transactional]:   ● Transactional › when using the @Transactional decorator › should start two transactions independently with decorator
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     expect(received).toEqual(expected) // deep equality
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Expected  - 7
[@nestjs-cls/transactional]:     + Received  + 1
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Array [
[@nestjs-cls/transactional]:     -   Array [
[@nestjs-cls/transactional]:     -     "BEGIN TRANSACTION;",
[@nestjs-cls/transactional]:     -     "SELECT 1",
[@nestjs-cls/transactional]:     -     "COMMIT TRANSACTION;",
[@nestjs-cls/transactional]:     -   ],
[@nestjs-cls/transactional]:     - ]
[@nestjs-cls/transactional]:     + Array []
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       153 |             });
[@nestjs-cls/transactional]:       154 |             const queries1 = mockDbConnection1.getClientsQueries();
[@nestjs-cls/transactional]:     > 155 |             expect(queries1).toEqual([
[@nestjs-cls/transactional]:           |                              ^
[@nestjs-cls/transactional]:       156 |                 ['BEGIN TRANSACTION;', 'SELECT 1', 'COMMIT TRANSACTION;'],
[@nestjs-cls/transactional]:       157 |             ]);
[@nestjs-cls/transactional]:       158 |             const queries2 = mockDbConnection2.getClientsQueries();
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at Object.<anonymous> (test/multiple-connections.spec.ts:155:30)
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:   ● Transactional › when using the @Transactional decorator › should start two transactions independently with startTransaction
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     expect(received).toEqual(expected) // deep equality
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Expected  - 7
[@nestjs-cls/transactional]:     + Received  + 1
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Array [
[@nestjs-cls/transactional]:     -   Array [
[@nestjs-cls/transactional]:     -     "BEGIN TRANSACTION;",
[@nestjs-cls/transactional]:     -     "SELECT 3",
[@nestjs-cls/transactional]:     -     "COMMIT TRANSACTION;",
[@nestjs-cls/transactional]:     -   ],
[@nestjs-cls/transactional]:     - ]
[@nestjs-cls/transactional]:     + Array []
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       169 |             });
[@nestjs-cls/transactional]:       170 |             const queries1 = mockDbConnection1.getClientsQueries();
[@nestjs-cls/transactional]:     > 171 |             expect(queries1).toEqual([
[@nestjs-cls/transactional]:           |                              ^
[@nestjs-cls/transactional]:       172 |                 ['BEGIN TRANSACTION;', 'SELECT 3', 'COMMIT TRANSACTION;'],
[@nestjs-cls/transactional]:       173 |             ]);
[@nestjs-cls/transactional]:       174 |             const queries2 = mockDbConnection2.getClientsQueries();
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at Object.<anonymous> (test/multiple-connections.spec.ts:171:30)
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:   ● Transactional › when using the @Transactional decorator › ignore transactions for other named connection
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     expect(received).toEqual(expected) // deep equality
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Expected  - 7
[@nestjs-cls/transactional]:     + Received  + 1
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:     - Array [
[@nestjs-cls/transactional]:     -   Array [
[@nestjs-cls/transactional]:     -     "BEGIN TRANSACTION;",
[@nestjs-cls/transactional]:     -     "SELECT 5",
[@nestjs-cls/transactional]:     -     "COMMIT TRANSACTION;",
[@nestjs-cls/transactional]:     -   ],
[@nestjs-cls/transactional]:     - ]
[@nestjs-cls/transactional]:     + Array []
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       186 |             });
[@nestjs-cls/transactional]:       187 |             const queries1 = mockDbConnection1.getClientsQueries();
[@nestjs-cls/transactional]:     > 188 |             expect(queries1).toEqual([
[@nestjs-cls/transactional]:           |                              ^
[@nestjs-cls/transactional]:       189 |                 ['BEGIN TRANSACTION;', 'SELECT 5', 'COMMIT TRANSACTION;'],
[@nestjs-cls/transactional]:       190 |             ]);
[@nestjs-cls/transactional]:       191 |             const queries2 = mockDbConnection2.getClientsQueries();
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]:       at Object.<anonymous> (test/multiple-connections.spec.ts:188:30)
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]: PASS test/propagation.spec.ts
[@nestjs-cls/transactional]: PASS test/transactional.spec.ts
[@nestjs-cls/transactional]: PASS test/named-connection.spec.ts
[@nestjs-cls/transactional]: 
[@nestjs-cls/transactional]: Test Suites: 2 failed, 3 passed, 5 total
[@nestjs-cls/transactional]: Tests:       6 failed, 25 passed, 31 total
[@nestjs-cls/transactional]: Snapshots:   0 total
[@nestjs-cls/transactional]: Time:        0.91 s, estimated 1 s
[@nestjs-cls/transactional]: Ran all test suites.
[@nestjs-cls/transactional]: Process exited (exit code 1), completed in 1s 452ms
The command failed for workspaces that are depended upon by other workspaces; can't satisfy the dependency graph
Failed with errors in 15s 382ms

[sam-artuso]

Additionally, TypeScript is complaining about lines 3, 4 and 126 of packages/transactional-adapters/transactional-adapter-pg-promise/test/transactional-adapter-pg-promise.spec.ts. You added those line last month, so you might have context on what's wrong with them?

[Papooch]

Oh damn, I updated typescript recently, but all tests were passing for me, so I thought all is fine. No new version has been released since then, so there's no immediate danger. I'll investigate.

[Papooch]

@sam-artuso
I was not able to reproduce the problems on my side, but I have some ideas:

Make sure you run yarn install to update the dependencies on your side as well

And run yarn build before running the test (that's likely why it is failing), to build dependent libraries. It's likely that your version@nestjs-cls/transactional is still using the old built files from nestjs-cls.

[sam-artuso]

@Papooch this is now read to review 🙏🏼

[Papooch]

Thanks for the PR, I'll go ahead and merge it.

On the other hand, I was thinking and I don't really like this implementation (and in turn how the tests work). The default options feature should be moved up one level and the options should be merged in the TransactionHost instance (for example here) and passed to the adapter from there. This way it would be easier to implement default options for all adapters.

You don't have to fix that if you don't feel like it. Once I find some more free time, I'll have a go at it.