Polymorphic relations broke SQL Transactions

[mgabeler-lee-6rs]

Describe the bug

PR #8026 broke using transactions with the PostgreSQL connector. The general problem is that doing cloneDeep() on the options parameter is not safe, because the transaction object lives inside there, and a connection object inside there. Cloning the entire DB connection is just ... not a good idea in general, and specifically breaks the PG connector.

This issue seems to apply to all the relations, but walking through it with has-one since that's where I found it.

This line clones the options, which clones the transaction which clones the connector/connection:

loopback-next/packages/repository/src/relations/has-one/has-one.repository.ts

Line 185 in 7198aac

Object.assign(cloneDeep(options ?? {}), {polymorphicType: key}),

Then, if nothing worse happens, we get to the connector checking to see if the transaction belongs to it here: https://github.com/loopbackio/loopback-connector-postgresql/blob/4e9d5ca2993609692bd69d8ce5d9de7fcb9739f7/lib/postgresql.js#L180

This test fails, because it was cloned, and so it instead runs the query on a new connection. This poses multiple problems, including:

• Work done earlier in the transaction is not visible
• Connection pools are limited, trying to open additional connections synchronously with holding a transaction will lead to application deadlocks under load.

Logs

No response

Additional information

No response

Reproduction

https://github.com/mgabeler-lee-6rs/lb4-bug-10194

• install deps
• have postgres available
• change the db name in the datasource if needed
• run npm run migrate
• run npm start
• Atttempt to GET the /ping endpoint
• Observe the error below
• Run it with DEBUG=loopback:connector:postgresql, or set some breakpoints, to see that the relation queries aren't using the transaction. Breakpoints will help prove the bug is as described.

Request GET /ping failed with status code 500. Error: where did my friend go?!
    at PingController.ping (/home/mgl/src/lb4-bug-10194/src/controllers/ping.controller.ts:60:15)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at /home/mgl/src/lb4-bug-10194/node_modules/@loopback/rest/src/providers/invoke-method.provider.ts:51:24
    at /home/mgl/src/lb4-bug-10194/node_modules/@loopback/express/src/middleware-interceptor.ts:131:19
    at /home/mgl/src/lb4-bug-10194/node_modules/@loopback/express/src/middleware-interceptor.ts:131:19
    at /home/mgl/src/lb4-bug-10194/node_modules/@loopback/rest/src/providers/send.provider.ts:46:24
    at MySequence.handle (/home/mgl/src/lb4-bug-10194/node_modules/@loopback/rest/src/sequence.ts:291:5)
    at HttpHandler._handleRequest (/home/mgl/src/lb4-bug-10194/node_modules/@loopback/rest/src/http-handler.ts:115:5)

[mgabeler-lee-6rs]

I don't use them, but it looks like this bug will affect pretty much every SQL connector, not just PG, since they all have copies of the same "is this my transaction" check:

• https://github.com/loopbackio/loopback-connector-mysql/blob/9045ad868ce07e5b7468429566c358ea5fdbf9f6/lib/mysql.js#L246
• https://github.com/loopbackio/loopback-connector-oracle/blob/ba7f88886dd49b091c54e25d5d08421ac934c680/lib/oracle.js#L208
• https://github.com/loopbackio/loopback-connector-sqlite3/blob/f72c27bfaadc2b58e6dec8427bd572bd70c0da14/lib/sqlite3.js#L158
• https://github.com/loopbackio/loopback-connector-mssql/blob/cfb97efa83e0684446a1ed617c3c6bca95279d92/lib/mssql.js#L121

[mgabeler-lee-6rs]

@OnTheThirdDay as the author of the PR that caused this, any thoughts?

It looks like this could be fixed by just using a shallow clone / spread of the options object instead of a deep clone?

[OnTheThirdDay]

Oh I see.. would the following work? I think maybe just don't clone it would be fine. Sorry that I can't quite remember why I deep-cloned it instead of using the options directly..

Object.assign(options ?? {}, {polymorphicType: key}),

@mgabeler-lee-6rs

[mgabeler-lee-6rs]

@OnTheThirdDay Yep, that's roughly what I submitted as #10195, but I used spread syntax in my PR. Is there a particular benefit of using Object.assign here instead of a spread?

[OnTheThirdDay]

@mgabeler-lee-6rs I think not much difference, just a styling thing across the repo.